fix: Verify the TRW tag is a semver tag (#619)

* update Signed-off-by: laurentsimon <laurentsimon@google.com> * update Signed-off-by: laurentsimon <laurentsimon@google.com> * Update verifiers/utils/builder.go Co-authored-by: Ian Lewis <ianlewis@google.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com> --------- Signed-off-by: laurentsimon <laurentsimon@google.com> Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com> Co-authored-by: Ian Lewis <ianlewis@google.com>
slsa-framework · May 26, 2023 · 93d3f8c · 93d3f8c
1 parent de79463
commit 93d3f8c
Show file tree

Hide file tree

Showing 4 changed files with 143 additions and 32 deletions.
diff --git a/verifiers/internal/gha/builder.go b/verifiers/internal/gha/builder.go
@@ -6,8 +6,6 @@ import (
 	"fmt"
 	"strings"
 
-	"golang.org/x/mod/semver"
-
 	fulcio "github.com/sigstore/fulcio/pkg/certificate"
 	serrors "github.com/slsa-framework/slsa-verifier/v2/errors"
 	"github.com/slsa-framework/slsa-verifier/v2/options"
@@ -170,38 +168,10 @@ func verifyTrustedBuilderRef(id *WorkflowIdentity, ref string) error {
 			return nil
 		}
 
-		// Extract the tag.
-		pin, err := utils.TagFromGitHubRef(ref)
-		if err != nil {
-			return err
-		}
-
-		// Tags on trusted repositories should be a valid semver with version
-		// core including all three parts and no build identifier.
-		versionCore := strings.Split(pin, "-")[0]
-		if !semver.IsValid(pin) ||
-			len(strings.Split(versionCore, ".")) != 3 ||
-			semver.Build(pin) != "" {
-			return fmt.Errorf("%w: %s: version tag not valid", serrors.ErrorInvalidRef, pin)
-		}
-
-		return nil
+		return utils.IsValidBuilderTag(ref, true)
 	}
 
-	// Extract the pin.
-	pin, err := utils.TagFromGitHubRef(ref)
-	if err != nil {
-		return err
-	}
-
-	// Valid semver of the form vX.Y.Z with no metadata.
-	if !(semver.IsValid(pin) &&
-		len(strings.Split(pin, ".")) == 3 &&
-		semver.Prerelease(pin) == "" &&
-		semver.Build(pin) == "") {
-		return fmt.Errorf("%w: %s: not of the form vX.Y.Z", serrors.ErrorInvalidRef, pin)
-	}
-	return nil
+	return utils.IsValidBuilderTag(ref, false)
 }
 
 func getExtension(cert *x509.Certificate, oid asn1.ObjectIdentifier, encoded bool) (string, error) {

diff --git a/verifiers/internal/gha/provenance.go b/verifiers/internal/gha/provenance.go
@@ -277,6 +277,19 @@ func VerifyNpmPackageProvenance(env *dsselib.Envelope, workflow *WorkflowIdentit
 	return nil
 }
 
+func isValidDelegatorBuilderID(prov slsaprovenance.Provenance) error {
+	// Verify the TRW was referenced at a proper tag by the user.
+	id, err := prov.BuilderID()
+	if err != nil {
+		return err
+	}
+	parts := strings.Split(id, "@")
+	if len(parts) != 2 {
+		return fmt.Errorf("%w: %s", serrors.ErrorInvalidBuilderID, id)
+	}
+	return utils.IsValidBuilderTag(parts[1], false)
+}
+
 func VerifyProvenance(env *dsselib.Envelope, provenanceOpts *options.ProvenanceOpts, byob bool,
 ) error {
 	prov, err := slsaprovenance.ProvenanceFromEnvelope(env)
@@ -286,6 +299,9 @@ func VerifyProvenance(env *dsselib.Envelope, provenanceOpts *options.ProvenanceO
 
 	// Verify Builder ID.
 	if byob {
+		if err := isValidDelegatorBuilderID(prov); err != nil {
+			return err
+		}
 		// Note: `provenanceOpts.ExpectedBuilderID` is provided by the user.
 		if err := verifyBuilderIDLooseMatch(prov, provenanceOpts.ExpectedBuilderID); err != nil {
 			return err

diff --git a/verifiers/internal/gha/provenance_test.go b/verifiers/internal/gha/provenance_test.go
@@ -382,6 +382,101 @@ func Test_verifySourceURI(t *testing.T) {
 	}
 }
 
+func Test_isValidDelegatorBuilderID(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name string
+		prov *intoto.ProvenanceStatement
+		err  error
+	}{
+		{
+			name: "no @",
+			prov: &intoto.ProvenanceStatement{
+				Predicate: slsa02.ProvenancePredicate{
+					Builder: slsacommon.ProvenanceBuilder{
+						ID: "some/builderID",
+					},
+				},
+			},
+			err: serrors.ErrorInvalidBuilderID,
+		},
+		{
+			name: "invalid ref",
+			prov: &intoto.ProvenanceStatement{
+				Predicate: slsa02.ProvenancePredicate{
+					Builder: slsacommon.ProvenanceBuilder{
+						ID: "some/builderID@v1.2.3",
+					},
+				},
+			},
+			err: serrors.ErrorInvalidRef,
+		},
+		{
+			name: "invalid ref not tag",
+			prov: &intoto.ProvenanceStatement{
+				Predicate: slsa02.ProvenancePredicate{
+					Builder: slsacommon.ProvenanceBuilder{
+						ID: "some/builderID@refs/head/v1.2.3",
+					},
+				},
+			},
+			err: serrors.ErrorInvalidRef,
+		},
+		{
+			name: "invalid ref not full semver",
+			prov: &intoto.ProvenanceStatement{
+				Predicate: slsa02.ProvenancePredicate{
+					Builder: slsacommon.ProvenanceBuilder{
+						ID: "some/builderID@refs/heads/v1.2",
+					},
+				},
+			},
+			err: serrors.ErrorInvalidRef,
+		},
+		{
+			name: "valid builder",
+			prov: &intoto.ProvenanceStatement{
+				Predicate: slsa02.ProvenancePredicate{
+					Builder: slsacommon.ProvenanceBuilder{
+						ID: "some/builderID@refs/tags/v1.2.3",
+					},
+				},
+			},
+		},
+	}
+	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()
+
+			prov := &v02.ProvenanceV02{
+				ProvenanceStatement: tt.prov,
+			}
+
+			err := isValidDelegatorBuilderID(prov)
+			if !errCmp(err, tt.err) {
+				t.Errorf(cmp.Diff(err, tt.err))
+			}
+
+			// Update to v1 SLSA provenance.
+			prov1 := &v1.ProvenanceV1{
+				Predicate: slsa1.ProvenancePredicate{
+					RunDetails: slsa1.ProvenanceRunDetails{
+						Builder: slsa1.Builder{
+							ID: tt.prov.Predicate.Builder.ID,
+						},
+					},
+				},
+			}
+
+			err = isValidDelegatorBuilderID(prov1)
+			if !errCmp(err, tt.err) {
+				t.Errorf(cmp.Diff(err, tt.err))
+			}
+		})
+	}
+}
+
 func Test_verifyBuilderIDExactMatch(t *testing.T) {
 	t.Parallel()
 	tests := []struct {

diff --git a/verifiers/utils/builder.go b/verifiers/utils/builder.go
@@ -4,6 +4,8 @@ import (
 	"fmt"
 	"strings"
 
+	"golang.org/x/mod/semver"
+
 	serrors "github.com/slsa-framework/slsa-verifier/v2/errors"
 )
 
@@ -131,3 +133,31 @@ func TagFromGitHubRef(ref string) (string, error) {
 	}
 	return strings.TrimPrefix(ref, "refs/tags/"), nil
 }
+
+func IsValidBuilderTag(ref string, testing bool) error {
+	// Extract the pin.
+	pin, err := TagFromGitHubRef(ref)
+	if err != nil {
+		return err
+	}
+
+	if testing {
+		// Tags on trusted repositories should be a valid semver with version
+		// core including all three parts and no build identifier.
+		versionCore := strings.Split(pin, "-")[0]
+		if !semver.IsValid(pin) ||
+			len(strings.Split(versionCore, ".")) != 3 ||
+			semver.Build(pin) != "" {
+			return fmt.Errorf("%w: %s: version tag not valid", serrors.ErrorInvalidRef, pin)
+		}
+	}
+
+	// Valid semver of the form vX.Y.Z with no metadata.
+	if !semver.IsValid(pin) ||
+		len(strings.Split(pin, ".")) != 3 ||
+		semver.Prerelease(pin) != "" ||
+		semver.Build(pin) != "" {
+		return fmt.Errorf("%w: %s: not of the form vX.Y.Z", serrors.ErrorInvalidRef, pin)
+	}
+	return nil
+}