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

Improve compliance with OIDC spec #823

Merged
merged 7 commits into from
Dec 2, 2023
Merged

Improve compliance with OIDC spec #823

merged 7 commits into from
Dec 2, 2023

Conversation

stephank
Copy link
Member

@stephank stephank commented Nov 29, 2023

This PR improves our score on OIDC self-certification for oidcc-basic-certification-test-plan. Full compliance may be outside the scope of the broker, up for discussion. IMO, it seems like we'd have to add a bunch of complexity for little benefit.

Where we still fail:

  • We don't implement userinfo, and the entire test suite depends on it, despite it being optional. Fundamental is that we don't have access tokens at all, so we'd have to add storage for those if we want to do that. I've been cheating the test by doing 'access token = email', but that doesn't sound smart for production. It gives an illusion our 'access tokens' / sessions are permanently valid (and there are other tests in the test suite we'd fail any way.)
  • We partially fail oidcc-prompt-none-logged-in. We forward prompt=none when bridging to OIDC, so it should work in those cases, but it'll never work for the email loop, because the broker doesn't have login sessions.
  • We probably fail oidcc-max-age-10000 even for OIDC bridging, because max_age is not forwarded. (But I only tested the email loop in general.)
  • We fail oidcc-id-token-hint, because we don't implement id_token_hint at all. I considered returning the hint as-is to the RP if it was still valid, but that idea ran into a wall. I think we could still do this by issuing a new token with the same exp, but we'd have to track it in a number of places.
  • We fail oidcc-ensure-registered-redirect-uri and oidcc-ensure-request-object-with-redirect-uri, because we allow any redirect URI in the client origin. The test suite expects specific redirect URIs that were registered only, but we have no registration.
  • We fail oidcc-server-client-secret-post because we have no registration and thus no client secret.
  • We fail oidcc-refresh-token because we don't have access tokens or refresh tokens.

Ensures we always return state, also for errors
The nonce protects against reuseing an id_token twice with the same RP.
With the authorization code flow, this is not necessary, because the
RP has to exchange the code at the broker, and the code is already
single-use.
@stephank stephank merged commit 82e2de4 into main Dec 2, 2023
5 checks passed
@stephank stephank deleted the feat/compliance branch December 2, 2023 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant