[go] [generic] add pull request support without signing #1238
Conversation
Signed-off-by: Asra Ali <asraa@google.com>
asraa
requested review from
ianlewis,
laurentsimon and
joshuagl
as code owners
There was a problem hiding this comment.
LGTM. I agree with you it'd be nice for users to specify an input acknowledging they want to opt-in for dry run. This will force them to read the doc that explains that on triggers with no OIDC clients, they should not expect signed provenance. Otherwise I suspect some user will miss it and complain?
if we don't want to commit to a new input argument, we could start by having this clearly in the documentation and re-visit in a few months.
| @@ -70,6 +70,11 @@ func GenerateProvenance(name, digest, command, envs, workingDir string, s signin | |||
| return nil, err | |||
| } | |||
|
|
|||
| // If the environment does not have access to an OIDC provider, use a nil one. | |||
| if provider == nil && !github.HasOIDCClient() { | |||
There was a problem hiding this comment.
are there cases where provider is not nil and has not OIDC client? Existing code suggest no.
There was a problem hiding this comment.
Yeah, I think unit tests just pass the NilClientProvider so that they could be run locally. Unit tests could just pass nil after this change.
| @@ -80,29 +85,19 @@ run in the context of a Github Actions workflow.`, | |||
| } | |||
| if provider != nil { | |||
There was a problem hiding this comment.
Can we remove the condition if provider != nil?
There was a problem hiding this comment.
provider could still be nil if github.HasOIDCClient returns true. In that case we don't call WithClients so DefaultClientProvider is used.
| @@ -234,7 +234,7 @@ func TestToken(t *testing.T) { | |||
| token, err := c.Token(context.Background(), tc.audience) | |||
| if err != nil { | |||
| if tc.err != nil { | |||
| if !errors.As(err, &tc.err) { | |||
| if !errors.As(err, tc.err) { | |||
There was a problem hiding this comment.
Revert this for now.
Make sure you're not compiling locally with Go 1.19. We have some fixes for this in #970
| @@ -80,29 +85,19 @@ run in the context of a Github Actions workflow.`, | |||
| } | |||
| if provider != nil { | |||
There was a problem hiding this comment.
provider could still be nil if github.HasOIDCClient returns true. In that case we don't call WithClients so DefaultClientProvider is used.
| @@ -70,6 +70,11 @@ func GenerateProvenance(name, digest, command, envs, workingDir string, s signin | |||
| return nil, err | |||
| } | |||
|
|
|||
| // If the environment does not have access to an OIDC provider, use a nil one. | |||
| if provider == nil && !github.HasOIDCClient() { | |||
There was a problem hiding this comment.
Yeah, I think unit tests just pass the NilClientProvider so that they could be run locally. Unit tests could just pass nil after this change.
| attBytes, err := pkg.GenerateProvenance(subject, digest, | ||
| commands, envs, workingDir, s, r, nil) | ||
| commands, envs, workingDir, s, r, provider) |
There was a problem hiding this comment.
I think you handle nil provider properly in GenerateProvenance so you shouldn't need to change this file?
|
Updates #131 |
|
Updates #358 |
|
@asraa This is pretty old. Should we close it? |
I think so. If we achieve this the better route to go is to refactor them to use the signing actions |
Signed-off-by: Asra Ali asraa@google.com
IsPresubmitTestnot necessary. Is that OK? I'm not sure if we want to continue gating on that, or maybe we want to add an explicit flag to users like "snapshot" that is required to allow pull_requests to generate the predicate (that way users don't accidentally generate unsigned provenance)