OIDC macaroon minting for GitHub by woodruffw · Pull Request #11272 · pypi/warehouse (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation166 Commits112 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This adds an API route for converting a JWT into a temporary API token.

~~Very WIP.~~

TODO:

Actually create the macaroon
Local testing
Fill in test coverage
Feature gate with the other OIDC functionality

~~Needs #11218.~~

Closes #10970.

Macaroons were previously strongly associated with individual users.

This was the correct association for user-minted tokens, but not generally. In particular, ODIC-minted tokens are associated with projects themselves and not the user who registered the OIDC provider.

This is ready for an initial review. I'll leave some comments flagging some things in particular.

Is including a link to/ID for the specific run practical? That'd give a greater degree of auditability.

Is including a link to/ID for the specific run practical? That'd give a greater degree of auditability.

Yeah, it should be (not with state on the OIDC provider itself, but with state from the JWT.) Let me see if I can figure out a way to pull that out.

Signed-off-by: William Woodruff william@trailofbits.com

Is including a link to/ID for the specific run practical? That'd give a greater degree of auditability.

Yeah, would be good to include this with the publishing event/journal, and we can figure out how to externalize it in a followup PR.

We can eventually use this to put more information in the token creation event, for nicer renderings.

Signed-off-by: William Woodruff william@trailofbits.com

Yeah, would be good to include this with the publishing event/journal, and we can figure out how to externalize it in a followup PR.

Did this with 8e3d5b1 -- nothing is exposed yet, but we now have access to the claims set that the verified JWT contained.

Signed-off-by: William Woodruff william@trailofbits.com

Tests not updated, yet.

Signed-off-by: William Woodruff william@trailofbits.com

This should be up-to-date and ready to go again.

Signed-off-by: William Woodruff william@trailofbits.com

di mentioned this pull request

Nov 11, 2022

di approved these changes Nov 15, 2022

Member

@di di left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Can you bring this up to date so we can merge? I don't have the ability to do so.

LGTM. Can you bring this up to date so we can merge? I don't have the ability to do so.

Done. I also need to rebase the migration, one moment...

Signed-off-by: William Woodruff william@trailofbits.com

Okay, should be good to go.