-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
REST: assume issued token type is access token #10314
base: main
Are you sure you want to change the base?
Conversation
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably leave this as-is, since issued_token_type
is required according to RFC 8693
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
@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`.
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 theclient_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 theissued_token_type
field inclient_credentials
responses, thus masking many test failures, e.g. intestCatalogTokenRefresh
.