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

Add URI OIDC type to support URI subjects #455

Merged
merged 2 commits into from
Mar 11, 2022

Conversation

haydentherapper
Copy link
Contributor

@haydentherapper haydentherapper commented Mar 4, 2022

Implementing the first part of #398, which adds support
for subjects in OIDC tokens that are URIs. The implementation
is very similar to SPIFFE-based tokens.

Tokens must conform to the following:

  • The issuer of the token must partially match the domain in the
    configuration. This means that the scheme, top level domain, and
    second level domain must match. It is also expected that we validate
    that the requester who adds the configuration for the issuer has
    control over both the issuer and domain configuration fields (ACME).
  • The domain of the configuration and hostname of the subject of the
    token must match exactly.

Slightly reworked the API test to test this issuer type. I'll
follow up in a later PR with some more refactoring around this
class, I think we can exercise the codepaths for all issuers.

Also planning to write documentation on the supported issuers.

Signed-off-by: Hayden Blauzvern [email protected]

Summary

Ticket Link

Ref #398

Release Note

Added OIDC issuer type for URI subjects

Implementing the first part of sigstore#398, which adds support
for subjects in OIDC tokens that are URIs. The implementation
is very similar to SPIFFE-based tokens.

Tokens must conform to the following:
* The issuer of the token must partially match the domain in the
  configuration. This means that the scheme, top level domain, and
  second level domain must match. It is also expected that we validate
  that the requester who adds the configuration for the issuer has
  control over both the issuer and domain configuration fields (ACME).
* The domain of the configuration and hostname of the subject of the
  token must match exactly.

Slightly reworked the API test to test this issuer type. I'll
follow up in a later PR with some more refactoring around this
class, I think we can exercise the codepaths for all issuers.

Signed-off-by: Hayden Blauzvern <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Mar 4, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@6a12159). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #455   +/-   ##
=======================================
  Coverage        ?   46.58%           
=======================================
  Files           ?       14           
  Lines           ?     1024           
  Branches        ?        0           
=======================================
  Hits            ?      477           
  Misses          ?      478           
  Partials        ?       69           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a12159...81979d5. Read the comment docs.

@dlorenc
Copy link
Member

dlorenc commented Mar 5, 2022

cc @znewman01

Copy link

@jchestershopify jchestershopify left a comment

Choose a reason for hiding this comment

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

LGTM. Just some nits, nothing I think of as blocking.

pkg/api/api_test.go Outdated Show resolved Hide resolved
pkg/challenges/challenges.go Outdated Show resolved Hide resolved
pkg/challenges/challenges.go Outdated Show resolved Hide resolved
pkg/challenges/challenges.go Outdated Show resolved Hide resolved
pkg/challenges/challenges.go Outdated Show resolved Hide resolved
pkg/challenges/challenges_test.go Outdated Show resolved Hide resolved
pkg/challenges/challenges_test.go Outdated Show resolved Hide resolved
Signed-off-by: Hayden Blauzvern <[email protected]>
@haydentherapper
Copy link
Contributor Author

Thanks for the comments @jchestershopify!

haydentherapper added a commit to haydentherapper/fulcio that referenced this pull request Mar 9, 2022
This implements the second part of sigstore#398, adding support for OIDC
subjects that are simply usernames. A configured domain will be appended
to the username and included as a SAN email address.

Like sigstore#455, token issuers must partially match the configured domain. The
top level and second level domain must match, and it's expected that we
validate ownership for what's configured in the issuer and domain
fields.

Signed-off-by: Hayden Blauzvern <[email protected]>
@dlorenc dlorenc merged commit d640505 into sigstore:main Mar 11, 2022
@haydentherapper haydentherapper deleted the oidc-uri branch March 14, 2022 17:37
haydentherapper added a commit to haydentherapper/fulcio that referenced this pull request Mar 14, 2022
This implements the second part of sigstore#398, adding support for OIDC
subjects that are simply usernames. A configured domain will be appended
to the username and included as a SAN email address.

Like sigstore#455, token issuers must partially match the configured domain. The
top level and second level domain must match, and it's expected that we
validate ownership for what's configured in the issuer and domain
fields.

Signed-off-by: Hayden Blauzvern <[email protected]>
haydentherapper added a commit to haydentherapper/fulcio that referenced this pull request Mar 14, 2022
This implements the second part of sigstore#398, adding support for OIDC
subjects that are simply usernames. A configured domain will be appended
to the username and included as a SAN email address.

Like sigstore#455, token issuers must partially match the configured domain. The
top level and second level domain must match, and it's expected that we
validate ownership for what's configured in the issuer and domain
fields.

Signed-off-by: Hayden Blauzvern <[email protected]>
haydentherapper added a commit to haydentherapper/fulcio that referenced this pull request Mar 14, 2022
This implements the second part of sigstore#398, adding support for OIDC
subjects that are simply usernames. A configured domain will be appended
to the username and included as a SAN email address.

Like sigstore#455, token issuers must partially match the configured domain. The
top level and second level domain must match, and it's expected that we
validate ownership for what's configured in the issuer and domain
fields.

Signed-off-by: Hayden Blauzvern <[email protected]>
dlorenc pushed a commit that referenced this pull request Mar 18, 2022
* Add Username scoped to domain OIDC type

This implements the second part of #398, adding support for OIDC
subjects that are simply usernames. A configured domain will be appended
to the username and included as a SAN email address.

Like #455, token issuers must partially match the configured domain. The
top level and second level domain must match, and it's expected that we
validate ownership for what's configured in the issuer and domain
fields.

Signed-off-by: Hayden Blauzvern <[email protected]>

* Refactor API tests

This refactor adds tests for all supported OIDC types, and makes it
simpler to add new tests for new OIDC types.

* Add tests for K8s and GitHub OIDC types.
* Add additional verification for issued certificate values
* Add dedicated test for RootCert success, don't call RootCert in every test.
* Move common expectations to function. This provides a single place to
  check response values.
* Move common set up to dedicated functions.
* Lowercase all error messages, because style.

Signed-off-by: Hayden Blauzvern <[email protected]>
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.

5 participants