Refactor fulcio signer to take in KeyOpts (take 2) (#1818)

* Refactor fulcio signer to take in KeyOpts. This commit should not have change in behavior. It changes the fulcio signer to take in the KeyOpts struct instead of passing individual parameters through, as well as moves common code dependent on the values to the same place. The motivation behind this change is to make it easier to add new options without needing to plumb through another param to the already long list. To avoid circular dependencies (sign -> fulcio -> sign), KeyOpts was moved to the options package. Signed-off-by: Billy Lynch <billy@chainguard.dev> * Re-add OIDC providers to sign command. Signed-off-by: Billy Lynch <billy@chainguard.dev>
sigstore · Apr 30, 2022 · d2d7464 · d2d7464
1 parent 9595922
commit d2d7464
Show file tree

Hide file tree

Showing 14 changed files with 102 additions and 81 deletions.
diff --git a/cmd/cosign/cli/attest.go b/cmd/cosign/cli/attest.go
@@ -22,7 +22,6 @@ import (
 	"github.com/sigstore/cosign/cmd/cosign/cli/attest"
 	"github.com/sigstore/cosign/cmd/cosign/cli/generate"
 	"github.com/sigstore/cosign/cmd/cosign/cli/options"
-	"github.com/sigstore/cosign/cmd/cosign/cli/sign"
 )
 
 func Attest() *cobra.Command {
@@ -63,7 +62,7 @@ func Attest() *cobra.Command {
 			if err != nil {
 				return err
 			}
-			ko := sign.KeyOpts{
+			ko := options.KeyOpts{
 				KeyRef:                   o.Key,
 				PassFunc:                 generate.GetPass,
 				Sk:                       o.SecurityKey.Use,

diff --git a/cmd/cosign/cli/attest/attest.go b/cmd/cosign/cli/attest/attest.go
@@ -74,7 +74,7 @@ func uploadToTlog(ctx context.Context, sv *sign.SignerVerifier, rekorURL string,
 }
 
 //nolint
-func AttestCmd(ctx context.Context, ko sign.KeyOpts, regOpts options.RegistryOptions, imageRef string, certPath string, certChainPath string,
+func AttestCmd(ctx context.Context, ko options.KeyOpts, regOpts options.RegistryOptions, imageRef string, certPath string, certChainPath string,
 	noUpload bool, predicatePath string, force bool, predicateType string, replace bool, timeout time.Duration) error {
 	// A key file or token is required unless we're in experimental mode!
 	if options.EnableExperimental() {

diff --git a/cmd/cosign/cli/fulcio/fulcio.go b/cmd/cosign/cli/fulcio/fulcio.go
@@ -30,8 +30,9 @@ import (
 	"golang.org/x/term"
 
 	"github.com/sigstore/cosign/cmd/cosign/cli/fulcio/fulcioroots"
-	clioptions "github.com/sigstore/cosign/cmd/cosign/cli/options"
+	"github.com/sigstore/cosign/cmd/cosign/cli/options"
 	"github.com/sigstore/cosign/pkg/cosign"
+	"github.com/sigstore/cosign/pkg/providers"
 	"github.com/sigstore/fulcio/pkg/api"
 	"github.com/sigstore/sigstore/pkg/oauthflow"
 	"github.com/sigstore/sigstore/pkg/signature"
@@ -110,7 +111,21 @@ type Signer struct {
 	*signature.ECDSASignerVerifier
 }
 
-func NewSigner(ctx context.Context, idToken, oidcIssuer, oidcClientID, oidcClientSecret, oidcRedirectURL string, fClient api.Client) (*Signer, error) {
+func NewSigner(ctx context.Context, ko options.KeyOpts) (*Signer, error) {
+	fClient, err := NewClient(ko.FulcioURL)
+	if err != nil {
+		return nil, errors.Wrap(err, "creating Fulcio client")
+	}
+
+	idToken := ko.IDToken
+	// If token is not set in the options, get one from the provders
+	if idToken == "" && providers.Enabled(ctx) {
+		idToken, err = providers.Provide(ctx, "sigstore")
+		if err != nil {
+			return nil, errors.Wrap(err, "fetching ambient OIDC credentials")
+		}
+	}
+
 	priv, err := cosign.GeneratePrivateKey()
 	if err != nil {
 		return nil, errors.Wrap(err, "generating cert")
@@ -131,7 +146,7 @@ func NewSigner(ctx context.Context, idToken, oidcIssuer, oidcClientID, oidcClien
 	default:
 		flow = FlowNormal
 	}
-	Resp, err := GetCert(ctx, priv, idToken, flow, oidcIssuer, oidcClientID, oidcClientSecret, oidcRedirectURL, fClient) // TODO, use the chain.
+	Resp, err := GetCert(ctx, priv, idToken, flow, ko.OIDCIssuer, ko.OIDCClientID, ko.OIDCClientSecret, ko.OIDCRedirectURL, fClient) // TODO, use the chain.
 	if err != nil {
 		return nil, errors.Wrap(err, "retrieving cert")
 	}
@@ -166,6 +181,6 @@ func NewClient(fulcioURL string) (api.Client, error) {
 	if err != nil {
 		return nil, err
 	}
-	fClient := api.NewClient(fulcioServer, api.WithUserAgent(clioptions.UserAgent()))
+	fClient := api.NewClient(fulcioServer, api.WithUserAgent(options.UserAgent()))
 	return fClient, nil
 }
diff --git a/cmd/cosign/cli/fulcio/fulcioverifier/fulcioverifier.go b/cmd/cosign/cli/fulcio/fulcioverifier/fulcioverifier.go
@@ -24,11 +24,11 @@ import (
 
 	"github.com/sigstore/cosign/cmd/cosign/cli/fulcio"
 	"github.com/sigstore/cosign/cmd/cosign/cli/fulcio/fulcioverifier/ctl"
-	"github.com/sigstore/fulcio/pkg/api"
+	"github.com/sigstore/cosign/cmd/cosign/cli/options"
 )
 
-func NewSigner(ctx context.Context, idToken, oidcIssuer, oidcClientID, oidcClientSecret, oidcRedirectURL string, fClient api.Client) (*fulcio.Signer, error) {
-	fs, err := fulcio.NewSigner(ctx, idToken, oidcIssuer, oidcClientID, oidcClientSecret, oidcRedirectURL, fClient)
+func NewSigner(ctx context.Context, ko options.KeyOpts) (*fulcio.Signer, error) {
+	fs, err := fulcio.NewSigner(ctx, ko)
 	if err != nil {
 		return nil, err
 	}

diff --git a/cmd/cosign/cli/options/key.go b/cmd/cosign/cli/options/key.go
@@ -0,0 +1,37 @@
+//
+// Copyright 2022 The Sigstore Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package options
+
+import "github.com/sigstore/cosign/pkg/cosign"
+
+type KeyOpts struct {
+	Sk               bool
+	Slot             string
+	KeyRef           string
+	FulcioURL        string
+	RekorURL         string
+	IDToken          string
+	PassFunc         cosign.PassFunc
+	OIDCIssuer       string
+	OIDCClientID     string
+	OIDCClientSecret string
+	OIDCRedirectURL  string
+	BundlePath       string
+
+	// Modeled after InsecureSkipVerify in tls.Config, this disables
+	// verifying the SCT.
+	InsecureSkipFulcioVerify bool
+}
diff --git a/cmd/cosign/cli/policy_init.go b/cmd/cosign/cli/policy_init.go
@@ -179,7 +179,7 @@ func signPolicy() *cobra.Command {
 			if err != nil {
 				return err
 			}
-			sv, err := sign.SignerFromKeyOpts(ctx, "", "", sign.KeyOpts{
+			sv, err := sign.SignerFromKeyOpts(ctx, "", "", options.KeyOpts{
 				FulcioURL:                o.Fulcio.URL,
 				IDToken:                  o.Fulcio.IdentityToken,
 				InsecureSkipFulcioVerify: o.Fulcio.InsecureSkipFulcioVerify,

diff --git a/cmd/cosign/cli/sign.go b/cmd/cosign/cli/sign.go
@@ -82,7 +82,7 @@ func Sign() *cobra.Command {
 			if err != nil {
 				return err
 			}
-			ko := sign.KeyOpts{
+			ko := options.KeyOpts{
 				KeyRef:                   o.Key,
 				PassFunc:                 generate.GetPass,
 				Sk:                       o.SecurityKey.Use,

diff --git a/cmd/cosign/cli/sign/sign.go b/cmd/cosign/cli/sign/sign.go
@@ -47,12 +47,14 @@ import (
 	"github.com/sigstore/cosign/pkg/oci/mutate"
 	ociremote "github.com/sigstore/cosign/pkg/oci/remote"
 	"github.com/sigstore/cosign/pkg/oci/walk"
-	providers "github.com/sigstore/cosign/pkg/providers/all"
 	sigs "github.com/sigstore/cosign/pkg/signature"
 	"github.com/sigstore/sigstore/pkg/cryptoutils"
 	"github.com/sigstore/sigstore/pkg/signature"
 	signatureoptions "github.com/sigstore/sigstore/pkg/signature/options"
 	sigPayload "github.com/sigstore/sigstore/pkg/signature/payload"
+
+	// Loads OIDC providers
+	_ "github.com/sigstore/cosign/pkg/providers/all"
 )
 
 func ShouldUploadToTlog(ctx context.Context, ref name.Reference, force bool, url string) bool {
@@ -93,7 +95,7 @@ func GetAttachedImageRef(ref name.Reference, attachment string, opts ...ociremot
 }
 
 // nolint
-func SignCmd(ro *options.RootOptions, ko KeyOpts, regOpts options.RegistryOptions, annotations map[string]interface{},
+func SignCmd(ro *options.RootOptions, ko options.KeyOpts, regOpts options.RegistryOptions, annotations map[string]interface{},
 	imgs []string, certPath string, certChainPath string, upload bool, outputSignature, outputCertificate string,
 	payloadPath string, force bool, recursive bool, attachment string) error {
 	if options.EnableExperimental() {
@@ -183,7 +185,7 @@ func SignCmd(ro *options.RootOptions, ko KeyOpts, regOpts options.RegistryOption
 	return nil
 }
 
-func signDigest(ctx context.Context, digest name.Digest, payload []byte, ko KeyOpts,
+func signDigest(ctx context.Context, digest name.Digest, payload []byte, ko options.KeyOpts,
 	regOpts options.RegistryOptions, annotations map[string]interface{}, upload bool, outputSignature, outputCertificate string, force bool, recursive bool,
 	dd mutate.DupeDetector, sv *SignerVerifier, se oci.SignedEntity) error {
 	var err error
@@ -436,29 +438,18 @@ func signerFromKeyRef(ctx context.Context, certPath, certChainPath, keyRef strin
 	return certSigner, nil
 }
 
-func keylessSigner(ctx context.Context, ko KeyOpts) (*SignerVerifier, error) {
-	fClient, err := fulcio.NewClient(ko.FulcioURL)
-	if err != nil {
-		return nil, errors.Wrap(err, "creating Fulcio client")
-	}
-
-	tok := ko.IDToken
-	// If token is not set in the options, get one from the provders
-	if tok == "" && providers.Enabled(ctx) {
-		tok, err = providers.Provide(ctx, "sigstore")
-		if err != nil {
-			return nil, errors.Wrap(err, "fetching ambient OIDC credentials")
-		}
-	}
-
-	var k *fulcio.Signer
+func keylessSigner(ctx context.Context, ko options.KeyOpts) (*SignerVerifier, error) {
+	var (
+		k   *fulcio.Signer
+		err error
+	)
 
 	if ko.InsecureSkipFulcioVerify {
-		if k, err = fulcio.NewSigner(ctx, tok, ko.OIDCIssuer, ko.OIDCClientID, ko.OIDCClientSecret, ko.OIDCRedirectURL, fClient); err != nil {
+		if k, err = fulcio.NewSigner(ctx, ko); err != nil {
 			return nil, errors.Wrap(err, "getting key from Fulcio")
 		}
 	} else {
-		if k, err = fulcioverifier.NewSigner(ctx, tok, ko.OIDCIssuer, ko.OIDCClientID, ko.OIDCClientSecret, ko.OIDCRedirectURL, fClient); err != nil {
+		if k, err = fulcioverifier.NewSigner(ctx, ko); err != nil {
 			return nil, errors.Wrap(err, "getting key from Fulcio")
 		}
 	}
@@ -470,7 +461,7 @@ func keylessSigner(ctx context.Context, ko KeyOpts) (*SignerVerifier, error) {
 	}, nil
 }
 
-func SignerFromKeyOpts(ctx context.Context, certPath string, certChainPath string, ko KeyOpts) (*SignerVerifier, error) {
+func SignerFromKeyOpts(ctx context.Context, certPath string, certChainPath string, ko options.KeyOpts) (*SignerVerifier, error) {
 	if ko.Sk {
 		return signerFromSecurityKey(ko.Slot)
 	}

diff --git a/cmd/cosign/cli/sign/sign_blob.go b/cmd/cosign/cli/sign/sign_blob.go
@@ -34,27 +34,8 @@ import (
 	signatureoptions "github.com/sigstore/sigstore/pkg/signature/options"
 )
 
-type KeyOpts struct {
-	Sk               bool
-	Slot             string
-	KeyRef           string
-	FulcioURL        string
-	RekorURL         string
-	IDToken          string
-	PassFunc         cosign.PassFunc
-	OIDCIssuer       string
-	OIDCClientID     string
-	OIDCClientSecret string
-	OIDCRedirectURL  string
-	BundlePath       string
-
-	// Modeled after InsecureSkipVerify in tls.Config, this disables
-	// verifying the SCT.
-	InsecureSkipFulcioVerify bool
-}
-
 // nolint
-func SignBlobCmd(ro *options.RootOptions, ko KeyOpts, regOpts options.RegistryOptions, payloadPath string, b64 bool, outputSignature string, outputCertificate string) ([]byte, error) {
+func SignBlobCmd(ro *options.RootOptions, ko options.KeyOpts, regOpts options.RegistryOptions, payloadPath string, b64 bool, outputSignature string, outputCertificate string) ([]byte, error) {
 	var payload []byte
 	var err error
 	var rekorBytes []byte

diff --git a/cmd/cosign/cli/sign/sign_test.go b/cmd/cosign/cli/sign/sign_test.go
@@ -110,7 +110,7 @@ func generateCertificateFiles(t *testing.T, tmpDir string, pf cosign.PassFunc) (
 func TestSignCmdLocalKeyAndSk(t *testing.T) {
 	ro := &options.RootOptions{Timeout: options.DefaultTimeout}
 
-	for _, ko := range []KeyOpts{
+	for _, ko := range []options.KeyOpts{
 		// local and sk keys
 		{
 			KeyRef:   "testLocalPath",

diff --git a/cmd/cosign/cli/signblob.go b/cmd/cosign/cli/signblob.go
@@ -68,7 +68,7 @@ func SignBlob() *cobra.Command {
 			if err != nil {
 				return err
 			}
-			ko := sign.KeyOpts{
+			ko := options.KeyOpts{
 				KeyRef:                   o.Key,
 				PassFunc:                 generate.GetPass,
 				Sk:                       o.SecurityKey.Use,

diff --git a/cmd/cosign/cli/verify.go b/cmd/cosign/cli/verify.go
@@ -20,7 +20,6 @@ import (
 	"github.com/spf13/cobra"
 
 	"github.com/sigstore/cosign/cmd/cosign/cli/options"
-	"github.com/sigstore/cosign/cmd/cosign/cli/sign"
 	"github.com/sigstore/cosign/cmd/cosign/cli/verify"
 )
 
@@ -249,7 +248,7 @@ The blob may be specified as a path to a file or - for stdin.`,
 
 		Args: cobra.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
-			ko := sign.KeyOpts{
+			ko := options.KeyOpts{
 				KeyRef:     o.Key,
 				Sk:         o.SecurityKey.Use,
 				Slot:       o.SecurityKey.Slot,

diff --git a/cmd/cosign/cli/verify/verify_blob.go b/cmd/cosign/cli/verify/verify_blob.go
@@ -35,7 +35,6 @@ import (
 	"github.com/sigstore/cosign/cmd/cosign/cli/fulcio"
 	"github.com/sigstore/cosign/cmd/cosign/cli/options"
 	"github.com/sigstore/cosign/cmd/cosign/cli/rekor"
-	"github.com/sigstore/cosign/cmd/cosign/cli/sign"
 	"github.com/sigstore/cosign/pkg/blob"
 	"github.com/sigstore/cosign/pkg/cosign"
 	"github.com/sigstore/cosign/pkg/cosign/pivkey"
@@ -61,7 +60,7 @@ func isb64(data []byte) bool {
 }
 
 // nolint
-func VerifyBlobCmd(ctx context.Context, ko sign.KeyOpts, certRef, certEmail,
+func VerifyBlobCmd(ctx context.Context, ko options.KeyOpts, certRef, certEmail,
 	certOidcIssuer, certChain, sigRef, blobRef string, enforceSCT bool) error {
 	var verifier signature.Verifier
 	var cert *x509.Certificate
@@ -186,7 +185,7 @@ func VerifyBlobCmd(ctx context.Context, ko sign.KeyOpts, certRef, certEmail,
 	return nil
 }
 
-func verifySigByUUID(ctx context.Context, ko sign.KeyOpts, rClient *client.Rekor, certEmail, certOidcIssuer, sig, b64sig string,
+func verifySigByUUID(ctx context.Context, ko options.KeyOpts, rClient *client.Rekor, certEmail, certOidcIssuer, sig, b64sig string,
 	uuids []string, blobBytes []byte, enforceSCT bool) error {
 	var validSigExists bool
 	for _, u := range uuids {
@@ -289,7 +288,7 @@ func payloadBytes(blobRef string) ([]byte, error) {
 	return blobBytes, nil
 }
 
-func verifyRekorEntry(ctx context.Context, ko sign.KeyOpts, e *models.LogEntryAnon, pubKey signature.Verifier, cert *x509.Certificate, b64sig string, blobBytes []byte) error {
+func verifyRekorEntry(ctx context.Context, ko options.KeyOpts, e *models.LogEntryAnon, pubKey signature.Verifier, cert *x509.Certificate, b64sig string, blobBytes []byte) error {
 	// If we have a bundle with a rekor entry, let's first try to verify offline
 	if ko.BundlePath != "" {
 		if err := verifyRekorBundle(ctx, ko.BundlePath, cert); err == nil {