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

Do fewer ID token validations on the CLI ~~, and require HTTPS OIDC issuers in the CLI~~ #941

Open
margocrawf opened this issue Jan 10, 2022 · 0 comments
Labels
enhancement New feature or request estimate/L Estimated effort/complexity/risk is large priority/undecided Not yet prioritized

Comments

@margocrawf
Copy link
Contributor

margocrawf commented Jan 10, 2022

Is your feature request related to a problem? Please describe.

This issue spurred from this discussion: #917 (comment)

We use the same function (ValidateTokenAndMergeWithUserInfo) on the CLI when we refresh supervisor tokens as well as on the Supervisor when we refresh upstream OIDC tokens. Many of the validations are not necessary on the CLI, so we shouldn't do them.

It is also probably unnecessary to fetch user info and merge it with the ID token. The supervisor doesn't advertise a userinfo endpoint so it is getting skipped in that case. But when a different issuer is configured for the CLI, then the userinfo endpoint could be called unnecessarily.

The CLI is presumably fetching the issuer's JWKS endpoint to aid in ID token validation. This is also unnecessary, as long as we only allow HTTPS issuers.

The pinniped login oidc CLI command should have a new validation on the value of the --issuer. The command should error whenever the issuer value does not start with https://. The OIDC spec requires that the authcode flow requests happen via TLS. This is a breaking change for the CLI, but it was an oversight that we ever allowed HTTP issuers.

In our server-side code we are more paranoid and in addition to validating that the issuer URL itself is https, we actually call the discovery endpoint and also validate that the authorize/token/revocation endpoints are also all https. The CLI doesn't use the revocation endpoint, but it could do a similar validation for the authorize and token endpoints.

Note: The parts about validating that the issuer, authorize, and token URLs are HTTPS was split off into a separate issue (#1012) and fixed in #1013. The parts about avoid calls to the JWKS and userinfo endpoint were not addressed by that PR.

@pinniped-ci-bot pinniped-ci-bot added enhancement New feature or request priority/backlog Prioritized for an upcoming iteration bug Something isn't working and removed enhancement New feature or request labels Jan 13, 2022
@pinniped-ci-bot pinniped-ci-bot added enhancement New feature or request and removed bug Something isn't working labels Feb 3, 2022
@pinniped-ci-bot pinniped-ci-bot changed the title Do fewer ID token validations on the CLI Do fewer ID token validations on the CLI, and require HTTPS OIDC issuers in the CLI Feb 3, 2022
@pinniped-ci-bot pinniped-ci-bot added the estimate/L Estimated effort/complexity/risk is large label Feb 3, 2022
@pinniped-ci-bot pinniped-ci-bot added the state/started Someone is working on it currently label Feb 3, 2022
@pinniped-ci-bot pinniped-ci-bot removed the state/started Someone is working on it currently label Feb 16, 2022
@pinniped-ci-bot pinniped-ci-bot added priority/undecided Not yet prioritized and removed priority/backlog Prioritized for an upcoming iteration labels Feb 16, 2022
@pinniped-ci-bot pinniped-ci-bot changed the title Do fewer ID token validations on the CLI, and require HTTPS OIDC issuers in the CLI Do fewer ID token validations on the CLI ~~, and require HTTPS OIDC issuers in the CLI~~ Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request estimate/L Estimated effort/complexity/risk is large priority/undecided Not yet prioritized
Projects
Status: No status
Development

No branches or pull requests

3 participants