Stream: inferno
Topic: Is client id optional on a refresh request?
Lee Surprenant (Aug 19 2020 at 13:27):
Our authorization server (based on KeyCloak) requires a client_id to be passed in the refresh token exchange, but (for public clients) inferno is only sending grant_type and refresh_token. Is client_id truly optional on this exchange?
Robert Scanlon (Aug 19 2020 at 13:30):
Are you looking at the 'Program' tests or the 'Community Edition' tests? (should be the same here, but just to be sure I'm looking in the right spot)
Lee Surprenant (Aug 19 2020 at 13:40):
this is the Community Edition "Token Refresh" test
Robert Scanlon (Aug 19 2020 at 13:41):
Right... Program Edition doesn't test refresh tokens on public clients.
Robert Scanlon (Aug 19 2020 at 13:44):
If we are just going by the spec, it doesn't appear to be something you pass for public clients, right? Neither SMART (http://www.hl7.org/fhir/smart-app-launch/#step-5-later-app-uses-a-refresh-token-to-obtain-a-new-access-token) nor OAuth (https://tools.ietf.org/html/rfc6749#section-6) mention passing that field.
Lee Surprenant (Aug 19 2020 at 13:49):
thanks, that is the info I was looking for...need to see if I can configure KeyCloak to do the right thing here :-)
Robert Scanlon (Aug 19 2020 at 13:50):
Note that refresh tokens on public clients are a tricky subject, because tokens are easier to steal on public clients, and it more damaging to have a refresh token stolen, so oauth servers may add extensions (for lack of better word) to help better secure the token exchange process for public clients that want refresh tokens.
Lee Surprenant (Aug 19 2020 at 13:55):
thanks Robert, I have definitely read that, but I saw the test and thought I'd give it a try. glad to hear its not tested in program edition (so as not to encourage its use)
Lee Surprenant (Aug 19 2020 at 13:59):
I've got one other similar (seemingly minor) thing I need to figure out with our keycloak and that is how to get it to echo the allowed scopes as part of the dynamic client registration:
Registration response did not include client_id and scope fields in JSON body
Lee Surprenant (Aug 19 2020 at 13:59):
just thought i'd leave it here in case any keycloak users are lurking :-)
Lee Surprenant (Aug 19 2020 at 14:05):
on this one, https://tools.ietf.org/html/rfc7591#section-3.2.1 says:
Additionally, the authorization server MUST return all registered metadata about this client, including any fields provisioned by the authorization server itself.
Is that the language that led to the assumption a DCR response should include scopes?
Robert Scanlon (Aug 19 2020 at 14:10):
Right, though we aren't making a statement about its usefulness, just that we can't really test it. Refresh tokens on public clients are useful.
Robert Scanlon (Aug 19 2020 at 14:20):
If we are registering an app with a scope, that line seems to make it clear that it needs to provide it back. But I'm not sure a scope technically needs to be registered with an app for the SMART App Launch flows (just in practice it seems everyone does). Does keycloak save that information with the registration, or does it just ignore the scope we tried to register with?
Robert Scanlon (Aug 19 2020 at 14:22):
Dynamic registration is something that probably should be profiled within SMART from my perspective, because from what I recall there is some optionality, and we may make a couple of assumptions to align things with the requirements of SMART that aren't quite hard requirements.
Robert Scanlon (Aug 19 2020 at 14:25):
Those tests are definitely less mature than other tests, so if after looking at it you think we made a mistake here let us know.
Josh Mandel (Aug 19 2020 at 14:27):
Re: testing the use of public clients with refresh tokens, certainly this is something that smart and specifies how to do, so it seems like it should be testable. (CARIN alliance and others I've been in discussion with once to try to clarify the language in the rules in alignment with the existing implementation guide.)
Josh Mandel (Aug 19 2020 at 14:28):
Re: profiling on companion specs -- yes, this is something we are starting to look at in the Argonaut project this year for SMART IG 2.0. Our focus right now has been on profiling token introspection because that is the place that seems most consequential -- and actually required rather than just recommended by the onc rule.
Robert Scanlon (Aug 19 2020 at 14:41):
Thanks @Josh Mandel . We can test public clients and refresh tokens, but if a system requires the use of PKCE (denying refresh tokens to clients that don't use it), we will fail that system because we expect to get a refresh token. Is that system wrong for denying refresh tokens to public clients without additional protections, or are we wrong for failing that system? Or am I misunderstanding how these fit together?
Robert Scanlon (Aug 19 2020 at 14:52):
(and by 'we' i mean Inferno Community Edition tests in this case, separate of rule language clarification)
Josh Mandel (Aug 19 2020 at 15:10):
I think for the moment, testing public clients with refresh tokens (sans PKCE) is a good set of tests to have in place; adding testing with PKCE might be worthwhile too, but we don't have specific profiling/recommendations on that front.
Lee Surprenant (Oct 20 2020 at 20:25):
@Robert Scanlon I've been meaning to follow up with you on this one for a while now. I posed our question wrt client id on a refresh token to the keycloak community at https://keycloak.discourse.group/t/is-client-id-always-required-to-refresh-a-token/4378 and, eventually, got this answer from a colleague at IBM:
Although client_id requirement for public client is not explicitly specified in oauth 2.0 spec, the Openid Connect spec seem indicates client_id is required to refresh a token regardless of confidential or public clients. See https://openid.net/specs/openid-connect-core-1_0.html#RefreshingAccessToken 1. It states “the Client MUST authenticate to the Token Endpoint using the authentication method registered for its client_id”, which implies client_id is required.
Lee Surprenant (Oct 20 2020 at 20:26):
I separately followed up with a colleague from RedHat and received a similar response:
From my understanding client_id is required even for public clients. I would raise this with the people behind the conformance test tool. If they object we can certainly ask on the OIDC/OAuth mailing groups if they can provide some clarification.
Lee Surprenant (Oct 20 2020 at 20:30):
Is this enough info to get this changed in inferno or would you suggest we pursue further clarification on the OIDC mailing list?
Josh Mandel (Oct 22 2020 at 21:35):
This seems like a SMART spec issue too -- right now we say for grant_type=authorization_code, client_id is a required body param for public clients (and a required header param, as part of the Authorize header value, for confidential clients). But for grant_type=refresh_token, we don't list or require client_id as a body param.
Josh Mandel (Oct 22 2020 at 21:35):
The assessment at https://keycloak.discourse.group/t/is-client-id-always-required-to-refresh-a-token/4378/3 that somehow OIDC "implies client_id is required" is unconvincing, on my reading.
Robert Scanlon (Oct 23 2020 at 15:44):
I was looking through the history of our test and as of February of this year we did pass the client_id body param for grant_type=refresh_token for public clients. But we explicitly removed it due to the SMART spec (we referenced this section in the PR). We also removed a test where we checked if you pass an invalid client_id the system did not return a refresh token.
Robert Scanlon (Oct 23 2020 at 15:58):
Is this enough info to get this changed in inferno or would you suggest we pursue further clarification on the OIDC mailing list?
Since we are testing to the SMART profile, and that is what drove our decision to not pass that body param, I think we need official direction from SMART before the Inferno test changes. As written, it seems very clear to NOT pass that param. But perhaps there is some wiggle-room I'm missing?
Robert Scanlon (Oct 23 2020 at 16:01):
As a general data point through, I think it is interesting that we initially implemented it to have that parameter and then had to go back in to fix our mistake to be consistent with SMART. That may be common (or maybe not, it is just one data point).
Last updated: Apr 12 2022 at 19:14 UTC
