Deprecate oauth/tokens endpoint

[snazy]

This PR implements "M1" of this document, see #10537.

[jackye1995]

This would be a problem for sigv4 users. The Sigv4 code path is currently hidden in the HTTPClient with:

private static final String SIGV4_ENABLED = "rest.sigv4-enabled";
  private static final String SIGV4_REQUEST_INTERCEPTOR_IMPL =
      "org.apache.iceberg.aws.RESTSigV4Signer";

So we should at least check this property and not log warning if the flag is set to true.

I can do some refactoring to bring them to public after this PR is merged, by formally introduce an additional property like auth-mode, that can be of values like oauth2, amazon-sigv4, and defaults to oauth2. And we only check this property if the auth mode is oauth2 or null.

[snazy]

Good point

[snazy]

But we can do the auth-mode change in the later "milestone"

[snazy]

@jackye1995 can you take a look at the updated if condition?

[nastra]

initialize is already quite long and the if condition is complicated here, so might be worth extracting this into a separate method

[snazy]

Is this really important or rather a nit for code that is going to be changes in the near future?

[jackye1995]

yeah I think if we want to formally refactor this, it might be okay to leave the warning here for now. What do you think Eduard?

[nastra]

I'm ok leaving it as-is but it adds complexity to the already difficult to understand initialize method, hence why I suggested to consider moving this into a separate method (even if the code eventually gets changed in the near future we don't necessarily have to add additional complexity if we can avoid it in the first place)

[jackye1995]

I am doing a prototype for the refactoring, maybe let me publish a draft and we can see how that would look like

[snazy]

I'd rather let this change not depend on a follow-up, but get it in.

[danielcweeks]

Can you update this to include the target version for removal for all deprecation comments (see deprecation notices?

[jackye1995]

What does target version mean here? The spec does not really have a general version the last time we discussed.

[danielcweeks]

I think the target version should be tied to the reference implementation where the default behaviors will change.

As we discussed before, we can change the requirement that URI be set with the 1.7 release. However, we can't change the behaviors until 2.0. So I think we should reference the 2.0 release as the target for removal.

In general, we shouldn't add deprecations that don't clearly indicate a target for removal and reference to the replacement. In this case we don't have clarity on the exact replacement but I think the current reference to the issue is fine.

[snazy]

I'm proposing to include it in 1.6.0 and not wait for 1.7.0 - added a note that mentions "Iceberg (Java) 1.6.0", because that's the next release this change can realistically go into.

Sure, the improvement proposal document mentioned Iceberg 1.7.0, because at the time of writing it (June), 1.7.0 was the next release the deprecation could go in. 1.6.0 was considered to be released in June (meeting notes). But now it's already July and 1.6.0 is still not ready to be released.

This change is part of an effort to address a potential security issue. I think that fixes and mitigations for security are a good example for the non-hard rules mentioned in the Contributing page.

The improvement proposal document assumes 6 months migration period (aka two non-patch Iceberg/Java releases), which is IMO okay considering that this effort is about security.

[Fokko]

I think what Daniel is pointing at when the endpoint will be removed:

So that should be 1.8 or 2.0 according to the proposal, where it is cleaner to remove it at 2.0.

[jbonofre]

I don't see blocker to have deprecation in 1.6.0 as soon as it doesn't actually change the current behavior (else it's a breaking change).
So I would deprecate (comment) for 1.6.0 and plan the removal in 1.8 or even better 2.0.

[snazy]

To calm people down a bit: this change deprecates the endpoint, with the plan to remove the endpoint from the REST specification. That's nothing to do with the REST client implementation.

[danielcweeks]

@snazy I don't really understand the pushback here. When we deprecate something, we need to document the target for removal (this is part of our deprecation process). While the client implementation is separate from the spec, the two are related in behavior.

To be clear, we need to include messaging on removal before this is ready to commit.

[snazy]

The misunderstanding is on my side. I've fixed it, but never pushed the change from my local branch.

[danielcweeks]

@snazy I think the discussion above was generally agreed to set the removal target at 2.0 and the reason I say that is the description contains aspect of the client behavior, so removing that prior to 2.0 would lose that context.

I don't think there's any harm in leaving this until 2.0 for removal as we've clearly covered the concerns in the notice.

[jackye1995]

Looks good to me

[nastra]

what's the implication of removing this (and all the request / response types) from the Spec in 1.7.0 but not actually from the implementation? To me it seems that this should be marked for removal with Iceberg 2.0

[snazy]

It's to let (new) adopters run into the trap of "blindly" implementing it and accidentally run into security issues

[dimas-b]

Removing the endpoint from the OpenAPI YAML could break auto-generated clients. Custom clients (e.g. the Iceberg java REST client) calling this endpoint in servers that offer backward compatibility will not be affected. AFAIK, PyIceberg also not affected by dropping the endpoint from OpenAPI.

Given the discussion of the negative security aspects of this endpoint (in the dev mail list), I tend to think that removing the endpoint from Open API sooner (1.7.0) is worth the potential hardship for auto-generated clients.