Do fewer ID token validations on the CLI ~~, and require HTTPS OIDC issuers in the CLI~~
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 739
- Forks
- 85
- Avg merge
- 5d 5h
- Merged PRs (30d)
- 1
Description
Is your feature request related to a problem? Please describe.
This issue spurred from this discussion: https://github.com/vmware-tanzu/pinniped/pull/917#discussion_r778526002
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 https://github.com/vmware-tanzu/pinniped/pull/1013. The parts about avoid calls to the JWKS and userinfo endpoint were not addressed by that PR.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the pinniped login oidc command and trace its use of ValidateTokenAndMergeWithUserInfo during supervisor token refresh. Check the remaining JWKS and userinfo calls, noting that HTTPS validation was split into issue #1012 and addressed by pull request #1013. Done means the CLI avoids those unnecessary endpoint calls without changing token refresh behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- authentication, cli
- Issue type
- Feature
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 42/100