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

internal/oidc, test: ambient credentials, refactoring #59

Merged
merged 4 commits into from
Apr 30, 2022

Conversation

woodruffw
Copy link
Member

Signed-off-by: William Woodruff [email protected]

Summary

Refactors sigstore._internal.oidc into a structured (directory) module and adds sigstore._internal.oidc.ambient for ambient credential detection. See #31.

Ticket Link

See #31. Does not close, since other detectors are needed.

@woodruffw woodruffw added enhancement New feature or request component:signing Core signing functionality labels Apr 30, 2022
@woodruffw woodruffw added this to the Stable release (1.0) milestone Apr 30, 2022
@woodruffw woodruffw requested review from di and tetsuo-cpp April 30, 2022 00:27
@woodruffw woodruffw self-assigned this Apr 30, 2022
@woodruffw
Copy link
Member Author

N.B.: I also moved OAuth flow support into its own module (sigstore._internal.oidc.oauth) to keep the top-level relatively clean.

tetsuo-cpp
tetsuo-cpp previously approved these changes Apr 30, 2022
Copy link
Contributor

@tetsuo-cpp tetsuo-cpp left a comment

Choose a reason for hiding this comment

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

LGTM! I think it'd be good to get this in now since I have some changes that need to go on top of the oauth module.

@woodruffw I'll follow up with a change to amend the release workflow to use this.

Signed-off-by: William Woodruff <[email protected]>
@woodruffw
Copy link
Member Author

I'll follow up with a change to amend the release workflow to use this.

Sounds good!

pass


def detect_credential() -> Optional[str]:
Copy link
Member Author

Choose a reason for hiding this comment

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

We don't need to address this in this PR, but I think Optional[Identity] would be a better signature here -- that way we could assert our validity guarantees about the token (e.g. having the right audience and being issued by a party we know).

Thoughts @tetsuo-cpp?

Copy link
Member Author

Choose a reason for hiding this comment

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

(If we were to do this, we should also refactor our top-level sign API to take an Identity instead of constructing one internally.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think that's a good idea. I'm going to take a stab at #4 in the coming days so it might be good to tackle it then so we get the interfaces right.

@woodruffw woodruffw requested a review from tetsuo-cpp April 30, 2022 01:05
@tetsuo-cpp tetsuo-cpp merged commit 5960edf into main Apr 30, 2022
@tetsuo-cpp tetsuo-cpp deleted the ww/ambient-credential-detection branch April 30, 2022 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:signing Core signing functionality enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants