Core: Assume issued_token_type is access_token to fully comply with RFC 6749 #10314

adutra · 2024-05-11T15:42:34Z

The REST client wrongly assumes that the issued_token_type field is present in all OAuth responses, but that isn't true: e.g. in the client_credentials flow, this field is undefined. See RFC 6749, section 4.4.3:

https://datatracker.ietf.org/doc/html/rfc6749#section-4.4.3

This causes the client to crash when creating a tokens exchange request, since the issued token type becomes the request's subject token type, which is mandatory.

This has been verified against a Keycloak 24.0 server.

This change fixes this issue by assuming that the issued token type is an access token, if the response did not specify any token type.

This change also fixes RESTCatalogAdapter: it was incorrectly including the issued_token_type field in client_credentials responses, thus masking many test failures, e.g. in testCatalogTokenRefresh.

nastra · 2024-05-15T08:39:11Z

core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java

@@ -763,11 +763,19 @@ private static AuthSession fromTokenResponse(
        long startTimeMillis,
        AuthSession parent,
        String credential) {
+      // issued token type is not present in every OAuth2 response:
+      // assume type is access token if none provided.
+      // See https://datatracker.ietf.org/doc/html/rfc6749#section-4.4.3


@adutra do you maybe know if Keycloak fully supports RFC 8693? The token exchange follows https://www.rfc-editor.org/rfc/rfc8693#name-successful-response, where issued_token_type is required

I found keycloak/keycloak#26502, which states unfortunately that Keycloak deviates from the standard.
I can see that we might add the null check as a workaround for such cases where the auth server doesn't send back an issued_token_type but I'd like to first see what other people in the community think about this.

Indeed, Keycloak does not fully implement it. Quoting from their Securing Applications and Services Guide:

Token exchange in Keycloak is a very loose implementation of the OAuth Token Exchange specification at the IETF. We have extended it a little, ignored some of it, and loosely interpreted other parts of the specification. It is a simple grant type invocation on a realm’s OpenID Connect token endpoint.

But the problem here is wider: issued_token_type is only defined for the token exchange flow (RFC 8693). This field does not exist for standard flows from RFC 6749. So the following scenario can happen even with fully-compliant servers:

client uses client_credentials to authenticate initially;

server sends a successful response similar to this one; note that it does not contain issued_token_type, which is normal since it's not defined for this flow.

client attempts to refresh the token using the token exchange flow;

client throws IAE because subjectTokenType is null.

@nastra also see this comment which gives some context on why we have the problem today: #4771 (comment)

nastra · 2024-05-15T08:47:15Z

core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java

@@ -254,7 +254,6 @@ private static OAuthTokenResponse handleOAuthRequest(Object body) {
      case "client_credentials":
        return OAuthTokenResponse.builder()
            .withToken("client-credentials-token:sub=" + request.get("client_id"))
-            .withIssuedTokenType("urn:ietf:params:oauth:token-type:access_token")


I would probably leave this as-is, since issued_token_type is required according to RFC 8693

I don't agree: this flow is not governed by RFC 8693, this is RFC 6749 section 4.4.

adutra · 2024-05-27T07:49:57Z

Hi @nastra any updates on this PR?

FYI here is an example of error from a Spark SQL session connected to REST catalog + external auth server:

24/05/26 21:39:19 WARN Tasks: Retrying task after failure: Invalid token type: null
java.lang.IllegalArgumentException: Invalid token type: null
        at org.apache.iceberg.relocated.com.google.common.base.Preconditions.checkArgument(Preconditions.java:218)
        at org.apache.iceberg.rest.auth.OAuth2Util.tokenExchangeRequest(OAuth2Util.java:244)
        at org.apache.iceberg.rest.auth.OAuth2Util.tokenExchangeRequest(OAuth2Util.java:235)
        at org.apache.iceberg.rest.auth.OAuth2Util.refreshToken(OAuth2Util.java:140)

nastra · 2024-05-27T07:53:13Z

@adutra I'm OOO this week, maybe @amogh-jahagirdar gets a chance to look at this PR

adutra · 2024-06-04T10:40:12Z

@nastra or @amogh-jahagirdar is it possible for you to have a another look here please? Thanks 🙏

The REST client wrongly assumes that the `issued_token_type` field is present in all OAuth responses, but that isn't true: e.g. in the `client_credentials` flow, this field is undefined. See RFC 6749, section 4.4.3: https://datatracker.ietf.org/doc/html/rfc6749#section-4.4.3 This causes the client to crash when creating a tokens exchange request, since the issued token type becomes the request's subject token type, which is mandatory. This has been verified against a Keycloak 24.0 server. This change fixes this issue by assuming that the issued token type is an access token, if the response did not specify any token type. This change also fixes `RESTCatalogAdapter`: it was incorrectly including the `issued_token_type` field in `client_credentials` responses, thus masking many test failures, e.g. in `testCatalogTokenRefresh`.

adutra · 2024-07-03T08:32:55Z

@nastra @amogh-jahagirdar This PR has been rebased. Could I get a review please? 🙏

nastra · 2024-07-03T09:03:59Z

@adutra IMO https://github.com/apache/iceberg/pull/10314/files#r1601220154 still needs to be reverted. It's ok to add a workaround for Keycloak but I don't see a good enough reason to adjust what the server (aka RESTCatalogAdapter is sending back)

adutra · 2024-07-03T10:33:02Z

@adutra IMO https://github.com/apache/iceberg/pull/10314/files#r1601220154 still needs to be reverted. It's ok to add a workaround for Keycloak but I don't see a good enough reason to adjust what the server (aka RESTCatalogAdapter is sending back)

@nastra I'm sorry but I really don't agree. This has nothing to do with Keycloak. This is not a workaround. This has to do with complying with the OAuth 2.0 spec.

Have you read the 5.1 section of RFC 6749? Quoting here so that we are on the same page:

   The authorization server issues an access token and optional refresh
   token, and constructs the response by adding the following parameters
   to the entity-body of the HTTP response with a 200 (OK) status code:

   access_token
         REQUIRED.  The access token issued by the authorization server.

   token_type
         REQUIRED.  The type of the token issued as described in
         [Section 7.1](https://datatracker.ietf.org/doc/html/rfc6749#section-7.1).  Value is case insensitive.

   expires_in
         RECOMMENDED.  The lifetime in seconds of the access token.  For
         example, the value "3600" denotes that the access token will
         expire in one hour from the time the response was generated.
         If omitted, the authorization server SHOULD provide the
         expiration time via other means or document the default value.

   refresh_token
         OPTIONAL.  The refresh token, which can be used to obtain new
         access tokens using the same authorization grant as described
         in [Section 6](https://datatracker.ietf.org/doc/html/rfc6749#section-6).

   scope
         OPTIONAL, if identical to the scope requested by the client;
         otherwise, REQUIRED.  The scope of the access token as
         described by [Section 3.3](https://datatracker.ietf.org/doc/html/rfc6749#section-3.3).

As you can see there is no issued_token_type field. We therefore must comply with that, and be prepared to receive responses that do NOT contain that field.

A client that breaks because that field is missing is not a OAuth2-compliant client.

By returning an unspecified field for a client_credentials grant, RESTCatalogAdapter is also infringing the OAuth 2.0 spec.

nastra · 2024-07-03T10:58:20Z

@adutra I've read RFC 6749 but as I mentioned in #10314 (comment) I'm assuming that the token exchange is following RFC 8693 (and my assumption can be wrong). It would be good to get confirmation on that though.

adutra · 2024-07-03T11:24:12Z

@adutra I've read RFC 6749 but as I mentioned in #10314 (comment) I'm assuming that the token exchange is following RFC 8693 (and my assumption can be wrong). It would be good to get confirmation on that though.

Sure, but RFC 8693 is not applicable here. All depends on the grant type being used. If the grant type is urn:ietf:params:oauth:token-type:access_token then yes, RFC 8693 applies, and yes, there is an issued_token_type field in the response, and yes it is a required field.

But if the grant type is client_credentials then RFC 6749 applies instead, and the field is not part of it at all.

adutra · 2024-07-03T11:34:03Z

@rdblue since you originally authored this part (#4771 (comment)) would you mind having a look as well? Thank you!

snazy · 2024-07-03T11:36:41Z

@nastra Alex is 100% right. I do not understand why it takes so long to review this fix for an OAuth spec-compliance.

danielcweeks · 2024-07-03T16:33:10Z

I think the application of extensions referenced in RFC 8693 are a little ambiguous due to the following:

RFC 6749 section 4.1 references the response described in section 5.1

RFC 8693 describe the extensions to what is defined in the response to that same section to expand upon the format of what the access_token field contains. It states in the section 2.2.1:

The identifier access_token is used for historical reasons and the issued token need not be an OAuth access token.

I believe that the intent includes that a client credential exchange could return any of the enumerated token types defined in section 3 and applies.

I don't think it's explicitly clear either way, but I would interpret it as the latter.

Either way, I don't think it's a huge issue to default to ensure RFC 6749 compatibility, but we should just add a note as to why we're defaulting (e.g. "defaulting issued_token_type to access_token for compatibility with RFC 6749 where the issued type is omitted).

snazy · 2024-07-03T16:52:59Z

@nastra requested to revert @adutra 's fix for the client-credentials flow, only RFC 6749 applies here. This part of the code has nothing to do with RFC 8693 (token-exchange). The request to revert is IMO not justified, because the flow in question does not specify it.

adutra · 2024-07-03T17:31:21Z

I believe that the intent includes that a client credential exchange could return any of the enumerated token types defined in section 3 and applies.

RFC 8693 builds on top of RFC 6749, but does not modify any of its structs. And how could it be otherwise? An RFC cannot modify another one's structs without officially superseding it. Imho it is wrong to read section 2.2.1 of RFC 8693 as a general rewrite of RFC 6749 section 5.1, valid for all grants. The correct reading is: section 2.2.1 expands section 5.1 by adding extra context, in the scope of a token exchange grant only.

And anyways, that's how all public OAuth 2.0 servers interpret it: none of them include the field issued_token_type in a client_credentials grant response, even if they support RFC 8693.

TLDR is: if you want Iceberg REST to be interoperable with any public OAuth 2.0 server, this PR needs to be in.

danielcweeks · 2024-07-03T17:52:17Z

@adutra

An RFC cannot modify another one's structs without officially superseding it.
. . . RFC 8693 as a general rewrite of RFC 6749 section 5.1

I'm not suggesting that this is a general rewrite, but was more interpreting that RFC 8693 as an extension that can apply to any access_token grant type response.

. . . in the scope of a token exchange grant only.

I think this is the best argument and I agree it makes any argument for issued_token_type for other grant flows to be weak.

And anyways, that's how all public OAuth 2.0 servers interpret it: none of them include the field issued_token_type in a client_credentials grant response, even if they support RFC 8693.

Maybe you misinterpreted my message, but I actually support adding this. I couldn't find any examples and I also understand that RFC 8693 isn't widely adopted, so we may run into a number of issues with existing implementations out there.

core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java

Co-authored-by: Eduard Tudenhoefner <etudenhoefner@gmail.com>

core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java

adutra · 2024-07-04T12:14:09Z

Thank you to all involved! I'm very glad that we could reach a consensus here 🙏

github-actions bot added the core label May 11, 2024

nastra requested review from nastra and amogh-jahagirdar May 13, 2024 08:34

snazy approved these changes May 14, 2024

View reviewed changes

nastra reviewed May 15, 2024

View reviewed changes

adutra force-pushed the issued-token-type branch from e9cf46e to e6da4dd Compare June 4, 2024 10:44

nastra added this to the Iceberg 1.6.0 milestone Jun 25, 2024

adutra force-pushed the issued-token-type branch from e6da4dd to 7534331 Compare July 3, 2024 08:27

adutra force-pushed the issued-token-type branch from 7534331 to e4cf801 Compare July 3, 2024 08:31

nastra reviewed Jul 4, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java Outdated Show resolved Hide resolved

Update core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java

46f2aa1

Co-authored-by: Eduard Tudenhoefner <etudenhoefner@gmail.com>

nastra reviewed Jul 4, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java Outdated Show resolved Hide resolved

Fix comment

1a42e58

nastra approved these changes Jul 4, 2024

View reviewed changes

nastra changed the title ~~REST: assume issued token type is access token~~ Core: Assume issued_token_type is access_token to fully comply with RFC 6749 Jul 4, 2024

nastra merged commit 4048987 into apache:main Jul 4, 2024
49 checks passed

adutra deleted the issued-token-type branch July 4, 2024 12:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Assume issued_token_type is access_token to fully comply with RFC 6749 #10314

Core: Assume issued_token_type is access_token to fully comply with RFC 6749 #10314

adutra commented May 11, 2024

nastra May 15, 2024

nastra May 15, 2024 •

edited

Loading

adutra May 15, 2024

adutra May 15, 2024

adutra May 15, 2024

nastra May 15, 2024

adutra May 15, 2024

adutra commented May 27, 2024

nastra commented May 27, 2024

adutra commented Jun 4, 2024

adutra commented Jul 3, 2024

nastra commented Jul 3, 2024

adutra commented Jul 3, 2024

nastra commented Jul 3, 2024

adutra commented Jul 3, 2024

adutra commented Jul 3, 2024

snazy commented Jul 3, 2024

danielcweeks commented Jul 3, 2024

snazy commented Jul 3, 2024

adutra commented Jul 3, 2024

danielcweeks commented Jul 3, 2024

adutra commented Jul 4, 2024

Core: Assume issued_token_type is access_token to fully comply with RFC 6749 #10314

Core: Assume issued_token_type is access_token to fully comply with RFC 6749 #10314

Conversation

adutra commented May 11, 2024

nastra May 15, 2024

Choose a reason for hiding this comment

nastra May 15, 2024 • edited Loading

Choose a reason for hiding this comment

adutra May 15, 2024

Choose a reason for hiding this comment

adutra May 15, 2024

Choose a reason for hiding this comment

adutra May 15, 2024

Choose a reason for hiding this comment

nastra May 15, 2024

Choose a reason for hiding this comment

adutra May 15, 2024

Choose a reason for hiding this comment

adutra commented May 27, 2024

nastra commented May 27, 2024

adutra commented Jun 4, 2024

adutra commented Jul 3, 2024

nastra commented Jul 3, 2024

adutra commented Jul 3, 2024

nastra commented Jul 3, 2024

adutra commented Jul 3, 2024

adutra commented Jul 3, 2024

snazy commented Jul 3, 2024

danielcweeks commented Jul 3, 2024

snazy commented Jul 3, 2024

adutra commented Jul 3, 2024

danielcweeks commented Jul 3, 2024

adutra commented Jul 4, 2024

nastra May 15, 2024 •

edited

Loading