From 44b1edd4dab17749625072d93f6a62d89d2948be Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Wed, 20 May 2026 17:33:26 +0200 Subject: [PATCH] fix: unify key-ops audit shape and surface per-key detail (#25534) Adding missed commit from https://github.com/coder/coder/pull/25484 This formats the audit logs correctly ![image.png](https://app.graphite.com/user-attachments/assets/598d018b-cdf5-4a2c-8321-24ba2c650a1a.png) --- coderd/ai_providers.go | 83 ++++++++++++-- coderd/ai_providers_test.go | 65 +++++++++++ coderd/audit/diff.go | 1 + coderd/audit/request.go | 10 ++ docs/admin/security/audit-logs.md | 1 + enterprise/audit/diff_internal_test.go | 47 ++++++++ enterprise/audit/table.go | 8 ++ .../coderd/aibridge_provider_audit_test.go | 104 ++++++++++++++++++ 8 files changed, 308 insertions(+), 11 deletions(-) create mode 100644 enterprise/coderd/aibridge_provider_audit_test.go diff --git a/coderd/ai_providers.go b/coderd/ai_providers.go index 7a504da3c5..d791cf9470 100644 --- a/coderd/ai_providers.go +++ b/coderd/ai_providers.go @@ -20,6 +20,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/httpapi" + "github.com/coder/coder/v2/coderd/httpmw" "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/codersdk" ) @@ -233,6 +234,8 @@ func (api *API) aiProvidersCreate(rw http.ResponseWriter, r *http.Request) { } aReq.New = row + auditAIProviderKeyChanges(ctx, r, *auditor, api.Logger, aiProviderKeyChanges{Added: keys}) + sdk, err := db2sdk.AIProvider(row, keys) if err != nil { api.Logger.Error(ctx, "convert AI provider", slog.F("provider_id", row.ID), slog.Error(err)) @@ -295,8 +298,9 @@ func (api *API) aiProvidersUpdate(rw http.ResponseWriter, r *http.Request) { idOrName := chi.URLParam(r, "idOrName") var ( - updated database.AIProvider - keys []database.AIProviderKey + updated database.AIProvider + keys []database.AIProviderKey + keyChanges aiProviderKeyChanges ) err := api.Database.InTx(func(tx database.Store) error { old, err := lookupAIProvider(ctx, tx, idOrName) @@ -354,7 +358,7 @@ func (api *API) aiProvidersUpdate(rw http.ResponseWriter, r *http.Request) { if req.APIKeys != nil { var ops aiProviderKeyOpsAudit - keys, ops, err = applyAIProviderKeyOps(ctx, tx, updated.ID, *req.APIKeys) + keys, ops, keyChanges, err = applyAIProviderKeyOps(ctx, tx, updated.ID, *req.APIKeys) if err != nil { return err } @@ -395,6 +399,8 @@ func (api *API) aiProvidersUpdate(rw http.ResponseWriter, r *http.Request) { return } + auditAIProviderKeyChanges(ctx, r, *auditor, api.Logger, keyChanges) + sdk, err := db2sdk.AIProvider(updated, keys) if err != nil { api.Logger.Error(ctx, "convert AI provider", slog.F("provider_id", updated.ID), slog.Error(err)) @@ -575,17 +581,70 @@ type aiProviderKeyOp struct { Masked string `json:"masked"` } +// aiProviderKeyChanges captures the rows added and removed by +// applyAIProviderKeyOps so the caller can emit one audit entry per +// affected key after the transaction commits. +type aiProviderKeyChanges struct { + Added []database.AIProviderKey + Removed []database.AIProviderKey +} + +// auditAIProviderKeyChanges emits one audit entry per added or removed +// key, attributed to the actor on the HTTP request. Per-key entries +// keep key rotation visible in the audit log because the parent +// AIProvider audit diff is empty for key-only PATCHes (keys live in a +// separate table). +// +// APIKey is replaced with the masked rendering before the row reaches +// the audit pipeline so plaintext keys never land in the diff or any +// audit backend, independent of the api_key column's audit policy. +func auditAIProviderKeyChanges(ctx context.Context, r *http.Request, auditor audit.Auditor, log slog.Logger, changes aiProviderKeyChanges) { + if len(changes.Added) == 0 && len(changes.Removed) == 0 { + return + } + key, ok := httpmw.APIKeyOptional(r) + if !ok { + return + } + requestID, _ := httpmw.RequestIDOptional(r) + emit := func(action database.AuditAction, before, after database.AIProviderKey) { + before.APIKey = aibridgeutils.MaskSecret(before.APIKey) + after.APIKey = aibridgeutils.MaskSecret(after.APIKey) + audit.BackgroundAudit(ctx, &audit.BackgroundAuditParams[database.AIProviderKey]{ + Audit: auditor, + Log: log, + UserID: key.UserID, + RequestID: requestID, + Status: http.StatusOK, + IP: r.RemoteAddr, + UserAgent: r.UserAgent(), + Action: action, + Old: before, + New: after, + }) + } + for _, k := range changes.Removed { + emit(database.AuditActionDelete, k, database.AIProviderKey{}) + } + for _, k := range changes.Added { + emit(database.AuditActionCreate, database.AIProviderKey{}, k) + } +} + // 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, aiProviderKeyOpsAudit, error) { - var ops aiProviderKeyOpsAudit +func applyAIProviderKeyOps(ctx context.Context, tx database.Store, providerID uuid.UUID, muts []codersdk.AIProviderKeyMutation) ([]database.AIProviderKey, aiProviderKeyOpsAudit, aiProviderKeyChanges, error) { + var ( + ops aiProviderKeyOpsAudit + changes aiProviderKeyChanges + ) existing, err := tx.GetAIProviderKeysByProviderID(ctx, providerID) if err != nil { - return nil, ops, xerrors.Errorf("load existing ai provider keys: %w", err) + return nil, ops, changes, xerrors.Errorf("load existing ai provider keys: %w", err) } existingByID := make(map[uuid.UUID]struct{}, len(existing)) for _, k := range existing { @@ -598,7 +657,7 @@ func applyAIProviderKeyOps(ctx context.Context, tx database.Store, providerID uu switch { case m.ID != nil: if _, ok := existingByID[*m.ID]; !ok { - return nil, ops, xerrors.Errorf("%w: %s", errAIProviderKeyUnknown, *m.ID) + return nil, ops, changes, xerrors.Errorf("%w: %s", errAIProviderKeyUnknown, *m.ID) } keep[*m.ID] = struct{}{} case m.APIKey != nil: @@ -611,25 +670,27 @@ func applyAIProviderKeyOps(ctx context.Context, tx database.Store, providerID uu continue } if err := tx.DeleteAIProviderKey(ctx, k.ID); err != nil { - return nil, ops, xerrors.Errorf("delete ai provider key %s: %w", k.ID, err) + return nil, ops, changes, 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)}) + changes.Removed = append(changes.Removed, k) } added, err := insertAIProviderKeys(ctx, tx, providerID, inserts) if err != nil { - return nil, ops, err + return nil, ops, changes, err } for _, k := range added { ops.Added = append(ops.Added, aiProviderKeyOp{ID: k.ID, Masked: aibridgeutils.MaskSecret(k.APIKey)}) } + changes.Added = append(changes.Added, added...) ops.Kept = len(keep) out, err := tx.GetAIProviderKeysByProviderID(ctx, providerID) if err != nil { - return nil, ops, xerrors.Errorf("reload ai provider keys: %w", err) + return nil, ops, changes, xerrors.Errorf("reload ai provider keys: %w", err) } - return out, ops, nil + return out, ops, changes, nil } // errAIProviderKeyUnknown is the sentinel returned by diff --git a/coderd/ai_providers_test.go b/coderd/ai_providers_test.go index 401f4d13dd..5e22023464 100644 --- a/coderd/ai_providers_test.go +++ b/coderd/ai_providers_test.go @@ -994,6 +994,52 @@ func TestAIProvidersKeyManagement(t *testing.T) { require.Contains(t, sdkErr.Validations[0].Detail, "already referenced") }) + t.Run("PATCHPropertiesAudited", 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) + + //nolint:gocritic // Owner role is the audience for this endpoint. + provider, err := client.CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{ + Type: codersdk.AIProviderTypeOpenAI, + Name: "keys-props-audit", + Enabled: true, + BaseURL: "https://api.openai.com/v1", + }) + require.NoError(t, err) + + // Reset before the update so we look only at audits produced by + // the PATCH (the create path emits its own AIProvider audit). + auditor.ResetLogs() + + newDisplay := "Renamed" + newURL := "https://api.openai.com/v2" + disabled := false + _, err = client.UpdateAIProvider(ctx, provider.Name, codersdk.UpdateAIProviderRequest{ + DisplayName: &newDisplay, + BaseURL: &newURL, + Enabled: &disabled, + }) + require.NoError(t, err) + + // The parent AIProvider audit entry fires for property-only + // PATCHes; the enterprise auditor populates the diff with the + // changed fields (display_name, base_url, enabled). The mock + // auditor used here returns an empty diff so we only assert the + // entry shape; the actual diff content is exercised by the + // enterprise audit unit tests. + var sawUpdate bool + for _, lg := range auditor.AuditLogs() { + if lg.Action == database.AuditActionWrite && lg.ResourceType == database.ResourceTypeAIProvider { + require.Equal(t, provider.ID, lg.ResourceID) + sawUpdate = true + } + } + require.True(t, sawUpdate, "expected parent AIProvider audit for property-only PATCH") + }) + t.Run("PATCHKeysSurfacesOpsInAudit", func(t *testing.T) { t.Parallel() auditor := audit.NewMock() @@ -1059,6 +1105,25 @@ func TestAIProvidersKeyManagement(t *testing.T) { }) require.NoError(t, err) require.JSONEq(t, string(expected), string(updated.AdditionalFields)) + + // Per-key audit entries surface the added/removed keys as their + // own log lines, so a key-only PATCH is visible even without + // frontend changes. The Create handler also emits per-key + // audits for the initial two keys, so match by ResourceID. + var sawCreate, sawDelete bool + for _, lg := range logs { + if lg.ResourceType != database.ResourceTypeAIProviderKey { + continue + } + switch { + case lg.Action == database.AuditActionCreate && lg.ResourceID == added.ID: + sawCreate = true + case lg.Action == database.AuditActionDelete && lg.ResourceID == removed.ID: + sawDelete = true + } + } + require.True(t, sawCreate, "expected create audit for added key") + require.True(t, sawDelete, "expected delete audit for removed key") }) t.Run("MutationUnknownIDRejected", func(t *testing.T) { diff --git a/coderd/audit/diff.go b/coderd/audit/diff.go index 0b85b0e700..a26924552b 100644 --- a/coderd/audit/diff.go +++ b/coderd/audit/diff.go @@ -35,6 +35,7 @@ type Auditable interface { database.TaskTable | database.AiSeatState | database.AIProvider | + database.AIProviderKey | database.Chat | database.AuditableGroupAiBudget | database.UserSecret | diff --git a/coderd/audit/request.go b/coderd/audit/request.go index f8a3a5b1e4..c690bd56f1 100644 --- a/coderd/audit/request.go +++ b/coderd/audit/request.go @@ -136,6 +136,8 @@ func ResourceTarget[T Auditable](tgt T) string { return "AI Seat" case database.AIProvider: return typed.Name + case database.AIProviderKey: + return typed.ID.String() case database.AuditableGroupAiBudget: return typed.GroupName case database.Chat: @@ -218,6 +220,8 @@ func ResourceID[T Auditable](tgt T) uuid.UUID { return typed.UserID case database.AIProvider: return typed.ID + case database.AIProviderKey: + return typed.ID case database.AuditableGroupAiBudget: return typed.GroupID case database.Chat: @@ -285,6 +289,8 @@ func ResourceType[T Auditable](tgt T) database.ResourceType { return database.ResourceTypeAiSeat case database.AIProvider: return database.ResourceTypeAIProvider + case database.AIProviderKey: + return database.ResourceTypeAIProviderKey case database.AuditableGroupAiBudget: return database.ResourceTypeGroupAiBudget case database.Chat: @@ -356,6 +362,10 @@ func ResourceRequiresOrgID[T Auditable]() bool { case database.AIProvider: // AI providers are deployment-scoped, not org-scoped. return false + case database.AIProviderKey: + // AI provider keys inherit the deployment scope of their parent + // provider. + return false case database.AuditableGroupAiBudget: // Group AI budgets are org-scoped through their parent group. return true diff --git a/docs/admin/security/audit-logs.md b/docs/admin/security/audit-logs.md index ee109541b3..712724e064 100644 --- a/docs/admin/security/audit-logs.md +++ b/docs/admin/security/audit-logs.md @@ -16,6 +16,7 @@ We track the following resources: | Resource | | | |-----------------------------------------------------------------|----------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | AIProvider
create, write, delete | |
FieldTracked
base_urltrue
created_atfalse
deletedtrue
display_nametrue
enabledtrue
idtrue
nametrue
settingstrue
settings_key_idfalse
typetrue
updated_atfalse
| +| AIProviderKey
create, delete | |
FieldTracked
api_keytrue
api_key_key_idfalse
created_atfalse
idtrue
provider_idtrue
updated_atfalse
| | APIKey
login, logout, register, create, write, delete | |
FieldTracked
allow_listfalse
created_attrue
expires_attrue
hashed_secretfalse
idfalse
ip_addressfalse
last_usedtrue
lifetime_secondsfalse
login_typefalse
scopesfalse
token_namefalse
updated_atfalse
user_idtrue
| | AiSeatState
create | |
FieldTracked
first_used_attrue
last_event_descriptiontrue
last_event_typetrue
last_used_atfalse
updated_atfalse
user_idtrue
| | AuditOAuthConvertState
| |
FieldTracked
created_attrue
expires_attrue
from_login_typetrue
to_login_typetrue
user_idtrue
| diff --git a/enterprise/audit/diff_internal_test.go b/enterprise/audit/diff_internal_test.go index 33566d64d1..a96a5abe23 100644 --- a/enterprise/audit/diff_internal_test.go +++ b/enterprise/audit/diff_internal_test.go @@ -457,6 +457,53 @@ func Test_diff(t *testing.T) { }, }, }) + + runDiffTests(t, []diffTest{ + { + name: "PropertyChange", + left: database.AIProvider{ + ID: uuid.UUID{1}, + Type: database.AiProviderTypeOpenai, + Name: "primary-openai", + DisplayName: sql.NullString{String: "Primary", Valid: true}, + Enabled: true, + BaseUrl: "https://api.openai.com/v1", + }, + right: database.AIProvider{ + ID: uuid.UUID{1}, + Type: database.AiProviderTypeOpenai, + Name: "primary-openai", + DisplayName: sql.NullString{String: "Renamed", Valid: true}, + Enabled: false, + BaseUrl: "https://api.openai.com/v2", + }, + exp: audit.Map{ + "display_name": audit.OldNew{Old: "Primary", New: "Renamed"}, + "enabled": audit.OldNew{Old: true, New: false}, + "base_url": audit.OldNew{Old: "https://api.openai.com/v1", New: "https://api.openai.com/v2"}, + }, + }, + }) + + runDiffTests(t, []diffTest{ + { + // api_key is tracked, but callers must pre-mask before the + // row reaches the audit pipeline. The pre-masked rendering + // (sk-prefix...suffix) is what flows into the diff. + name: "PreMaskedKeyFlowsThrough", + left: audit.Empty[database.AIProviderKey](), + right: database.AIProviderKey{ + ID: uuid.UUID{1}, + ProviderID: uuid.UUID{2}, + APIKey: "sk-a...wxyz", + }, + exp: audit.Map{ + "id": audit.OldNew{Old: "", New: uuid.UUID{1}.String()}, + "provider_id": audit.OldNew{Old: "", New: uuid.UUID{2}.String()}, + "api_key": audit.OldNew{Old: "", New: "sk-a...wxyz"}, + }, + }, + }) } func runDiffTests(t *testing.T, tests []diffTest) { diff --git a/enterprise/audit/table.go b/enterprise/audit/table.go index 565072ef8c..e97a76daed 100644 --- a/enterprise/audit/table.go +++ b/enterprise/audit/table.go @@ -392,6 +392,14 @@ var auditableResourcesTypes = map[any]map[string]Action{ "created_at": ActionIgnore, // Implicit; not useful in a diff. "updated_at": ActionIgnore, // Changes; not useful in a diff. }, + &database.AIProviderKey{}: { + "id": ActionTrack, + "provider_id": ActionTrack, + "api_key": ActionTrack, // Callers must pre-mask before auditing; the audit pipeline never sees plaintext. + "api_key_key_id": ActionIgnore, // dbcrypt key reference, derivable. + "created_at": ActionIgnore, // Implicit; not useful in a diff. + "updated_at": ActionIgnore, // Changes; not useful in a diff. + }, &database.TaskTable{}: { "id": ActionTrack, "organization_id": ActionIgnore, // Never changes. diff --git a/enterprise/coderd/aibridge_provider_audit_test.go b/enterprise/coderd/aibridge_provider_audit_test.go new file mode 100644 index 0000000000..1a67340646 --- /dev/null +++ b/enterprise/coderd/aibridge_provider_audit_test.go @@ -0,0 +1,104 @@ +package coderd_test + +import ( + "context" + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "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/dbauthz" + "github.com/coder/coder/v2/coderd/database/dbtestutil" + "github.com/coder/coder/v2/codersdk" + entaudit "github.com/coder/coder/v2/enterprise/audit" + "github.com/coder/coder/v2/enterprise/audit/backends" + "github.com/coder/coder/v2/enterprise/coderd/coderdenttest" + "github.com/coder/coder/v2/enterprise/coderd/license" + "github.com/coder/coder/v2/testutil" +) + +// TestAIProviderAuditDiff exercises the full HTTP -> enterprise auditor +// -> Postgres write path for AI provider updates. The mock auditor used +// elsewhere returns an empty diff, so this is the only place that +// proves changed properties land in the audit_logs row. +func TestAIProviderAuditDiff(t *testing.T) { + t.Parallel() + + db, ps := dbtestutil.NewDB(t) + auditor := entaudit.NewAuditor( + db, + entaudit.DefaultFilter, + backends.NewPostgres(db, true), + ) + + ownerClient, _ := coderdenttest.New(t, &coderdenttest.Options{ + AuditLogging: true, + Options: &coderdtest.Options{ + Database: db, + Pubsub: ps, + Auditor: auditor, + }, + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureAuditLog: 1, + }, + }, + }) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitMedium) + defer cancel() + + //nolint:gocritic // Owner role is the audience for this endpoint. + provider, err := ownerClient.CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{ + Type: codersdk.AIProviderTypeOpenAI, + Name: "audit-target", + DisplayName: "Audit Target", + Enabled: true, + BaseURL: "https://api.openai.com/v1", + }) + require.NoError(t, err) + + newDisplay := "Renamed" + newURL := "https://api.openai.com/v2" + disabled := false + _, err = ownerClient.UpdateAIProvider(ctx, provider.Name, codersdk.UpdateAIProviderRequest{ + DisplayName: &newDisplay, + BaseURL: &newURL, + Enabled: &disabled, + }) + require.NoError(t, err) + + rows, err := db.GetAuditLogsOffset( + dbauthz.AsSystemRestricted(ctx), + database.GetAuditLogsOffsetParams{ + ResourceType: string(database.ResourceTypeAIProvider), + LimitOpt: 10, + }, + ) + require.NoError(t, err) + require.Len(t, rows, 2, "expected one create and one update audit row") + + // GetAuditLogsOffset returns entries sorted by time in descending order. + updateLog := rows[0].AuditLog + require.Equal(t, database.AuditActionWrite, updateLog.Action) + + var updateDiff audit.Map + require.NoError(t, json.Unmarshal(updateLog.Diff, &updateDiff)) + + if assert.Contains(t, updateDiff, "display_name", "display_name missing from diff") { + assert.Equal(t, "Audit Target", updateDiff["display_name"].Old) + assert.Equal(t, newDisplay, updateDiff["display_name"].New) + } + if assert.Contains(t, updateDiff, "base_url", "base_url missing from diff") { + assert.Equal(t, "https://api.openai.com/v1", updateDiff["base_url"].Old) + assert.Equal(t, newURL, updateDiff["base_url"].New) + } + if assert.Contains(t, updateDiff, "enabled", "enabled missing from diff") { + assert.Equal(t, true, updateDiff["enabled"].Old) + assert.Equal(t, false, updateDiff["enabled"].New) + } +}