From 117e728b337ed99bfc530e117473b8088594819f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Banaszewski?= Date: Wed, 23 Sep 2026 11:24:46 +0200 Subject: [PATCH] feat: restore AI Provider key checks (#29677) Adds key validation to the AI Providers API that was present in env variables path. * Reject duplicate API keys on create and update, including duplicates between new keys and retained keys. * Enforce a maximum of five keys per provider, counting retained and new keys on update. * Require Bedrock access keys and secrets to be both set or both absent/empty. Validate merged settings on update to allow partial rotations but reject unpaired credentials and partial clears. Extends and consolidates SDK/API tests covering these checks and unchanged state after rejected updates. > Generated with Coder Agents. --- coderd/ai_providers.go | 41 +- coderd/ai_providers_test.go | 861 +++++++++++++++++++++----------- codersdk/aiproviders.go | 57 ++- codersdk/aiproviders_bedrock.go | 20 + codersdk/aiproviders_test.go | 166 ++++++ site/src/api/typesGenerated.ts | 6 + 6 files changed, 848 insertions(+), 303 deletions(-) diff --git a/coderd/ai_providers.go b/coderd/ai_providers.go index 93b7908b27..0c7738a8d8 100644 --- a/coderd/ai_providers.go +++ b/coderd/ai_providers.go @@ -657,12 +657,28 @@ func (api *API) aiProviderStatusFromDB(ctx context.Context, provider database.AI return api.aiProviderStatus(provider, buildHostnameCollisionMap(rows)) } +type aiProviderValidationError struct { + Validations []codersdk.ValidationError +} + +func (*aiProviderValidationError) Error() string { + return "invalid AI provider request" +} + // writeAIProviderError translates an error from the AI provider // lookup/update/delete paths into the right HTTP status code. logMsg // labels the log line for operator debugging, and userMsg is the // internal-error response message shown to the API consumer when no // more specific branch fires. func writeAIProviderError(ctx context.Context, logger slog.Logger, rw http.ResponseWriter, err error, logMsg, userMsg string) { + var validationErr *aiProviderValidationError + if errors.As(err, &validationErr) { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Invalid AI provider request.", + Validations: validationErr.Validations, + }) + return + } if errors.Is(err, errAIProviderInvalidName) { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: fmt.Sprintf("Invalid provider id or name: must be a UUID or match %s.", codersdk.AIProviderNameRegex), @@ -810,23 +826,33 @@ func applyAIProviderKeyOps(ctx context.Context, tx database.Store, providerID uu if err != nil { return nil, ops, changes, xerrors.Errorf("load existing ai provider keys: %w", err) } - existingByID := make(map[uuid.UUID]struct{}, len(existing)) + existingByID := make(map[uuid.UUID]string, len(existing)) for _, k := range existing { - existingByID[k.ID] = struct{}{} + existingByID[k.ID] = k.APIKey } keep := make(map[uuid.UUID]struct{}, len(muts)) + seenKeys := make(map[string]int, len(muts)) var inserts []string - for _, m := range muts { + for i, m := range muts { + var key string switch { case m.ID != nil: - if _, ok := existingByID[*m.ID]; !ok { + var ok bool + key, ok = existingByID[*m.ID] + if !ok { return nil, ops, changes, xerrors.Errorf("%w: %s", errAIProviderKeyUnknown, *m.ID) } keep[*m.ID] = struct{}{} case m.APIKey != nil: - inserts = append(inserts, *m.APIKey) + key = *m.APIKey + inserts = append(inserts, key) } + // Compare the final key set, not keys being removed by this patch. + if validations := codersdk.ValidateAIProviderKeyUniqueness(key, "api_keys", seenKeys); len(validations) > 0 { + return nil, ops, changes, &aiProviderValidationError{Validations: validations} + } + seenKeys[key] = i } for _, k := range existing { @@ -892,6 +918,11 @@ func lookupAndMergeSettings(ctx context.Context, db database.Store, idOrName str } if patch != nil { settings = mergeAIProviderSettings(settings, *patch) + if settings.Bedrock != nil { + if validations := settings.Bedrock.ValidateCredentials(); len(validations) > 0 { + return database.AIProvider{}, codersdk.AIProviderSettings{}, &aiProviderValidationError{Validations: validations} + } + } } return old, settings, nil } diff --git a/coderd/ai_providers_test.go b/coderd/ai_providers_test.go index afd6cb23fa..432a1af3c0 100644 --- a/coderd/ai_providers_test.go +++ b/coderd/ai_providers_test.go @@ -673,6 +673,69 @@ func TestAIProvidersCRUD(t *testing.T) { require.Equal(t, "anthropic.claude-3-5-haiku", updated.Settings.Bedrock.SmallFastModel) }) + t.Run("CredentialsMustBePairedOnCreate", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, nil) + _ = coderdtest.CreateFirstUser(t, client) + + tests := []struct { + name string + provider string + accessKey *string + accessKeySecret *string + missingField string + detail string + secretNotInError string + }{ + { + name: "MissingAccessKey", + provider: "bedrock-create-missing-access-key", + accessKeySecret: new("create-secret-only"), + missingField: "settings.access_key", + detail: "access_key_secret is set, but access_key is missing or empty", + secretNotInError: "create-secret-only", + }, + { + name: "MissingAccessKeySecret", + provider: "bedrock-create-missing-access-key-secret", + accessKey: new("AKIA-create-key-only"), //nolint:gosec // test fixture + missingField: "settings.access_key_secret", + detail: "access_key is set, but access_key_secret is missing or empty", + secretNotInError: "AKIA-create-key-only", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + _, err := client.CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{ + Type: codersdk.AIProviderTypeAnthropic, + Name: tt.provider, + Enabled: true, + BaseURL: "https://bedrock-runtime.us-east-1.amazonaws.com/", + Settings: codersdk.AIProviderSettings{ + Bedrock: &codersdk.AIProviderBedrockSettings{ + Region: "us-east-1", + Model: "anthropic.claude-3-5-sonnet", + SmallFastModel: "anthropic.claude-3-5-haiku", + AccessKey: tt.accessKey, + AccessKeySecret: tt.accessKeySecret, + }, + }, + }) + sdkErr := requireSDKError(t, err, http.StatusBadRequest) + require.Equal(t, "Invalid AI provider request.", sdkErr.Message) + require.Contains(t, sdkErr.Validations, codersdk.ValidationError{ + Field: tt.missingField, + Detail: tt.detail, + }) + body, marshalErr := json.Marshal(sdkErr) + require.NoError(t, marshalErr) + require.NotContains(t, string(body), tt.secretNotInError) + }) + } + }) + t.Run("BedrockSecretsHidden", func(t *testing.T) { t.Parallel() client := coderdtest.New(t, nil) @@ -734,10 +797,15 @@ func TestAIProvidersKeyManagement(t *testing.T) { Name: "keys-openai", Enabled: true, BaseURL: "https://api.openai.com/v1", - APIKeys: []string{primary, secondary}, + APIKeys: []string{ + primary, secondary, + "sk-openai-third-fixture-cccccc", //nolint:gosec // test fixture + "sk-openai-fourth-fixture-dddddd", //nolint:gosec // test fixture + "sk-openai-fifth-fixture-eeeeee", //nolint:gosec // test fixture + }, }) require.NoError(t, err) - require.Len(t, provider.APIKeys, 2) + require.Len(t, provider.APIKeys, 5) // Masked form preserves prefix and suffix while hiding the // middle, so it's enough for an operator to recognize the key // without recovering the plaintext. @@ -745,8 +813,9 @@ func TestAIProvidersKeyManagement(t *testing.T) { require.True(t, strings.HasSuffix(provider.APIKeys[0].Masked, "aaaa")) require.NotContains(t, provider.APIKeys[0].Masked, primary) require.NotContains(t, provider.APIKeys[1].Masked, secondary) - require.NotEqual(t, uuid.Nil, provider.APIKeys[0].ID) - require.NotEqual(t, uuid.Nil, provider.APIKeys[1].ID) + for _, key := range provider.APIKeys { + require.NotEqual(t, uuid.Nil, key.ID) + } }) t.Run("ResponseHidesPlaintext", func(t *testing.T) { @@ -782,33 +851,52 @@ func TestAIProvidersKeyManagement(t *testing.T) { t.Parallel() client := coderdtest.New(t, nil) _ = 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-replace", - Enabled: true, - BaseURL: "https://api.openai.com/v1", - APIKeys: []string{"sk-openai-original-ddddddddddddddd"}, //nolint:gosec // test fixture, not a real credential - }) - require.NoError(t, err) - require.Len(t, provider.APIKeys, 1) - originalID := provider.APIKeys[0].ID + const original = "sk-openai-original-ddddddddddddddd" //nolint:gosec // test fixture + for _, tt := range []struct { + name string + replacement []codersdk.AIProviderKeyMutation + }{ + { + name: "DifferentPlaintext", + replacement: []codersdk.AIProviderKeyMutation{ + {APIKey: new("sk-openai-rotated-eeeeeeeeeeeeeeeeeee")}, //nolint:gosec // test fixture + {APIKey: new("sk-openai-rotated-second-ffffffffffffffff")}, //nolint:gosec // test fixture + }, + }, + { + name: "SamePlaintext", + replacement: []codersdk.AIProviderKeyMutation{{APIKey: new(original)}}, + }, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) - // Omitting the original ID from the mutation list deletes it; - // the two APIKey-bearing entries add fresh rows. - replacement := []codersdk.AIProviderKeyMutation{ - {APIKey: new("sk-openai-rotated-eeeeeeeeeeeeeeeeeee")}, //nolint:gosec // test fixture - {APIKey: new("sk-openai-rotated-second-ffffffffffffffff")}, //nolint:gosec // test fixture - } - updated, err := client.UpdateAIProvider(ctx, provider.Name, codersdk.UpdateAIProviderRequest{ - APIKeys: &replacement, - }) - require.NoError(t, err) - require.Len(t, updated.APIKeys, 2) - for _, k := range updated.APIKeys { - require.NotEqual(t, originalID, k.ID) + //nolint:gocritic // Owner role is the audience for this endpoint. + provider, err := client.CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{ + Type: codersdk.AIProviderTypeOpenAI, + Name: "keys-replace-" + strings.ToLower(tt.name), + Enabled: true, + BaseURL: "https://api.openai.com/v1", + APIKeys: []string{original}, + }) + require.NoError(t, err) + require.Len(t, provider.APIKeys, 1) + originalID := provider.APIKeys[0].ID + + // Omitting the original ID deletes it; plaintext entries create + // fresh rows even when the secret is unchanged. + updated, err := client.UpdateAIProvider(ctx, provider.Name, codersdk.UpdateAIProviderRequest{ + APIKeys: &tt.replacement, + }) + require.NoError(t, err) + require.Len(t, updated.APIKeys, len(tt.replacement)) + for _, k := range updated.APIKeys { + require.NotEqual(t, uuid.Nil, k.ID) + require.NotEqual(t, originalID, k.ID) + } + }) } }) @@ -835,17 +923,20 @@ func TestAIProvidersKeyManagement(t *testing.T) { keepMasked := provider.APIKeys[0].Masked evictID := provider.APIKeys[1].ID - // Reference only keepID and add one new plaintext: evictID is - // implicitly removed. + // Reference only keepID and add four new keys, reaching the five-key + // limit. evictID is implicitly removed. patch := []codersdk.AIProviderKeyMutation{ {ID: &keepID}, {APIKey: new("sk-openai-added-cccccccccccccccccccccc")}, //nolint:gosec // test fixture + {APIKey: new("sk-openai-added-2-dddddddddddddddddddd")}, //nolint:gosec // test fixture + {APIKey: new("sk-openai-added-3-eeeeeeeeeeeeeeeeeeee")}, //nolint:gosec // test fixture + {APIKey: new("sk-openai-added-4-ffffffffffffffffffff")}, //nolint:gosec // test fixture } updated, err := client.UpdateAIProvider(ctx, provider.Name, codersdk.UpdateAIProviderRequest{ APIKeys: &patch, }) require.NoError(t, err) - require.Len(t, updated.APIKeys, 2) + require.Len(t, updated.APIKeys, 5) ids := keyIDs(updated.APIKeys) require.Contains(t, ids, keepID) require.NotContains(t, ids, evictID) @@ -1082,68 +1173,77 @@ func TestAIProvidersKeyManagement(t *testing.T) { require.Contains(t, sdkErr.Message, "Copilot providers do not accept api_keys") }) - t.Run("EmptyKeyRejected", func(t *testing.T) { + t.Run("CreateRejectsInvalidKeys", func(t *testing.T) { t.Parallel() client := coderdtest.New(t, nil) _ = coderdtest.CreateFirstUser(t, client) - ctx := testutil.Context(t, testutil.WaitLong) - //nolint:gocritic // Owner role is the audience for this endpoint. - _, err := client.CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{ - Type: codersdk.AIProviderTypeOpenAI, - Name: "keys-empty-element", - Enabled: true, - BaseURL: "https://api.openai.com/v1", - APIKeys: []string{""}, - }) - require.Error(t, err) - var sdkErr *codersdk.Error - require.ErrorAs(t, err, &sdkErr) - require.Equal(t, http.StatusBadRequest, sdkErr.StatusCode()) - require.Contains(t, sdkErr.Message, "Invalid AI provider request") - require.Len(t, sdkErr.Validations, 1) - require.Equal(t, "api_keys[0]", sdkErr.Validations[0].Field) - require.Contains(t, sdkErr.Validations[0].Detail, "must not be empty") - }) - - t.Run("WhitespaceKeyRejected", func(t *testing.T) { - t.Parallel() - client := coderdtest.New(t, nil) - _ = coderdtest.CreateFirstUser(t, client) - ctx := testutil.Context(t, testutil.WaitLong) - - // Surrounding whitespace would silently break upstream auth, - // since the server stores credentials verbatim. Reject up-front - // so the operator gets a clear signal instead of a 401 later. - //nolint:gocritic // Owner role is the audience for this endpoint. - _, err := client.CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{ - Type: codersdk.AIProviderTypeOpenAI, - Name: "keys-whitespace-create", - Enabled: true, - BaseURL: "https://api.openai.com/v1", - APIKeys: []string{" sk-openai-padded-nnnnnnnnnnnnnnnnnnnn "}, //nolint:gosec // test fixture - }) - require.Error(t, err) - var sdkErr *codersdk.Error - require.ErrorAs(t, err, &sdkErr) - require.Equal(t, http.StatusBadRequest, sdkErr.StatusCode()) - - provider, err := client.CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{ - Type: codersdk.AIProviderTypeOpenAI, - Name: "keys-whitespace-update", - Enabled: true, - BaseURL: "https://api.openai.com/v1", - APIKeys: []string{"sk-openai-clean-oooooooooooooooooooo"}, //nolint:gosec // test fixture - }) - require.NoError(t, err) - padded := " sk-openai-padded-pppppppppppppppppppp " - muts := []codersdk.AIProviderKeyMutation{{APIKey: &padded}} - _, err = client.UpdateAIProvider(ctx, provider.Name, codersdk.UpdateAIProviderRequest{ - APIKeys: &muts, - }) - require.Error(t, err) - require.ErrorAs(t, err, &sdkErr) - require.Equal(t, http.StatusBadRequest, sdkErr.StatusCode()) + const duplicateKey = "sk-openai-create-duplicate-vvvvvvvvvvvvvvvv" //nolint:gosec // test fixture + for _, tt := range []struct { + name string + keys []string + field string + detail string + notContain string + }{ + { + name: "Empty", + keys: []string{""}, + field: "api_keys[0]", + detail: "must not be empty", + }, + { + // Surrounding whitespace would silently break upstream auth, + // since credentials are stored verbatim. + name: "Whitespace", + keys: []string{" sk-openai-padded-nnnnnnnnnnnnnnnnnnnn "}, //nolint:gosec // test fixture + field: "api_keys[0]", + detail: "must not contain leading or trailing whitespace", + }, + { + name: "Duplicate", + keys: []string{duplicateKey, duplicateKey}, + field: "api_keys[1]", + detail: "duplicate key already provided at api_keys[0]", + notContain: duplicateKey, + }, + { + name: "TooMany", + keys: []string{ + "sk-openai-create-max-1-vvvvvvvvvvvvvvvv", //nolint:gosec // test fixture + "sk-openai-create-max-2-vvvvvvvvvvvvvvvv", //nolint:gosec // test fixture + "sk-openai-create-max-3-vvvvvvvvvvvvvvvv", //nolint:gosec // test fixture + "sk-openai-create-max-4-vvvvvvvvvvvvvvvv", //nolint:gosec // test fixture + "sk-openai-create-max-5-vvvvvvvvvvvvvvvv", //nolint:gosec // test fixture + "sk-openai-create-max-6-vvvvvvvvvvvvvvvv", //nolint:gosec // test fixture + }, + field: "api_keys", + detail: "api_keys must contain at most 5 keys", + }, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + //nolint:gocritic // Owner role is the audience for this endpoint. + _, err := client.CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{ + Type: codersdk.AIProviderTypeOpenAI, + Name: "keys-create-invalid-" + strings.ToLower(tt.name), + Enabled: true, + BaseURL: "https://api.openai.com/v1", + APIKeys: tt.keys, + }) + sdkErr := requireSDKError(t, err, http.StatusBadRequest) + require.Equal(t, "Invalid AI provider request.", sdkErr.Message) + require.Len(t, sdkErr.Validations, 1) + require.Equal(t, tt.field, sdkErr.Validations[0].Field) + require.Contains(t, sdkErr.Validations[0].Detail, tt.detail) + if tt.notContain != "" { + body, marshalErr := json.Marshal(sdkErr) + require.NoError(t, marshalErr) + require.NotContains(t, string(body), tt.notContain) + } + }) + } }) t.Run("NonOwnerForbidden", func(t *testing.T) { @@ -1175,102 +1275,6 @@ func TestAIProvidersKeyManagement(t *testing.T) { require.Equal(t, http.StatusForbidden, sdkErr.StatusCode()) }) - t.Run("MutationBothFieldsRejected", func(t *testing.T) { - t.Parallel() - client := coderdtest.New(t, nil) - _ = 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-mut-both", - Enabled: true, - BaseURL: "https://api.openai.com/v1", - APIKeys: []string{"sk-openai-existing-kkkkkkkkkkkkkkkk"}, //nolint:gosec // test fixture - }) - require.NoError(t, err) - existingID := provider.APIKeys[0].ID - - muts := []codersdk.AIProviderKeyMutation{ - {ID: &existingID, APIKey: new("sk-conflict")}, //nolint:gosec // test fixture - } - _, err = client.UpdateAIProvider(ctx, provider.Name, codersdk.UpdateAIProviderRequest{ - APIKeys: &muts, - }) - require.Error(t, err) - var sdkErr *codersdk.Error - require.ErrorAs(t, err, &sdkErr) - require.Equal(t, http.StatusBadRequest, sdkErr.StatusCode()) - require.Contains(t, sdkErr.Message, "Invalid AI provider request") - require.Len(t, sdkErr.Validations, 1) - require.Equal(t, "api_keys[0]", sdkErr.Validations[0].Field) - require.Contains(t, sdkErr.Validations[0].Detail, "exactly one of id or api_key must be set") - }) - - t.Run("MutationNeitherFieldRejected", func(t *testing.T) { - t.Parallel() - client := coderdtest.New(t, nil) - _ = 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-mut-empty", - Enabled: true, - BaseURL: "https://api.openai.com/v1", - }) - require.NoError(t, err) - - muts := []codersdk.AIProviderKeyMutation{{}} - _, err = client.UpdateAIProvider(ctx, provider.Name, codersdk.UpdateAIProviderRequest{ - APIKeys: &muts, - }) - require.Error(t, err) - var sdkErr *codersdk.Error - require.ErrorAs(t, err, &sdkErr) - require.Equal(t, http.StatusBadRequest, sdkErr.StatusCode()) - require.Contains(t, sdkErr.Message, "Invalid AI provider request") - require.Len(t, sdkErr.Validations, 1) - require.Equal(t, "api_keys[0]", sdkErr.Validations[0].Field) - require.Contains(t, sdkErr.Validations[0].Detail, "exactly one of id or api_key must be set") - }) - - t.Run("MutationDuplicateIDRejected", func(t *testing.T) { - t.Parallel() - client := coderdtest.New(t, nil) - _ = 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-mut-dup", - Enabled: true, - BaseURL: "https://api.openai.com/v1", - APIKeys: []string{"sk-openai-dup-llllllllllllllllllll"}, //nolint:gosec // test fixture - }) - require.NoError(t, err) - id := provider.APIKeys[0].ID - - muts := []codersdk.AIProviderKeyMutation{ - {ID: &id}, - {ID: &id}, - } - _, err = client.UpdateAIProvider(ctx, provider.Name, codersdk.UpdateAIProviderRequest{ - APIKeys: &muts, - }) - require.Error(t, err) - var sdkErr *codersdk.Error - require.ErrorAs(t, err, &sdkErr) - require.Equal(t, http.StatusBadRequest, sdkErr.StatusCode()) - require.Contains(t, sdkErr.Message, "Invalid AI provider request") - require.Len(t, sdkErr.Validations, 1) - require.Equal(t, "api_keys[1].id", sdkErr.Validations[0].Field) - require.Contains(t, sdkErr.Validations[0].Detail, "already referenced") - }) - t.Run("PATCHPropertiesAudited", func(t *testing.T) { t.Parallel() auditor := audit.NewMock() @@ -1403,38 +1407,161 @@ func TestAIProvidersKeyManagement(t *testing.T) { require.True(t, sawDelete, "expected delete audit for removed key") }) - t.Run("MutationUnknownIDRejected", func(t *testing.T) { + t.Run("UpdateRejectsInvalidKeys", func(t *testing.T) { t.Parallel() client := coderdtest.New(t, nil) _ = coderdtest.CreateFirstUser(t, client) ctx := testutil.Context(t, testutil.WaitLong) + const ( + retainedKey = "sk-openai-update-retained-xxxxxxxxxxxxxxxx" //nolint:gosec // test fixture + otherKey = "sk-openai-update-other-yyyyyyyyyyyyyyyyyyy" //nolint:gosec // test fixture + duplicate = "sk-openai-update-duplicate-zzzzzzzzzzzzzzz" //nolint:gosec // test fixture + ) //nolint:gocritic // Owner role is the audience for this endpoint. provider, err := client.CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{ - Type: codersdk.AIProviderTypeOpenAI, - Name: "keys-mut-unknown", - Enabled: true, - BaseURL: "https://api.openai.com/v1", - APIKeys: []string{"sk-openai-real-mmmmmmmmmmmmmmmmmmmm"}, //nolint:gosec // test fixture + Type: codersdk.AIProviderTypeOpenAI, + Name: "keys-update-validation", + DisplayName: "Original key provider", + Enabled: true, + BaseURL: "https://api.openai.com/v1", + APIKeys: []string{retainedKey, otherKey}, }) require.NoError(t, err) + require.Len(t, provider.APIKeys, 2) + originalIDs := keyIDs(provider.APIKeys) + retainedID := provider.APIKeys[0].ID - bogus := uuid.New() - muts := []codersdk.AIProviderKeyMutation{{ID: &bogus}} - _, err = client.UpdateAIProvider(ctx, provider.Name, codersdk.UpdateAIProviderRequest{ - APIKeys: &muts, - }) - require.Error(t, err) - var sdkErr *codersdk.Error - require.ErrorAs(t, err, &sdkErr) - require.Equal(t, http.StatusBadRequest, sdkErr.StatusCode()) - require.Contains(t, sdkErr.Message, "api_keys references an unknown id for this provider") + tests := []struct { + name string + mutations []codersdk.AIProviderKeyMutation + message string + field string + detail string + notContain string + }{ + { + name: "BothFields", + mutations: []codersdk.AIProviderKeyMutation{{ID: &retainedID, APIKey: new(duplicate)}}, + message: "Invalid AI provider request.", + field: "api_keys[0]", + detail: "exactly one of id or api_key must be set", + }, + { + name: "NeitherField", + mutations: []codersdk.AIProviderKeyMutation{{}}, + message: "Invalid AI provider request.", + field: "api_keys[0]", + detail: "exactly one of id or api_key must be set", + }, + { + name: "DuplicateID", + mutations: []codersdk.AIProviderKeyMutation{{ID: &retainedID}, {ID: &retainedID}}, + message: "Invalid AI provider request.", + field: "api_keys[1].id", + detail: "already referenced", + }, + { + name: "Whitespace", + mutations: []codersdk.AIProviderKeyMutation{{APIKey: new(" sk-openai-padded-pppppppppppppppppppp ")}}, //nolint:gosec // test fixture + message: "Invalid AI provider request.", + field: "api_keys[0].api_key", + detail: "must not contain leading or trailing whitespace", + }, + { + name: "UnknownID", + mutations: []codersdk.AIProviderKeyMutation{{ID: new(uuid.New())}}, + message: "api_keys references an unknown id for this provider", + }, + { + name: "DuplicateKey", + mutations: []codersdk.AIProviderKeyMutation{ + {APIKey: new(duplicate)}, + {APIKey: new(duplicate)}, + }, + message: "Invalid AI provider request.", + field: "api_keys[1].api_key", + detail: "duplicate key already provided at api_keys[0]", + notContain: duplicate, + }, + { + name: "DuplicateKeyWithRetainedIDs", + mutations: []codersdk.AIProviderKeyMutation{ + {ID: &retainedID}, + {APIKey: new(duplicate)}, + {ID: &provider.APIKeys[1].ID}, + {APIKey: new(duplicate)}, + }, + message: "Invalid AI provider request.", + field: "api_keys[3].api_key", + detail: "duplicate key already provided at api_keys[1]", + notContain: duplicate, + }, + { + name: "RetainedThenDuplicatePlaintext", + mutations: []codersdk.AIProviderKeyMutation{ + {ID: &retainedID}, + {APIKey: new(retainedKey)}, + }, + message: "Invalid AI provider request.", + field: "api_keys", + detail: "duplicate key already provided at api_keys[0]", + notContain: retainedKey, + }, + { + name: "DuplicatePlaintextThenRetained", + mutations: []codersdk.AIProviderKeyMutation{ + {APIKey: new(retainedKey)}, + {ID: &retainedID}, + }, + message: "Invalid AI provider request.", + field: "api_keys", + detail: "duplicate key already provided at api_keys[0]", + notContain: retainedKey, + }, + { + name: "TooManyFinalKeys", + mutations: []codersdk.AIProviderKeyMutation{ + {ID: &provider.APIKeys[0].ID}, + {ID: &provider.APIKeys[1].ID}, + {APIKey: new("sk-openai-update-max-3-aaaaaaaaaaaaaaaa")}, //nolint:gosec // test fixture + {APIKey: new("sk-openai-update-max-4-bbbbbbbbbbbbbbbb")}, //nolint:gosec // test fixture + {APIKey: new("sk-openai-update-max-5-cccccccccccccccc")}, //nolint:gosec // test fixture + {APIKey: new("sk-openai-update-max-6-dddddddddddddddd")}, //nolint:gosec // test fixture + }, + message: "Invalid AI provider request.", + field: "api_keys", + detail: "api_keys must contain at most 5 keys", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + changedDisplay := "Must not be persisted by " + tt.name + _, err := client.UpdateAIProvider(ctx, provider.Name, codersdk.UpdateAIProviderRequest{ + DisplayName: &changedDisplay, + APIKeys: &tt.mutations, + }) + sdkErr := requireSDKError(t, err, http.StatusBadRequest) + require.Equal(t, tt.message, sdkErr.Message) + if tt.field != "" { + require.Len(t, sdkErr.Validations, 1) + require.Equal(t, tt.field, sdkErr.Validations[0].Field) + require.Contains(t, sdkErr.Validations[0].Detail, tt.detail) + } else { + require.Empty(t, sdkErr.Validations) + } + if tt.notContain != "" { + body, marshalErr := json.Marshal(sdkErr) + require.NoError(t, marshalErr) + require.NotContains(t, string(body), tt.notContain) + } - // Provider's real key is left untouched. - reread, err := client.AIProvider(ctx, provider.Name) - require.NoError(t, err) - require.Len(t, reread.APIKeys, 1) - require.Equal(t, provider.APIKeys[0].ID, reread.APIKeys[0].ID) + reread, getErr := client.AIProvider(ctx, provider.Name) + require.NoError(t, getErr) + require.Equal(t, provider.DisplayName, reread.DisplayName) + require.ElementsMatch(t, originalIDs, keyIDs(reread.APIKeys)) + }) + } }) } @@ -1446,6 +1573,87 @@ func TestAIProvidersKeyManagement(t *testing.T) { func TestAIProviderSettingsMerge(t *testing.T) { t.Parallel() + t.Run("RejectsUnpairedCredentialsWithoutStoredCredentials", func(t *testing.T) { + t.Parallel() + client, db := coderdtest.NewWithDatabase(t, nil) + _ = coderdtest.CreateFirstUser(t, client) + + for _, tt := range []struct { + name string + accessKey *string + accessKeySecret *string + missingField string + detail string + secretNotInError string + }{ + { + name: "AccessKeyOnly", + accessKey: new("AKIA-introduced"), //nolint:gosec // test fixture + missingField: "settings.access_key_secret", + detail: "access_key is set, but access_key_secret is missing or empty", + secretNotInError: "AKIA-introduced", + }, + { + name: "AccessKeySecretOnly", + accessKeySecret: new("introduced-secret"), + missingField: "settings.access_key", + detail: "access_key_secret is set, but access_key is missing or empty", + secretNotInError: "introduced-secret", + }, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + provider, err := client.CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{ + Type: codersdk.AIProviderTypeAnthropic, + Name: "bedrock-introduce-" + strings.ToLower(tt.name), + DisplayName: "Original Bedrock provider", + Enabled: true, + BaseURL: "https://bedrock-runtime.us-east-1.amazonaws.com/", + Settings: codersdk.AIProviderSettings{ + Bedrock: &codersdk.AIProviderBedrockSettings{ + Region: "us-east-1", + Model: "anthropic.claude-3-5-sonnet", + SmallFastModel: "anthropic.claude-3-5-haiku", + }, + }, + }) + require.NoError(t, err) + _, err = client.UpdateAIProvider(ctx, provider.Name, codersdk.UpdateAIProviderRequest{ + DisplayName: new("Must not be persisted"), + Settings: &codersdk.AIProviderSettings{ + Bedrock: &codersdk.AIProviderBedrockSettings{ + Region: "us-west-2", + Model: "anthropic.claude-3-7-sonnet", + SmallFastModel: "anthropic.claude-3-5-haiku", + AccessKey: tt.accessKey, + AccessKeySecret: tt.accessKeySecret, + }, + }, + }) + sdkErr := requireSDKError(t, err, http.StatusBadRequest) + require.Equal(t, "Invalid AI provider request.", sdkErr.Message) + require.Contains(t, sdkErr.Validations, codersdk.ValidationError{ + Field: tt.missingField, + Detail: tt.detail, + }) + body, err := json.Marshal(sdkErr) + require.NoError(t, err) + require.NotContains(t, string(body), tt.secretNotInError) + + reread, err := client.AIProvider(ctx, provider.Name) + require.NoError(t, err) + require.Equal(t, provider, reread) + //nolint:gocritic // Test reads the row to verify write-only fields. + row, err := db.GetAIProviderByID(dbauthz.AsSystemRestricted(ctx), provider.ID) + require.NoError(t, err) + persisted, err := db2sdk.AIProviderSettings(row.Settings) + require.NoError(t, err) + require.Equal(t, provider.Settings, persisted) + }) + } + }) + t.Run("OmittedSecretsPreserveExisting", func(t *testing.T) { t.Parallel() // A PATCH that only rotates non-secret fields must keep the @@ -1498,107 +1706,178 @@ func TestAIProviderSettingsMerge(t *testing.T) { require.Equal(t, "secret-old", *persisted.Bedrock.AccessKeySecret) }) - t.Run("ExplicitEmptyClearsSecrets", func(t *testing.T) { + t.Run("ClearSecrets", func(t *testing.T) { t.Parallel() - // An admin migrating from static AWS credentials to IAM - // role-based auth needs to clear AccessKey and AccessKeySecret - // in a single PATCH. Sending the field with an empty string is - // the explicit clear signal; the *string field distinguishes - // "omitted" (nil) from "set to empty" (pointer to ""). + // Empty strings explicitly clear credentials; omitted fields retain + // their stored values. Clearing just one half must be rejected. client, db := coderdtest.NewWithDatabase(t, nil) _ = coderdtest.CreateFirstUser(t, client) - ctx := testutil.Context(t, testutil.WaitLong) - //nolint:gocritic // Owner role is the audience for this endpoint. - created, err := client.CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{ - Type: codersdk.AIProviderTypeAnthropic, - Name: "merge-clear", - Enabled: true, - BaseURL: "https://bedrock-runtime.us-east-1.amazonaws.com/", - Settings: codersdk.AIProviderSettings{ - Bedrock: &codersdk.AIProviderBedrockSettings{ - Region: "us-east-1", - Model: "anthropic.claude-3-5-sonnet", - SmallFastModel: "anthropic.claude-3-5-haiku", - AccessKey: new("AKIA-old"), //nolint:gosec // test fixture, not a real credential - AccessKeySecret: new("secret-old"), - }, + for _, tt := range []struct { + name string + accessKey *string + accessKeySecret *string + missingField string + detail string + }{ + { + name: "Both", + accessKey: new(""), + accessKeySecret: new(""), }, - }) - require.NoError(t, err) - - _, err = client.UpdateAIProvider(ctx, created.Name, codersdk.UpdateAIProviderRequest{ - Settings: &codersdk.AIProviderSettings{ - Bedrock: &codersdk.AIProviderBedrockSettings{ - Region: "us-east-1", - Model: "anthropic.claude-3-5-sonnet", - SmallFastModel: "anthropic.claude-3-5-haiku", - AccessKey: new(""), - AccessKeySecret: new(""), - }, + { + name: "AccessKeyOnly", + accessKey: new(""), + missingField: "settings.access_key", + detail: "access_key_secret is set, but access_key is missing or empty", }, - }) - require.NoError(t, err) + { + name: "AccessKeySecretOnly", + accessKeySecret: new(""), + missingField: "settings.access_key_secret", + detail: "access_key is set, but access_key_secret is missing or empty", + }, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + req := codersdk.CreateAIProviderRequest{ + Type: codersdk.AIProviderTypeAnthropic, + Name: "merge-clear-" + strings.ToLower(tt.name), + DisplayName: "Original Bedrock provider", + Enabled: true, + BaseURL: "https://bedrock-runtime.us-east-1.amazonaws.com/", + Settings: codersdk.AIProviderSettings{ + Bedrock: &codersdk.AIProviderBedrockSettings{ + Region: "us-east-1", + Model: "anthropic.claude-3-5-sonnet", + SmallFastModel: "anthropic.claude-3-5-haiku", + AccessKey: new("AKIA-old"), //nolint:gosec // test fixture + AccessKeySecret: new("secret-old"), + }, + }, + } + //nolint:gocritic // Owner role is the audience for this endpoint. + created, err := client.CreateAIProvider(ctx, req) + require.NoError(t, err) - //nolint:gocritic // Test reads the row to verify write-only fields. - row, err := db.GetAIProviderByID(dbauthz.AsSystemRestricted(ctx), created.ID) - require.NoError(t, err) - persisted, err := db2sdk.AIProviderSettings(row.Settings) - require.NoError(t, err) - require.NotNil(t, persisted.Bedrock) - require.NotNil(t, persisted.Bedrock.AccessKey) - require.Equal(t, "", *persisted.Bedrock.AccessKey) - require.NotNil(t, persisted.Bedrock.AccessKeySecret) - require.Equal(t, "", *persisted.Bedrock.AccessKeySecret) + patch := codersdk.UpdateAIProviderRequest{ + DisplayName: new("Updated Bedrock provider"), + Settings: &codersdk.AIProviderSettings{ + Bedrock: &codersdk.AIProviderBedrockSettings{ + Region: "us-west-2", + Model: "anthropic.claude-3-7-sonnet", + SmallFastModel: "anthropic.claude-3-5-haiku", + AccessKey: tt.accessKey, + AccessKeySecret: tt.accessKeySecret, + }, + }, + } + _, err = client.UpdateAIProvider(ctx, created.Name, patch) + expected := req.Settings + if tt.missingField != "" { + sdkErr := requireSDKError(t, err, http.StatusBadRequest) + require.Equal(t, "Invalid AI provider request.", sdkErr.Message) + require.Contains(t, sdkErr.Validations, codersdk.ValidationError{ + Field: tt.missingField, + Detail: tt.detail, + }) + reread, err := client.AIProvider(ctx, created.Name) + require.NoError(t, err) + require.Equal(t, created, reread) + } else { + require.NoError(t, err) + expected = *patch.Settings + } + + //nolint:gocritic // Test reads the row to verify write-only fields. + row, err := db.GetAIProviderByID(dbauthz.AsSystemRestricted(ctx), created.ID) + require.NoError(t, err) + persisted, err := db2sdk.AIProviderSettings(row.Settings) + require.NoError(t, err) + require.Equal(t, expected, persisted) + }) + } }) t.Run("ExplicitRotatesSecrets", func(t *testing.T) { t.Parallel() client, db := coderdtest.NewWithDatabase(t, nil) _ = coderdtest.CreateFirstUser(t, client) - ctx := testutil.Context(t, testutil.WaitLong) - //nolint:gocritic // Owner role is the audience for this endpoint. - created, err := client.CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{ - Type: codersdk.AIProviderTypeAnthropic, - Name: "merge-rotate", - Enabled: true, - BaseURL: "https://bedrock-runtime.us-east-1.amazonaws.com/", - Settings: codersdk.AIProviderSettings{ - Bedrock: &codersdk.AIProviderBedrockSettings{ - Region: "us-east-1", - Model: "anthropic.claude-3-5-sonnet", - SmallFastModel: "anthropic.claude-3-5-haiku", - AccessKey: new("AKIA-old"), //nolint:gosec // test fixture, not a real credential - AccessKeySecret: new("secret-old"), - }, + for _, tt := range []struct { + name string + accessKey *string + accessKeySecret *string + expectedAccessKey string + expectedAccessKeySecret string + }{ + { + name: "Both", + accessKey: new("AKIA-new"), //nolint:gosec // test fixture + accessKeySecret: new("secret-new"), + expectedAccessKey: "AKIA-new", + expectedAccessKeySecret: "secret-new", }, - }) - require.NoError(t, err) - - _, err = client.UpdateAIProvider(ctx, created.Name, codersdk.UpdateAIProviderRequest{ - Settings: &codersdk.AIProviderSettings{ - Bedrock: &codersdk.AIProviderBedrockSettings{ - Region: "us-east-1", - Model: "anthropic.claude-3-5-sonnet", - SmallFastModel: "anthropic.claude-3-5-haiku", - AccessKey: new("AKIA-new"), //nolint:gosec // test fixture, not a real credential - AccessKeySecret: new("secret-new"), - }, + { + name: "AccessKeyOnly", + accessKey: new("AKIA-new"), //nolint:gosec // test fixture + expectedAccessKey: "AKIA-new", + expectedAccessKeySecret: "secret-old", }, - }) - require.NoError(t, err) + { + name: "AccessKeySecretOnly", + accessKeySecret: new("secret-new"), + expectedAccessKey: "AKIA-old", + expectedAccessKeySecret: "secret-new", + }, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + //nolint:gocritic // Owner role is the audience for this endpoint. + created, err := client.CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{ + Type: codersdk.AIProviderTypeAnthropic, + Name: "merge-rotate-" + strings.ToLower(tt.name), + Enabled: true, + BaseURL: "https://bedrock-runtime.us-east-1.amazonaws.com/", + Settings: codersdk.AIProviderSettings{ + Bedrock: &codersdk.AIProviderBedrockSettings{ + Region: "us-east-1", + Model: "anthropic.claude-3-5-sonnet", + SmallFastModel: "anthropic.claude-3-5-haiku", + AccessKey: new("AKIA-old"), //nolint:gosec // test fixture, not a real credential + AccessKeySecret: new("secret-old"), + }, + }, + }) + require.NoError(t, err) - //nolint:gocritic // Test reads the row to verify write-only fields. - row, err := db.GetAIProviderByID(dbauthz.AsSystemRestricted(ctx), created.ID) - require.NoError(t, err) - persisted, err := db2sdk.AIProviderSettings(row.Settings) - require.NoError(t, err) - require.NotNil(t, persisted.Bedrock) - require.NotNil(t, persisted.Bedrock.AccessKey) - require.Equal(t, "AKIA-new", *persisted.Bedrock.AccessKey) - require.NotNil(t, persisted.Bedrock.AccessKeySecret) - require.Equal(t, "secret-new", *persisted.Bedrock.AccessKeySecret) + _, err = client.UpdateAIProvider(ctx, created.Name, codersdk.UpdateAIProviderRequest{ + Settings: &codersdk.AIProviderSettings{ + Bedrock: &codersdk.AIProviderBedrockSettings{ + Region: "us-east-1", + Model: "anthropic.claude-3-5-sonnet", + SmallFastModel: "anthropic.claude-3-5-haiku", + AccessKey: tt.accessKey, + AccessKeySecret: tt.accessKeySecret, + }, + }, + }) + require.NoError(t, err) + + //nolint:gocritic // Test reads the row to verify write-only fields. + row, err := db.GetAIProviderByID(dbauthz.AsSystemRestricted(ctx), created.ID) + require.NoError(t, err) + persisted, err := db2sdk.AIProviderSettings(row.Settings) + require.NoError(t, err) + require.NotNil(t, persisted.Bedrock) + require.NotNil(t, persisted.Bedrock.AccessKey) + require.Equal(t, tt.expectedAccessKey, *persisted.Bedrock.AccessKey) + require.NotNil(t, persisted.Bedrock.AccessKeySecret) + require.Equal(t, tt.expectedAccessKeySecret, *persisted.Bedrock.AccessKeySecret) + }) + } }) t.Run("MigrateStaticToRole", func(t *testing.T) { diff --git a/codersdk/aiproviders.go b/codersdk/aiproviders.go index 6e81e3616a..b5e738e870 100644 --- a/codersdk/aiproviders.go +++ b/codersdk/aiproviders.go @@ -16,6 +16,9 @@ import ( "golang.org/x/xerrors" ) +// MaxAIProviderAPIKeys is the maximum number of API keys per AI provider. +const MaxAIProviderAPIKeys = 5 + // AIProviderNameRegex mirrors the CHECK constraint on ai_providers.name. // Provider names are lowercase alphanumeric with hyphen separators so // they are safe in URLs. @@ -285,6 +288,7 @@ func (req CreateAIProviderRequest) Validate() []ValidationError { }) } if req.Settings.Bedrock != nil { + validations = append(validations, req.Settings.Bedrock.ValidateCredentials()...) validations = append(validations, validateAIProviderRoleARN(req.Settings.Bedrock.RoleARN)...) validations = append(validations, validateAIProviderBedrockProtocol(req.Settings.Bedrock.Protocol)...) if req.Settings.Bedrock.ExternalID != "" { @@ -352,6 +356,9 @@ func (req UpdateAIProviderRequest) Validate() []ValidationError { // omitted. Omitting any other field clears it, so the checks below apply // to the patch exactly as they would to what gets stored. if req.Settings != nil && req.Settings.Bedrock != nil { + if req.Settings.Bedrock.AccessKey != nil && req.Settings.Bedrock.AccessKeySecret != nil { + validations = append(validations, req.Settings.Bedrock.ValidateCredentials()...) + } validations = append(validations, validateAIProviderRoleARN(req.Settings.Bedrock.RoleARN)...) validations = append(validations, validateAIProviderBedrockProtocol(req.Settings.Bedrock.Protocol)...) validations = append(validations, validateAIProviderBedrockMantleRegion(*req.Settings.Bedrock)...) @@ -475,14 +482,37 @@ func validateAIProviderBaseURL(raw string) []ValidationError { return validations } -// validateAIProviderAPIKeys checks that each supplied key is non-empty -// and free of leading/trailing whitespace. An empty slice itself is +func validateAIProviderKeyCount(count int) []ValidationError { + if count > MaxAIProviderAPIKeys { + return []ValidationError{{ + Field: "api_keys", + Detail: fmt.Sprintf("api_keys must contain at most %d keys", MaxAIProviderAPIKeys), + }} + } + return nil +} + +// ValidateAIProviderKeyUniqueness checks whether key is already in seen. +// The field argument is used for error formatting. +func ValidateAIProviderKeyUniqueness(key, field string, seen map[string]int) []ValidationError { + if prev, ok := seen[key]; ok { + return []ValidationError{{ + Field: field, + Detail: fmt.Sprintf("duplicate key already provided at api_keys[%d]", prev), + }} + } + return nil +} + +// validateAIProviderAPIKeys checks that at most five distinct keys are supplied, +// each non-empty and free of leading/trailing whitespace. An empty slice is // permitted: on create it means "no keys yet"; on update it means // "clear all keys". Keys are stored verbatim; surrounding whitespace // would silently corrupt the credential, so callers must trim before // sending. func validateAIProviderAPIKeys(keys []string) []ValidationError { - var validations []ValidationError + validations := validateAIProviderKeyCount(len(keys)) + seenKeys := make(map[string]int, len(keys)) for i, key := range keys { switch { case key == "": @@ -496,17 +526,23 @@ func validateAIProviderAPIKeys(keys []string) []ValidationError { Detail: "api_keys entries must not contain leading or trailing whitespace", }) } + if duplicate := ValidateAIProviderKeyUniqueness(key, fmt.Sprintf("api_keys[%d]", i), seenKeys); len(duplicate) > 0 { + validations = append(validations, duplicate...) + } else { + seenKeys[key] = i + } } return validations } // validateAIProviderKeyMutations checks each entry has exactly one of -// ID or APIKey set, that plaintexts are non-empty after trimming, and -// that no ID is referenced twice in the same request. An empty slice -// itself is permitted (it clears all keys). +// ID or APIKey set, that plaintexts are non-empty without surrounding whitespace, +// and that no ID or plaintext is repeated. At most five entries are permitted. +// An empty slice is permitted (it clears all keys). func validateAIProviderKeyMutations(muts []AIProviderKeyMutation) []ValidationError { - var validations []ValidationError + validations := validateAIProviderKeyCount(len(muts)) seen := make(map[uuid.UUID]int, len(muts)) + seenKeys := make(map[string]int, len(muts)) for i, m := range muts { hasID := m.ID != nil hasKey := m.APIKey != nil @@ -527,6 +563,13 @@ func validateAIProviderKeyMutations(muts []AIProviderKeyMutation) []ValidationEr Detail: "api_key must not contain leading or trailing whitespace", }) } + if hasKey && !hasID { + if duplicate := ValidateAIProviderKeyUniqueness(*m.APIKey, fmt.Sprintf("api_keys[%d].api_key", i), seenKeys); len(duplicate) > 0 { + validations = append(validations, duplicate...) + } else { + seenKeys[*m.APIKey] = i + } + } if hasID && !hasKey { if prev, ok := seen[*m.ID]; ok { validations = append(validations, ValidationError{ diff --git a/codersdk/aiproviders_bedrock.go b/codersdk/aiproviders_bedrock.go index a4d87d9f53..52034e4863 100644 --- a/codersdk/aiproviders_bedrock.go +++ b/codersdk/aiproviders_bedrock.go @@ -78,6 +78,26 @@ func (b AIProviderBedrockSettings) ResolvedProtocol() AIProviderBedrockProtocol return b.Protocol } +// ValidateCredentials checks that static credentials are paired. Callers must +// merge omitted credential fields from storage before validating a patch. +func (b AIProviderBedrockSettings) ValidateCredentials() []ValidationError { + hasKey := b.AccessKey != nil && *b.AccessKey != "" + hasSecret := b.AccessKeySecret != nil && *b.AccessKeySecret != "" + if hasKey == hasSecret { + return nil + } + field := "settings.access_key" + detail := "access_key_secret is set, but access_key is missing or empty" + if hasKey { + field = "settings.access_key_secret" + detail = "access_key is set, but access_key_secret is missing or empty" + } + return []ValidationError{{ + Field: field, + Detail: detail, + }} +} + // IsConfigured reports whether any load-bearing Bedrock field is set, // indicating that the operator wants the provider to authenticate via // AWS Bedrock rather than as a bearer-token Anthropic provider. diff --git a/codersdk/aiproviders_test.go b/codersdk/aiproviders_test.go index e98adeef10..ac6aa20e48 100644 --- a/codersdk/aiproviders_test.go +++ b/codersdk/aiproviders_test.go @@ -160,6 +160,172 @@ func TestAIProviderSettings_Roundtrip(t *testing.T) { require.Equal(t, orig, got) } +func TestAIProviderRequest_ValidateAPIKeys(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + keys []string + wantCreateField string + wantUpdateField string + wantDetail string + }{ + {name: "Omitted"}, + {name: "Empty", keys: []string{}}, + {name: "One", keys: []string{"key-1"}}, + {name: "Five", keys: []string{"key-1", "key-2", "key-3", "key-4", "key-5"}}, + { + name: "Six", + keys: []string{"key-1", "key-2", "key-3", "key-4", "key-5", "key-6"}, + wantCreateField: "api_keys", + wantUpdateField: "api_keys", + wantDetail: "api_keys must contain at most 5 keys", + }, + { + name: "Duplicate", + keys: []string{"key-1", "key-2", "key-1"}, + wantCreateField: "api_keys[2]", + wantUpdateField: "api_keys[2].api_key", + wantDetail: "duplicate key already provided at api_keys[0]", + }, + { + name: "DuplicateAfterDifferentKey", + keys: []string{"key-1", "key-2", "key-2"}, + wantCreateField: "api_keys[2]", + wantUpdateField: "api_keys[2].api_key", + wantDetail: "duplicate key already provided at api_keys[1]", + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + create := codersdk.CreateAIProviderRequest{ + Type: codersdk.AIProviderTypeOpenAI, + Name: "keys", + BaseURL: "https://api.openai.com/v1", + APIKeys: tc.keys, + } + createValidations := create.Validate() + if tc.wantCreateField == "" { + require.Empty(t, createValidations) + } else { + require.Len(t, createValidations, 1) + require.Equal(t, tc.wantCreateField, createValidations[0].Field) + require.Equal(t, tc.wantDetail, createValidations[0].Detail) + for _, key := range tc.keys { + require.NotContains(t, createValidations[0].Detail, key) + } + } + + muts := make([]codersdk.AIProviderKeyMutation, 0, len(tc.keys)) + for _, key := range tc.keys { + muts = append(muts, codersdk.AIProviderKeyMutation{APIKey: new(key)}) + } + update := codersdk.UpdateAIProviderRequest{APIKeys: &muts} + updateValidations := update.Validate() + if tc.wantUpdateField == "" { + require.Empty(t, updateValidations) + } else { + require.Len(t, updateValidations, 1) + require.Equal(t, tc.wantUpdateField, updateValidations[0].Field) + require.Equal(t, tc.wantDetail, updateValidations[0].Detail) + for _, key := range tc.keys { + require.NotContains(t, updateValidations[0].Detail, key) + } + } + }) + } +} + +func TestValidateAIProviderKeyUniqueness(t *testing.T) { + t.Parallel() + + seen := map[string]int{"key-1": 1} + require.Empty(t, codersdk.ValidateAIProviderKeyUniqueness("key-2", "api_keys[2]", seen)) + require.Equal(t, map[string]int{"key-1": 1}, seen) + + require.Equal(t, []codersdk.ValidationError{{ + Field: "api_keys[3]", + Detail: "duplicate key already provided at api_keys[1]", + }}, codersdk.ValidateAIProviderKeyUniqueness("key-1", "api_keys[3]", seen)) + require.Equal(t, map[string]int{"key-1": 1}, seen) +} + +func TestAIProviderRequest_ValidateBedrockCredentials(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + key *string + secret *string + wantField string + wantDetail string + }{ + {name: "Omitted"}, + {name: "Empty", key: new(""), secret: new("")}, + {name: "EmptyKey", key: new("")}, + {name: "EmptySecret", secret: new("")}, + {name: "Paired", key: new("key"), secret: new("longer-secret")}, + { + name: "MissingKey", + secret: new("secret"), + wantField: "settings.access_key", + wantDetail: "access_key_secret is set, but access_key is missing or empty", + }, + { + name: "MissingSecret", + key: new("key"), + wantField: "settings.access_key_secret", + wantDetail: "access_key is set, but access_key_secret is missing or empty", + }, + { + name: "ClearedKey", + key: new(""), + secret: new("secret"), + wantField: "settings.access_key", + wantDetail: "access_key_secret is set, but access_key is missing or empty", + }, + { + name: "ClearedSecret", + key: new("key"), + secret: new(""), + wantField: "settings.access_key_secret", + wantDetail: "access_key is set, but access_key_secret is missing or empty", + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + settings := codersdk.AIProviderSettings{Bedrock: &codersdk.AIProviderBedrockSettings{ + Region: "us-east-1", + Model: "model", + SmallFastModel: "small-model", + AccessKey: tc.key, + AccessKeySecret: tc.secret, + }} + create := codersdk.CreateAIProviderRequest{ + Type: codersdk.AIProviderTypeBedrock, Name: "bedrock", + BaseURL: "https://bedrock-runtime.us-east-1.amazonaws.com/", Settings: settings, + } + var want []codersdk.ValidationError + if tc.wantField != "" { + want = []codersdk.ValidationError{{ + Field: tc.wantField, Detail: tc.wantDetail, + }} + } + require.Equal(t, want, create.Validate()) + require.Equal(t, want, settings.Bedrock.ValidateCredentials()) + + update := codersdk.UpdateAIProviderRequest{Settings: &settings} + if tc.key == nil || tc.secret == nil { + // Omitted credentials are merged from storage by the API. + require.Empty(t, update.Validate()) + } else { + require.Equal(t, want, update.Validate()) + } + }) + } +} + func TestAIProviderRequest_ValidateRoleARN(t *testing.T) { t.Parallel() diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index e331425e27..2e4b49ff47 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -6119,6 +6119,12 @@ export interface MatchedProvisioners { */ export const MaxAIModelPricesBytes = 1048576; // 1 MiB +// From codersdk/aiproviders.go +/** + * MaxAIProviderAPIKeys is the maximum number of API keys per AI provider. + */ +export const MaxAIProviderAPIKeys = 5; + // From codersdk/aibridge.go /** * MaxAISpendLimitMicros is the highest AI spend limit that can be configured,