diff --git a/go.mod b/go.mod index abc21efae..5191e3c78 100644 --- a/go.mod +++ b/go.mod @@ -15,7 +15,7 @@ require ( github.com/cli/go-internal v0.0.0-20241025142207-6c48bcd5ce24 github.com/cli/oauth v1.1.1 github.com/cli/safeexec v1.0.1 - github.com/cpuguy83/go-md2man/v2 v2.0.5 + github.com/cpuguy83/go-md2man/v2 v2.0.6 github.com/creack/pty v1.1.24 github.com/digitorus/timestamp v0.0.0-20231217203849-220c5c2851b7 github.com/distribution/reference v0.5.0 diff --git a/go.sum b/go.sum index a13a3c261..0777687a8 100644 --- a/go.sum +++ b/go.sum @@ -112,8 +112,8 @@ github.com/containerd/stargz-snapshotter/estargz v0.14.3 h1:OqlDCK3ZVUO6C3B/5FSk github.com/containerd/stargz-snapshotter/estargz v0.14.3/go.mod h1:KY//uOCIkSuNAHhJogcZtrNHdKrA99/FCCRjE3HD36o= github.com/cpuguy83/go-md2man/v2 v2.0.2/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o= github.com/cpuguy83/go-md2man/v2 v2.0.4/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o= -github.com/cpuguy83/go-md2man/v2 v2.0.5 h1:ZtcqGrnekaHpVLArFSe4HK5DoKx1T0rq2DwVB0alcyc= -github.com/cpuguy83/go-md2man/v2 v2.0.5/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o= +github.com/cpuguy83/go-md2man/v2 v2.0.6 h1:XJtiaUW6dEEqVuZiMTn1ldk455QWwEIsMIJlo5vtkx0= +github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= github.com/creack/pty v1.1.17/go.mod h1:MOBLtS5ELjhRRrroQr9kyvTxUAFNvYEK993ew/Vr4O4= github.com/creack/pty v1.1.24 h1:bJrF4RRfyJnbTJqzRLHzcGaZK1NeM5kTC9jGgovnR1s= github.com/creack/pty v1.1.24/go.mod h1:08sCNb52WyoAwi2QDyzUCTgcvVFhUzewun7wtTfvcwE= diff --git a/pkg/cmd/attestation/verify/options.go b/pkg/cmd/attestation/verify/options.go index 126159023..4296cb8ec 100644 --- a/pkg/cmd/attestation/verify/options.go +++ b/pkg/cmd/attestation/verify/options.go @@ -50,26 +50,6 @@ func (opts *Options) Clean() { } } -func (opts *Options) SetPolicyFlags() { - // check that Repo is in the expected format if provided - if opts.Repo != "" { - // we expect the repo argument to be in the format / - splitRepo := strings.Split(opts.Repo, "/") - - // if Repo is provided but owner is not, set the OWNER portion of the Repo value - // to Owner - opts.Owner = splitRepo[0] - - if !isSignerIdentityProvided(opts) { - opts.SANRegex = expandToGitHubURL(opts.Tenant, opts.Repo) - } - return - } - if !isSignerIdentityProvided(opts) { - opts.SANRegex = expandToGitHubURL(opts.Tenant, opts.Owner) - } -} - // AreFlagsValid checks that the provided flag combination is valid // and returns an error otherwise func (opts *Options) AreFlagsValid() error { @@ -108,11 +88,6 @@ func (opts *Options) AreFlagsValid() error { return nil } -// check if any of the signer identity flags have been provided -func isSignerIdentityProvided(opts *Options) bool { - return opts.SAN != "" || opts.SANRegex != "" || opts.SignerRepo != "" || opts.SignerWorkflow != "" -} - func isProvidedRepoValid(repo string) bool { // we expect a provided repository argument be in the format / splitRepo := strings.Split(repo, "/") diff --git a/pkg/cmd/attestation/verify/options_test.go b/pkg/cmd/attestation/verify/options_test.go index 77c0e3b23..bdb851e7b 100644 --- a/pkg/cmd/attestation/verify/options_test.go +++ b/pkg/cmd/attestation/verify/options_test.go @@ -80,63 +80,3 @@ func TestAreFlagsValid(t *testing.T) { require.ErrorContains(t, err, "bundle-from-oci flag cannot be used with bundle-path flag") }) } - -func TestSetPolicyFlags(t *testing.T) { - t.Run("sets Owner and SANRegex when Repo is provided", func(t *testing.T) { - opts := Options{ - ArtifactPath: publicGoodArtifactPath, - DigestAlgorithm: "sha512", - OIDCIssuer: "some issuer", - Repo: "sigstore/sigstore-js", - } - - opts.SetPolicyFlags() - require.Equal(t, "sigstore", opts.Owner) - require.Equal(t, "sigstore/sigstore-js", opts.Repo) - require.Equal(t, "(?i)^https://github.com/sigstore/sigstore-js/", opts.SANRegex) - }) - - t.Run("does not set SANRegex when SANRegex and Repo are provided", func(t *testing.T) { - opts := Options{ - ArtifactPath: publicGoodArtifactPath, - DigestAlgorithm: "sha512", - OIDCIssuer: "some issuer", - Repo: "sigstore/sigstore-js", - SANRegex: "^https://github/foo", - } - - opts.SetPolicyFlags() - require.Equal(t, "sigstore", opts.Owner) - require.Equal(t, "sigstore/sigstore-js", opts.Repo) - require.Equal(t, "^https://github/foo", opts.SANRegex) - }) - - t.Run("sets SANRegex when Owner is provided", func(t *testing.T) { - opts := Options{ - ArtifactPath: publicGoodArtifactPath, - BundlePath: publicGoodBundlePath, - DigestAlgorithm: "sha512", - OIDCIssuer: "some issuer", - Owner: "sigstore", - } - - opts.SetPolicyFlags() - require.Equal(t, "sigstore", opts.Owner) - require.Equal(t, "(?i)^https://github.com/sigstore/", opts.SANRegex) - }) - - t.Run("does not set SANRegex when SANRegex and Owner are provided", func(t *testing.T) { - opts := Options{ - ArtifactPath: publicGoodArtifactPath, - BundlePath: publicGoodBundlePath, - DigestAlgorithm: "sha512", - OIDCIssuer: "some issuer", - Owner: "sigstore", - SANRegex: "^https://github/foo", - } - - opts.SetPolicyFlags() - require.Equal(t, "sigstore", opts.Owner) - require.Equal(t, "^https://github/foo", opts.SANRegex) - }) -} diff --git a/pkg/cmd/attestation/verify/policy.go b/pkg/cmd/attestation/verify/policy.go index 207cc829e..41b9ea27e 100644 --- a/pkg/cmd/attestation/verify/policy.go +++ b/pkg/cmd/attestation/verify/policy.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "regexp" + "strings" "github.com/sigstore/sigstore-go/pkg/fulcio/certificate" "github.com/sigstore/sigstore-go/pkg/verify" @@ -16,31 +17,57 @@ const hostRegex = `^[a-zA-Z0-9-]+\.[a-zA-Z0-9-]+.*$` func expandToGitHubURL(tenant, ownerOrRepo string) string { if tenant == "" { - return fmt.Sprintf("(?i)^https://github.com/%s/", ownerOrRepo) + return fmt.Sprintf("https://github.com/%s", ownerOrRepo) } - return fmt.Sprintf("(?i)^https://%s.ghe.com/%s/", tenant, ownerOrRepo) + return fmt.Sprintf("https://%s.ghe.com/%s", tenant, ownerOrRepo) +} + +func expandToGitHubURLRegex(tenant, ownerOrRepo string) string { + url := expandToGitHubURL(tenant, ownerOrRepo) + return fmt.Sprintf("(?i)^%s/", url) } func newEnforcementCriteria(opts *Options) (verification.EnforcementCriteria, error) { + // initialize the enforcement criteria with the provided PredicateType c := verification.EnforcementCriteria{ PredicateType: opts.PredicateType, } - // Set SANRegex using either the opts.SignerRepo or opts.SignerWorkflow values - if opts.SignerRepo != "" { - signedRepoRegex := expandToGitHubURL(opts.Tenant, opts.SignerRepo) + // set the owner value by checking the repo and owner options + var owner string + if opts.Repo != "" { + // we expect the repo argument to be in the format / + splitRepo := strings.Split(opts.Repo, "/") + // if Repo is provided but owner is not, set the OWNER portion of the Repo value + // to Owner + owner = splitRepo[0] + } else { + // otherwise use the user provided owner value + owner = opts.Owner + } + + // Set the SANRegex and SAN values using the provided options + // First check if the opts.SANRegex or opts.SAN values are provided + if opts.SANRegex != "" || opts.SAN != "" { + c.SANRegex = opts.SANRegex + c.SAN = opts.SAN + } else if opts.SignerRepo != "" { + // next check if opts.SignerRepo was provided + signedRepoRegex := expandToGitHubURLRegex(opts.Tenant, opts.SignerRepo) c.SANRegex = signedRepoRegex } else if opts.SignerWorkflow != "" { validatedWorkflowRegex, err := validateSignerWorkflow(opts) if err != nil { return verification.EnforcementCriteria{}, err } - c.SANRegex = validatedWorkflowRegex + } else if opts.Repo != "" { + // if the user has not provided the SAN, SANRegex, SignerRepo, or SignerWorkflow options + // then we default to the repo option + c.SANRegex = expandToGitHubURLRegex(opts.Tenant, opts.Repo) } else { - // If neither of those values were set, default to the provided SANRegex and SAN values - c.SANRegex = opts.SANRegex - c.SAN = opts.SAN + // if opts.Repo was not provided, we fallback to the opts.Owner value + c.SANRegex = expandToGitHubURLRegex(opts.Tenant, owner) } // if the DenySelfHostedRunner option is set to true, set the @@ -56,22 +83,11 @@ func newEnforcementCriteria(opts *Options) (verification.EnforcementCriteria, er // If the Repo option is provided, set the SourceRepositoryURI extension if opts.Repo != "" { - // If the Tenant options is also provided, set the SourceRepositoryURI extension - // using the specific URI format - if opts.Tenant != "" { - c.Certificate.SourceRepositoryURI = fmt.Sprintf("https://%s.ghe.com/%s", opts.Tenant, opts.Repo) - } else { - c.Certificate.SourceRepositoryURI = fmt.Sprintf("https://github.com/%s", opts.Repo) - } + c.Certificate.SourceRepositoryURI = expandToGitHubURL(opts.Tenant, opts.Repo) } - // If the tenant option is provided, set the SourceRepositoryOwnerURI extension - // using the specific URI format - if opts.Tenant != "" { - c.Certificate.SourceRepositoryOwnerURI = fmt.Sprintf("https://%s.ghe.com/%s", opts.Tenant, opts.Owner) - } else { - c.Certificate.SourceRepositoryOwnerURI = fmt.Sprintf("https://github.com/%s", opts.Owner) - } + // Set the SourceRepositoryOwnerURI extension using owner and tenant if provided + c.Certificate.SourceRepositoryOwnerURI = expandToGitHubURL(opts.Tenant, owner) // if the tenant is provided and OIDC issuer provided matches the default // use the tenant-specific issuer diff --git a/pkg/cmd/attestation/verify/policy_test.go b/pkg/cmd/attestation/verify/policy_test.go index 420c57f3a..30724afef 100644 --- a/pkg/cmd/attestation/verify/policy_test.go +++ b/pkg/cmd/attestation/verify/policy_test.go @@ -12,12 +12,30 @@ import ( func TestNewEnforcementCriteria(t *testing.T) { artifactPath := "../test/data/sigstore-js-2.1.0.tgz" + t.Run("sets SANRegex and SAN using SANRegex and SAN", func(t *testing.T) { + opts := &Options{ + ArtifactPath: artifactPath, + Owner: "foo", + Repo: "foo/bar", + SAN: "https://github/foo/bar/.github/workflows/attest.yml", + SANRegex: "(?i)^https://github/foo", + SignerRepo: "wrong/value", + SignerWorkflow: "wrong/value/.github/workflows/attest.yml", + } + + c, err := newEnforcementCriteria(opts) + require.NoError(t, err) + require.Equal(t, "https://github/foo/bar/.github/workflows/attest.yml", c.SAN) + require.Equal(t, "(?i)^https://github/foo", c.SANRegex) + }) + t.Run("sets SANRegex using SignerRepo", func(t *testing.T) { opts := &Options{ - ArtifactPath: artifactPath, - Owner: "foo", - Repo: "foo/bar", - SignerRepo: "foo/bar", + ArtifactPath: artifactPath, + Owner: "wrong", + Repo: "wrong/value", + SignerRepo: "foo/bar", + SignerWorkflow: "wrong/value/.github/workflows/attest.yml", } c, err := newEnforcementCriteria(opts) @@ -26,11 +44,27 @@ func TestNewEnforcementCriteria(t *testing.T) { require.Zero(t, c.SAN) }) + t.Run("sets SANRegex using SignerRepo and Tenant", func(t *testing.T) { + opts := &Options{ + ArtifactPath: artifactPath, + Owner: "wrong", + Repo: "wrong/value", + SignerRepo: "foo/bar", + SignerWorkflow: "wrong/value/.github/workflows/attest.yml", + Tenant: "baz", + } + + c, err := newEnforcementCriteria(opts) + require.NoError(t, err) + require.Equal(t, "(?i)^https://baz.ghe.com/foo/bar/", c.SANRegex) + require.Zero(t, c.SAN) + }) + t.Run("sets SANRegex using SignerWorkflow matching host regex", func(t *testing.T) { opts := &Options{ ArtifactPath: artifactPath, - Owner: "foo", - Repo: "foo/bar", + Owner: "wrong", + Repo: "wrong/value", SignerWorkflow: "foo/bar/.github/workflows/attest.yml", Hostname: "github.com", } @@ -41,19 +75,27 @@ func TestNewEnforcementCriteria(t *testing.T) { require.Zero(t, c.SAN) }) - t.Run("sets SANRegex and SAN using SANRegex and SAN", func(t *testing.T) { + t.Run("sets SANRegex using opts.Repo", func(t *testing.T) { opts := &Options{ ArtifactPath: artifactPath, - Owner: "foo", + Owner: "wrong", Repo: "foo/bar", - SAN: "https://github/foo/bar/.github/workflows/attest.yml", - SANRegex: "(?i)^https://github/foo", } c, err := newEnforcementCriteria(opts) require.NoError(t, err) - require.Equal(t, "https://github/foo/bar/.github/workflows/attest.yml", c.SAN) - require.Equal(t, "(?i)^https://github/foo", c.SANRegex) + require.Equal(t, "(?i)^https://github.com/foo/bar/", c.SANRegex) + }) + + t.Run("sets SANRegex using opts.Owner", func(t *testing.T) { + opts := &Options{ + ArtifactPath: artifactPath, + Owner: "foo", + } + + c, err := newEnforcementCriteria(opts) + require.NoError(t, err) + require.Equal(t, "(?i)^https://github.com/foo/", c.SANRegex) }) t.Run("sets Extensions.RunnerEnvironment to GitHubRunner value if opts.DenySelfHostedRunner is true", func(t *testing.T) { @@ -107,6 +149,22 @@ func TestNewEnforcementCriteria(t *testing.T) { require.Equal(t, "https://github.com/foo/bar", c.Certificate.SourceRepositoryURI) }) + t.Run("sets SANRegex and SAN using SANRegex and SAN, sets Extensions.SourceRepositoryURI using opts.Repo", func(t *testing.T) { + opts := &Options{ + ArtifactPath: artifactPath, + Owner: "baz", + Repo: "baz/xyz", + SAN: "https://github/foo/bar/.github/workflows/attest.yml", + SANRegex: "(?i)^https://github/foo", + } + + c, err := newEnforcementCriteria(opts) + require.NoError(t, err) + require.Equal(t, "https://github/foo/bar/.github/workflows/attest.yml", c.SAN) + require.Equal(t, "(?i)^https://github/foo", c.SANRegex) + require.Equal(t, "https://github.com/baz/xyz", c.Certificate.SourceRepositoryURI) + }) + t.Run("sets Extensions.SourceRepositoryOwnerURI using opts.Owner and opts.Tenant", func(t *testing.T) { opts := &Options{ ArtifactPath: artifactPath, diff --git a/pkg/cmd/attestation/verify/verify.go b/pkg/cmd/attestation/verify/verify.go index 837e2024e..016ec1fa8 100644 --- a/pkg/cmd/attestation/verify/verify.go +++ b/pkg/cmd/attestation/verify/verify.go @@ -157,9 +157,6 @@ func NewVerifyCmd(f *cmdutil.Factory, runF func(*Options) error) *cobra.Command opts.Tenant = tenant } - // set policy flags based on what has been provided - opts.SetPolicyFlags() - if runF != nil { return runF(opts) } diff --git a/pkg/cmd/attestation/verify/verify_integration_test.go b/pkg/cmd/attestation/verify/verify_integration_test.go index 4d4c9599c..f25055d22 100644 --- a/pkg/cmd/attestation/verify/verify_integration_test.go +++ b/pkg/cmd/attestation/verify/verify_integration_test.go @@ -76,15 +76,6 @@ func TestVerifyIntegration(t *testing.T) { require.ErrorContains(t, err, "expected SourceRepositoryOwnerURI to be https://github.com/fakeowner, got https://github.com/sigstore") }) - t.Run("with invalid owner and invalid repo", func(t *testing.T) { - opts := publicGoodOpts - opts.Repo = "fakeowner/fakerepo" - - err := runVerify(&opts) - require.Error(t, err) - require.ErrorContains(t, err, "expected SourceRepositoryURI to be https://github.com/fakeowner/fakerepo, got https://github.com/sigstore/sigstore-js") - }) - t.Run("with no matching OIDC issuer", func(t *testing.T) { opts := publicGoodOpts opts.OIDCIssuer = "some-other-issuer" diff --git a/pkg/cmd/attestation/verify/verify_test.go b/pkg/cmd/attestation/verify/verify_test.go index 9a2e9f18c..5e4f33507 100644 --- a/pkg/cmd/attestation/verify/verify_test.go +++ b/pkg/cmd/attestation/verify/verify_test.go @@ -91,7 +91,6 @@ func TestNewVerifyCmd(t *testing.T) { OIDCIssuer: verification.GitHubOIDCIssuer, Owner: "sigstore", PredicateType: verification.SLSAPredicateV1, - SANRegex: "(?i)^https://github.com/sigstore/", SigstoreVerifier: verification.NewMockSigstoreVerifier(t), }, wantsErr: false, @@ -108,7 +107,6 @@ func TestNewVerifyCmd(t *testing.T) { OIDCIssuer: verification.GitHubOIDCIssuer, Owner: "sigstore", PredicateType: verification.SLSAPredicateV1, - SANRegex: "(?i)^https://foo.ghe.com/sigstore/", SigstoreVerifier: verification.NewMockSigstoreVerifier(t), }, wantsErr: false, @@ -125,7 +123,6 @@ func TestNewVerifyCmd(t *testing.T) { OIDCIssuer: verification.GitHubOIDCIssuer, Owner: "sigstore", PredicateType: verification.SLSAPredicateV1, - SANRegex: "(?i)^https://github.com/sigstore/", SigstoreVerifier: verification.NewMockSigstoreVerifier(t), }, wantsErr: true, @@ -142,7 +139,6 @@ func TestNewVerifyCmd(t *testing.T) { OIDCIssuer: verification.GitHubOIDCIssuer, Owner: "sigstore", PredicateType: verification.SLSAPredicateV1, - SANRegex: "(?i)^https://github.com/sigstore/", SigstoreVerifier: verification.NewMockSigstoreVerifier(t), }, wantsErr: false, @@ -190,7 +186,6 @@ func TestNewVerifyCmd(t *testing.T) { OIDCIssuer: verification.GitHubOIDCIssuer, Owner: "sigstore", PredicateType: verification.SLSAPredicateV1, - SANRegex: "(?i)^https://github.com/sigstore/", SigstoreVerifier: verification.NewMockSigstoreVerifier(t), }, wantsErr: false, @@ -206,7 +201,6 @@ func TestNewVerifyCmd(t *testing.T) { OIDCIssuer: verification.GitHubOIDCIssuer, Owner: "sigstore", PredicateType: verification.SLSAPredicateV1, - SANRegex: "(?i)^https://github.com/sigstore/", SigstoreVerifier: verification.NewMockSigstoreVerifier(t), }, wantsErr: false, @@ -256,7 +250,6 @@ func TestNewVerifyCmd(t *testing.T) { OIDCIssuer: verification.GitHubOIDCIssuer, Owner: "sigstore", PredicateType: verification.SLSAPredicateV1, - SANRegex: "(?i)^https://github.com/sigstore/", SigstoreVerifier: verification.NewMockSigstoreVerifier(t), }, wantsExporter: true, @@ -273,7 +266,6 @@ func TestNewVerifyCmd(t *testing.T) { OIDCIssuer: verification.GitHubOIDCIssuer, Owner: "sigstore", PredicateType: "https://spdx.dev/Document/v2.3", - SANRegex: "(?i)^https://github.com/sigstore/", SigstoreVerifier: verification.NewMockSigstoreVerifier(t), }, wantsExporter: true, @@ -457,10 +449,10 @@ func TestRunVerify(t *testing.T) { t.Run("with repo which not matches SourceRepositoryURI", func(t *testing.T) { opts := publicGoodOpts opts.BundlePath = "" - opts.Repo = "wrong/example" + opts.Repo = "sigstore/wrong" err := runVerify(&opts) - require.ErrorContains(t, err, "expected SourceRepositoryURI to be https://github.com/wrong/example, got https://github.com/sigstore/sigstore-js") + require.ErrorContains(t, err, "expected SourceRepositoryURI to be https://github.com/sigstore/wrong, got https://github.com/sigstore/sigstore-js") }) t.Run("with invalid repo", func(t *testing.T) {