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

oidc.md: Add section for how to select SANs. #1127

Merged
merged 2 commits into from
Apr 20, 2023

Conversation

wlynch
Copy link
Member

@wlynch wlynch commented Apr 17, 2023

Summary

This adds some advice for how to choose SANs and some of the considerations.

Release Note

NONE

Documentation

@wlynch
Copy link
Member Author

wlynch commented Apr 17, 2023

cc @haydentherapper

@haydentherapper
Copy link
Contributor

Could you undo the formatting changes? Just makes it a bit easier to review

This adds some advice for how to choose SANs and some of the
considerations.

Signed-off-by: Billy Lynch <billy@chainguard.dev>
@wlynch
Copy link
Member Author

wlynch commented Apr 17, 2023

Sorry about that. Done!

@codecov
Copy link

codecov bot commented Apr 17, 2023

Codecov Report

Merging #1127 (ee01849) into main (6a1dfb2) will increase coverage by 0.28%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1127      +/-   ##
==========================================
+ Coverage   55.73%   56.01%   +0.28%     
==========================================
  Files          48       48              
  Lines        2783     2783              
==========================================
+ Hits         1551     1559       +8     
+ Misses       1101     1095       -6     
+ Partials      131      129       -2     

see 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

Just some nits, this is really great!

docs/oidc.md Outdated Show resolved Hide resolved
- Is the identifier well-defined?

All SANs for a provider should be defined and documented. If an issuer has the
ability to produce different SANs, differences and conditions for these SANs
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean by this section? When would an issue produce different SANs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Trying to cover bases here 😅

GitLab's issuer serves both users and CI/CD, but will only populate certain claims for each. (they do want to stand up another CI-only issuer, but that's down the road).

I wouldn't be surprised if we see this kind of behavior in a few issuers. I don't think this is necessarily a problem so long as we can clearly distinguish different types of users.

So if we eventually want to support gitlab.com user JWTs for email SANs as well, it's doable so long as both us and GitLab are clear on when a token would be a CI token vs a user token.

docs/oidc.md Outdated Show resolved Hide resolved
docs/oidc.md Outdated Show resolved Hide resolved
docs/oidc.md Outdated Show resolved Hide resolved
docs/oidc.md Show resolved Hide resolved
docs/oidc.md Outdated Show resolved Hide resolved
Signed-off-by: Billy Lynch <billy@chainguard.dev>
Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

Nice work!

@haydentherapper haydentherapper merged commit bcbcc68 into sigstore:main Apr 20, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants