vmware / vmware/pinniped

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

Open
#941 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

enhancement estimate/L priority/undecided
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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.