diff --git a/e2e/agentnetwork/credential_check_live_test.go b/e2e/agentnetwork/credential_check_live_test.go new file mode 100644 index 000000000..f859cb329 --- /dev/null +++ b/e2e/agentnetwork/credential_check_live_test.go @@ -0,0 +1,204 @@ +//go:build e2e + +package agentnetwork + +import ( + "context" + "net/http" + "os" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/shared/management/client/rest" + "github.com/netbirdio/netbird/shared/management/http/api" +) + +// credentialCase is one vendor to try the save-time check against. The key is +// the real one the suite already sources; corrupting it is what produces the +// refusal, so the pair of cases differ only in the credential. +type credentialCase struct { + name string + catalogID string + upstream string + apiKey string +} + +// liveCredentialCases mirrors the discovery matrix's env gating so a partial +// key set still yields partial coverage. Vertex is left out: its credential is +// a service-account keyfile, and mangling one produces a client-side parse +// failure rather than the vendor refusal this is about. +func liveCredentialCases() []credentialCase { + var cases []credentialCase + + if k := os.Getenv("OPENAI_TOKEN"); k != "" { + cases = append(cases, credentialCase{ + name: "openai", catalogID: "openai_api", + upstream: "https://api.openai.com", apiKey: k, + }) + } + if k := os.Getenv("ANTHROPIC_TOKEN"); k != "" { + cases = append(cases, credentialCase{ + name: "anthropic", catalogID: "anthropic_api", + upstream: "https://api.anthropic.com", apiKey: k, + }) + } + if k := os.Getenv("AWS_BEARER_TOKEN_BEDROCK"); k != "" { + region := os.Getenv("AWS_REGION") + if region == "" { + region = "eu-central-1" + } + cases = append(cases, credentialCase{ + name: "bedrock", catalogID: "bedrock_api", + upstream: "https://bedrock-runtime." + region + ".amazonaws.com", apiKey: k, + }) + } + + return cases +} + +// TestLiveProviderCredentialCheck drives the save-time check against the real +// vendors. A unit test can only assert that a mocked refusal is classified; +// what it cannot show is that these vendors refuse a bad key on their listing +// endpoint at all, which is the assumption the whole feature rests on. +// +// The good-key case matters just as much as the bad one: a check that refused +// everything would pass a test asserting only the refusal, and would make the +// product unusable. +func TestLiveProviderCredentialCheck(t *testing.T) { + cases := liveCredentialCases() + if len(cases) == 0 { + t.Skip("no live provider credentials in the environment; source ~/.llm-keys to run") + } + + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) + defer cancel() + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Run("a good credential saves", func(t *testing.T) { + prov, err := srv.CreateProvider(ctx, credentialProviderRequest(tc, "e2e-cred-ok-"+tc.name, tc.apiKey)) + require.NoError(t, err, "the suite's own credential must pass its check") + t.Cleanup(func() { _ = srv.DeleteProvider(context.Background(), prov.Id) }) + require.NotEmpty(t, prov.Id) + }) + + t.Run("a rejected credential is refused", func(t *testing.T) { + _, err := srv.CreateProvider(ctx, credentialProviderRequest(tc, "e2e-cred-bad-"+tc.name, corrupt(tc.apiKey))) + require.Error(t, err, "a key the vendor rejects must not save") + + var apiErr *rest.APIError + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusUnprocessableEntity, apiErr.StatusCode, + "a refused credential is the caller's problem to fix, not a server fault") + require.Contains(t, strings.ToLower(apiErr.Message), "rejected the credential", + "the message must name the credential rather than the url") + + // The record must be absent, not merely unusable: a provider + // saved despite its check is the state this prevents. + all, listErr := srv.ListProviders(ctx) + require.NoError(t, listErr) + for _, p := range all { + require.NotEqual(t, "e2e-cred-bad-"+tc.name, p.Name, "a refused provider must not be stored") + } + }) + }) + } +} + +// TestLiveProviderUrlCheck points a real credential at a host that is not the +// vendor's API. It is the half of the split a wrong key cannot exercise: the +// operator has to be told the URL is at fault while their key is fine. +func TestLiveProviderUrlCheck(t *testing.T) { + cases := liveCredentialCases() + if len(cases) == 0 { + t.Skip("no live provider credentials in the environment; source ~/.llm-keys to run") + } + tc := cases[0] + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + + // A name that resolves nowhere. The check has to reach a verdict without + // the vendor's help, which is the transport half of the classification. + req := credentialProviderRequest(tc, "e2e-cred-badurl", tc.apiKey) + req.UpstreamUrl = "https://not-a-real-vendor-host.netbird-e2e.invalid" + + _, err := srv.CreateProvider(ctx, req) + require.Error(t, err, "an upstream that does not resolve must not save") + + var apiErr *rest.APIError + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusUnprocessableEntity, apiErr.StatusCode) + require.Contains(t, strings.ToLower(apiErr.Message), "could not be reached", + "the message must name the url rather than the credential") +} + +// TestLiveProviderUpdateKeepsTheWorkingKey is the state the check exists to +// prevent on the update path: a rejected rotation that has already replaced +// the credential would take a working provider down. +func TestLiveProviderUpdateKeepsTheWorkingKey(t *testing.T) { + cases := liveCredentialCases() + if len(cases) == 0 { + t.Skip("no live provider credentials in the environment; source ~/.llm-keys to run") + } + tc := cases[0] + + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) + defer cancel() + + prov, err := srv.CreateProvider(ctx, credentialProviderRequest(tc, "e2e-cred-rotate", tc.apiKey)) + require.NoError(t, err) + t.Cleanup(func() { _ = srv.DeleteProvider(context.Background(), prov.Id) }) + + rotation := credentialProviderRequest(tc, "e2e-cred-rotate", corrupt(tc.apiKey)) + _, err = srv.UpdateProvider(ctx, prov.Id, rotation) + require.Error(t, err, "a rotation the vendor rejects must not be stored") + + var apiErr *rest.APIError + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusUnprocessableEntity, apiErr.StatusCode) + + // The stored key is never returned by the API, so the proof that it + // survived is that an edit which reuses it still passes its check. A + // replaced key would fail here exactly as the rotation just did. + renamed := credentialProviderRequest(tc, "e2e-cred-rotate-renamed", "") + updated, err := srv.UpdateProvider(ctx, prov.Id, renamed) + require.NoError(t, err, "the working key must still be the stored one") + require.Equal(t, "e2e-cred-rotate-renamed", updated.Name) +} + +// credentialProviderRequest builds a create/update body for a case. An empty apiKey is +// omitted rather than sent blank, which is how the form asks to keep whatever +// is already stored. +func credentialProviderRequest(tc credentialCase, name, apiKey string) api.AgentNetworkProviderRequest { + req := api.AgentNetworkProviderRequest{ + Name: name, + ProviderId: tc.catalogID, + UpstreamUrl: tc.upstream, + Enabled: ptr(true), + } + if apiKey != "" { + req.ApiKey = &apiKey + } + return req +} + +// corrupt returns a key the vendor will reject while keeping the shape of the +// original. Replacing the last character rather than appending keeps any +// length or prefix validation satisfied, so the refusal comes from the vendor +// checking the secret rather than from it rejecting an obviously malformed +// one. +func corrupt(key string) string { + if key == "" { + return key + } + last := key[len(key)-1] + replacement := byte('A') + if last == 'A' { + replacement = 'B' + } + return key[:len(key)-1] + string(replacement) +} diff --git a/shared/management/http/api/openapi.yml b/shared/management/http/api/openapi.yml index 3ab5a2e42..c318c8b04 100644 --- a/shared/management/http/api/openapi.yml +++ b/shared/management/http/api/openapi.yml @@ -14143,7 +14143,10 @@ paths: "$ref": "#/components/responses/internal_error" post: summary: Create an Agent Network Provider - description: Connects a new Agent Network AI provider for the account. + description: | + Connects a new Agent Network AI provider for the account. + + The upstream URL and credential are checked against the vendor before the provider is stored, so a record that cannot reach its vendor is refused rather than saved. Returns 422 with a message naming whichever of the two is at fault. A catalog provider with no listing endpoint, and one whose upstream the check cannot reach from the management service, are stored without being checked. tags: [ Agent Network ] security: - BearerAuth: [ ] @@ -14169,6 +14172,8 @@ paths: "$ref": "#/components/responses/forbidden" '409': "$ref": "#/components/responses/conflict" + '422': + "$ref": "#/components/responses/validation_failed_simple" '500': "$ref": "#/components/responses/internal_error" /api/agent-network/providers/{providerId}: @@ -14205,7 +14210,10 @@ paths: "$ref": "#/components/responses/internal_error" put: summary: Update an Agent Network Provider - description: Update an existing Agent Network AI provider. + description: | + Update an existing Agent Network AI provider. + + When the upstream URL or the API key changes, the pair is checked against the vendor before the change is stored, and a refusal returns 422 without replacing what was there. An update omitting the API key is checked against the stored one. Edits touching neither field are stored without a check. tags: [ Agent Network ] security: - BearerAuth: [ ] @@ -14240,6 +14248,8 @@ paths: "$ref": "#/components/responses/not_found" '409': "$ref": "#/components/responses/conflict" + '422': + "$ref": "#/components/responses/validation_failed_simple" '500': "$ref": "#/components/responses/internal_error" delete: