Do fewer ID token validations on the CLI ~~, and require HTTPS OIDC issuers in the CLI~~ #941
Labels
enhancement
New feature or request
estimate/L
Estimated effort/complexity/risk is large
priority/undecided
Not yet prioritized
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 withhttps://
. 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.
The text was updated successfully, but these errors were encountered: