chore: surface key add/remove/keep counts in audit log (#25484)

This commit is contained in:
Danny Kopping
2026-05-20 14:44:57 +02:00
committed by GitHub
parent 7ffeac711c
commit 00e8b40cb0
5 changed files with 126 additions and 20 deletions
+48 -13
View File
@@ -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
+69
View File
@@ -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)
+1 -1
View File
@@ -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:
+6 -6
View File
@@ -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,
}
+2
View File
@@ -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