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

Trusted publishing: ensure aud _only_ contains our audience. #13887

Closed
di opened this issue Jun 7, 2023 · 3 comments · Fixed by #14158
Closed

Trusted publishing: ensure aud _only_ contains our audience. #13887

di opened this issue Jun 7, 2023 · 3 comments · Fixed by #14158
Assignees
Labels
security Security-related issues and pull requests trusted-publishing

Comments

@di
Copy link
Member

di commented Jun 7, 2023

This was raised as a result of CircleCI's OIDC implementation and related discussion here: sigstore/fulcio#591 (comment).

Currently we accept OIDC tokens for which the aud claim contains our required audiences (pypi or testpypi respectively), however this doesn't preclude the aud claim from containing other audiences, because PyJWT will verify any token that simply contains the requested audience: https://github.com/jpadilla/pyjwt/blob/master/jwt/api_jwt.py#L335-L336.

The potential issue is that for a provider like CircleCI, the OIDC token could be shared across multiple 3rd party services with multiple audiences. This means that a CircleCI use that wants to use a token for both aud=pypi and aud=some_other_provider only gets a single token that would be valid for either audience, which means the other provider technically gains the ability to publish to PyPI as a result.

To mitigate this, we should restrict our verification of OIDC tokens to those that only specify our audience for the aud claim.

(While we're at it, we should also update the tests around our JWT verification to do less mocking/stubbing of the 3P library we're using here)

@woodruffw
Copy link
Member

xref jpadilla/pyjwt#894 for a potential upstream API change that would enable this.

@woodruffw
Copy link
Member

Upstream change: jpadilla/pyjwt#902

Once that's accepted/in a release, I can tackle the fix here.

@woodruffw woodruffw self-assigned this Jul 17, 2023
@woodruffw
Copy link
Member

pyjwt 2.8.0 includes the change we need, so I'll do this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Security-related issues and pull requests trusted-publishing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants