[adutra]

This is the first PR related to the "Auth Manager v2" effort. Cf. design document.

This PR introduces the configuration layer changes (section 5.1 of the design doc). It also introduces a few class deprecations. Finally, it also updates LICENSE and runtime-deps.txt files.

The most important part is the migration of configuration properties. This is handled by ConfigMigrator. Its logic may seem complex, that's because we need to handle some edge cases, as detailed in the design doc, section 5.6. The most complex edge case is when a session context initiates a token exchange flow, using the catalog session's parent token as the actor token (a.k.a. the "Trino use case"): this is handled in v2 by introducing a special sentinel value that will be recognized and processed at runtime (this will come in a later PR though).

Merging this PR has only two visible consequences:

1. Deprecation warnings will be printed when loading the legacy (v1) manager, and when any deprecated v1 configuration is used.
2. Runtime jars (Spark, Flink, Kafka Connect) will now include the Nimbus libraries and their dependencies.

UPDATE: this PR does not introduce any visible change anymore.

[adutra]

FYI the full changes can be reviewed here for reference: #15704 .

[bharos]

Taking the example of long-running Spark jobs against an Iceberg REST catalog backed by Azure Entra ID.
These jobs require per-user identity (user-delegated access), not a shared service account (client_credentials flow).

As a first step, we can have the users handle the Authorization Code + PKCE flow externally (via a CLI wrapper) and obtain an access token + refresh token pair before submitting the job.

Currently there's no way to pass that refresh token to Iceberg. The v1 OAuth2Util.refreshToken() only supports token exchange or client credentials re-auth. In the v2 config, TokenRefreshConfig controls refresh behavior but has no property to seed an external refresh token.

The flow would be:

1. External tooling handles interactive auth (PKCE, device code, etc.) before job submission
2. User configures rest.auth.oauth2.token and rest.auth.oauth2.refresh-token
3. Iceberg uses the access token until near-expiry, then calls the token endpoint with grant_type=refresh_token
4. Subsequent refreshes use the rotating refresh token from each response

Should we consider adding a config property like rest.auth.oauth2.refresh-token that provides an initial refresh token to RefreshTokenFlow, enabling grant_type=refresh_token without Iceberg needing to implement the interactive PKCE flow (which can be considered in a future update)?

We can support it in later PR as well after this, but want to understand if it already works , or if it makes sense to try supporting this case ?

[adutra]

Currently there's no way to pass that refresh token to Iceberg.

That is a Bad Idea ™️ 😄

Refresh tokens are specific to the clients they were issued to, and are not meant to be shared. Sharing refresh tokens is not only a potential security issue, but also doesn't work in practice: most IDPs revoke the refresh token once used, so if the catalog is given a refresh token and shares it with all the FileIO instances, only the first refresh would succeed; all others would fail because the token was revoked.

Instead, the Iceberg catalog must be autonomous in terms of OAuth2 tokens: ideally, each component (the catalog, the SessionContext, the FileIO instances, etc.) should manage its token internally.

In AuthManager v2, this will eventually be possible, and the catalog itself will handle the Authorization Code flow. There will be no need for users to do the authorization flow externally.

You may ask: but what if the Authorization Code flow is handled externally (e.g. in Trino)? In this case the best alternative is to use token exchange:

1. Trino logs in the user and gets an access token for them;
2. Trino instantiates the catalog with a SessionContext configured with the user's access token and the token exchange grant;
3. The SessionContext exchanges the user token against a token for itself;
4. The SessionContext receives an exchanged access token, and a refresh token.

In this way, the catalog's SessionContext contains an independent token that can be refreshed with its own refresh token. It also doesn't interfere with FileIO instances, because the SessionContext is not shared with them.

[bharos]

for users who obtain tokens externally (PKCE, device code), this is the only entry point. As per my other comment, if refresh-token is supported, this option may become viable as well ?

[adutra]

See my answer to your other comment.

In an ideal world, there should be no reason to pass an access token "as is" to the catalog.

You would instead either:

1. Let the catalog fetch the token for you; or
2. Exchange an external token for a new token, using the token exchange grant.

Passing a raw access token effectively beats the purpose of using OAuth2, and in most cases, the token cannot be refreshed.

This option only exists to "bridge the gap" until everyone is able to use OAuth2 optimally.

[adutra]

This PR has been updated to incorporate the changes from #16081.

Since Nimbus is now a dependency of iceberg-core, the following dependencies are now visible in all runtime jars:

com.nimbusds:content-type:2.3
com.nimbusds:lang-tag:1.7
com.nimbusds:nimbus-jose-jwt:10.9
com.nimbusds:oauth2-oidc-sdk:11.37
net.minidev:json-smart:2.5.2

All runtime-deps.txt and LICENSE files were updated. No NOTICE file requires updates, because none of the above dependencies ships with a NOTICE file.

[adutra]

These exclusions allow to narrow the amount of transitive dependencies brought by Nimbus:

• accessors-smart is an optional bytecode-generation optimizer for json-smart; json-smart falls back to reflection without it. Besides accessors-smart also brings ASM, which is a big jar. Removing these is safe because Nimbus objects all implement JSONAware (i.e. they expose a .toJSONString() method) and thus do not need reflection in order to be serialized.
• jcip-annotations are compile-time thread-safety markers; they can be safely excluded.

[rdblue]

Does this need another JSON library? How larger are the dependency additions to the runtime Jars?

[danielcweeks]

I don't think we need a config package separate from the parent oauth2. I understand that this provides some structural separation, but it also forces all of these classes into the public, which isn't necessary.

We should move all of these up.

[adutra]

OK, I understand the concern. I will move everything to a single package and use package-private visibility as the default.

[adutra]

This will end up with a potentially very large package eventually. But I guess there is no better option.

[danielcweeks]

This isn't actually parsing, so I'd prefer from(<properties>) as opposed to parse

[danielcweeks]

Minor point here, but this isn't Migrating anything since it's not updating the source based on what I see. This is more of a ConfigAdaptor (or maybe LegacyConfigAdaptor?).

[adutra]

It takes a map of properties as its input, and outputs a map of properties semantically equivalent, but with all legacy properties migrated to their new counterparts – hence the Migrator suffix.

But if you still prefer LegacyConfigAdaptor, I'm fine with that.

[danielcweeks]

I think we should avoid using specific implementations like keycloak in the documentation. We can add site docs later for how to configure specific OAuth servers, but the javadocs should just stick to the spec/RFC definitions.

[danielcweeks]

How is this Optional? If there's a case where the grant type is unknown, we should enumerate that in the type. If that's not an option, it should be required.

[adutra]

While the config classes are not considered public API, their javadocs will be used later to create a documentation page in markdown format (I wrote a small tool for that).

For example, this method's javadocs will render as below:

### `rest.auth.oauth2.grant-type`

The grant type to use when authenticating against the OAuth2 server. Valid values are:

- `client_credentials`
- `password`
- `authorization_code`
- `urn:ietf:params:oauth:grant-type:device_code`
- `urn:ietf:params:oauth:grant-type:token-exchange`

Optional, defaults to `client_credentials`.

So "optional" here is a way to tell the users that if they don't specify the grant type, it will default to client_credentials.

You will notice that all javadocs of config classes were written with this documentation goal in mind.

[adutra]

FYI I revisited some of the method javadocs and removed the word "optional" when it was misleading. Let me know if that's better.

[danielcweeks]

We seem to be really inconsistent about how we're handling optional fields. This says optional but isn't wrapped as Optional<TokenRefreshConfig>. This is one of the places where I'd expect to see optional used.

[adutra]

I rephrased a few method javadocs, lmk if that's better.

[danielcweeks]

I don't feel like this should be necessary. If there's a refresh config, then you should refresh. If there's not, you don't. I don't see the value in providing a refresh config, but then saying you can't use it.

[adutra]

But users need to tell if they want token refreshes or not. So there has to be a property for that, namely rest.auth.oauth2.token-refresh.enabled.

Each method in the configuration classes maps 1x1 to a user-facing configuration option.

[danielcweeks]

Why do we need optional logic here? Seems like this would only be true or false.

[adutra]

No, we can't have a static default here: the default is true if the initial grant is client_credentials, and false otherwise. This is awkward indeed, but it's a necessary precaution for people migrating from v1.

[danielcweeks]

We shouldn't rely on parsed time durations like this when there's an explicit and more readable option: Duration.ofHours(1);

[adutra]

That is true, but the documentation tool I wrote will leverage the Duration.parse() method to extract the ISO-8601 litteral and include it in the documentation.

For example, the accessTokenLifespan() method will render as :

### `rest.auth.oauth2.token-refresh.access-token-lifespan`

Default access token lifespan; if the OAuth2 server returns an access token without specifying its expiration time, this value will be used.

Optional, defaults to `PT1H`. Must be a valid [ISO-8601 duration](https://en.wikipedia.org/wiki/ISO_8601#Durations).

[danielcweeks]

This doesn't appear to be optional.

[adutra]

As I explained in my other comment, the word "optional" here is intended for end users.

[adutra]

Rephrased.

[danielcweeks]

Same here, prefer from as opposed to parse

[danielcweeks]

Naming is a little awkward here. Something more like prefechDuration makes more sense than safety. Also, seeing the impact of this in the wild, we'll need to make sure to include reasonable jitter (refreshes across hundreds or thousands of workers can cause thundering herd problems). Not sure if this is where want that or in the actual fetch code.

[adutra]

I took note of the jitter requirement. Will include it in the actual fetch code (that will come later).

[adutra]

Renamed to prefetch() and added jitter(). PTAL.

[danielcweeks]

I'm not convinced it makes sense to introduce this now if there's no implementation.

[adutra]

OK let's remove and introduce them later on.

[danielcweeks]

A few high-level observations:

1. I think we should keep all the implementation in the same package and restrict everything to package private where possible. It's not worth losing the encapsulation to add a little bit if package structure.
2. We're leaking many of the Nimbus primitives through the interfaces. If we can keep those within the package, I'm probably ok with that, but if we expose them through public interfaces, it binds us to that implementation.
3. We use Optional and optional fields very inconsistently. If it's truly optional, then it should be wrapped in Optional.
4. I'm not convinced we should be breaking out separate configs like BasicConfig, TokenRefreshConfig into composed separate sets of interfaces. I might be able to get behind keeping refresh and exchange configs separate, but it feels like a lot of levels of indirection.

[obelix74]

This PR will help with some of the use cases we (Workday) face with regards to refresh tokens. How can we help to get this uptaken?

[danielcweeks]

@adutra I'm out through the end of the week, but will look first thing next week.

[danielcweeks]

I'm not sure how to address the dependency issues, but this pulls in dependencies for the runtime bundles without actually using them yet. Is there an easy way to exclude these until we have an implementation that would actually be used?

[adutra]

We can use compileOnly for now and switch to implementation later.

[danielcweeks]

Suggested change

	public OAuth2Config migrateCatalogConfig(Map<String, String> properties, String catalogUri) {
	public OAuth2Config fromCatalogConfig(Map<String, String> properties, String catalogUri) {

[danielcweeks]

Suggested change

	public OAuth2Config migrateContextualConfig(
	public OAuth2Config fromContextualConfig(

[danielcweeks]

Is there a reason this neads to be public at this point? If you configure it, it will just throw so it may be best to just keep it package private until there's functionality that can be used.

[adutra]

I'll do the requested change, but in order for this to work we'll need to use hiddenImpl() in AuthManagers – hope that's OK.

[adutra]

In fact, given the direction we are taking, and since @rdblue also requested that we revert the deprecations introduced in this PR, I think we should re-center this PR around config classes exclusively.

Let's remove this OAuth2Manager for now, revert the deprecations and revert the changes to the AuthManagers class. This will considerably reduce the PR scope.

[danielcweeks]

Strong preference to not include TODO, we already call it out LEGACY/NEW, we should drop this comment.

[danielcweeks]

Our convention is that messages should start with a capitol letter. There's a lot of new check/validation code being added and it's unclear to me if we're holding true to the project convention on this.

[adutra]

All messages generated by ConfigValidator respect that rule. Here is an example error message, where 3 validation failures are found:

Invalid OAuth2 configuration:
  - either issuer URL or token endpoint must be set (rest.auth.oauth2.issuer-url / rest.auth.oauth2.token-endpoint)
  - client ID must not be empty (rest.auth.oauth2.client-id)
  - client secret must not be empty when client authentication is 'client_secret_basic' (rest.auth.oauth2.client-auth / rest.auth.oauth2.client-secret)

[danielcweeks]

Any duration values we have in could should use explicit, compile-time checked APIs (e.g. Duration.ofSeconds(30) vs Duration.parse("PT30S"). It's both more readable and compile-time checked whereas parsed values are not.

[danielcweeks]

I'm a little concerned about the complexity we're adding here. I understand what you're trying to accomplish by trying to validate all of the properites at once and throw an exception that includes everything that's misconfigured. However, this is a big shift from the conventions used everywhere else in the project and adds a lot of overhead for understanding checks.

Looking at all of the validation logic, it seems like we could just use the standard convention of Preconditions.check everywhere and avoid this. What value is this really adding?

[adutra]

The added value is standardized error checks (e.g. when checking an endpoint URL) and standardized error messages. We can achieve the same with just Preconditions.check but it's going to be more brittle.

Should I remove ConfigValidator and replace it with Preconditions?

[danielcweeks]

The project has a standardized way of doing this using either Precondition checks or ValidationException (with the latter being used for more config validation). This is not following that model and introduces more complexity for very little added value.

[rdblue]

I don't think we should deprecate these in this PR. Before we deprecate, we need to fully replace this. Plus, this is unrelated and can go in a separate PR so this one doesn't require touching so many files. It's a best practice to keep unrelated changes in separate PRs.

[adutra]

You are right, I shouldn't have mixed deprecations and config classes in the same PR. The revised PR now contains only changes related to configuration and nothing else.

[adutra]

@danielcweeks and @rdblue I incorporated all your feedback. The PR now is considerably reduced in size (15 files only). PTAL 🙏

[danielcweeks]

Strong preference to not in include TODOs in the codebase. Even if this is a work-in-progress, it's best to not use the codebase as a tracker.

[danielcweeks]

Is there really a practical reason to expose configurable timeouts like this? We're introducing more config complexity and the library defaults should be adequate for OAuth. Does Nimbus not have reasonable defaults?

[adutra]

It's exposed as a configurable property only for testing purposes. Let me remove the configurable option, I will figure out some other way to tweak this value for tests.

[adutra]

Does Nimbus not have reasonable defaults?

Nimbus does not ship with an OAuth client. It only has the building blocks to create such a client.

[danielcweeks]

minor: we've been moving away from the test prefix, but I'm also thinking this is probably not a very descriptive test name. Something more like configFromProperties is closer to what it is doing.

[danielcweeks]

Similar naming: prefer something more descriptive like invalidConfigFromProperties

[danielcweeks]

This sentinel has me concerned because I don't see the context in which it would be used. At this point I would have to assume I understand and agree with how it will be used, but I don't. I'd rather leave things like this out and introduce them only when they're used so it makes sense in context of the introduced changes.

[adutra]

For the record, you could see how it would be used here:

https://github.com/apache/iceberg/pull/15704/changes#diff-51fed95e2595a7597626d432abf3fe6cde63de001185eca1f97192652da36bb5R106-R117

Let me remove this for now. We can decide later if we really need to support this edge case.

The more it goes the more I feel that people are OK with us dropping support for all the weird edge cases in v2 anyways.

[adutra]

FYI, Trino is moving away from vended token exchange. Since that was the only identified usage of this feature, I think it's fine to just drop it.

[adutra]

FYI I rebased and squashed as the commit history was becoming quite large.

[github-actions]

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

[adutra]

Not stale.