REST: assume issued token type is access token #10314
Conversation
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`.
| @@ -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 | |||
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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_credentialsto 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
subjectTokenTypeis null.
There was a problem hiding this comment.
@nastra also see this comment which gives some context on why we have the problem today: #4771 (comment)
| @@ -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") | |||
There was a problem hiding this comment.
I would probably leave this as-is, since issued_token_type is required according to RFC 8693
There was a problem hiding this comment.
I don't agree: this flow is not governed by RFC 8693, this is RFC 6749 section 4.4.
|
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: |
|
@adutra I'm OOO this week, maybe @amogh-jahagirdar gets a chance to look at this PR |
The REST client wrongly assumes that the
issued_token_typefield is present in all OAuth responses, but that isn't true: e.g. in theclient_credentialsflow, 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 theissued_token_typefield inclient_credentialsresponses, thus masking many test failures, e.g. intestCatalogTokenRefresh.