From eac999c627a2aee52465924607af8ef6bd70a037 Mon Sep 17 00:00:00 2001 From: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com> Date: Sun, 22 Oct 2023 15:56:19 -0500 Subject: [PATCH] Enhancing Testability with Swappable Interfaces in SPIFFE Why: This PR introduces swappable interfaces to enhance testability in the SPIFFE package. Previously lacking such interfaces, this change simplifies unit testing and mocking, making our code more reliable. What is Covered: Swappable Interfaces: The SPIFFE package now includes interfaces for better unit testing and mocking, facilitating isolated component testing. Advantages: Improved Testing: Swappable interfaces empower more comprehensive and reliable testing, speeding up development cycles. Increased Reliability: Enhanced testing reduces the chances of unexpected issues, ensuring a smoother user experience. Enhanced Maintainability: Easier issue identification and resolution simplify codebase maintenance. The goal is to bolster SPIFFE's testability with swappable interfaces, resulting in a more reliable, maintainable, and efficient codebase. Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com> --- go.mod | 3 +- go.sum | 6 +- signer/spiffe/spiffe.go | 101 ++++++++++++++++++++---- signer/spiffe/spiffe_mock.go | 143 ++++++++++++++++++++++++++++++++++ signer/spiffe/spiffe_test.go | 144 +++++++++++++++++++++++++++++++++++ 5 files changed, 380 insertions(+), 17 deletions(-) create mode 100644 signer/spiffe/spiffe_mock.go create mode 100644 signer/spiffe/spiffe_test.go diff --git a/go.mod b/go.mod index 76b61d6d..0fcec63f 100644 --- a/go.mod +++ b/go.mod @@ -15,6 +15,7 @@ require ( github.com/stretchr/testify v1.8.1 github.com/testifysec/archivista-api v0.0.0-20230220215059-632b84b82b76 go.step.sm/crypto v0.25.0 + go.uber.org/mock v0.3.0 golang.org/x/sys v0.13.0 google.golang.org/grpc v1.53.0 gopkg.in/square/go-jose.v2 v2.6.0 @@ -50,7 +51,7 @@ require ( github.com/tchap/go-patricia/v2 v2.3.1 // indirect github.com/titanous/rocacheck v0.0.0-20171023193734-afe73141d399 // indirect github.com/zclconf/go-cty v1.12.1 // indirect - golang.org/x/mod v0.8.0 // indirect + golang.org/x/mod v0.11.0 // indirect golang.org/x/oauth2 v0.5.0 // indirect golang.org/x/tools v0.6.0 // indirect google.golang.org/appengine v1.6.7 // indirect diff --git a/go.sum b/go.sum index 10aa8b70..5e66c2ce 100644 --- a/go.sum +++ b/go.sum @@ -233,6 +233,8 @@ github.com/zeebo/errs v1.3.0/go.mod h1:sgbWHsvVuTPHcqJJGQ1WhI5KbWlHYz+2+2C/LSEtC go.opencensus.io v0.24.0 h1:y73uSU6J157QMP2kn2r30vwW1A2W2WFwSCGnAVxeaD0= go.step.sm/crypto v0.25.0 h1:a+7sKyozZH9B30s0dHluygxreUxI1NtCBEmuNXx7a4k= go.step.sm/crypto v0.25.0/go.mod h1:kr1rzO6SzeQnLm6Zu6lNtksHZLiFe9k8LolSJNhoc94= +go.uber.org/mock v0.3.0 h1:3mUxI1No2/60yUYax92Pt8eNOEecx2D3lcXZh2NEZJo= +go.uber.org/mock v0.3.0/go.mod h1:a6FSlNadKUHUa9IP5Vyt1zh4fC7uAwxMutEAscFbkZc= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20190911031432-227b76d455e7/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= @@ -247,8 +249,8 @@ golang.org/x/crypto v0.14.0/go.mod h1:MVFd36DqK4CsrnJYDkBA3VC4m2GkXAM0PvzMCn4JQf golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4= -golang.org/x/mod v0.8.0 h1:LUYupSeNrTNCGzR/hVBk2NHZO4hXcVaW1k4Qx7rjPx8= -golang.org/x/mod v0.8.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= +golang.org/x/mod v0.11.0 h1:bUO06HqtnRcc/7l71XBe4WcqTZ+3AH1J59zWDDwLKgU= +golang.org/x/mod v0.11.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= golang.org/x/net v0.0.0-20190603091049-60506f45cf65/go.mod h1:HSz+uSET+XFnRR8LxR5pz3Of3rY3CfYBVs4xY44aLks= golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= diff --git a/signer/spiffe/spiffe.go b/signer/spiffe/spiffe.go index ee76fe6e..75ff6fa7 100644 --- a/signer/spiffe/spiffe.go +++ b/signer/spiffe/spiffe.go @@ -12,18 +12,59 @@ // See the License for the specific language governing permissions and // limitations under the License. +// Package spiffe provides a SPIFFE implementation of the SignerProvider interface. +// It uses the SPIFFE Workload API to fetch SVIDs and uses them to create signers. package spiffe import ( "context" + "crypto" "fmt" + "github.com/spiffe/go-spiffe/v2/svid/x509svid" "github.com/spiffe/go-spiffe/v2/workloadapi" "github.com/testifysec/go-witness/cryptoutil" "github.com/testifysec/go-witness/registry" "github.com/testifysec/go-witness/signer" ) +// X509SVID interface represents the X509-SVID. It provides a method to get the default SVID. +type X509SVID interface { + // DefaultSVID returns the default SVID. + DefaultSVID() *x509svid.SVID +} + +// WorkloadAPI interface represents the Workload API. It provides a method to fetch the X509 context. +type WorkloadAPI interface { + // FetchX509Context fetches the X509 context. + FetchX509Context(ctx context.Context, opts ...workloadapi.ClientOption) (X509SVID, error) +} + +// SignerCreator interface represents the Signer Creator. It provides a method to create a new signer. +type SignerCreator interface { + // NewSigner creates a new signer with the provided private key and options. + NewSigner(privateKey crypto.PrivateKey, opts ...cryptoutil.SignerOption) (cryptoutil.Signer, error) +} + +// SpiffeSignerProvider struct represents the Spiffe Signer Provider. It contains the socket path, workload API and signer creator. +type SpiffeSignerProvider struct { + SocketPath string + workload WorkloadAPI + signer SignerCreator +} + +// ErrInvalidSVID represents an error for invalid SVID. It implements the error interface. +type ErrInvalidSVID string + +// Option represents a function that modifies SpiffeSignerProvider. It is used to set the socket path, workload API and signer creator. +type Option func(*SpiffeSignerProvider) + +// spiffe struct represents the spiffe and impl. It contains the X509 context. +type spiffe struct { + x509Context *workloadapi.X509Context +} + +// init registers the spiffe signer provider. func init() { signer.Register("spiffe", func() signer.SignerProvider { return New() }, registry.StringConfigOption( @@ -31,36 +72,46 @@ func init() { "Path to the SPIFFE Workload API Socket", "", func(sp signer.SignerProvider, socketPath string) (signer.SignerProvider, error) { - ssp, ok := sp.(SpiffeSignerProvider) + ssp, ok := sp.(*SpiffeSignerProvider) if !ok { - return sp, fmt.Errorf("provided signer provider is not a spiffe signer provider") + return nil, fmt.Errorf("provided signer provider is not a spiffe signer provider") } - - WithSocketPath(socketPath)(&ssp) + WithSocketPath(socketPath)(ssp) + s := &spiffe{} + ssp.workload, ssp.signer = s, s return ssp, nil }, ), ) } -type SpiffeSignerProvider struct { - SocketPath string -} - -type ErrInvalidSVID string - +// Error method for ErrInvalidSVID. It returns a formatted error message. func (e ErrInvalidSVID) Error() string { return fmt.Sprintf("invalid svid: %v", string(e)) } -type Option func(*SpiffeSignerProvider) - +// WithSocketPath returns an Option that sets the SocketPath in the SpiffeSignerProvider. func WithSocketPath(socketPath string) Option { return func(ssp *SpiffeSignerProvider) { ssp.SocketPath = socketPath } } +// WithWorkloadAPI returns an Option that sets the WorkloadAPI in the SpiffeSignerProvider. +func WithWorkloadAPI(workloadAPI WorkloadAPI) Option { + return func(ssp *SpiffeSignerProvider) { + ssp.workload = workloadAPI + } +} + +// WithSignerCreator returns an Option that sets the SignerCreator in the SpiffeSignerProvider. +func WithSignerCreator(signerCreator SignerCreator) Option { + return func(ssp *SpiffeSignerProvider) { + ssp.signer = signerCreator + } +} + +// New returns a new SpiffeSignerProvider. It applies the provided options to the SpiffeSignerProvider. func New(opts ...Option) SpiffeSignerProvider { ssp := SpiffeSignerProvider{} for _, opt := range opts { @@ -70,12 +121,33 @@ func New(opts ...Option) SpiffeSignerProvider { return ssp } +// FetchX509Context fetches the X509 context from the workload API. +func (s *spiffe) FetchX509Context(ctx context.Context, opts ...workloadapi.ClientOption) (X509SVID, error) { + var err error + s.x509Context, err = workloadapi.FetchX509Context(ctx, opts...) + if err != nil { + return nil, err + } + return s.x509Context, nil +} + +// DefaultSVID returns the default SVID from the X509 context. +func (s *spiffe) DefaultSVID() *x509svid.SVID { + return s.x509Context.DefaultSVID() +} + +// NewSigner creates a new signer with the provided private key and options. +func (c *spiffe) NewSigner(privateKey crypto.PrivateKey, opts ...cryptoutil.SignerOption) (cryptoutil.Signer, error) { + return cryptoutil.NewSigner(privateKey, opts...) +} + +// Signer returns a cryptoutil.Signer. It fetches the X509 context from the workload API and creates a new signer with the SVID's private key. func (ssp SpiffeSignerProvider) Signer(ctx context.Context) (cryptoutil.Signer, error) { if len(ssp.SocketPath) == 0 { return nil, fmt.Errorf("socker path cannot be empty") } - svidCtx, err := workloadapi.FetchX509Context(ctx, workloadapi.WithAddr(ssp.SocketPath)) + svidCtx, err := ssp.workload.FetchX509Context(ctx, workloadapi.WithAddr(ssp.SocketPath)) if err != nil { return nil, err } @@ -89,5 +161,6 @@ func (ssp SpiffeSignerProvider) Signer(ctx context.Context) (cryptoutil.Signer, return nil, ErrInvalidSVID("no private key") } - return cryptoutil.NewSigner(svid.PrivateKey, cryptoutil.SignWithIntermediates(svid.Certificates[1:]), cryptoutil.SignWithCertificate(svid.Certificates[0])) + return ssp.signer.NewSigner(svid.PrivateKey, + cryptoutil.SignWithIntermediates(svid.Certificates[1:]), cryptoutil.SignWithCertificate(svid.Certificates[0])) } diff --git a/signer/spiffe/spiffe_mock.go b/signer/spiffe/spiffe_mock.go new file mode 100644 index 00000000..5c1f0f50 --- /dev/null +++ b/signer/spiffe/spiffe_mock.go @@ -0,0 +1,143 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: signer/spiffe/spiffe.go +// +// Generated by this command: +// +// mockgen -source=signer/spiffe/spiffe.go -destination=signer/spiffe/spiffe_mock.go -package=spiffe X509SVID,WorkloadAPI,SignerCreator +// +// Package spiffe is a generated GoMock package. +package spiffe + +import ( + context "context" + crypto "crypto" + reflect "reflect" + + x509svid "github.com/spiffe/go-spiffe/v2/svid/x509svid" + workloadapi "github.com/spiffe/go-spiffe/v2/workloadapi" + cryptoutil "github.com/testifysec/go-witness/cryptoutil" + gomock "go.uber.org/mock/gomock" +) + +// MockX509SVID is a mock of X509SVID interface. +type MockX509SVID struct { + ctrl *gomock.Controller + recorder *MockX509SVIDMockRecorder +} + +// MockX509SVIDMockRecorder is the mock recorder for MockX509SVID. +type MockX509SVIDMockRecorder struct { + mock *MockX509SVID +} + +// NewMockX509SVID creates a new mock instance. +func NewMockX509SVID(ctrl *gomock.Controller) *MockX509SVID { + mock := &MockX509SVID{ctrl: ctrl} + mock.recorder = &MockX509SVIDMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockX509SVID) EXPECT() *MockX509SVIDMockRecorder { + return m.recorder +} + +// DefaultSVID mocks base method. +func (m *MockX509SVID) DefaultSVID() *x509svid.SVID { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DefaultSVID") + ret0, _ := ret[0].(*x509svid.SVID) + return ret0 +} + +// DefaultSVID indicates an expected call of DefaultSVID. +func (mr *MockX509SVIDMockRecorder) DefaultSVID() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DefaultSVID", reflect.TypeOf((*MockX509SVID)(nil).DefaultSVID)) +} + +// MockWorkloadAPI is a mock of WorkloadAPI interface. +type MockWorkloadAPI struct { + ctrl *gomock.Controller + recorder *MockWorkloadAPIMockRecorder +} + +// MockWorkloadAPIMockRecorder is the mock recorder for MockWorkloadAPI. +type MockWorkloadAPIMockRecorder struct { + mock *MockWorkloadAPI +} + +// NewMockWorkloadAPI creates a new mock instance. +func NewMockWorkloadAPI(ctrl *gomock.Controller) *MockWorkloadAPI { + mock := &MockWorkloadAPI{ctrl: ctrl} + mock.recorder = &MockWorkloadAPIMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockWorkloadAPI) EXPECT() *MockWorkloadAPIMockRecorder { + return m.recorder +} + +// FetchX509Context mocks base method. +func (m *MockWorkloadAPI) FetchX509Context(ctx context.Context, opts ...workloadapi.ClientOption) (X509SVID, error) { + m.ctrl.T.Helper() + varargs := []any{ctx} + for _, a := range opts { + varargs = append(varargs, a) + } + ret := m.ctrl.Call(m, "FetchX509Context", varargs...) + ret0, _ := ret[0].(X509SVID) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// FetchX509Context indicates an expected call of FetchX509Context. +func (mr *MockWorkloadAPIMockRecorder) FetchX509Context(ctx any, opts ...any) *gomock.Call { + mr.mock.ctrl.T.Helper() + varargs := append([]any{ctx}, opts...) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FetchX509Context", reflect.TypeOf((*MockWorkloadAPI)(nil).FetchX509Context), varargs...) +} + +// MockSignerCreator is a mock of SignerCreator interface. +type MockSignerCreator struct { + ctrl *gomock.Controller + recorder *MockSignerCreatorMockRecorder +} + +// MockSignerCreatorMockRecorder is the mock recorder for MockSignerCreator. +type MockSignerCreatorMockRecorder struct { + mock *MockSignerCreator +} + +// NewMockSignerCreator creates a new mock instance. +func NewMockSignerCreator(ctrl *gomock.Controller) *MockSignerCreator { + mock := &MockSignerCreator{ctrl: ctrl} + mock.recorder = &MockSignerCreatorMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockSignerCreator) EXPECT() *MockSignerCreatorMockRecorder { + return m.recorder +} + +// NewSigner mocks base method. +func (m *MockSignerCreator) NewSigner(privateKey crypto.PrivateKey, opts ...cryptoutil.SignerOption) (cryptoutil.Signer, error) { + m.ctrl.T.Helper() + varargs := []any{privateKey} + for _, a := range opts { + varargs = append(varargs, a) + } + ret := m.ctrl.Call(m, "NewSigner", varargs...) + ret0, _ := ret[0].(cryptoutil.Signer) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// NewSigner indicates an expected call of NewSigner. +func (mr *MockSignerCreatorMockRecorder) NewSigner(privateKey any, opts ...any) *gomock.Call { + mr.mock.ctrl.T.Helper() + varargs := append([]any{privateKey}, opts...) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NewSigner", reflect.TypeOf((*MockSignerCreator)(nil).NewSigner), varargs...) +} diff --git a/signer/spiffe/spiffe_test.go b/signer/spiffe/spiffe_test.go new file mode 100644 index 00000000..225eb04a --- /dev/null +++ b/signer/spiffe/spiffe_test.go @@ -0,0 +1,144 @@ +package spiffe + +import ( + "context" + "crypto/rand" + "crypto/rsa" + "crypto/x509" + "crypto/x509/pkix" + "errors" + "math/big" + "testing" + "time" + + "github.com/spiffe/go-spiffe/v2/spiffeid" + "github.com/spiffe/go-spiffe/v2/svid/x509svid" + "github.com/testifysec/go-witness/cryptoutil" + gomock "go.uber.org/mock/gomock" +) + +// TestSpiffeSignerProvider_Signer tests the Signer method of the SpiffeSignerProvider. +func TestSpiffeSignerProvider_Signer(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + socketPath string + setupMocks func(*MockWorkloadAPI, *MockSignerCreator, *MockX509SVID) + expectErr bool + }{ + { + name: "Successful Signer Creation", + socketPath: "/tmp/spiffe.sock", + setupMocks: setupSuccessfulCase, + expectErr: false, + }, + { + name: "Failed Signer Creation", + socketPath: "/tmp/spiffe.sock", + setupMocks: setupFailureCase, + expectErr: true, + }, + { + name: "Empty Socket Path", + socketPath: "", + setupMocks: setupEmptySocketPathCase, + expectErr: true, + }, + { + name: "Invalid Socket Path", + socketPath: "/invalid/path", + setupMocks: setupInvalidSocketPathCase, + expectErr: true, + }, + } + + for _, testCase := range testCases { + tt := testCase // capture range variable + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + workloadAPI := NewMockWorkloadAPI(ctrl) + signerCreator := NewMockSignerCreator(ctrl) + x509SVID := NewMockX509SVID(ctrl) + + tt.setupMocks(workloadAPI, signerCreator, x509SVID) + + signerProvider := New(WithWorkloadAPI(workloadAPI), WithSignerCreator(signerCreator), WithSocketPath(tt.socketPath)) + _, err := signerProvider.Signer(context.Background()) + + if (err != nil) != tt.expectErr { + t.Errorf("SpiffeSignerProvider.Signer() error = %v, expectErr %v", err, tt.expectErr) + } + }) + } +} + +// generatePrivateKey generates a private key for testing. +func generatePrivateKey() *rsa.PrivateKey { + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + panic(err) + } + return key +} + +// generateTestCertificate generates a test certificate for testing. +func generateTestCertificate() []*x509.Certificate { + return []*x509.Certificate{ + { + SerialNumber: big.NewInt(1), + Subject: pkix.Name{ + Organization: []string{"Test Org"}, + CommonName: "Test CN", + }, + NotBefore: time.Now(), + NotAfter: time.Now().Add(time.Hour), + IsCA: false, + }, + } +} + +// setupSuccessfulCase sets up the mocks for the successful case. +func setupSuccessfulCase(workloadAPI *MockWorkloadAPI, signerCreator *MockSignerCreator, x509SVID *MockX509SVID) { + workloadAPI.EXPECT().FetchX509Context(gomock.Any(), gomock.Any()).Return(x509SVID, nil).Times(1) + signerCreator.EXPECT().NewSigner(gomock.Any(), gomock.Any()).DoAndReturn(func(privateKey interface{}, opts ...cryptoutil.SignerOption) (cryptoutil.Signer, error) { + signer, err := cryptoutil.NewSigner(privateKey, opts...) + return signer, err + }).Times(1) + privateKey := generatePrivateKey() + x509SVID.EXPECT().DefaultSVID().Return(&x509svid.SVID{ + ID: spiffeid.ID{}, + Certificates: generateTestCertificate(), + PrivateKey: privateKey, + }) +} + +// setupFailureCase sets up the mocks for the failure case. +func setupFailureCase(workloadAPI *MockWorkloadAPI, signerCreator *MockSignerCreator, x509SVID *MockX509SVID) { + workloadAPI.EXPECT().FetchX509Context(gomock.Any(), gomock.Any()).Return(x509SVID, nil).Times(1) + signerCreator.EXPECT().NewSigner(gomock.Any(), gomock.Any()).Return(nil, errors.New("error")).Times(1) + privateKey := generatePrivateKey() + x509SVID.EXPECT().DefaultSVID().Return(&x509svid.SVID{ + ID: spiffeid.ID{}, + Certificates: generateTestCertificate(), + PrivateKey: privateKey, + }).Times(1) +} + +// setupEmptySocketPathCase sets up the mocks for the empty socket path case. +func setupEmptySocketPathCase(workloadAPI *MockWorkloadAPI, signerCreator *MockSignerCreator, x509SVID *MockX509SVID) { + workloadAPI.EXPECT().FetchX509Context(gomock.Any(), gomock.Any()).Times(0) + signerCreator.EXPECT().NewSigner(gomock.Any(), gomock.Any()).Times(0) + x509SVID.EXPECT().DefaultSVID().Times(0) +} + +// setupInvalidSocketPathCase sets up the mocks for the invalid socket path case. +func setupInvalidSocketPathCase(workloadAPI *MockWorkloadAPI, signerCreator *MockSignerCreator, x509SVID *MockX509SVID) { + workloadAPI.EXPECT().FetchX509Context(gomock.Any(), gomock.Any()).Return(nil, errors.New("invalid socket path")).Times(1) + signerCreator.EXPECT().NewSigner(gomock.Any(), gomock.Any()).Times(0) + x509SVID.EXPECT().DefaultSVID().Times(0) +}