Skip to content
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

OIDC Dynamic Client Registration expiry hinders OIDC token refresh #142

Closed
felix-schwarz opened this issue Feb 2, 2021 · 9 comments
Closed

Comments

@felix-schwarz
Copy link

Summary

The ownCloud Android, Desktop and iOS clients are running into an issue with Kopano's OIDC implementation if:

  • the refresh_token was retrieved using a dynamically registered client ID
  • the dynamically registered client ID has expired before the refresh_token was used

Tested cases

When sending the refresh_token together with the client_id/ client_secret with which is was retrieved, before the client_id has expired: all is fine.

When sending the refresh_token together with the client_id/ client_secret with which is was retrieved, but after the client_id has expired:

{
  "error": "access_denied",
  "error_description": "unknown client_id: dyn.eyJhbGciOiJQ…"
}

When sending the refresh_token together with a freshly issued client_id/ client_secret at the time of sending the refresh request to the token endpoint:

{
  "error": "invalid_grant",
  "error_description": "client_id mismatch"
}

Can you help @longsleep?

@longsleep
Copy link
Collaborator

Well @felix-schwarz - Don't use the client_id anymore, when its secret is expired. Since the refresh token is bound to the client_id value, also means you cannot use the refresh token once the client_id is expires.

Having that said, and that the dynamic client secrets are only valid for 1 hour (as issued by Kopano Konnect), I can see that this is a problem.

@felix-schwarz
Copy link
Author

@longsleep Thanks for the clarification. I took another look into the specification and as you wrote above the refresh_token's usage is bound to the client_id it was issued to:

The Authorization Server MUST validate the Refresh Token, MUST verify that it was issued to the Client, and must verify that the Client successfully authenticated it has a Client Authentication method.

I couldn't find any explicit information in the Dynamic Client Registration spec as to whether a client_id may still be used for token refreshs.

So I'm not sure if it would be in line with the standard to allow the usage of expired client_ids for token refresh requests. I can't see any other way how Dynamic Client Registration and token refreshs could play well together for native clients, though.

@longsleep
Copy link
Collaborator

@felix-schwarz the only way i see which works would be non-expiring dynamic client registration. This is currently not possible with Kopano Konnect, but could be easily changed.

I checked a bit around, and others also seem to be sending back 0 as client_secret_expires_at which seems to mean "does not expire". Not sure what the security implications are, but its certainly possible.

@felix-schwarz
Copy link
Author

@longsleep Thanks for sharing that! In the specs, nothing pointed to 0 as a possible/plausible value and the implementation in the iOS client would have seen that as expiration on the first minute of the year 1970. 😅

Regarding security, the dynamic client_id - in this context - imposes a de facto hard limit on the maximum duration of a "session": if the client_id expires, the refresh_token is no longer usable, so the next refresh will fail and require a new login by the user.

A non-expiring dynamic client_id would mean that a "session" can stay open and alive for any period of time, whereas an expiring client_id limits a session to the expiration of the client_id + the remaining lifetime of the current access_token at the time the client_id expires.

So I think a good solution might be to allow admins to configure the timeout so that it meets their use case. A 0 would give them the same behaviour as previously with hard-coded client_id / client_secret pairs - and any other value would control how frequently users need to fully re-authenticate: once an hour / day / week / month / year - they all seem reasonable to me from an admin's point of view.

@longsleep
Copy link
Collaborator

longsleep commented Feb 3, 2021

@longsleep Thanks for sharing that! In the specs, nothing pointed to 0 as a possible/plausible value and the implementation in the iOS client would have seen that as expiration on the first minute of the year 1970.

@felix-schwarz the 0 value is part of the spec (see https://openid.net/specs/openid-connect-registration-1_0.html#RegistrationResponse). So treating it as 1970 is certainly an implementation bug.

A non-expiring dynamic client_id would mean that a "session" can stay open and alive for any period of time, whereas an expiring client_id limits a session to the expiration of the client_id + the remaining lifetime of the current access_token at the time the client_id expires.

It cannot stay open for any period of time since it is bound to the validity of the refresh token (which usually have some very long expiration like 3 years or so as it is by default with Kopano Konnect https://github.com/Kopano-dev/konnect/blob/master/cmd/konnectd/serve.go#L98).

I don't see a problem by simply giving avay dynamic client secrets which do not expire (by default) - i will play around with that a bit and change this in Konnect as dynamic client ids with 1 hour duration does not seem to make that much of sense.

@DeepDiver1975
Copy link
Member

I am closing this in here - there is not much we can do within our openidconnect app

@felix-schwarz
Copy link
Author

@longsleep After @mmattel brought up this issue with me again, I did a quick research in the konnect code and ended up at this line: https://github.com/Kopano-dev/konnect/blob/master/cmd/konnectd/serve.go#L99

That line suggests to me that konnect now returns 0 for client_secret_expires_at by default. Is that correct?

@longsleep
Copy link
Collaborator

That line suggests to me that konnect now returns 0 for client_secret_expires_at by default. Is that correct?

Yes, see libregraph/lico@08b5f4f for the full change and further expaination. That change got released with Konnect v0.34.0 about a year ago.

@felix-schwarz
Copy link
Author

Thanks for the confirmation and link to the commit @longsleep !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants