From 69a983e71340b30c77c79fcadf7a9245e2e0091d Mon Sep 17 00:00:00 2001 From: Sascha Grunert Date: Thu, 17 Feb 2022 09:55:54 +0100 Subject: [PATCH] Remove log from signer impl and add it to options We now feature a custom logger within the `Options` structure and remove the logger from the `defaultImpl` by passing it around. We also add a convenience method to the options to create a context based on the internally provided timeout. Signed-off-by: Sascha Grunert --- sign/impl.go | 12 ++++----- sign/impl_test.go | 3 +-- sign/options.go | 14 ++++++++--- sign/sign.go | 50 ++++++++++++++++++++----------------- sign/signfakes/fake_impl.go | 19 ++++++++------ 5 files changed, 55 insertions(+), 43 deletions(-) diff --git a/sign/impl.go b/sign/impl.go index 8147531..e4caf2b 100644 --- a/sign/impl.go +++ b/sign/impl.go @@ -33,9 +33,7 @@ import ( "sigs.k8s.io/release-utils/util" ) -type defaultImpl struct { - log *logrus.Logger -} +type defaultImpl struct{} //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -generate //counterfeiter:generate . impl @@ -50,7 +48,7 @@ type impl interface { recursive bool, attachment string) error Setenv(string, string) error EnvDefault(string, string) string - TokenFromProviders(context.Context) (string, error) + TokenFromProviders(context.Context, *logrus.Logger) (string, error) FileExists(string) bool } @@ -112,9 +110,9 @@ func (*defaultImpl) IsImageSignedInternal( // TokenFromProviders will try the cosign OIDC providers to get an // oidc token from them. -func (di *defaultImpl) TokenFromProviders(ctx context.Context) (string, error) { - if !di.IdentityProvidersEnabled(ctx) { - di.log.Warn("No OIDC provider enabled. Token cannot be obtained autmatically.") +func (d *defaultImpl) TokenFromProviders(ctx context.Context, logger *logrus.Logger) (string, error) { + if !d.IdentityProvidersEnabled(ctx) { + logger.Warn("No OIDC provider enabled. Token cannot be obtained autmatically.") return "", nil } diff --git a/sign/impl_test.go b/sign/impl_test.go index e997618..67d487a 100644 --- a/sign/impl_test.go +++ b/sign/impl_test.go @@ -21,7 +21,6 @@ import ( "os" "testing" - "github.com/sirupsen/logrus" "github.com/stretchr/testify/require" ) @@ -60,7 +59,7 @@ func TestFileExists(t *testing.T) { require.NoError(t, err) defer os.Remove(f.Name()) require.NoError(t, os.WriteFile(f.Name(), []byte("hey"), os.FileMode(0o644))) - sut := &defaultImpl{log: logrus.New()} + sut := &defaultImpl{} require.True(t, sut.FileExists(f.Name())) require.False(t, sut.FileExists(f.Name()+"a")) } diff --git a/sign/options.go b/sign/options.go index 54fa4e9..f4c5553 100644 --- a/sign/options.go +++ b/sign/options.go @@ -17,6 +17,7 @@ limitations under the License. package sign import ( + "context" "errors" "time" @@ -27,6 +28,9 @@ import ( // Options can be used to modify the behavior of the signer. type Options struct { + // Logger is the custom logger to be used for message printing. + Logger *logrus.Logger + // Verbose can be used to enable a higher log verbosity Verbose bool @@ -58,6 +62,7 @@ type Options struct { // Default returns a default Options instance. func Default() *Options { return &Options{ + Logger: logrus.StandardLogger(), Timeout: 3 * time.Minute, PassFunc: generate.GetPass, EnableTokenProviders: true, @@ -75,11 +80,14 @@ func (o *Options) verifySignOptions() error { } // Ensure that the private key file exists - i := defaultImpl{ - log: logrus.New(), - } + i := defaultImpl{} if o.PrivateKeyPath != "" && !i.FileExists(o.PrivateKeyPath) { return errors.New("specified private key file not found") } return nil } + +// context creates a new context with the timeout set within the options. +func (o *Options) context() (context.Context, context.CancelFunc) { + return context.WithTimeout(context.Background(), o.Timeout) +} diff --git a/sign/sign.go b/sign/sign.go index 2d040c5..537b679 100644 --- a/sign/sign.go +++ b/sign/sign.go @@ -31,24 +31,22 @@ import ( type Signer struct { impl impl options *Options - log *logrus.Logger } // New returns a new Signer instance. func New(options *Options) *Signer { - logger := logrus.New() + if options.Logger == nil { + options.Logger = logrus.New() + } if options.Verbose { logs.Debug.SetOutput(os.Stderr) - logger.SetLevel(logrus.DebugLevel) + options.Logger.SetLevel(logrus.DebugLevel) } return &Signer{ - impl: &defaultImpl{ - log: logger, - }, + impl: &defaultImpl{}, options: options, - log: logger, } } @@ -58,8 +56,13 @@ func (s *Signer) SetImpl(impl impl) { s.impl = impl } +// log returns the internally set logger. +func (s *Signer) log() *logrus.Logger { + return s.options.Logger +} + func (s *Signer) UploadBlob(path string) error { - s.log.Infof("Uploading blob: %s", path) + s.log().Infof("Uploading blob: %s", path) // TODO: unimplemented @@ -69,7 +72,7 @@ func (s *Signer) UploadBlob(path string) error { // SignImage can be used to sign any provided container image reference by // using keyless signing. func (s *Signer) SignImage(reference string) (*SignedObject, error) { - s.log.Infof("Signing reference: %s", reference) + s.log().Infof("Signing reference: %s", reference) // Ensure options to sign are correct if err := s.options.verifySignOptions(); err != nil { @@ -82,12 +85,15 @@ func (s *Signer) SignImage(reference string) (*SignedObject, error) { } defer resetFn() + ctx, cancel := s.options.context() + defer cancel() + // If we don't have a key path, we must ensure we can get an OIDC // token or there is no way to sign. Depending on the options set, // we may get the ID token from the cosign providers identityToken := "" if s.options.PrivateKeyPath == "" { - tok, err := s.identityToken() + tok, err := s.identityToken(ctx) if err != nil { return nil, errors.Wrap(err, "getting identity token for keyless signing") } @@ -126,9 +132,6 @@ func (s *Signer) SignImage(reference string) (*SignedObject, error) { outputCertificate = s.options.OutputCertificatePath } - ctx, cancel := context.WithTimeout(context.Background(), s.options.Timeout) - defer cancel() - if err := s.impl.SignImageInternal(ctx, ko, regOpts, s.options.Annotations, images, "", true, outputSignature, outputCertificate, "", true, false, "", @@ -147,7 +150,7 @@ func (s *Signer) SignImage(reference string) (*SignedObject, error) { // SignFile can be used to sign any provided file path by using keyless // signing. func (s *Signer) SignFile(path string) (*SignedObject, error) { - s.log.Infof("Signing file path: %s", path) + s.log().Infof("Signing file path: %s", path) // TODO: unimplemented @@ -161,7 +164,7 @@ func (s *Signer) SignFile(path string) (*SignedObject, error) { // VerifyImage can be used to validate any provided container image reference by // using keyless signing. func (s *Signer) VerifyImage(reference string) (*SignedObject, error) { - s.log.Infof("Verifying reference: %s", reference) + s.log().Infof("Verifying reference: %s", reference) resetFn, err := s.enableExperimental() if err != nil { @@ -169,8 +172,9 @@ func (s *Signer) VerifyImage(reference string) (*SignedObject, error) { } defer resetFn() - ctx, cancel := context.WithTimeout(context.Background(), s.options.Timeout) + ctx, cancel := s.options.context() defer cancel() + images := []string{reference} object, err := s.impl.VerifyImageInternal(ctx, s.options.PublicKeyPath, images) if err != nil { @@ -181,7 +185,7 @@ func (s *Signer) VerifyImage(reference string) (*SignedObject, error) { // VerifyFile can be used to validate any provided file path. func (s *Signer) VerifyFile(path string) (*SignedObject, error) { - s.log.Infof("Verifying file path: %s", path) + s.log().Infof("Verifying file path: %s", path) // TODO: unimplemented @@ -198,7 +202,7 @@ func (s *Signer) enableExperimental() (resetFn func(), err error) { } return func() { if err := s.impl.Setenv(key, previousValue); err != nil { - s.log.Errorf("Unable to reset cosign experimental mode: %v", err) + s.log().Errorf("Unable to reset cosign experimental mode: %v", err) } }, nil } @@ -207,7 +211,7 @@ func (s *Signer) enableExperimental() (resetFn func(), err error) { // signatures available for it. It makes no signature verification, only // checks to see if more than one signature is available. func (s *Signer) IsImageSigned(imageRef string) (bool, error) { - ctx, cancel := context.WithTimeout(context.Background(), s.options.Timeout) + ctx, cancel := s.options.context() defer cancel() return s.impl.IsImageSignedInternal(ctx, imageRef) @@ -217,17 +221,17 @@ func (s *Signer) IsImageSigned(imageRef string) (bool, error) { // If there is one set in the options we will use that one. If not, // signer will try to get one from the cosign OIDC identity providers // if options.EnableTokenProviders is set -func (s *Signer) identityToken() (string, error) { +func (s *Signer) identityToken(ctx context.Context) (string, error) { tok := s.options.IdentityToken if s.options.PrivateKeyPath == "" && s.options.IdentityToken == "" { // We only attempt to pull from the providers if the option is set if !s.options.EnableTokenProviders { - logrus.Warn("No token set in options and OIDC providers are disabled") + s.log().Warn("No token set in options and OIDC providers are disabled") return "", nil } - logrus.Info("No identity token was provided. Attempting to get one from supported providers.") - token, err := s.impl.TokenFromProviders(context.Background()) + s.log().Info("No identity token was provided. Attempting to get one from supported providers.") + token, err := s.impl.TokenFromProviders(ctx, s.log()) if err != nil { return "", errors.Wrap(err, "getting identity token from providers") } diff --git a/sign/signfakes/fake_impl.go b/sign/signfakes/fake_impl.go index d12847f..cfc12c4 100644 --- a/sign/signfakes/fake_impl.go +++ b/sign/signfakes/fake_impl.go @@ -23,6 +23,7 @@ import ( "github.com/sigstore/cosign/cmd/cosign/cli/options" signa "github.com/sigstore/cosign/cmd/cosign/cli/sign" + "github.com/sirupsen/logrus" "sigs.k8s.io/release-sdk/sign" ) @@ -99,10 +100,11 @@ type FakeImpl struct { signImageInternalReturnsOnCall map[int]struct { result1 error } - TokenFromProvidersStub func(context.Context) (string, error) + TokenFromProvidersStub func(context.Context, *logrus.Logger) (string, error) tokenFromProvidersMutex sync.RWMutex tokenFromProvidersArgsForCall []struct { arg1 context.Context + arg2 *logrus.Logger } tokenFromProvidersReturns struct { result1 string @@ -473,18 +475,19 @@ func (fake *FakeImpl) SignImageInternalReturnsOnCall(i int, result1 error) { }{result1} } -func (fake *FakeImpl) TokenFromProviders(arg1 context.Context) (string, error) { +func (fake *FakeImpl) TokenFromProviders(arg1 context.Context, arg2 *logrus.Logger) (string, error) { fake.tokenFromProvidersMutex.Lock() ret, specificReturn := fake.tokenFromProvidersReturnsOnCall[len(fake.tokenFromProvidersArgsForCall)] fake.tokenFromProvidersArgsForCall = append(fake.tokenFromProvidersArgsForCall, struct { arg1 context.Context - }{arg1}) + arg2 *logrus.Logger + }{arg1, arg2}) stub := fake.TokenFromProvidersStub fakeReturns := fake.tokenFromProvidersReturns - fake.recordInvocation("TokenFromProviders", []interface{}{arg1}) + fake.recordInvocation("TokenFromProviders", []interface{}{arg1, arg2}) fake.tokenFromProvidersMutex.Unlock() if stub != nil { - return stub(arg1) + return stub(arg1, arg2) } if specificReturn { return ret.result1, ret.result2 @@ -498,17 +501,17 @@ func (fake *FakeImpl) TokenFromProvidersCallCount() int { return len(fake.tokenFromProvidersArgsForCall) } -func (fake *FakeImpl) TokenFromProvidersCalls(stub func(context.Context) (string, error)) { +func (fake *FakeImpl) TokenFromProvidersCalls(stub func(context.Context, *logrus.Logger) (string, error)) { fake.tokenFromProvidersMutex.Lock() defer fake.tokenFromProvidersMutex.Unlock() fake.TokenFromProvidersStub = stub } -func (fake *FakeImpl) TokenFromProvidersArgsForCall(i int) context.Context { +func (fake *FakeImpl) TokenFromProvidersArgsForCall(i int) (context.Context, *logrus.Logger) { fake.tokenFromProvidersMutex.RLock() defer fake.tokenFromProvidersMutex.RUnlock() argsForCall := fake.tokenFromProvidersArgsForCall[i] - return argsForCall.arg1 + return argsForCall.arg1, argsForCall.arg2 } func (fake *FakeImpl) TokenFromProvidersReturns(result1 string, result2 error) {