From 652b169724ac717d203d145d6e648542d719092e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D7=A0=CF=85=CE=B1=CE=B7=20=D7=A0=CF=85=CE=B1=CE=B7=D1=95?= =?UTF-8?q?=CF=83=CE=B7?= Date: Sun, 16 Aug 2026 11:26:01 -0700 Subject: [PATCH] test: generate the audit store and PKI signer doubles Both were hand-written for interfaces this repository defines, and both had been recorded as relying on an exception. Neither needed one. captureStore stood in for a write the audit middleware dispatches from a goroutine it does not join. That is real, but the test coped with it by sleeping fifty milliseconds, seven times. The generated mock closes a channel from its write handler, so the test waits on the call rather than on a duration. A generated MockStore already existed beside it. mockPKISigner was kept because the test verifies a real ed25519 signature, which a scripted double would make vacuous. A generated mock delegates to ed25519.Sign from its return handler, so the signature stays real and the assertion keeps its meaning. Paths that skip auditing now assert Times(0) rather than sleeping and finding nothing, so a regression that starts auditing an excluded path fails instead of passing. Co-Authored-By: Claude Opus 5 --- .../api/middleware_audit_public_test.go | 173 ++++++------------ internal/job/client/agent_public_test.go | 4 +- internal/job/client/client_public_test.go | 13 +- internal/job/client/jobs_public_test.go | 6 +- internal/job/client/signing_public_test.go | 77 ++++---- internal/job/mocks/generate.go | 1 + internal/job/mocks/pki_signer.gen.go | 83 +++++++++ 7 files changed, 198 insertions(+), 159 deletions(-) create mode 100644 internal/job/mocks/pki_signer.gen.go 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) +}