diff --git a/.github/workflows/pre-submit.lint.yml b/.github/workflows/pre-submit.lint.yml index 8596daf58..740a0cc98 100644 --- a/.github/workflows/pre-submit.lint.yml +++ b/.github/workflows/pre-submit.lint.yml @@ -15,8 +15,8 @@ jobs: with: go-version: "1.18" - env: - GOLANGCI_LINT_VERSION: "1.46.2" - GOLANGCI_LINT_CHECKSUM: "242cd4f2d6ac0556e315192e8555784d13da5d1874e51304711570769c4f2b9b" + GOLANGCI_LINT_VERSION: "1.52.2" + GOLANGCI_LINT_CHECKSUM: "c9cf72d12058a131746edd409ed94ccd578fbd178899d1ed41ceae3ce5f54501" run: | set -euo pipefail diff --git a/.golangci.yml b/.golangci.yml index 8480a9dc7..5c11d7a65 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -8,6 +8,9 @@ issues: - EXC0013 - EXC0014 - EXC0015 + exclude: + # TODO(#589): Remove after updating to Go 1.20 + - "non-wrapping format verb for fmt.Errorf. Use `%w` to format errors" # Maximum issues count per one linter. # Set to 0 to disable. # Default: 50 diff --git a/verifiers/internal/gha/builder.go b/verifiers/internal/gha/builder.go index 8d0fa815e..fa3f89059 100644 --- a/verifiers/internal/gha/builder.go +++ b/verifiers/internal/gha/builder.go @@ -67,39 +67,39 @@ func VerifyCertficateSourceRepository(id *WorkflowIdentity, func VerifyBuilderIdentity(id *WorkflowIdentity, builderOpts *options.BuilderOpts, defaultBuilders map[string]bool, -) (*utils.TrustedBuilderID, error) { +) (*utils.TrustedBuilderID, bool, error) { // Issuer verification. // NOTE: this is necessary before we do any further verification. if id.Issuer != certOidcIssuer { - return nil, fmt.Errorf("%w: %s", serrors.ErrorInvalidOIDCIssuer, id.Issuer) + return nil, false, fmt.Errorf("%w: %s", serrors.ErrorInvalidOIDCIssuer, id.Issuer) } // cert URI path is /org/repo/path/to/workflow@ref workflowPath := strings.SplitN(id.SubjectWorkflowRef, "@", 2) if len(workflowPath) < 2 { - return nil, fmt.Errorf("%w: workflow uri: %s", serrors.ErrorMalformedURI, id.SubjectWorkflowRef) + return nil, false, fmt.Errorf("%w: workflow uri: %s", serrors.ErrorMalformedURI, id.SubjectWorkflowRef) } // Verify trusted workflow. reusableWorkflowPath := strings.Trim(workflowPath[0], "/") reusableWorkflowTag := strings.Trim(workflowPath[1], "/") - builderID, err := verifyTrustedBuilderID(reusableWorkflowPath, reusableWorkflowTag, + builderID, byob, err := verifyTrustedBuilderID(reusableWorkflowPath, reusableWorkflowTag, builderOpts.ExpectedID, defaultBuilders) if err != nil { - return nil, err + return nil, byob, err } // Verify the ref is a full semantic version tag. if err := verifyTrustedBuilderRef(id, reusableWorkflowTag); err != nil { - return nil, err + return nil, byob, err } - return builderID, nil + return builderID, byob, nil } // Verifies the builder ID at path against an expected builderID. // If an expected builderID is not provided, uses the defaultBuilders. -func verifyTrustedBuilderID(certPath, certTag string, expectedBuilderID *string, defaultBuilders map[string]bool) (*utils.TrustedBuilderID, error) { +func verifyTrustedBuilderID(certPath, certTag string, expectedBuilderID *string, defaultTrustedBuilders map[string]bool) (*utils.TrustedBuilderID, bool, error) { var trustedBuilderID *utils.TrustedBuilderID var err error certBuilderName := httpsGithubCom + certPath @@ -107,31 +107,55 @@ func verifyTrustedBuilderID(certPath, certTag string, expectedBuilderID *string, // refs/heads/main for e2e tests. See verifyTrustedBuilderRef(). // No builder ID provided by user: use the default trusted workflows. if expectedBuilderID == nil || *expectedBuilderID == "" { - if _, ok := defaultBuilders[certPath]; !ok { - return nil, fmt.Errorf("%w: %s got %t", serrors.ErrorUntrustedReusableWorkflow, certPath, expectedBuilderID == nil) + if _, ok := defaultTrustedBuilders[certPath]; !ok { + return nil, false, fmt.Errorf("%w: %s got %t", serrors.ErrorUntrustedReusableWorkflow, certPath, expectedBuilderID == nil) } // Construct the builderID using the certificate's builder's name and tag. trustedBuilderID, err = utils.TrustedBuilderIDNew(certBuilderName+"@"+certTag, true) if err != nil { - return nil, err + return nil, false, err } } else { // Verify the builderID. // We only accept IDs on github.com. trustedBuilderID, err = utils.TrustedBuilderIDNew(certBuilderName+"@"+certTag, true) if err != nil { - return nil, err + return nil, false, err + } + + // Check if: + // - the builder in the cert is a BYOB builder + // - the caller trusts the BYOB builder + // If both are true, we don't match the user-provided builder ID + // against the certificate. Instead that will be done by the caller. + if isTrustedDelegatorBuilder(trustedBuilderID, defaultTrustedBuilders) { + return trustedBuilderID, true, nil } - // BuilderID provided by user should match the certificate. + // Not a BYOB builder. BuilderID provided by user should match the certificate. // Note: the certificate builderID has the form `name@refs/tags/v1.2.3`, // so we pass `allowRef = true`. if err := trustedBuilderID.MatchesLoose(*expectedBuilderID, true); err != nil { - return nil, fmt.Errorf("%w: %v", serrors.ErrorUntrustedReusableWorkflow, err) + return nil, false, fmt.Errorf("%w: %v", serrors.ErrorUntrustedReusableWorkflow, err) } } - return trustedBuilderID, nil + return trustedBuilderID, false, nil +} + +func isTrustedDelegatorBuilder(certBuilder *utils.TrustedBuilderID, trustedBuilders map[string]bool) bool { + for byobBuilder := range defaultBYOBReusableWorkflows { + // Check that the certificate builder is a BYOB workflow. + if err := certBuilder.MatchesLoose(httpsGithubCom+byobBuilder, true); err == nil { + // We found a delegator workflow that matches the certificate identity. + // Check that the BYOB builder is trusted by the caller. + if _, ok := trustedBuilders[byobBuilder]; !ok { + return false + } + return true + } + } + return false } // Only allow `@refs/heads/main` for the builder and the e2e tests that need to work at HEAD. diff --git a/verifiers/internal/gha/builder_test.go b/verifiers/internal/gha/builder_test.go index b3ac030dd..d3996bc0d 100644 --- a/verifiers/internal/gha/builder_test.go +++ b/verifiers/internal/gha/builder_test.go @@ -13,6 +13,7 @@ import ( fulcio "github.com/sigstore/fulcio/pkg/certificate" serrors "github.com/slsa-framework/slsa-verifier/v2/errors" "github.com/slsa-framework/slsa-verifier/v2/options" + "github.com/slsa-framework/slsa-verifier/v2/verifiers/utils" ) func Test_VerifyBuilderIdentity(t *testing.T) { @@ -24,6 +25,7 @@ func Test_VerifyBuilderIdentity(t *testing.T) { builderID string defaults map[string]bool err error + byob bool }{ { name: "invalid job workflow ref", @@ -85,6 +87,32 @@ func Test_VerifyBuilderIdentity(t *testing.T) { defaults: defaultArtifactTrustedReusableWorkflows, builderID: "https://github.com/" + trustedBuilderRepository + "/.github/workflows/builder_go_slsa3.yml", }, + { + name: "valid generic delegator builder without tag", + workflow: &WorkflowIdentity{ + SourceRepository: trustedBuilderRepository, + SourceSha1: "0dfcd24824432c4ce587f79c918eef8fc2c44d7b", + SubjectWorkflowRef: trustedBuilderRepository + "/.github/workflows/delegator_generic_slsa3.yml@refs/tags/v1.2.3", + BuildTrigger: "workflow_dispatch", + Issuer: "https://token.actions.githubusercontent.com", + }, + defaults: defaultBYOBReusableWorkflows, + builderID: "https://github.com/" + trustedBuilderRepository + "/.github/workflows/delegator_generic_slsa3.yml", + byob: true, + }, + { + name: "valid low-perms delegator builder with short tag", + workflow: &WorkflowIdentity{ + SourceRepository: trustedBuilderRepository, + SourceSha1: "0dfcd24824432c4ce587f79c918eef8fc2c44d7b", + SubjectWorkflowRef: trustedBuilderRepository + "/.github/workflows/delegator_lowperms-generic_slsa3.yml@refs/tags/v1.2.3", + BuildTrigger: "workflow_dispatch", + Issuer: "https://token.actions.githubusercontent.com", + }, + defaults: defaultBYOBReusableWorkflows, + builderID: "https://github.com/" + trustedBuilderRepository + "/.github/workflows/delegator_lowperms-generic_slsa3.yml@v1.2.3", + byob: true, + }, { name: "valid main ref for e2e test", workflow: &WorkflowIdentity{ @@ -271,7 +299,14 @@ func Test_VerifyBuilderIdentity(t *testing.T) { if opts == nil { opts = &options.BuilderOpts{} } - id, err := VerifyBuilderIdentity(tt.workflow, opts, tt.defaults) + if tt.builderID != "" { + opts.ExpectedID = &tt.builderID + } + id, byob, err := VerifyBuilderIdentity(tt.workflow, opts, tt.defaults) + if byob != tt.byob { + t.Errorf(cmp.Diff(byob, tt.byob)) + } + if !errCmp(err, tt.err) { t.Errorf(cmp.Diff(err, tt.err, cmpopts.EquateErrors())) } @@ -286,6 +321,51 @@ func Test_VerifyBuilderIdentity(t *testing.T) { } } +func Test_isTrustedDelegatorBuilder(t *testing.T) { + t.Parallel() + tests := []struct { + name string + certBuilderID string + trustedBuilderIDs map[string]bool + result bool + }{ + { + name: "match byob", + certBuilderID: "https://github.com/slsa-framework/slsa-github-generator/.github/workflows/delegator_lowperms-generic_slsa3.yml@refs/tags/v1.6.0", + trustedBuilderIDs: map[string]bool{ + "slsa-framework/slsa-github-generator/.github/workflows/delegator_lowperms-generic_slsa3.yml": true, + "slsa-framework/slsa-github-generator/.github/workflows/some_delegator.yml": true, + }, + result: true, + }, + { + name: "match byob but not caller trusted", + certBuilderID: "https://github.com/slsa-framework/slsa-github-generator/.github/workflows/delegator_lowperms-generic_slsa3.yml@refs/tags/v1.6.0", + trustedBuilderIDs: map[string]bool{ + "slsa-framework/slsa-github-generator/.github/workflows/some_other_delegator.yml": true, + "slsa-framework/slsa-github-generator/.github/workflows/some_delegator.yml": true, + }, + result: false, + }, + } + for _, tt := range tests { + tt := tt // Re-initializing variable so it is not changed while executing the closure below + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + trustedBuilderID, err := utils.TrustedBuilderIDNew(tt.certBuilderID, true) + if err != nil { + t.Fatalf(err.Error()) + } + + res := isTrustedDelegatorBuilder(trustedBuilderID, tt.trustedBuilderIDs) + if res != tt.result { + t.Errorf(cmp.Diff(res, tt.result)) + } + }) + } +} + func Test_VerifyCertficateSourceRepository(t *testing.T) { t.Parallel() tests := []struct { @@ -365,7 +445,8 @@ func Test_verifyTrustedBuilderID(t *testing.T) { path string tag string defaults map[string]bool - expected error + err error + byob bool }{ { name: "default trusted short tag", @@ -379,12 +460,34 @@ func Test_verifyTrustedBuilderID(t *testing.T) { tag: "refs/tags/v1.2.3", defaults: defaultArtifactTrustedReusableWorkflows, }, + { + name: "generic delegator workflow long tag", + path: trustedBuilderRepository + "/.github/workflows/delegator_generic_slsa3.yml", + id: asStringPointer(trustedBuilderRepository + "/.github/workflows/delegator_generic_slsa3.yml@refs/tags/v1.2.3"), + tag: "refs/tags/v1.2.3", + defaults: defaultBYOBReusableWorkflows, + byob: true, + }, + { + name: "low perms delegator workflow short tag", + path: trustedBuilderRepository + "/.github/workflows/delegator_lowperms-generic_slsa3.yml", + id: asStringPointer(trustedBuilderRepository + "/.github/workflows/delegator_lowperms-generic_slsa3.yml@refs/tags/v1.2.3"), + tag: "v1.2.3", + defaults: defaultBYOBReusableWorkflows, + byob: true, + }, + { + name: "low perms delegator workflow no ID provided", + path: trustedBuilderRepository + "/.github/workflows/delegator_lowperms-generic_slsa3.yml", + tag: "v1.2.3", + defaults: defaultBYOBReusableWorkflows, + }, { name: "default mismatch against container defaults long tag", path: trustedBuilderRepository + "/.github/workflows/generator_generic_slsa3.yml", tag: "refs/tags/v1.2.3", defaults: defaultContainerTrustedReusableWorkflows, - expected: serrors.ErrorUntrustedReusableWorkflow, + err: serrors.ErrorUntrustedReusableWorkflow, }, { name: "valid ID for GitHub builder short tag", @@ -405,11 +508,11 @@ func Test_verifyTrustedBuilderID(t *testing.T) { id: asStringPointer("https://github.com/some/repo/someBuilderID@v1.2.3"), }, { - name: "valid long ID for GitHub builder short tag", - path: "some/repo/someBuilderID", - tag: "v1.2.3", - id: asStringPointer("https://github.com/some/repo/someBuilderID@refs/tags/v1.2.3"), - expected: serrors.ErrorUntrustedReusableWorkflow, + name: "valid long ID for GitHub builder short tag", + path: "some/repo/someBuilderID", + tag: "v1.2.3", + id: asStringPointer("https://github.com/some/repo/someBuilderID@refs/tags/v1.2.3"), + err: serrors.ErrorUntrustedReusableWorkflow, }, { name: "valid ID for GitHub builder long tag", @@ -430,56 +533,58 @@ func Test_verifyTrustedBuilderID(t *testing.T) { id: asStringPointer("https://github.com/some/repo/someBuilderID@v1.2.3"), }, { - name: "valid long ID for GitHub builder short tag", - path: "some/repo/someBuilderID", - tag: "v1.2.3", - id: asStringPointer("https://github.com/some/repo/someBuilderID@refs/tags/v1.2.3"), - expected: serrors.ErrorUntrustedReusableWorkflow, + name: "valid long ID for GitHub builder short tag", + path: "some/repo/someBuilderID", + tag: "v1.2.3", + id: asStringPointer("https://github.com/some/repo/someBuilderID@refs/tags/v1.2.3"), + err: serrors.ErrorUntrustedReusableWorkflow, }, { - name: "non GitHub builder ID long builder tag", - path: "some/repo/someBuilderID", - tag: "refs/tags/v1.2.3", - id: asStringPointer("https://not-github.com/some/repo/someBuilderID"), - expected: serrors.ErrorUntrustedReusableWorkflow, + name: "non GitHub builder ID long builder tag", + path: "some/repo/someBuilderID", + tag: "refs/tags/v1.2.3", + id: asStringPointer("https://not-github.com/some/repo/someBuilderID"), + err: serrors.ErrorUntrustedReusableWorkflow, }, { - name: "mismatch org GitHub short builder tag", - path: "some/repo/someBuilderID", - tag: "v1.2.3", - id: asStringPointer("https://github.com/other/repo/someBuilderID"), - expected: serrors.ErrorUntrustedReusableWorkflow, + name: "mismatch org GitHub short builder tag", + path: "some/repo/someBuilderID", + tag: "v1.2.3", + id: asStringPointer("https://github.com/other/repo/someBuilderID"), + err: serrors.ErrorUntrustedReusableWorkflow, }, { - name: "mismatch org GitHub long builder tag", - path: "some/repo/someBuilderID", - tag: "refs/tags/v1.2.3", - id: asStringPointer("https://github.com/other/repo/someBuilderID"), - expected: serrors.ErrorUntrustedReusableWorkflow, + name: "mismatch org GitHub long builder tag", + path: "some/repo/someBuilderID", + tag: "refs/tags/v1.2.3", + id: asStringPointer("https://github.com/other/repo/someBuilderID"), + err: serrors.ErrorUntrustedReusableWorkflow, }, { - name: "mismatch name GitHub long builder tag", - path: "some/repo/someBuilderID", - tag: "refs/tags/v1.2.3", - id: asStringPointer("https://github.com/some/other/someBuilderID"), - expected: serrors.ErrorUntrustedReusableWorkflow, + name: "mismatch name GitHub long builder tag", + path: "some/repo/someBuilderID", + tag: "refs/tags/v1.2.3", + id: asStringPointer("https://github.com/some/other/someBuilderID"), + err: serrors.ErrorUntrustedReusableWorkflow, }, { - name: "mismatch id GitHub long builder tag", - path: "some/repo/someBuilderID", - tag: "refs/tags/v1.2.3", - id: asStringPointer("https://github.com/some/repo/ID"), - expected: serrors.ErrorUntrustedReusableWorkflow, + name: "mismatch id GitHub long builder tag", + path: "some/repo/someBuilderID", + tag: "refs/tags/v1.2.3", + id: asStringPointer("https://github.com/some/repo/ID"), + err: serrors.ErrorUntrustedReusableWorkflow, }, } for _, tt := range tests { tt := tt // Re-initializing variable so it is not changed while executing the closure below t.Run(tt.name, func(t *testing.T) { t.Parallel() - - id, err := verifyTrustedBuilderID(tt.path, tt.tag, tt.id, tt.defaults) - if !errCmp(err, tt.expected) { - t.Errorf(cmp.Diff(err, tt.expected, cmpopts.EquateErrors())) + id, byob, err := verifyTrustedBuilderID(tt.path, tt.tag, tt.id, tt.defaults) + if byob != tt.byob { + t.Errorf(cmp.Diff(byob, tt.byob)) + } + if !errCmp(err, tt.err) { + t.Errorf(cmp.Diff(err, tt.err, cmpopts.EquateErrors())) } if err != nil { return diff --git a/verifiers/internal/gha/provenance.go b/verifiers/internal/gha/provenance.go index b1186752b..05cc98ec0 100644 --- a/verifiers/internal/gha/provenance.go +++ b/verifiers/internal/gha/provenance.go @@ -52,8 +52,9 @@ func verifyBuilderIDExactMatch(prov slsaprovenance.Provenance, expectedBuilderID if err != nil { return err } + if err := provBuilderID.MatchesFull(expectedBuilderID, true); err != nil { - return fmt.Errorf("%w", err) + return err } return nil } @@ -71,7 +72,7 @@ func verifyBuilderIDLooseMatch(prov slsaprovenance.Provenance, expectedBuilderID return err } if err := provBuilderID.MatchesLoose(expectedBuilderID, true); err != nil { - return fmt.Errorf("%w", err) + return err } return nil } @@ -273,7 +274,7 @@ func VerifyNpmPackageProvenance(env *dsselib.Envelope, workflow *WorkflowIdentit return nil } -func VerifyProvenance(env *dsselib.Envelope, provenanceOpts *options.ProvenanceOpts, +func VerifyProvenance(env *dsselib.Envelope, provenanceOpts *options.ProvenanceOpts, byob bool, ) error { prov, err := slsaprovenance.ProvenanceFromEnvelope(env) if err != nil { @@ -281,10 +282,17 @@ func VerifyProvenance(env *dsselib.Envelope, provenanceOpts *options.ProvenanceO } // Verify Builder ID. - // Note: `provenanceOpts.ExpectedBuilderID` is not provided by the user, - // but taken from the certificate. It always is of the form `name@refs/tags/`. - if err := verifyBuilderIDExactMatch(prov, provenanceOpts.ExpectedBuilderID); err != nil { - return err + if byob { + // Note: `provenanceOpts.ExpectedBuilderID` is provided by the user. + if err := verifyBuilderIDLooseMatch(prov, provenanceOpts.ExpectedBuilderID); err != nil { + return err + } + } else { + // Note: `provenanceOpts.ExpectedBuilderID` is not provided by the user, + // but taken from the certificate. It always is of the form `name@refs/tags/`. + if err := verifyBuilderIDExactMatch(prov, provenanceOpts.ExpectedBuilderID); err != nil { + return err + } } return VerifyProvenanceCommonOptions(prov, provenanceOpts, false) diff --git a/verifiers/internal/gha/verifier.go b/verifiers/internal/gha/verifier.go index 34b6d7cbb..137b33848 100644 --- a/verifiers/internal/gha/verifier.go +++ b/verifiers/internal/gha/verifier.go @@ -54,7 +54,7 @@ func verifyEnvAndCert(env *dsse.Envelope, } // Verify the builder identity. - builderID, err := VerifyBuilderIdentity(workflowInfo, builderOpts, defaultBuilders) + builderID, byob, err := VerifyBuilderIdentity(workflowInfo, builderOpts, defaultBuilders) if err != nil { return nil, nil, err } @@ -67,7 +67,18 @@ func verifyEnvAndCert(env *dsse.Envelope, // Verify properties of the SLSA provenance. // Unpack and verify info in the provenance, including the subject Digest. provenanceOpts.ExpectedBuilderID = builderID.String() - if err := VerifyProvenance(env, provenanceOpts); err != nil { + // There is a corner-case to handle: if the verified builder ID from the cert + // is a delegator builder, the user MUST provide an expected builder ID + // and we MUST match it against the content of the provenance. + if byob { + if builderOpts.ExpectedID == nil || *builderOpts.ExpectedID == "" { + // NOTE: we will need to update the logic here once our default trusted builders + // are migrated to using BYOB. + return nil, nil, fmt.Errorf("%w: empty ID", serrors.ErrorInvalidBuilderID) + } + provenanceOpts.ExpectedBuilderID = *builderOpts.ExpectedID + } + if err := VerifyProvenance(env, provenanceOpts, byob); err != nil { return nil, nil, err } @@ -104,16 +115,13 @@ func verifyNpmEnvAndCert(env *dsse.Envelope, delegatorBuilderOpts := options.BuilderOpts{ ExpectedID: &expectedDelegatorWorkflow, } - trustedBuilderID, err := VerifyBuilderIdentity(workflowInfo, &delegatorBuilderOpts, defaultBuilders) + trustedBuilderID, byob, err := VerifyBuilderIdentity(workflowInfo, &delegatorBuilderOpts, defaultBuilders) // We accept a non-trusted builder for the default npm builder // that uses npm CLI. if err != nil && !errors.Is(err, serrors.ErrorUntrustedReusableWorkflow) { return nil, err } - // TODO(#493): retrieve certificate information to match - // with the provenance. - // Today it's not possible due to lack of information in the cert. // Verify the source repository from the certificate. if err := VerifyCertficateSourceRepository(workflowInfo, provenanceOpts.ExpectedSourceURI); err != nil { return nil, err @@ -132,6 +140,9 @@ func verifyNpmEnvAndCert(env *dsse.Envelope, // we compare against in the call to VerifyBuilderIdentity() above. // The delegator workflow will set the builder ID to the caller's path, // which is what users match against. + if !byob { + return nil, fmt.Errorf("%w: byob is false", serrors.ErrorInternal) + } provenanceOpts.ExpectedBuilderID = *builderOpts.ExpectedID if workflowInfo.SubjectHosted != nil && *workflowInfo.SubjectHosted != HostedGitHub { @@ -228,7 +239,7 @@ func (v *GHAVerifier) VerifyArtifact(ctx context.Context, return verifyEnvAndCert(signedAtt.Envelope, signedAtt.SigningCert, provenanceOpts, builderOpts, - defaultArtifactTrustedReusableWorkflows) + utils.MergeMaps(defaultArtifactTrustedReusableWorkflows, defaultBYOBReusableWorkflows)) } // VerifyImage verifies provenance for an OCI image. diff --git a/verifiers/utils/structures.go b/verifiers/utils/structures.go new file mode 100644 index 000000000..34906cc83 --- /dev/null +++ b/verifiers/utils/structures.go @@ -0,0 +1,12 @@ +package utils + +func MergeMaps[K comparable, V any](m1, m2 map[K]V) map[K]V { + m := make(map[K]V, len(m1)+len(m2)) + for k, v := range m2 { + m[k] = v + } + for k, v := range m1 { + m[k] = v + } + return m +}