From 00e8b40cb0c580b74f800f7fc2ab130212a50612 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Wed, 20 May 2026 14:44:57 +0200 Subject: [PATCH] chore: surface key add/remove/keep counts in audit log (#25484) --- coderd/ai_providers.go | 61 +++++++++++++++++++++++++------- coderd/ai_providers_test.go | 69 +++++++++++++++++++++++++++++++++++++ coderd/audit/request.go | 2 +- coderd/database/models.go | 12 +++---- coderd/database/sqlc.yaml | 2 ++ 5 files changed, 126 insertions(+), 20 deletions(-) diff --git a/coderd/ai_providers.go b/coderd/ai_providers.go index ef41143695..7a504da3c5 100644 --- a/coderd/ai_providers.go +++ b/coderd/ai_providers.go @@ -13,6 +13,7 @@ import ( "golang.org/x/xerrors" "cdr.dev/slog/v3" + aibridgeutils "github.com/coder/coder/v2/aibridge/utils" "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/db2sdk" @@ -255,14 +256,19 @@ func (api *API) aiProvidersCreate(rw http.ResponseWriter, r *http.Request) { // @Success 200 {object} codersdk.AIProvider // @Router /api/v2/ai/providers/{idOrName} [patch] func (api *API) aiProvidersUpdate(rw http.ResponseWriter, r *http.Request) { + // keyOpsAudit attaches per-key add/remove/keep counts to the audit + // entry. Keys live in a separate table, so a key-only PATCH would + // otherwise produce an empty diff and hide rotation from the log. + keyOpsAudit := &aiProviderKeyOpsAudit{} var ( ctx = r.Context() auditor = api.Auditor.Load() aReq, commitAudit = audit.InitRequest[database.AIProvider](rw, &audit.RequestParams{ - Audit: *auditor, - Log: api.Logger, - Request: r, - Action: database.AuditActionWrite, + Audit: *auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionWrite, + AdditionalFields: keyOpsAudit, }) ) defer commitAudit() @@ -347,10 +353,12 @@ func (api *API) aiProvidersUpdate(rw http.ResponseWriter, r *http.Request) { aReq.New = updated if req.APIKeys != nil { - keys, err = applyAIProviderKeyOps(ctx, tx, updated.ID, *req.APIKeys) + var ops aiProviderKeyOpsAudit + keys, ops, err = applyAIProviderKeyOps(ctx, tx, updated.ID, *req.APIKeys) if err != nil { return err } + *keyOpsAudit = ops return nil } @@ -547,16 +555,37 @@ func insertAIProviderKeys(ctx context.Context, tx database.Store, providerID uui return out, nil } +// aiProviderKeyOpsAudit is serialized into the audit entry's +// additional_fields. Surfacing the per-key ID and masked secret for +// adds and removes gives operators a precise record of which keys +// rotated on a PATCH whose top-level diff would otherwise look empty. +// Kept is a count: a steady-state rotation commonly retains many keys, +// and per-entry detail there is noise. +type aiProviderKeyOpsAudit struct { + Added []aiProviderKeyOp `json:"added"` + Removed []aiProviderKeyOp `json:"removed"` + Kept int `json:"kept"` +} + +// aiProviderKeyOp identifies a single key affected by a PATCH. Masked +// is the one-way rendering produced by aibridgeutils.MaskSecret, so +// plaintext never lands in the audit log. +type aiProviderKeyOp struct { + ID uuid.UUID `json:"id"` + Masked string `json:"masked"` +} + // applyAIProviderKeyOps reconciles a provider's keys against the // supplied mutation list inside a transaction: kept-by-ID rows stay, // rows whose ID is absent from the list are deleted, and entries // carrying a plaintext APIKey are inserted as new rows. Caller is // responsible for prior validation (XOR per entry, no duplicate IDs). // IDs that do not belong to this provider return errAIProviderKeyUnknown. -func applyAIProviderKeyOps(ctx context.Context, tx database.Store, providerID uuid.UUID, muts []codersdk.AIProviderKeyMutation) ([]database.AIProviderKey, error) { +func applyAIProviderKeyOps(ctx context.Context, tx database.Store, providerID uuid.UUID, muts []codersdk.AIProviderKeyMutation) ([]database.AIProviderKey, aiProviderKeyOpsAudit, error) { + var ops aiProviderKeyOpsAudit existing, err := tx.GetAIProviderKeysByProviderID(ctx, providerID) if err != nil { - return nil, xerrors.Errorf("load existing ai provider keys: %w", err) + return nil, ops, xerrors.Errorf("load existing ai provider keys: %w", err) } existingByID := make(map[uuid.UUID]struct{}, len(existing)) for _, k := range existing { @@ -569,7 +598,7 @@ func applyAIProviderKeyOps(ctx context.Context, tx database.Store, providerID uu switch { case m.ID != nil: if _, ok := existingByID[*m.ID]; !ok { - return nil, xerrors.Errorf("%w: %s", errAIProviderKeyUnknown, *m.ID) + return nil, ops, xerrors.Errorf("%w: %s", errAIProviderKeyUnknown, *m.ID) } keep[*m.ID] = struct{}{} case m.APIKey != nil: @@ -582,19 +611,25 @@ func applyAIProviderKeyOps(ctx context.Context, tx database.Store, providerID uu continue } if err := tx.DeleteAIProviderKey(ctx, k.ID); err != nil { - return nil, xerrors.Errorf("delete ai provider key %s: %w", k.ID, err) + return nil, ops, xerrors.Errorf("delete ai provider key %s: %w", k.ID, err) } + ops.Removed = append(ops.Removed, aiProviderKeyOp{ID: k.ID, Masked: aibridgeutils.MaskSecret(k.APIKey)}) } - if _, err := insertAIProviderKeys(ctx, tx, providerID, inserts); err != nil { - return nil, err + added, err := insertAIProviderKeys(ctx, tx, providerID, inserts) + if err != nil { + return nil, ops, err } + for _, k := range added { + ops.Added = append(ops.Added, aiProviderKeyOp{ID: k.ID, Masked: aibridgeutils.MaskSecret(k.APIKey)}) + } + ops.Kept = len(keep) out, err := tx.GetAIProviderKeysByProviderID(ctx, providerID) if err != nil { - return nil, xerrors.Errorf("reload ai provider keys: %w", err) + return nil, ops, xerrors.Errorf("reload ai provider keys: %w", err) } - return out, nil + return out, ops, nil } // errAIProviderKeyUnknown is the sentinel returned by diff --git a/coderd/ai_providers_test.go b/coderd/ai_providers_test.go index 3e855d1bf5..401f4d13dd 100644 --- a/coderd/ai_providers_test.go +++ b/coderd/ai_providers_test.go @@ -10,7 +10,9 @@ import ( "github.com/google/uuid" "github.com/stretchr/testify/require" + "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/coderdtest" + "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/db2sdk" "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/util/ptr" @@ -992,6 +994,73 @@ func TestAIProvidersKeyManagement(t *testing.T) { require.Contains(t, sdkErr.Validations[0].Detail, "already referenced") }) + t.Run("PATCHKeysSurfacesOpsInAudit", func(t *testing.T) { + t.Parallel() + auditor := audit.NewMock() + client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) + _ = coderdtest.CreateFirstUser(t, client) + ctx := testutil.Context(t, testutil.WaitLong) + + // Without surfacing per-op detail, a PATCH that only rotates + // keys would produce an audit entry whose top-level diff is + // empty: invisible key rotation in the log. + //nolint:gocritic // Owner role is the audience for this endpoint. + provider, err := client.CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{ + Type: codersdk.AIProviderTypeOpenAI, + Name: "keys-audit-ops", + Enabled: true, + BaseURL: "https://api.openai.com/v1", + APIKeys: []string{ + "sk-openai-audit-1-ssssssssssssssssssss", //nolint:gosec // test fixture + "sk-openai-audit-2-tttttttttttttttttttt", //nolint:gosec // test fixture + }, + }) + require.NoError(t, err) + keepID := provider.APIKeys[0].ID + + // Keep one, drop one, add one. + mutations := []codersdk.AIProviderKeyMutation{ + {ID: &keepID}, + {APIKey: ptr.Ref("sk-openai-audit-3-uuuuuuuuuuuuuuuuuuuu")}, //nolint:gosec // test fixture + } + updatedProvider, err := client.UpdateAIProvider(ctx, provider.Name, codersdk.UpdateAIProviderRequest{ + APIKeys: &mutations, + }) + require.NoError(t, err) + + // The newly-inserted row's ID and masked rendering are dynamic; + // pull them from the PATCH response so we can build the expected + // audit payload without re-declaring the audit struct shape. + var added codersdk.AIProviderKey + for _, k := range updatedProvider.APIKeys { + if k.ID != keepID { + added = k + break + } + } + require.NotEqual(t, uuid.Nil, added.ID) + require.NotEmpty(t, added.Masked) + require.NotContains(t, added.Masked, "sk-openai-audit-3-uuuuuuuuuuuuuuuuuuuu") + removed := provider.APIKeys[1] + + logs := auditor.AuditLogs() + var updated *database.AuditLog + for i := range logs { + if logs[i].Action == database.AuditActionWrite && logs[i].ResourceType == database.ResourceTypeAIProvider { + updated = &logs[i] + } + } + require.NotNil(t, updated, "expected audit log for AI provider update") + + expected, err := json.Marshal(map[string]any{ + "added": []map[string]any{{"id": added.ID, "masked": added.Masked}}, + "removed": []map[string]any{{"id": removed.ID, "masked": removed.Masked}}, + "kept": 1, + }) + require.NoError(t, err) + require.JSONEq(t, string(expected), string(updated.AdditionalFields)) + }) + t.Run("MutationUnknownIDRejected", func(t *testing.T) { t.Parallel() client := coderdtest.New(t, nil) diff --git a/coderd/audit/request.go b/coderd/audit/request.go index 396f7b15bb..f8a3a5b1e4 100644 --- a/coderd/audit/request.go +++ b/coderd/audit/request.go @@ -284,7 +284,7 @@ func ResourceType[T Auditable](tgt T) database.ResourceType { case database.AiSeatState: return database.ResourceTypeAiSeat case database.AIProvider: - return database.ResourceTypeAiProvider + return database.ResourceTypeAIProvider case database.AuditableGroupAiBudget: return database.ResourceTypeGroupAiBudget case database.Chat: diff --git a/coderd/database/models.go b/coderd/database/models.go index e202da2795..f801be833b 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -3334,8 +3334,8 @@ const ( ResourceTypeAiSeat ResourceType = "ai_seat" ResourceTypeChat ResourceType = "chat" ResourceTypeUserSecret ResourceType = "user_secret" - ResourceTypeAiProvider ResourceType = "ai_provider" - ResourceTypeAiProviderKey ResourceType = "ai_provider_key" + ResourceTypeAIProvider ResourceType = "ai_provider" + ResourceTypeAIProviderKey ResourceType = "ai_provider_key" ResourceTypeGroupAiBudget ResourceType = "group_ai_budget" ResourceTypeUserSkill ResourceType = "user_skill" ) @@ -3406,8 +3406,8 @@ func (e ResourceType) Valid() bool { ResourceTypeAiSeat, ResourceTypeChat, ResourceTypeUserSecret, - ResourceTypeAiProvider, - ResourceTypeAiProviderKey, + ResourceTypeAIProvider, + ResourceTypeAIProviderKey, ResourceTypeGroupAiBudget, ResourceTypeUserSkill: return true @@ -3446,8 +3446,8 @@ func AllResourceTypeValues() []ResourceType { ResourceTypeAiSeat, ResourceTypeChat, ResourceTypeUserSecret, - ResourceTypeAiProvider, - ResourceTypeAiProviderKey, + ResourceTypeAIProvider, + ResourceTypeAIProviderKey, ResourceTypeGroupAiBudget, ResourceTypeUserSkill, } diff --git a/coderd/database/sqlc.yaml b/coderd/database/sqlc.yaml index 2b984d4c7e..62143bb247 100644 --- a/coderd/database/sqlc.yaml +++ b/coderd/database/sqlc.yaml @@ -260,6 +260,8 @@ sql: ai_provider: AIProvider ai_provider_key: AIProviderKey ai_provider_type: AIProviderType + resource_type_ai_provider: ResourceTypeAIProvider + resource_type_ai_provider_key: ResourceTypeAIProviderKey mcp_server_config: MCPServerConfig mcp_server_configs: MCPServerConfigs mcp_server_user_token: MCPServerUserToken