add generate-key-pair GitHub Enterprise server support #2676
Conversation
|
please sign the DCO |
Fixes #2675 Signed-off-by: Christian Loos <cloos@netsandbox.de>
| @@ -51,9 +51,19 @@ func (g *Gh) PutSecret(ctx context.Context, ref string, pf cosign.PassFunc) erro | |||
| ) | |||
| httpClient = oauth2.NewClient(ctx, ts) | |||
| } else { | |||
| return fmt.Errorf("could not find %q environment variable", env.VariableGitHubRequestToken.String()) | |||
| return fmt.Errorf("could not find %q environment variable", env.VariableGitHubToken.String()) | |||
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #2676 +/- ##
=======================================
Coverage 30.00% 30.00%
=======================================
Files 146 146
Lines 9299 9299
=======================================
Hits 2790 2790
Misses 6077 6077
Partials 432 432
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| @@ -57,6 +57,7 @@ const ( | |||
| VariableSigstoreRekorPublicKey Variable = "SIGSTORE_REKOR_PUBLIC_KEY" | |||
|
|
|||
| // Other external environment variables | |||
| VariableGitHubHost Variable = "GITHUB_HOST" | |||
There was a problem hiding this comment.
Just want to check that this is a standard env var recognized by e.g. official GitHub tooling. We're trying to define as few env vars that don't begin with COSIGN_ as possible 🙂 The other GitHub env vars here are populated by GH Actions.
A cursory Google search doesn't reveal anything obvious but I could be missing something.
Otherwise, can you rename COSIGN_GITHUB_HOST and mark External: false?
There was a problem hiding this comment.
As mentioned in #2675 (comment) I choose GITHUB_HOST here to make it similar to the existing GITLAB_HOST environment variable.
Also as GITHUB_TOKEN is already defined, the common prefix would make users aware that these two environment variables belong together.
We don't have a dependency to any GitHub tooling here.
But if you mean to reuse an environment variable that is also used in GitHub tooling, the GH_HOST environment variable from GitHub CLI would be the right one (also mentioned in #2675 (comment)).
Or do you mean that someone will use cosign generate-key-pair github: ... in a GitHub action?
Then GITHUB_SERVER_URL will be the right one, because GitHub actions runner (Cloud and self-hosted) expose this environment variable (see https://docs.github.com/en/actions/learn-github-actions/variables#default-environment-variables).
There was a problem hiding this comment.
Thanks, that clarifies a lot. (I missed your earlier comment, sorry.)
Strictly speaking, External is supposed to mean "this is provided/specified/set by an external tool that we have no control over, and therefore we're going to exempt it from the COSIGN_ naming convention.
That said, following this rule is less important than a good experience for users, so we can easily make an exception.
I choose
GITHUB_HOSThere to make it similar to the existingGITLAB_HOSTenvironment variable.
To be honest, I think consistency between CI/CD providers is less useful than consistency within CI/CD providers.
Also as
GITHUB_TOKENis already defined, the common prefix would make users aware that these two environment variables belong together.But if you mean to reuse an environment variable that is also used in GitHub tooling, the
GH_HOSTenvironment variable from GitHub CLI would be the right one (also mentioned in #2675 (comment)).
Annoying that the "official" (GitHub CLI) way to set this is inconsistent between GH_HOST and GITHUB_TOKEN (I assume historical accident).
Or do you mean that someone will use
cosign generate-key-pair github: ...in a GitHub action?
ThenGITHUB_SERVER_URLwill be the right one, because GitHub actions runner (Cloud and self-hosted) expose this environment variable (see https://docs.github.com/en/actions/learn-github-actions/variables#default-environment-variables).
That's the reason for e.g. ACTIONS_ID_TOKEN_REQUEST_TOKEN.
The "maximalist" approach here is probably to do all of the above, with a well-defined priority order (this order is up for debate):
GH_HOSTGITHUB_HOSTGITHUB_SERVER_URL
Then, all are marked External: true. This would support whatever methods users are already using to set these variables.
I'd be okay with merging what you have for now and doing this in a follow-up, so I'll approve this PR
There was a problem hiding this comment.
(Also, let me know if that "maximalist approach" generally makes sense to you)
There was a problem hiding this comment.
I'm not sure if the "maximalist approach" makes sense here, but as you sad, we can create a follow-up issue where we gather opinions regarding this.
Fixes sigstore#2675 Signed-off-by: Christian Loos <cloos@netsandbox.de>
Summary
GitHub Enterprise Server (GHES) support for 'cosign generate-key-pair github:'
Fixes #2675
Release Note
GitHub Enterprise Server (GHES) support for 'cosign generate-key-pair github:'
Documentation