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

If custom server url exists, use that instead of the default one. #1776

Merged
merged 2 commits into from
Aug 19, 2024

Conversation

kommendorkapten
Copy link
Member

Summary

When producing certificates from ID Tokens issued by GitHub Actions, the server url is always hardcoded to github.com. This is not true for all cases, as custom domains may be possible. This PR modifies the behaviour to look for the enterprise claim which indicates that a custom domain is used.

Release Note

  • When issuing certificates for GitHub Actions jobs, the server url is now properly created if custom domains are used.

Documentation

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>
Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>
Copy link

codecov bot commented Aug 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.35%. Comparing base (cf238ac) to head (4a3edea).
Report is 177 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1776      +/-   ##
==========================================
- Coverage   57.93%   50.35%   -7.58%     
==========================================
  Files          50       70      +20     
  Lines        3119     4184    +1065     
==========================================
+ Hits         1807     2107     +300     
- Misses       1154     1844     +690     
- Partials      158      233      +75     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@trevrosen trevrosen left a comment

Choose a reason for hiding this comment

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

:shipit:

@trevrosen trevrosen merged commit bc852fd into sigstore:main Aug 19, 2024
13 checks passed
@@ -159,10 +161,16 @@ func WorkflowPrincipalFromIDToken(_ context.Context, token *oidc.IDToken) (ident
return nil, errors.New("missing run_attempt claim in ID token")
}

baseURL := `https://github.com/`

if claims.Enterprise != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

@javanlacerda Is this something we can add for the new configuration?

@kommendorkapten Is this something you want enabled for the public instance or is this just for the GitHub deployment?

Copy link
Contributor

@javanlacerda javanlacerda Aug 19, 2024

Choose a reason for hiding this comment

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

Yes, I think this template {{if .enterprise }}https://{{ .enterprise }.ghe.com{{else}}{{ .url }}{{end}} should replace every place that the default {{url}} is used for github on identity/config.yaml.
Also, It could be defined in a variable for avoiding replicating it into the config.

And btw, I saw that this PR was recently merged, but this modifies will not work as we migrated github to use the new generic logic that is in pkg/identity/ciprovider/principal.go

Copy link
Member Author

Choose a reason for hiding this comment

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

@haydentherapper this is only needed for our internal deployments as of now.

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.

4 participants