diff --git a/internal/controller/api/middleware_audit_public_test.go b/internal/controller/api/middleware_audit_public_test.go index 263552441..27cea7af1 100644 --- a/internal/controller/api/middleware_audit_public_test.go +++ b/internal/controller/api/middleware_audit_public_test.go @@ -26,73 +26,18 @@ import ( "log/slog" "net/http" "net/http/httptest" - "sync" "testing" - "time" "github.com/labstack/echo/v4" "github.com/stretchr/testify/suite" "go.opentelemetry.io/otel/trace" "github.com/osapi-io/osapi/internal/audit" + "github.com/osapi-io/osapi/internal/audit/mocks" "github.com/osapi-io/osapi/internal/controller/api" + "go.uber.org/mock/gomock" ) -// captureStore is a concurrency-safe audit store spy that records Write calls. -// It is kept as a hand-written spy rather than a gomock mock because the -// auditMiddleware fires writes in a goroutine after the HTTP response is sent, -// making gomock's strict call-count semantics impractical. -type captureStore struct { - mu sync.Mutex - entries []audit.Entry - err error -} - -func (f *captureStore) Write( - _ context.Context, - entry audit.Entry, -) error { - f.mu.Lock() - defer f.mu.Unlock() - - if f.err != nil { - return f.err - } - - f.entries = append(f.entries, entry) - return nil -} - -func (f *captureStore) Get( - _ context.Context, - _ string, -) (*audit.Entry, error) { - return nil, nil -} - -func (f *captureStore) List( - _ context.Context, - _ int, - _ int, -) ([]audit.Entry, int, error) { - return nil, 0, nil -} - -func (f *captureStore) ListAll( - _ context.Context, -) ([]audit.Entry, error) { - return nil, nil -} - -func (f *captureStore) getEntries() []audit.Entry { - f.mu.Lock() - defer f.mu.Unlock() - - cp := make([]audit.Entry, len(f.entries)) - copy(cp, f.entries) - return cp -} - type AuditMiddlewarePublicTestSuite struct { suite.Suite } @@ -104,71 +49,50 @@ func (s *AuditMiddlewarePublicTestSuite) TestAuditMiddleware() { subject string roles []string storeErr error + wantWrite bool setupReq func(req *http.Request) *http.Request - validateFunc func(store *captureStore) + validateFunc func(entry audit.Entry) }{ { - name: "authenticated request is logged", - path: "/api/node/hostname", - subject: "user@example.com", - roles: []string{"admin"}, - validateFunc: func(store *captureStore) { - // Give goroutine time to write - time.Sleep(50 * time.Millisecond) - entries := store.getEntries() - s.Len(entries, 1) - s.Equal("user@example.com", entries[0].User) - s.Equal("GET", entries[0].Method) - s.Equal("/api/node/hostname", entries[0].Path) - s.Equal(http.StatusOK, entries[0].ResponseCode) - s.Equal([]string{"admin"}, entries[0].Roles) + name: "authenticated request is logged", + path: "/api/node/hostname", + subject: "user@example.com", + roles: []string{"admin"}, + wantWrite: true, + validateFunc: func(entry audit.Entry) { + s.Equal("user@example.com", entry.User) + s.Equal("GET", entry.Method) + s.Equal("/api/node/hostname", entry.Path) + s.Equal(http.StatusOK, entry.ResponseCode) + s.Equal([]string{"admin"}, entry.Roles) }, }, { name: "unauthenticated request is skipped", path: "/api/node/hostname", subject: "", - validateFunc: func(store *captureStore) { - time.Sleep(50 * time.Millisecond) - entries := store.getEntries() - s.Empty(entries) - }, }, { name: "health path is excluded", path: "/api/health", subject: "user@example.com", - validateFunc: func(store *captureStore) { - time.Sleep(50 * time.Millisecond) - entries := store.getEntries() - s.Empty(entries) - }, }, { name: "health ready path is excluded", path: "/api/health/ready", subject: "user@example.com", - validateFunc: func(store *captureStore) { - time.Sleep(50 * time.Millisecond) - entries := store.getEntries() - s.Empty(entries) - }, }, { name: "metrics path is excluded", path: "/metrics", subject: "user@example.com", - validateFunc: func(store *captureStore) { - time.Sleep(50 * time.Millisecond) - entries := store.getEntries() - s.Empty(entries) - }, }, { - name: "authenticated request with trace context captures trace ID", - path: "/api/node/hostname", - subject: "user@example.com", - roles: []string{"admin"}, + name: "authenticated request with trace context captures trace ID", + path: "/api/node/hostname", + subject: "user@example.com", + roles: []string{"admin"}, + wantWrite: true, setupReq: func(req *http.Request) *http.Request { traceID, _ := trace.TraceIDFromHex( "4bf92f3577b34da6a3ce929d0e0e4736", @@ -183,34 +107,48 @@ func (s *AuditMiddlewarePublicTestSuite) TestAuditMiddleware() { ) return req.WithContext(ctx) }, - validateFunc: func(store *captureStore) { - time.Sleep(50 * time.Millisecond) - entries := store.getEntries() - s.Len(entries, 1) + validateFunc: func(entry audit.Entry) { s.Equal( "4bf92f3577b34da6a3ce929d0e0e4736", - entries[0].TraceID, + entry.TraceID, ) }, }, { - name: "store error is handled gracefully", - path: "/api/node/hostname", - subject: "user@example.com", - roles: []string{"admin"}, - storeErr: fmt.Errorf("write failed"), - validateFunc: func(store *captureStore) { - // Should not panic; the middleware logs the error - time.Sleep(50 * time.Millisecond) - entries := store.getEntries() - s.Empty(entries) - }, + name: "store error is handled gracefully", + path: "/api/node/hostname", + subject: "user@example.com", + roles: []string{"admin"}, + storeErr: fmt.Errorf("write failed"), + wantWrite: true, }, } for _, tt := range tests { s.Run(tt.name, func() { - store := &captureStore{err: tt.storeErr} + ctrl := gomock.NewController(s.T()) + store := mocks.NewMockStore(ctrl) + + // The middleware writes from a goroutine it does not join. The mock + // closes this channel from the write, so the test waits on the call + // itself rather than on a duration. + written := make(chan struct{}) + var got audit.Entry + + if tt.wantWrite { + store.EXPECT(). + Write(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, entry audit.Entry) error { + got = entry + close(written) + return tt.storeErr + }) + } else { + // Excluded and unauthenticated paths return before the + // goroutine is started, so any write is a regression. + store.EXPECT().Write(gomock.Any(), gomock.Any()).Times(0) + } + logger := slog.Default() e := echo.New() @@ -232,7 +170,14 @@ func (s *AuditMiddlewarePublicTestSuite) TestAuditMiddleware() { e.ServeHTTP(rec, req) s.Equal(http.StatusOK, rec.Code) - tt.validateFunc(store) + + if tt.wantWrite { + <-written + + if tt.validateFunc != nil { + tt.validateFunc(got) + } + } }) } } diff --git a/internal/job/client/agent_public_test.go b/internal/job/client/agent_public_test.go index c142f399d..487d86d2b 100644 --- a/internal/job/client/agent_public_test.go +++ b/internal/job/client/agent_public_test.go @@ -270,7 +270,7 @@ func (s *AgentPublicTestSuite) TestWriteJobResponse() { } func (s *AgentPublicTestSuite) TestWriteJobResponseWithPKISigner() { - signer := newMockPKISigner() + signer, _ := newSigner(gomock.NewController(s.T())) tests := []struct { name string @@ -342,7 +342,7 @@ func (s *AgentPublicTestSuite) TestWriteJobResponseWithPKISigner() { } func (s *AgentPublicTestSuite) TestWriteJobResponseWithPKISignerError() { - signer := newMockPKISigner() + signer, _ := newSigner(gomock.NewController(s.T())) // Inject a failing marshal function to trigger the sign error path. client.SetSigningMarshalFn(func(_ any) ([]byte, error) { diff --git a/internal/job/client/client_public_test.go b/internal/job/client/client_public_test.go index 5801c75f6..5aa6ab0f4 100644 --- a/internal/job/client/client_public_test.go +++ b/internal/job/client/client_public_test.go @@ -1175,7 +1175,7 @@ func (s *ClientPublicTestSuite) TestQueryWithPKISigner() { subject = "jobs.query.host.server1" ) - signer := newMockPKISigner() + signer, _ := newSigner(gomock.NewController(s.T())) tests := []struct { name string @@ -1271,7 +1271,7 @@ func (s *ClientPublicTestSuite) TestQueryWithPKISignerSignError() { operation = job.OperationType("node.hostname.get") ) - signer := newMockPKISigner() + signer, _ := newSigner(gomock.NewController(s.T())) tests := []struct { name string @@ -1321,9 +1321,7 @@ func (s *ClientPublicTestSuite) TestQueryWithPKISignerUnwrapPaths() { subject = "jobs.query.host.server1" ) - signer := newMockPKISigner() - // Set ControllerPublicKey so verification is attempted on responses. - signer.ctrlKey = signer.pubKey + signer, _ := newSignerWithControllerKey(gomock.NewController(s.T())) tests := []struct { name string @@ -1413,11 +1411,10 @@ func (s *ClientPublicTestSuite) TestModifyBroadcastWithPKISigner() { subject = "jobs.modify._all" ) - signer := newMockPKISigner() + signer, _ := newSigner(gomock.NewController(s.T())) // Use a signer with ControllerPublicKey set so verification is attempted. - signerWithCtrl := newMockPKISigner() - signerWithCtrl.ctrlKey = signerWithCtrl.pubKey + signerWithCtrl, _ := newSignerWithControllerKey(gomock.NewController(s.T())) tests := []struct { name string diff --git a/internal/job/client/jobs_public_test.go b/internal/job/client/jobs_public_test.go index 952c5dd98..b37b24afe 100644 --- a/internal/job/client/jobs_public_test.go +++ b/internal/job/client/jobs_public_test.go @@ -2143,7 +2143,7 @@ func (s *JobsPublicTestSuite) TestComputeStatusFromKeyNames() { } func (s *JobsPublicTestSuite) TestCreateJobWithPKISigner() { - signer := newMockPKISigner() + signer, _ := newSigner(gomock.NewController(s.T())) tests := []struct { name string @@ -2220,9 +2220,7 @@ func (s *JobsPublicTestSuite) TestCreateJobWithPKISigner() { } func (s *JobsPublicTestSuite) TestGetJobStatusWithPKISigner() { - signer := newMockPKISigner() - // Set ControllerPublicKey so signature verification is attempted. - signer.ctrlKey = signer.pubKey + signer, _ := newSignerWithControllerKey(gomock.NewController(s.T())) jobID := "pki-job-123" tests := []struct { diff --git a/internal/job/client/signing_public_test.go b/internal/job/client/signing_public_test.go index 33e3c1175..75206ab66 100644 --- a/internal/job/client/signing_public_test.go +++ b/internal/job/client/signing_public_test.go @@ -31,35 +31,50 @@ import ( "github.com/osapi-io/osapi/internal/job" "github.com/osapi-io/osapi/internal/job/client" + "github.com/osapi-io/osapi/internal/job/mocks" + "go.uber.org/mock/gomock" ) -// mockPKISigner implements client.PKISigner for testing. -type mockPKISigner struct { - pubKey ed25519.PublicKey - privKey ed25519.PrivateKey - ctrlKey ed25519.PublicKey -} - -func newMockPKISigner() *mockPKISigner { +// newSigner returns a generated PKISigner mock backed by a real ed25519 key +// pair. Sign delegates to the real implementation, so a signature it produces +// verifies against the public key returned alongside it. +func newSigner( + ctrl *gomock.Controller, +) (*mocks.MockPKISigner, ed25519.PublicKey) { pub, priv, _ := ed25519.GenerateKey(rand.Reader) - return &mockPKISigner{ - pubKey: pub, - privKey: priv, - } -} -func (m *mockPKISigner) Sign( - data []byte, -) []byte { - return ed25519.Sign(m.privKey, data) + m := mocks.NewMockPKISigner(ctrl) + m.EXPECT(). + Sign(gomock.Any()). + DoAndReturn(func(data []byte) []byte { + return ed25519.Sign(priv, data) + }). + AnyTimes() + m.EXPECT().Fingerprint().Return("SHA256:test-fingerprint").AnyTimes() + m.EXPECT().ControllerPublicKey().Return(nil).AnyTimes() + + return m, pub } -func (m *mockPKISigner) Fingerprint() string { - return "SHA256:test-fingerprint" -} +// newSignerWithControllerKey is newSigner with the signer's own public key +// reported as the controller's, for paths that verify controller-signed +// messages against it. +func newSignerWithControllerKey( + ctrl *gomock.Controller, +) (*mocks.MockPKISigner, ed25519.PublicKey) { + pub, priv, _ := ed25519.GenerateKey(rand.Reader) -func (m *mockPKISigner) ControllerPublicKey() ed25519.PublicKey { - return m.ctrlKey + m := mocks.NewMockPKISigner(ctrl) + m.EXPECT(). + Sign(gomock.Any()). + DoAndReturn(func(data []byte) []byte { + return ed25519.Sign(priv, data) + }). + AnyTimes() + m.EXPECT().Fingerprint().Return("SHA256:test-fingerprint").AnyTimes() + m.EXPECT().ControllerPublicKey().Return(pub).AnyTimes() + + return m, pub } type SigningPublicTestSuite struct { @@ -107,7 +122,7 @@ func (s *SigningPublicTestSuite) TestWrapInSignedEnvelope() { tt.setupFn() } - signer := newMockPKISigner() + signer, pubKey := newSigner(gomock.NewController(s.T())) result, err := client.ExportWrapInSignedEnvelope(signer, tt.payload) @@ -129,13 +144,13 @@ func (s *SigningPublicTestSuite) TestWrapInSignedEnvelope() { s.Equal("SHA256:test-fingerprint", envelope.Fingerprint) // Verify signature is valid. - s.True(ed25519.Verify(signer.pubKey, tt.payload, envelope.Signature)) + s.True(ed25519.Verify(pubKey, tt.payload, envelope.Signature)) }) } } func (s *SigningPublicTestSuite) TestUnwrapSignedEnvelope() { - signer := newMockPKISigner() + signer, pubKey := newSigner(gomock.NewController(s.T())) tests := []struct { name string @@ -153,7 +168,7 @@ func (s *SigningPublicTestSuite) TestUnwrapSignedEnvelope() { wrapped, _ := client.ExportWrapInSignedEnvelope(signer, payload) return wrapped }, - pubKey: signer.pubKey, + pubKey: pubKey, wantPayload: []byte(`{"id":"test"}`), wantEnv: true, expectError: false, @@ -191,7 +206,7 @@ func (s *SigningPublicTestSuite) TestUnwrapSignedEnvelope() { setupData: func() []byte { return []byte(`{"id":"test","operation":{"type":"node.hostname.get"}}`) }, - pubKey: signer.pubKey, + pubKey: pubKey, wantPayload: []byte(`{"id":"test","operation":{"type":"node.hostname.get"}}`), wantEnv: false, expectError: false, @@ -201,7 +216,7 @@ func (s *SigningPublicTestSuite) TestUnwrapSignedEnvelope() { setupData: func() []byte { return []byte(`not json at all`) }, - pubKey: signer.pubKey, + pubKey: pubKey, wantPayload: []byte(`not json at all`), wantEnv: false, expectError: false, @@ -211,7 +226,7 @@ func (s *SigningPublicTestSuite) TestUnwrapSignedEnvelope() { setupData: func() []byte { return []byte(`{"payload":"","signature":"","fingerprint":""}`) }, - pubKey: signer.pubKey, + pubKey: pubKey, wantPayload: []byte(`{"payload":"","signature":"","fingerprint":""}`), wantEnv: false, expectError: false, @@ -240,7 +255,7 @@ func (s *SigningPublicTestSuite) TestUnwrapSignedEnvelope() { } func (s *SigningPublicTestSuite) TestRoundTrip() { - signer := newMockPKISigner() + signer, pubKey := newSigner(gomock.NewController(s.T())) originalPayload := []byte(`{"id":"round-trip-test","status":"unprocessed"}`) // Wrap @@ -248,7 +263,7 @@ func (s *SigningPublicTestSuite) TestRoundTrip() { s.NoError(err) // Unwrap with correct key - unwrapped, isEnvelope, err := client.ExportUnwrapSignedEnvelope(wrapped, signer.pubKey) + unwrapped, isEnvelope, err := client.ExportUnwrapSignedEnvelope(wrapped, pubKey) s.NoError(err) s.True(isEnvelope) s.Equal(originalPayload, unwrapped) diff --git a/internal/job/mocks/generate.go b/internal/job/mocks/generate.go index 8f16621f7..16daa2c97 100644 --- a/internal/job/mocks/generate.go +++ b/internal/job/mocks/generate.go @@ -24,3 +24,4 @@ package mocks //go:generate go tool go.uber.org/mock/mockgen -destination=./messaging.gen.go -package=mocks github.com/osapi-io/osapi/internal/job/client NATSClient //go:generate go tool go.uber.org/mock/mockgen -destination=./kv.gen.go -package=mocks github.com/nats-io/nats.go/jetstream KeyValue,KeyValueEntry,KeyWatcher,KeyLister //go:generate go tool go.uber.org/mock/mockgen -destination=./job_client.gen.go -package=mocks github.com/osapi-io/osapi/internal/job/client JobClient +//go:generate go tool go.uber.org/mock/mockgen -destination=./pki_signer.gen.go -package=mocks github.com/osapi-io/osapi/internal/job/client PKISigner diff --git a/internal/job/mocks/pki_signer.gen.go b/internal/job/mocks/pki_signer.gen.go new file mode 100644 index 000000000..34d15eb14 --- /dev/null +++ b/internal/job/mocks/pki_signer.gen.go @@ -0,0 +1,83 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/osapi-io/osapi/internal/job/client (interfaces: PKISigner) +// +// Generated by this command: +// +// mockgen -destination=./pki_signer.gen.go -package=mocks github.com/osapi-io/osapi/internal/job/client PKISigner +// + +// Package mocks is a generated GoMock package. +package mocks + +import ( + ed25519 "crypto/ed25519" + reflect "reflect" + + gomock "go.uber.org/mock/gomock" +) + +// MockPKISigner is a mock of PKISigner interface. +type MockPKISigner struct { + ctrl *gomock.Controller + recorder *MockPKISignerMockRecorder + isgomock struct{} +} + +// MockPKISignerMockRecorder is the mock recorder for MockPKISigner. +type MockPKISignerMockRecorder struct { + mock *MockPKISigner +} + +// NewMockPKISigner creates a new mock instance. +func NewMockPKISigner(ctrl *gomock.Controller) *MockPKISigner { + mock := &MockPKISigner{ctrl: ctrl} + mock.recorder = &MockPKISignerMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockPKISigner) EXPECT() *MockPKISignerMockRecorder { + return m.recorder +} + +// ControllerPublicKey mocks base method. +func (m *MockPKISigner) ControllerPublicKey() ed25519.PublicKey { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ControllerPublicKey") + ret0, _ := ret[0].(ed25519.PublicKey) + return ret0 +} + +// ControllerPublicKey indicates an expected call of ControllerPublicKey. +func (mr *MockPKISignerMockRecorder) ControllerPublicKey() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ControllerPublicKey", reflect.TypeOf((*MockPKISigner)(nil).ControllerPublicKey)) +} + +// Fingerprint mocks base method. +func (m *MockPKISigner) Fingerprint() string { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Fingerprint") + ret0, _ := ret[0].(string) + return ret0 +} + +// Fingerprint indicates an expected call of Fingerprint. +func (mr *MockPKISignerMockRecorder) Fingerprint() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Fingerprint", reflect.TypeOf((*MockPKISigner)(nil).Fingerprint)) +} + +// Sign mocks base method. +func (m *MockPKISigner) Sign(data []byte) []byte { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Sign", data) + ret0, _ := ret[0].([]byte) + return ret0 +} + +// Sign indicates an expected call of Sign. +func (mr *MockPKISignerMockRecorder) Sign(data any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Sign", reflect.TypeOf((*MockPKISigner)(nil).Sign), data) +}