diff --git a/e2e/agentnetwork/credential_check_live_test.go b/e2e/agentnetwork/credential_check_live_test.go new file mode 100644 index 00000000000..f83a18129b7 --- /dev/null +++ b/e2e/agentnetwork/credential_check_live_test.go @@ -0,0 +1,235 @@ +//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. +// +// The suite asserts on the vendors themselves, so it inherits their +// availability: the check blocks on 5xx and 429 by design, and a vendor outage +// or a rate limit during a run fails "a good credential saves" with a +// perfectly valid key. There is no retry here on purpose — a retry loop would +// also mask the outage classification these tests exist to prove. Re-run the +// job. +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. +// +// One vendor, deliberately. The transport classification under test happens +// before any vendor is reached, so running it per configured vendor would +// repeat the same code path and multiply the wall-clock of a suite that +// already creates real records. cases[0] is whichever vendor the environment +// supplies first. +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") + + // An error is not the same fact as an absent record: a handler that saved + // first and reported afterwards would satisfy everything above. + all, listErr := srv.ListProviders(ctx) + require.NoError(t, listErr) + for _, p := range all { + require.NotEqual(t, "e2e-cred-badurl", p.Name, "a refused provider must not be stored") + } +} + +// 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. +// +// Also one vendor: the behaviour is in the manager's merge, not in any +// vendor's response, and each run creates and mutates a real provider record. +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. + // + // The trailing slash is what makes that an actual check: an edit touching + // neither the url, the key nor the catalog entry is stored without asking + // the vendor anything, so a rename alone would pass whatever is on the + // record. Only the host is read out of the upstream, so the same vendor is + // reached — but the string differs, and the check runs. + recheck := credentialProviderRequest(tc, "e2e-cred-rotate-renamed", "") + recheck.UpstreamUrl = tc.upstream + "/" + updated, err := srv.UpdateProvider(ctx, prov.Id, recheck) + 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/e2e/agentnetwork/management_test.go b/e2e/agentnetwork/management_test.go index 9e3176d68e2..6868a9d4d9c 100644 --- a/e2e/agentnetwork/management_test.go +++ b/e2e/agentnetwork/management_test.go @@ -16,14 +16,20 @@ import ( func ptr[T any](v T) *T { return &v } -// newProvider creates an OpenAI-catalog provider with a dummy key (these tests -// never call the upstream) and registers cleanup. +// newProvider creates an OpenAI-catalog provider these tests can hang a policy +// off, and registers cleanup. Nothing here calls the upstream. func newProvider(t *testing.T, ctx context.Context, name string) api.AgentNetworkProvider { t.Helper() + // A provider save is credential-checked against the vendor, and every + // caller here wants a provider row to hang a policy off rather than a + // working upstream. A private address is left unchecked — the proxy would + // reach it through the tunnel, management cannot reach it at all — which + // keeps this fixture independent of whether the run has vendor keys, and + // covers the unchecked-provider-still-saves path while it is at it. prov, err := srv.CreateProvider(ctx, api.AgentNetworkProviderRequest{ Name: name, ProviderId: "openai_api", - UpstreamUrl: "https://api.openai.com", + UpstreamUrl: "https://10.255.255.1", ApiKey: ptr("sk-dummy-e2e-key"), }) require.NoError(t, err, "create provider %q", name) diff --git a/management/internals/modules/agentnetwork/credentialcheck.go b/management/internals/modules/agentnetwork/credentialcheck.go new file mode 100644 index 00000000000..a4787d01955 --- /dev/null +++ b/management/internals/modules/agentnetwork/credentialcheck.go @@ -0,0 +1,135 @@ +package agentnetwork + +import ( + "context" + "errors" + "net/http" + + log "github.com/sirupsen/logrus" + + "github.com/netbirdio/netbird/management/internals/modules/agentnetwork/modeldiscovery" + "github.com/netbirdio/netbird/management/internals/modules/agentnetwork/types" + "github.com/netbirdio/netbird/shared/management/status" +) + +// ModelLister is the vendor-facing half of the credential check. +// modeldiscovery.Client is the only production implementation; it is an +// interface because the check runs on a write path, so without a seam every +// test that saves a provider would reach a vendor to do it. +type ModelLister interface { + Fetch(ctx context.Context, req modeldiscovery.Request) ([]modeldiscovery.Model, error) +} + +// checkProviderCredential refuses a record whose upstream or credential the +// vendor will not accept. +// +// It reuses the discovery Fetch rather than a lighter status probe so it +// exercises the path the model picker takes: a URL answering 200 with a login +// page fails here instead of producing an empty picker later. +func (m *managerImpl) checkProviderCredential(ctx context.Context, provider *types.Provider) error { + // A record that asks the proxy to skip certificate verification is one this + // check cannot speak for. Discovery verifies certificates, so a self-hosted + // endpoint behind a self-signed one would be refused for a reason the + // operator already told us to ignore — a lockout of exactly the setup the + // flag exists for. Sending the credential over a connection management + // declines to verify is the other way out, and a worse one. + if provider.SkipTLSVerification { + log.WithContext(ctx).Debugf("agent network provider %s not credential-checked: tls verification is disabled for it", provider.ProviderID) + return nil + } + + _, err := m.modelDiscovery.Fetch(ctx, modeldiscovery.Request{ + CatalogID: provider.ProviderID, + UpstreamURL: provider.UpstreamURL, + APIKey: provider.APIKey, + }) + if err == nil { + return nil + } + + message, blocking := credentialCheckFailure(err) + if !blocking { + log.WithContext(ctx).Debugf("agent network provider %s not credential-checked: %v", provider.ProviderID, err) + return nil + } + + // WriteError logs only what we return, and that carries no status code, + // so the vendor's number is recorded here or nowhere. + log.WithContext(ctx).Infof("agent network provider %s failed its credential check: %v", provider.ProviderID, err) + + return status.Errorf(status.InvalidArgument, "%s", message) +} + +// discoveryFailure renders a failed model listing for the operator who pressed +// the button. Every outcome here is something they did or configured — a key +// the vendor refused, an upstream that does not answer — so it owes them the +// same sentence a refused save gives, not the generic 500 an unclassified +// error turns into. +// +// ErrNoDiscovery and ErrInvalidRequest pass through untouched: the handler +// already maps them, and "this provider has no listing endpoint" is a fact +// about the catalog rather than a failure to report as one. +func discoveryFailure(ctx context.Context, catalogID string, err error) error { + if errors.Is(err, modeldiscovery.ErrNoDiscovery) || errors.Is(err, modeldiscovery.ErrInvalidRequest) { + return err + } + + message, _ := credentialCheckFailure(err) + if message == "" { + return err + } + + // The operator's message carries no status code, so the vendor's number is + // recorded here or nowhere. + log.WithContext(ctx).Infof("agent network model discovery for %s failed: %v", catalogID, err) + + return status.Errorf(status.InvalidArgument, "%s", message) +} + +// credentialCheckFailure renders a discovery failure as the sentence the +// provider form shows, and reports whether it should block the write. +// +// The strings survive WriteError lowercasing them, and never echo the +// operator's URL: paths are case-sensitive, so an echoed URL comes back +// altered and describes something they did not type. +func credentialCheckFailure(err error) (message string, blocking bool) { + // Not checkable. The record may be perfectly good and we have no way to + // ask, so reporting a failure would be a guess. + switch { + case errors.Is(err, modeldiscovery.ErrNoDiscovery), + errors.Is(err, modeldiscovery.ErrNoDiscoveryHost), + errors.Is(err, modeldiscovery.ErrPrivateHost): + return "", false + } + + var vendor *modeldiscovery.VendorStatusError + if errors.As(err, &vendor) { + switch vendor.Status { + case http.StatusUnauthorized, http.StatusForbidden: + return "the provider rejected the credential", true + case http.StatusNotFound, http.StatusMethodNotAllowed: + return "the upstream url did not answer a model listing", true + default: + // 5xx and 429 included: an outage still leaves the record + // unverified, which is what this refuses to save. + return "the provider returned an error", true + } + } + + var unreachable *modeldiscovery.UnreachableError + if errors.As(err, &unreachable) { + if reason := unreachable.Reason(); reason != "" { + return "the upstream url could not be reached: " + reason, true + } + return "the upstream url could not be reached", true + } + + if errors.Is(err, modeldiscovery.ErrUnparseableListing) { + return "the upstream url answered, but not with a model listing", true + } + + // Ours rather than the vendor's — a request this code built badly, or a + // catalog entry that does not match its parser. Still unverified, so it + // still blocks. + return "the provider could not be checked", true +} diff --git a/management/internals/modules/agentnetwork/credentialcheck_test.go b/management/internals/modules/agentnetwork/credentialcheck_test.go new file mode 100644 index 00000000000..8bc8f0b58d8 --- /dev/null +++ b/management/internals/modules/agentnetwork/credentialcheck_test.go @@ -0,0 +1,605 @@ +package agentnetwork + +import ( + "context" + "crypto/tls" + "errors" + "fmt" + "net" + "os" + "syscall" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/management/internals/modules/agentnetwork/modeldiscovery" + "github.com/netbirdio/netbird/management/internals/modules/agentnetwork/types" + "github.com/netbirdio/netbird/management/server/permissions/modules" + "github.com/netbirdio/netbird/management/server/permissions/operations" + "github.com/netbirdio/netbird/management/server/store" + "github.com/netbirdio/netbird/shared/management/status" +) + +// stubLister stands in for the vendor on the write path. It records what it +// was asked so a test can assert not only that the check ran, but that it ran +// against the right upstream and the right credential — and, for an edit that +// touches neither, that it did not run at all. +type stubLister struct { + err error + requests []modeldiscovery.Request +} + +func (s *stubLister) Fetch(_ context.Context, req modeldiscovery.Request) ([]modeldiscovery.Model, error) { + s.requests = append(s.requests, req) + if s.err != nil { + return nil, s.err + } + return []modeldiscovery.Model{{ID: "a-model", PricingKnown: true}}, nil +} + +func (s *stubLister) calls() int { return len(s.requests) } + +func (s *stubLister) only(t *testing.T) modeldiscovery.Request { + t.Helper() + require.Len(t, s.requests, 1, "the vendor must be asked exactly once") + return s.requests[0] +} + +// TestCredentialCheckFailure_SeparatesTheUrlFromTheCredential is the contract +// the provider form is written against: an operator gets told which of the two +// fields they have to look at, and the message says so without a status code +// and without echoing the URL back at them. +func TestCredentialCheckFailure_SeparatesTheUrlFromTheCredential(t *testing.T) { + cases := []struct { + name string + err error + want string + }{ + { + name: "401 is the credential", + err: &modeldiscovery.VendorStatusError{Provider: "OpenAI", Status: 401}, + want: "the provider rejected the credential", + }, + { + name: "403 is the credential", + err: &modeldiscovery.VendorStatusError{Provider: "Bedrock", Status: 403}, + want: "the provider rejected the credential", + }, + { + // The host authenticated us fine and then said it has no such + // endpoint, which is the URL being wrong rather than the key. + name: "404 is the url", + err: &modeldiscovery.VendorStatusError{Provider: "OpenAI", Status: 404}, + want: "the upstream url did not answer a model listing", + }, + { + name: "405 is the url", + err: &modeldiscovery.VendorStatusError{Provider: "OpenAI", Status: 405}, + want: "the upstream url did not answer a model listing", + }, + { + name: "500 is the vendor", + err: &modeldiscovery.VendorStatusError{Provider: "Anthropic", Status: 500}, + want: "the provider returned an error", + }, + { + name: "503 is the vendor", + err: &modeldiscovery.VendorStatusError{Provider: "Anthropic", Status: 503}, + want: "the provider returned an error", + }, + { + name: "429 is the vendor", + err: &modeldiscovery.VendorStatusError{Provider: "Anthropic", Status: 429}, + want: "the provider returned an error", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, blocking := credentialCheckFailure(tc.err) + require.True(t, blocking, "a vendor refusal must block the write") + require.Equal(t, tc.want, got) + }) + } +} + +// TestCredentialCheckFailure_NamesTheTransportFault covers the failures that +// never reached the vendor. The distinction inside them is worth keeping: a +// refused connection is a wrong port and an unknown host is a wrong hostname, +// and an operator staring at a URL they believe in needs to be told which. +func TestCredentialCheckFailure_NamesTheTransportFault(t *testing.T) { + cases := []struct { + name string + err error + want string + }{ + { + name: "unknown host", + err: &net.DNSError{Err: "no such host", Name: "api.example.com", IsNotFound: true}, + want: "the upstream url could not be reached: no such host", + }, + { + name: "dns failure that is not a missing name", + err: &net.DNSError{Err: "server misbehaving", Name: "api.example.com"}, + want: "the upstream url could not be reached: dns lookup failed", + }, + { + name: "connection refused", + err: &net.OpError{Op: "dial", Net: "tcp", Err: syscall.ECONNREFUSED}, + want: "the upstream url could not be reached: connection refused", + }, + { + name: "host unreachable", + err: &net.OpError{Op: "dial", Net: "tcp", Err: syscall.EHOSTUNREACH}, + want: "the upstream url could not be reached: host unreachable", + }, + { + name: "timeout", + err: fmt.Errorf("dial: %w", os.ErrDeadlineExceeded), + want: "the upstream url could not be reached: connection timed out", + }, + { + name: "context deadline", + err: fmt.Errorf("dial: %w", context.DeadlineExceeded), + want: "the upstream url could not be reached: connection timed out", + }, + { + name: "untrusted certificate", + err: &tls.CertificateVerificationError{}, + want: "the upstream url could not be reached: tls certificate not trusted", + }, + { + name: "plaintext service on an https url", + err: tls.RecordHeaderError{Msg: "first record does not look like a TLS handshake"}, + want: "the upstream url could not be reached: not a tls endpoint", + }, + { + // Nothing we recognise. Better to say only that it could not be + // reached than to paste a Go error into the provider form. + name: "cause we do not recognise", + err: errors.New("something went sideways"), + want: "the upstream url could not be reached", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + wrapped := &modeldiscovery.UnreachableError{Provider: "OpenAI", Err: tc.err} + got, blocking := credentialCheckFailure(wrapped) + require.True(t, blocking, "an unreachable upstream must block the write") + require.Equal(t, tc.want, got) + }) + } +} + +// TestCredentialCheckFailure_AnAnsweringUrlThatIsNotTheApi covers the case a +// status probe would wave through: the host is up, the credential was accepted +// or not required, and the body is a login page. Reusing the discovery parser +// for the check is what catches it. +func TestCredentialCheckFailure_AnAnsweringUrlThatIsNotTheApi(t *testing.T) { + err := fmt.Errorf("%w: decode model listing: unexpected token", modeldiscovery.ErrUnparseableListing) + + got, blocking := credentialCheckFailure(err) + require.True(t, blocking) + require.Equal(t, "the upstream url answered, but not with a model listing", got) +} + +// TestCredentialCheckFailure_WhatCannotBeCheckedIsNotAFailure pins the +// difference between "this record is wrong" and "we have no way to ask". A +// gateway with no listing endpoint, a Bedrock record pointed at a proxy, and a +// self-hosted endpoint the proxy reaches through the tunnel are all legitimate +// providers. Blocking them would make the feature a lockout. +func TestCredentialCheckFailure_WhatCannotBeCheckedIsNotAFailure(t *testing.T) { + cases := map[string]error{ + "no listing endpoint": modeldiscovery.ErrNoDiscovery, + "no derivable host": fmt.Errorf("%w: %w: bedrock", modeldiscovery.ErrInvalidRequest, modeldiscovery.ErrNoDiscoveryHost), + "private upstream": fmt.Errorf("%w: %w: 10.0.0.5", modeldiscovery.ErrInvalidRequest, modeldiscovery.ErrPrivateHost), + } + + for name, err := range cases { + t.Run(name, func(t *testing.T) { + message, blocking := credentialCheckFailure(err) + require.False(t, blocking, "a provider we cannot check must still save") + require.Empty(t, message) + }) + } +} + +// TestCredentialCheckFailure_AnUnrecognisedFailureStillBlocks covers a fault of +// ours rather than the vendor's — a malformed request this code built, or a +// catalog entry whose parser does not match its endpoint. The record went +// unverified either way, and silently saving what we could not check is the +// thing this feature exists to prevent. +func TestCredentialCheckFailure_AnUnrecognisedFailureStillBlocks(t *testing.T) { + message, blocking := credentialCheckFailure(errors.New("no parser for listing shape \"\"")) + require.True(t, blocking) + require.Equal(t, "the provider could not be checked", message) +} + +// newCheckedProvider returns a record shaped the way the handler guarantees +// one: a known catalog id, a public upstream and a key. +func newCheckedProvider(accountID string) *types.Provider { + provider := types.NewProvider(accountID) + provider.ProviderID = "openai_api" + provider.Name = "openai" + provider.UpstreamURL = "https://api.openai.com" + provider.APIKey = "sk-good" + provider.Enabled = true + return provider +} + +// TestCreateProvider_RefusesARecordTheVendorRejects is the whole point of the +// feature: a key with a character missing used to save cleanly and surface +// minutes later as a failed request with nothing pointing back at the record. +func TestCreateProvider_RefusesARecordTheVendorRejects(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + f.vendor.err = &modeldiscovery.VendorStatusError{Provider: "OpenAI", Status: 401} + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) + + _, err := f.manager.CreateProvider(ctx, "user1", newCheckedProvider("account1")) + + require.Error(t, err) + require.Contains(t, err.Error(), "the provider rejected the credential") + + var sErr *status.Error + require.ErrorAs(t, err, &sErr) + require.Equal(t, status.InvalidArgument, sErr.Type(), "the refusal must reach the caller as a 422") + + stored, err := f.store.GetAccountAgentNetworkProviders(ctx, store.LockingStrengthNone, "account1") + require.NoError(t, err) + require.Empty(t, stored, "a record that failed its check must not be written") +} + +// TestCreateProvider_ChecksTheCredentialItWasGiven pins what the vendor is +// asked with, since a check run against the wrong upstream or a stale key +// would pass while proving nothing. +func TestCreateProvider_ChecksTheCredentialItWasGiven(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) + + _, err := f.manager.CreateProvider(ctx, "user1", newCheckedProvider("account1")) + require.NoError(t, err) + + asked := f.vendor.only(t) + require.Equal(t, "openai_api", asked.CatalogID) + require.Equal(t, "https://api.openai.com", asked.UpstreamURL) + require.Equal(t, "sk-good", asked.APIKey) +} + +// TestUpdateProvider_AUrlOnlyChangeIsCheckedAgainstTheStoredKey covers the +// case that shaped where the check sits. The key never returns to the browser, +// so an operator editing only the URL has none to offer — the stored one is +// the only credential there is, and the new URL still has to be proven with +// it. +func TestUpdateProvider_AUrlOnlyChangeIsCheckedAgainstTheStoredKey(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) + + created, err := f.manager.CreateProvider(ctx, "user1", newCheckedProvider("account1")) + require.NoError(t, err) + f.vendor.requests = nil + + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Update, true) + edit := newCheckedProvider("account1") + edit.ID = created.ID + edit.UpstreamURL = "https://gateway.example.com" + edit.APIKey = "" // the form sends no key when it was not retyped + + _, err = f.manager.UpdateProvider(ctx, "user1", edit) + require.NoError(t, err) + + asked := f.vendor.only(t) + require.Equal(t, "https://gateway.example.com", asked.UpstreamURL, "the new url must be what gets tested") + require.Equal(t, "sk-good", asked.APIKey, "and the stored key must be what tests it") +} + +// TestUpdateProvider_AFailedRotationLeavesTheWorkingKeyInPlace is the +// half-applied state the check must never produce: refusing the new key while +// having already replaced the old one would take the provider down. +func TestUpdateProvider_AFailedRotationLeavesTheWorkingKeyInPlace(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) + + created, err := f.manager.CreateProvider(ctx, "user1", newCheckedProvider("account1")) + require.NoError(t, err) + + f.vendor.err = &modeldiscovery.VendorStatusError{Provider: "OpenAI", Status: 403} + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Update, true) + rotation := newCheckedProvider("account1") + rotation.ID = created.ID + rotation.APIKey = "sk-typo" + + _, err = f.manager.UpdateProvider(ctx, "user1", rotation) + require.Error(t, err) + require.Contains(t, err.Error(), "the provider rejected the credential") + + stored, err := f.store.GetAgentNetworkProviderByID(ctx, store.LockingStrengthNone, "account1", created.ID) + require.NoError(t, err) + require.Equal(t, "sk-good", stored.APIKey, "the rejected key must not have replaced the working one") +} + +// TestUpdateProvider_AnEditTouchingNeitherFieldAsksNoVendor keeps renames, +// model rows and price edits off the vendor's doorstep. They have nothing new +// to prove, and making them wait on a vendor — or fail because one is having a +// bad day — would be a tax on edits that carry no risk. +func TestUpdateProvider_AnEditTouchingNeitherFieldAsksNoVendor(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) + + created, err := f.manager.CreateProvider(ctx, "user1", newCheckedProvider("account1")) + require.NoError(t, err) + f.vendor.requests = nil + // Any call at all now would fail the update, which is what makes the + // assertion below load-bearing rather than decorative. + f.vendor.err = &modeldiscovery.VendorStatusError{Provider: "OpenAI", Status: 500} + + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Update, true) + rename := newCheckedProvider("account1") + rename.ID = created.ID + rename.Name = "openai-renamed" + rename.APIKey = "" + + _, err = f.manager.UpdateProvider(ctx, "user1", rename) + require.NoError(t, err, "an edit that changes neither url nor key must not be checked") + require.Zero(t, f.vendor.calls(), "and must not reach the vendor at all") +} + +// TestCreateProvider_AProviderWeCannotCheckStillSaves covers the eleven +// catalog entries with no listing endpoint, a Bedrock record behind a proxy, +// and a self-hosted endpoint on a private network. None of those are evidence +// the record is wrong, and refusing them would make this a lockout. +func TestCreateProvider_AProviderWeCannotCheckStillSaves(t *testing.T) { + cases := map[string]error{ + "gateway with no listing endpoint": modeldiscovery.ErrNoDiscovery, + "bedrock behind a proxy": fmt.Errorf("%w: %w", modeldiscovery.ErrInvalidRequest, modeldiscovery.ErrNoDiscoveryHost), + "self-hosted on a private network": fmt.Errorf("%w: %w", modeldiscovery.ErrInvalidRequest, modeldiscovery.ErrPrivateHost), + } + + for name, vendorErr := range cases { + t.Run(name, func(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + f.vendor.err = vendorErr + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) + + created, err := f.manager.CreateProvider(ctx, "user1", newCheckedProvider("account1")) + require.NoError(t, err) + require.NotNil(t, created) + + stored, err := f.store.GetAccountAgentNetworkProviders(ctx, store.LockingStrengthNone, "account1") + require.NoError(t, err) + require.Len(t, stored, 1, "a provider we cannot check must still be written") + }) + } +} + +// TestDiscoveryFailure_TellsTheOperatorWhatWentWrong covers the button, not the +// save. Pressing "Load models from provider" against a bad key used to answer +// "internal server error", which names neither the thing that failed nor +// anything the operator could act on — every outcome here is their key or their +// URL. +func TestDiscoveryFailure_TellsTheOperatorWhatWentWrong(t *testing.T) { + cases := map[string]struct { + err error + want string + }{ + "refused credential": { + err: &modeldiscovery.VendorStatusError{Provider: "Bedrock", Status: 403}, + want: "the provider rejected the credential", + }, + "upstream that is not the api": { + err: &modeldiscovery.VendorStatusError{Provider: "OpenAI", Status: 404}, + want: "the upstream url did not answer a model listing", + }, + "upstream that does not resolve": { + err: &modeldiscovery.UnreachableError{ + Provider: "OpenAI", + Err: &net.DNSError{Err: "no such host", Name: "api.example.com", IsNotFound: true}, + }, + want: "the upstream url could not be reached: no such host", + }, + "vendor having a bad day": { + err: &modeldiscovery.VendorStatusError{Provider: "Anthropic", Status: 503}, + want: "the provider returned an error", + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + err := discoveryFailure(context.Background(), "openai_api", tc.err) + require.EqualError(t, err, tc.want) + + var sErr *status.Error + require.ErrorAs(t, err, &sErr) + require.Equal(t, status.InvalidArgument, sErr.Type(), + "a failure the operator caused must not read as a server fault") + }) + } +} + +// TestDiscoveryFailure_LeavesTheCatalogFactsAlone keeps the two outcomes the +// handler already maps. A provider with no listing endpoint is a fact about the +// catalog entry, and the caller falls back to the catalog's own models rather +// than showing an error at all — rewriting it as a refusal would turn a normal +// path into one. +func TestDiscoveryFailure_LeavesTheCatalogFactsAlone(t *testing.T) { + for name, err := range map[string]error{ + "no listing endpoint": modeldiscovery.ErrNoDiscovery, + "bad request": fmt.Errorf("%w: unknown catalog provider", modeldiscovery.ErrInvalidRequest), + } { + t.Run(name, func(t *testing.T) { + require.Equal(t, err, discoveryFailure(context.Background(), "openai_api", err), + "the handler's own mapping must still see the original error") + }) + } +} + +// TestDiscoverProviderModels_SurfacesTheVendorRefusal drives the manager rather +// than the classifier, so a future refactor that stops translating on this path +// fails here rather than silently going back to 500s. +func TestDiscoverProviderModels_SurfacesTheVendorRefusal(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + f.vendor.err = &modeldiscovery.VendorStatusError{Provider: "OpenAI", Status: 401} + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) + + _, err := f.manager.DiscoverProviderModels(ctx, "account1", "user1", modeldiscovery.Request{ + CatalogID: "openai_api", + UpstreamURL: "https://api.openai.com", + APIKey: "sk-wrong", + }, "") + + require.EqualError(t, err, "the provider rejected the credential") +} + +// TestDiscoverProviderModels_ListsAgainstTheUrlOnTheForm covers the edit the +// operator cannot otherwise make: the upstream has been retyped and the +// credential has not, because the API never returned it to be retyped. Naming +// the record supplies the key; the request supplies the URL under test. +func TestDiscoverProviderModels_ListsAgainstTheUrlOnTheForm(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + // Twice: the create, and the listing, which is gated on Create too. + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) + + created, err := f.manager.CreateProvider(ctx, "user1", newCheckedProvider("account1")) + require.NoError(t, err) + f.vendor.requests = nil + + _, err = f.manager.DiscoverProviderModels(ctx, "account1", "user1", modeldiscovery.Request{ + CatalogID: "openai_api", + UpstreamURL: "https://gateway.example.com", + }, created.ID) + require.NoError(t, err) + + asked := f.vendor.only(t) + require.Equal(t, "https://gateway.example.com", asked.UpstreamURL, "the typed url must be the one listed against") + require.Equal(t, "sk-good", asked.APIKey, "and the stored key must be what lists it") +} + +// TestDiscoverProviderModels_FallsBackToTheStoredUrl keeps the plain refresh +// working: a request naming only the record still reaches the saved upstream. +func TestDiscoverProviderModels_FallsBackToTheStoredUrl(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) + + created, err := f.manager.CreateProvider(ctx, "user1", newCheckedProvider("account1")) + require.NoError(t, err) + stored := f.vendor.only(t).UpstreamURL + f.vendor.requests = nil + + _, err = f.manager.DiscoverProviderModels(ctx, "account1", "user1", modeldiscovery.Request{ + CatalogID: "openai_api", + }, created.ID) + require.NoError(t, err) + + require.Equal(t, stored, f.vendor.only(t).UpstreamURL) +} + +// TestUpdateProvider_MovingARecordToAnotherVendorIsChecked covers the edit that +// changes neither field the vendor judges and still invalidates both. The +// catalog entry decides which vendor is asked and under which auth header, so +// the unchanged credential is now being offered somewhere it has never been +// accepted. +func TestUpdateProvider_MovingARecordToAnotherVendorIsChecked(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) + + created, err := f.manager.CreateProvider(ctx, "user1", newCheckedProvider("account1")) + require.NoError(t, err) + f.vendor.requests = nil + + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Update, true) + edit := newCheckedProvider("account1") + edit.ID = created.ID + edit.ProviderID = "anthropic_api" + edit.APIKey = "" + + _, err = f.manager.UpdateProvider(ctx, "user1", edit) + require.NoError(t, err) + + require.Equal(t, "anthropic_api", f.vendor.only(t).CatalogID, + "the new vendor is the one that has to accept the key") +} + +// TestCreateProvider_ASkipTlsRecordIsNotCheckedAgainstItsCertificate covers the +// lockout the check would otherwise be: the flag exists for a self-hosted +// endpoint behind a certificate nothing public can verify, and discovery +// verifies certificates. Refusing the save would reject the record for the one +// reason the operator already declared they accept. +func TestCreateProvider_ASkipTlsRecordIsNotCheckedAgainstItsCertificate(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + f.vendor.err = &modeldiscovery.UnreachableError{ + Provider: "OpenAI", + Err: &tls.CertificateVerificationError{}, + } + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) + + provider := newCheckedProvider("account1") + provider.SkipTLSVerification = true + + created, err := f.manager.CreateProvider(ctx, "user1", provider) + require.NoError(t, err, "a record we were told not to verify must still save") + require.NotEmpty(t, created.ID) + require.Zero(t, f.vendor.calls(), "and the vendor must not be asked at all") +} + +// TestCreateProvider_TheStoredKeyIsTheOneThatWasChecked pins the two halves to +// one value. The vendor call trims the credential before building its auth +// header; the synthesiser substitutes the stored one verbatim. A key pasted +// with surrounding whitespace would otherwise pass its check and then fail +// every request the provider serves. +func TestCreateProvider_TheStoredKeyIsTheOneThatWasChecked(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) + + provider := newCheckedProvider("account1") + provider.APIKey = " sk-good\n" + + created, err := f.manager.CreateProvider(ctx, "user1", provider) + require.NoError(t, err) + + require.Equal(t, "sk-good", f.vendor.only(t).APIKey, "the vendor is asked about the trimmed key") + + stored, err := f.store.GetAgentNetworkProviderByID(ctx, store.LockingStrengthNone, "account1", created.ID) + require.NoError(t, err) + require.Equal(t, "sk-good", stored.APIKey, "and that is the one the proxy will send") +} + +// TestUpdateProvider_TurningTlsVerificationBackOnChecksTheRecord covers the +// hole the skip-TLS exemption opens on its own. Such a record is stored without +// ever being checked, so the moment verification is switched back on is the +// first moment it can be checked at all — and none of the three fields the +// re-check usually watches has to move for that to happen. +func TestUpdateProvider_TurningTlsVerificationBackOnChecksTheRecord(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) + + unchecked := newCheckedProvider("account1") + unchecked.SkipTLSVerification = true + created, err := f.manager.CreateProvider(ctx, "user1", unchecked) + require.NoError(t, err) + require.Zero(t, f.vendor.calls(), "the create was exempt") + + f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Update, true) + edit := newCheckedProvider("account1") + edit.ID = created.ID + edit.APIKey = "" + edit.SkipTLSVerification = false + + _, err = f.manager.UpdateProvider(ctx, "user1", edit) + require.NoError(t, err) + require.Equal(t, 1, f.vendor.calls(), "switching verification on must check what was never checked") +} diff --git a/management/internals/modules/agentnetwork/handlers/providers_handler.go b/management/internals/modules/agentnetwork/handlers/providers_handler.go index 645d1da619e..64b0e4e0ae2 100644 --- a/management/internals/modules/agentnetwork/handlers/providers_handler.go +++ b/management/internals/modules/agentnetwork/handlers/providers_handler.go @@ -339,6 +339,14 @@ func validate(req *api.AgentNetworkProviderRequest, requireAPIKey bool) error { if requireAPIKey && (req.ApiKey == nil || strings.TrimSpace(*req.ApiKey) == "") { return status.Errorf(status.InvalidArgument, "api_key is required") } + // An update omits api_key to keep the stored credential. A key that is + // present but blank is not that: Provider.FromAPIRequest drops it exactly + // as if it were absent, so a rotation the operator believes they performed + // would answer 200 having changed nothing. Refuse it here, where the + // request still carries the difference between absent and blank. + if req.ApiKey != nil && strings.TrimSpace(*req.ApiKey) == "" { + return status.Errorf(status.InvalidArgument, "api_key must be omitted to keep the stored credential rather than sent blank") + } if req.Models != nil { for i, m := range *req.Models { if err := validateModel(i, m); err != nil { diff --git a/management/internals/modules/agentnetwork/handlers/providers_handler_test.go b/management/internals/modules/agentnetwork/handlers/providers_handler_test.go index 05024cde917..033de3b8aa6 100644 --- a/management/internals/modules/agentnetwork/handlers/providers_handler_test.go +++ b/management/internals/modules/agentnetwork/handlers/providers_handler_test.go @@ -54,6 +54,39 @@ func TestValidate_ModelRates(t *testing.T) { } } +// TestValidate_ABlankApiKeyIsNotTheSameAsAnOmittedOne covers the one shape the +// manager's own guard cannot see. Provider.FromAPIRequest assigns the key only +// when it trims to something, so a request carrying " " arrives at +// UpdateProvider indistinguishable from one that omitted it — the stored +// credential is kept and the write answers 200, telling an operator who thinks +// they just rotated a key that it worked. +// +// The request still knows the difference, so the refusal belongs here. +func TestValidate_ABlankApiKeyIsNotTheSameAsAnOmittedOne(t *testing.T) { + req := func(key *string) *api.AgentNetworkProviderRequest { + return &api.AgentNetworkProviderRequest{ + ProviderId: "openai_api", + Name: "OpenAI", + UpstreamUrl: "https://api.openai.com", + ApiKey: key, + } + } + + blank := " " + err := validate(req(&blank), false) + require.Error(t, err, "a blank api_key on update must not be read as 'keep what is stored'") + assert.Contains(t, err.Error(), "api_key") + + require.NoError(t, validate(req(nil), false), "an omitted api_key is how an update keeps the stored credential") + + // Create already refuses this, and keeps its own message: a caller who sent + // no usable key is told the field is required rather than being told how to + // preserve a credential that does not exist yet. + err = validate(req(&blank), true) + require.Error(t, err) + assert.Contains(t, err.Error(), "api_key is required") +} + // TestProviderHandler_UpdateReplacesFullState pins the update contract shared // with the other PUT endpoints: the request replaces the provider's mutable // state, so optional fields absent from the JSON land as their zero values. @@ -64,10 +97,13 @@ func TestValidate_ModelRates(t *testing.T) { func TestProviderHandler_UpdateReplacesFullState(t *testing.T) { f := newAgentNetworkHandlerFixture(t) + // A private upstream: the save-time credential check leaves it unchecked + // rather than spending "sk-test" against the real api.openai.com, which + // the vendor refuses. create := `{ "provider_id": "openai_api", "name": "openai", - "upstream_url": "https://api.openai.com", + "upstream_url": "https://10.255.255.1", "api_key": "sk-test", "enabled": true, "metadata_disabled": true, @@ -84,7 +120,7 @@ func TestProviderHandler_UpdateReplacesFullState(t *testing.T) { // Minimal update: only the required fields, no api_key. Everything // optional must land as its zero value. - update := `{"provider_id": "openai_api", "name": "openai-renamed", "upstream_url": "https://api.openai.com", "enabled": true}` + update := `{"provider_id": "openai_api", "name": "openai-renamed", "upstream_url": "https://10.255.255.1", "enabled": true}` rec = f.do(t, nethttp.MethodPut, "/agent-network/providers/"+created.Id, update) require.Equal(t, nethttp.StatusOK, rec.Code, "update without api_key must succeed (key is preserved): %s", rec.Body.String()) diff --git a/management/internals/modules/agentnetwork/manager.go b/management/internals/modules/agentnetwork/manager.go index 41789195e52..c3d396b6666 100644 --- a/management/internals/modules/agentnetwork/manager.go +++ b/management/internals/modules/agentnetwork/manager.go @@ -126,13 +126,15 @@ type managerImpl struct { proxyController proxy.Controller // modelDiscovery queries vendors for the models a credential can reach. - // A field rather than a package call so tests can drive it without - // reaching the network. + // An interface rather than the concrete client because it is now on a + // write path: the credential check runs inside CreateProvider and + // UpdateProvider, so every test that saves a provider would otherwise + // reach a vendor over the network to do it. // // One instance serves every request for the process's lifetime, so its // fields must stay read-only after construction: lazy initialisation // inside Fetch or httpClient would race across request goroutines. - modelDiscovery *modeldiscovery.Client + modelDiscovery ModelLister // reconcileCache holds the last set of synthesised proxy mappings // per account, each paired with the proxy that served it, so a change @@ -146,6 +148,19 @@ type managerImpl struct { labelRng *rand.Rand } +// ManagerOption replaces a manager dependency at construction. Production +// passes none; each option exists for something a test cannot let run for +// real. +type ManagerOption func(*managerImpl) + +// WithModelLister replaces the vendor call behind the provider credential +// check. A test that saves a provider needs this — the check runs inside +// CreateProvider and UpdateProvider, so the write path reaches a vendor +// without it. +func WithModelLister(lister ModelLister) ManagerOption { + return func(m *managerImpl) { m.modelDiscovery = lister } +} + // NewManager constructs the persistent Agent Network manager. The // manager persists provider/policy/guardrail configuration and, on // every mutation, reconciles the in-memory synthesised reverse-proxy @@ -156,8 +171,9 @@ func NewManager( permissionsManager permissions.Manager, accountManager account.Manager, proxyController proxy.Controller, + opts ...ManagerOption, ) Manager { - return &managerImpl{ + m := &managerImpl{ store: store, accountManager: accountManager, permissionsManager: permissionsManager, @@ -166,6 +182,10 @@ func NewManager( reconcileCache: make(map[string]map[string]syntheticMapping), labelRng: rand.New(rand.NewSource(time.Now().UnixNano())), } + for _, opt := range opts { + opt(m) + } + return m } func (m *managerImpl) GetAllProviders(ctx context.Context, accountID, userID string) ([]*types.Provider, error) { @@ -184,9 +204,11 @@ func (m *managerImpl) GetProvider(ctx context.Context, accountID, userID, provid // DiscoverProviderModels asks the vendor which models a credential can reach. // -// recordID, when set, names an existing provider whose stored credential and -// upstream are used instead of the ones in req — so the dashboard can refresh -// the list without ever holding the key. +// recordID, when set, names an existing provider whose stored credential is +// used instead of the one in req — so the dashboard can refresh the list +// without ever holding the key. An upstream in req overrides the stored one, +// which is what lets a form list against a URL the operator has typed but not +// saved yet, using the credential they cannot retype. // // Gated on Create rather than Read: this spends the operator's credential // against a third party, which is not something a read-only role should be @@ -207,11 +229,29 @@ func (m *managerImpl) DiscoverProviderModels(ctx context.Context, accountID, use // name a different one would run a provider's credential against // whichever vendor endpoint they picked. req.CatalogID = record.ProviderID - req.UpstreamURL = record.UpstreamURL req.APIKey = record.APIKey + // The upstream is the one field the caller may override, so that a URL + // typed into the form can be listed against before it is saved. + // + // It sends the stored credential to a host the caller named, which is + // a capability they already have: the same permission set updates the + // record's upstream, and that write runs this same check against + // whatever it is pointed at. What it would not otherwise be is silent, + // since the write leaves an activity event behind — so the override is + // recorded here. + if strings.TrimSpace(req.UpstreamURL) == "" { + req.UpstreamURL = record.UpstreamURL + } else if req.UpstreamURL != record.UpstreamURL { + log.WithContext(ctx).Infof("agent network provider %s listed against caller-supplied upstream %s by user %s", + recordID, req.UpstreamURL, userID) + } } - return m.modelDiscovery.Fetch(ctx, req) + models, err := m.modelDiscovery.Fetch(ctx, req) + if err != nil { + return nil, discoveryFailure(ctx, req.CatalogID, err) + } + return models, nil } // CreateProvider persists a new provider for the account. Providers have no @@ -229,6 +269,18 @@ func (m *managerImpl) CreateProvider(ctx context.Context, userID string, provide if strings.TrimSpace(provider.APIKey) == "" { return nil, status.Errorf(status.InvalidArgument, "api_key is required when creating an agent network provider") } + // Stored as it will be sent. The vendor call below trims the key before + // building the auth header while the synthesiser substitutes the stored + // value verbatim, so a key pasted with surrounding whitespace would pass + // its check and then fail every request the provider serves. + provider.APIKey = strings.TrimSpace(provider.APIKey) + + // Before anything is persisted: a record whose upstream or credential does + // not work is rejected here rather than discovered later as a failed + // request with nothing pointing back at it. + if err := m.checkProviderCredential(ctx, provider); err != nil { + return nil, err + } if provider.ID == "" { fresh := types.NewProvider(provider.AccountID) @@ -264,11 +316,47 @@ func (m *managerImpl) UpdateProvider(ctx context.Context, userID string, provide // Preserve the API key if the caller didn't rotate it. A // whitespace-only value is treated as "not rotated" rather than a // real key, but it must not silently overwrite a valid stored key. - if provider.APIKey == "" { - provider.APIKey = existing.APIKey - } else if strings.TrimSpace(provider.APIKey) == "" { + switch trimmed := strings.TrimSpace(provider.APIKey); { + case provider.APIKey == "": + // Trimmed on the way through: a record stored before keys were + // normalised carries whitespace the proxy still sends, and an edit + // that preserves the key is the occasion to repair it. Doing so makes + // the comparison below see a change, which is correct — that key has + // never been tested in the form it is about to be sent in. + provider.APIKey = strings.TrimSpace(existing.APIKey) + case trimmed == "": return nil, status.Errorf(status.InvalidArgument, "api_key must be non-blank when rotating an agent network provider") + default: + // See CreateProvider: the key is stored in the form the proxy will + // send, so the check below tests what the provider will actually use. + provider.APIKey = trimmed } + + // Only the fields the vendor would judge are worth a round-trip. This same + // call carries renames, model rows and price edits, and none of those + // should wait on a vendor — or be refused because one is having a bad day. + // + // The catalog entry counts as one of them: it decides which vendor is + // asked, under which auth header, so moving a record from one to another + // sends an unchanged credential somewhere it has never been accepted. + // + // The comparison runs after the merge above, so an update that changes only + // the URL reads as unchanged on the key and is checked against the stored + // one, which is the only credential the operator has to offer here. + // + // Turning TLS verification back on is the fourth: the record was stored + // unchecked precisely because that flag was set, so this is the first + // moment it can be checked at all, and nothing else about it need change + // for that to be true. + if provider.UpstreamURL != existing.UpstreamURL || + provider.APIKey != existing.APIKey || + provider.ProviderID != existing.ProviderID || + (existing.SkipTLSVerification && !provider.SkipTLSVerification) { + if err := m.checkProviderCredential(ctx, provider); err != nil { + return nil, err + } + } + // Always preserve the session keypair across updates so existing // session cookies stay valid. The keys are server-managed and // never surfaced through the API. diff --git a/management/internals/modules/agentnetwork/modeldiscovery/discovery.go b/management/internals/modules/agentnetwork/modeldiscovery/discovery.go index 253cc63b3f4..8485e827f90 100644 --- a/management/internals/modules/agentnetwork/modeldiscovery/discovery.go +++ b/management/internals/modules/agentnetwork/modeldiscovery/discovery.go @@ -124,14 +124,28 @@ func (c *Client) Fetch(ctx context.Context, req Request) ([]Model, error) { return nil, ErrNoDiscovery } - endpoint, err := c.discoveryURL(entry, req) + // One deadline over the whole operation. Both host lookups and the request + // itself run under it, so a vendor cannot be slow twice, and a caller that + // gives up is not left waiting on a resolver. + ctx, cancel := context.WithTimeout(ctx, fetchTimeout) + defer cancel() + + // An entry with a listing host of its own answers from somewhere other + // than the upstream on the record — Bedrock lists from the control plane + // and infers on the runtime host. Reaching the listing therefore proves + // nothing about the host requests will actually go to, so that one is + // checked separately or not at all. + if entry.Discovery.Host != "" { + if err := c.checkUpstreamHost(ctx, entry, req.UpstreamURL); err != nil { + return nil, err + } + } + + endpoint, err := c.discoveryURL(ctx, entry, req) if err != nil { return nil, err } - ctx, cancel := context.WithTimeout(ctx, fetchTimeout) - defer cancel() - httpReq, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil) if err != nil { return nil, fmt.Errorf("build discovery request: %w", err) @@ -146,7 +160,7 @@ func (c *Client) Fetch(ctx context.Context, req Request) ([]Model, error) { resp, err := c.httpClient().Do(httpReq) if err != nil { - return nil, fmt.Errorf("reach %s: %w", entry.Name, err) + return nil, &UnreachableError{Provider: entry.Name, Err: err} } defer func() { _ = resp.Body.Close() }() @@ -157,7 +171,7 @@ func (c *Client) Fetch(ctx context.Context, req Request) ([]Model, error) { if resp.StatusCode != http.StatusOK { // Surface the vendor's own status. An operator whose key lacks a scope // needs to see 403 rather than a generic failure. - return nil, fmt.Errorf("%s returned %d for its model listing", entry.Name, resp.StatusCode) + return nil, &VendorStatusError{Provider: entry.Name, Status: resp.StatusCode} } ids, err := parseListing(entry.Discovery.Shape, body) @@ -176,12 +190,16 @@ func (c *Client) Fetch(ctx context.Context, req Request) ([]Model, error) { // management holds credentials for every provider, and an upstream pointed at // an internal address would turn this endpoint into a probe of the management // server's own network. -func (c *Client) discoveryURL(entry catalog.Provider, req Request) (string, error) { +func (c *Client) discoveryURL(ctx context.Context, entry catalog.Provider, req Request) (string, error) { host := entry.Discovery.Host if host == "" { parsed, err := url.Parse(strings.TrimSpace(req.UpstreamURL)) if err != nil || parsed.Host == "" { - return "", fmt.Errorf("%w: provider upstream %q is not a usable URL", ErrInvalidRequest, req.UpstreamURL) + // The URL is left out of the message on purpose: it reaches the + // operator through an endpoint that does not lowercase it, but the + // rest of this feature's copy never echoes what they typed, and one + // path that does is the one that ends up quoted in a bug report. + return "", fmt.Errorf("%w: the provider upstream is not a usable URL", ErrInvalidRequest) } host = parsed.Host } @@ -194,19 +212,47 @@ func (c *Client) discoveryURL(entry catalog.Provider, req Request) (string, erro region = RegionFromUpstream(entry, req.UpstreamURL) } if region == "" { - return "", fmt.Errorf("%w: %s discovery needs a region, and none could be read from the provider upstream", - ErrInvalidRequest, entry.Name) + return "", fmt.Errorf("%w: %w: %s discovery needs a region, and none could be read from the provider upstream", + ErrInvalidRequest, ErrNoDiscoveryHost, entry.Name) } host = strings.ReplaceAll(host, catalog.RegionPlaceholder, region) } target := &url.URL{Scheme: "https", Host: host, Path: entry.Discovery.Path, RawQuery: entry.Discovery.Query} - if err := c.checkPublicHost(target.Hostname()); err != nil { + if err := c.classifyHost(ctx, entry, target.Hostname()); err != nil { return "", err } return target.String(), nil } +// checkUpstreamHost verifies the host the operator configured, for entries +// whose listing lives elsewhere and so cannot vouch for it. +// +// A name that does not resolve is the record being wrong. One that resolves +// privately is not: an upstream behind a proxy is a supported configuration, +// and ErrPrivateHost carries that difference on to the caller, which treats it +// as unverifiable rather than as a failure. +func (c *Client) checkUpstreamHost(ctx context.Context, entry catalog.Provider, upstreamURL string) error { + parsed, err := url.Parse(strings.TrimSpace(upstreamURL)) + if err != nil || parsed.Hostname() == "" { + return fmt.Errorf("%w: the provider upstream is not a usable URL", ErrInvalidRequest) + } + return c.classifyHost(ctx, entry, parsed.Hostname()) +} + +// classifyHost renders a failed host check as the two outcomes the caller +// distinguishes. A host that refuses to resolve is the commonest way for an +// upstream to be wrong and has to arrive as unreachable rather than as an +// unclassified fault. ErrPrivateHost means something else entirely — not a bad +// host, one we decline to dial. +func (c *Client) classifyHost(ctx context.Context, entry catalog.Provider, host string) error { + err := c.checkPublicHost(ctx, host) + if err == nil || errors.Is(err, ErrPrivateHost) { + return err + } + return &UnreachableError{Provider: entry.Name, Err: err} +} + // RegionFromUpstream recovers the region an operator embedded in the provider // upstream, by matching it against the catalog's own host template. Bedrock's // template is "bedrock-runtime..amazonaws.com" and Vertex's is @@ -244,7 +290,7 @@ func RegionFromUpstream(entry catalog.Provider, upstreamURL string) string { // checkPublicHost refuses hosts that resolve to an address the management // server should never be asked to reach on an operator's behalf. -func (c *Client) checkPublicHost(host string) error { +func (c *Client) checkPublicHost(ctx context.Context, host string) error { if c.AllowPrivateHosts { return nil } @@ -255,9 +301,6 @@ func (c *Client) checkPublicHost(host string) error { if resolver == nil { resolver = net.DefaultResolver } - ctx, cancel := context.WithTimeout(context.Background(), fetchTimeout) - defer cancel() - addrs, err := resolver.LookupNetIP(ctx, "ip", host) if err != nil { return fmt.Errorf("resolve discovery host %q: %w", host, err) @@ -266,7 +309,7 @@ func (c *Client) checkPublicHost(host string) error { // loopback address is still a way to reach loopback. for _, addr := range addrs { if !isPublic(addr) { - return fmt.Errorf("%w: discovery host %q resolves to a non-public address", ErrInvalidRequest, host) + return fmt.Errorf("%w: %w: discovery host %q resolves to a non-public address", ErrInvalidRequest, ErrPrivateHost, host) } } return nil @@ -463,6 +506,13 @@ func guardDialAddress(address string) error { return fmt.Errorf("discovery dial address %q is not an IP", host) } if !isPublic(addr) { + // Deliberately not ErrPrivateHost, which means "this upstream is on a + // private network, so we cannot check it" and lets a save through + // unchecked. checkPublicHost has already cleared the target by the + // time anything is dialled, so an address refused here is not the + // operator's upstream: it is a rebinding attempt, or an HTTP proxy in + // the path. Neither may quietly skip the check — one is hostile, and + // the other would silently disable this on every provider. return fmt.Errorf("discovery refused to dial non-public address %s", addr) } return nil diff --git a/management/internals/modules/agentnetwork/modeldiscovery/discovery_test.go b/management/internals/modules/agentnetwork/modeldiscovery/discovery_test.go index 133bd5148ac..772f01436e1 100644 --- a/management/internals/modules/agentnetwork/modeldiscovery/discovery_test.go +++ b/management/internals/modules/agentnetwork/modeldiscovery/discovery_test.go @@ -2,7 +2,9 @@ package modeldiscovery import ( "context" + "errors" "io" + "net" "net/http" "net/http/httptest" "net/netip" @@ -288,7 +290,7 @@ func TestHostGuardRejectsNonPublicAddresses(t *testing.T) { func TestHostGuardResolvesAndRejectsLocalhost(t *testing.T) { cl := &Client{} - err := cl.checkPublicHost("localhost") + err := cl.checkPublicHost(context.Background(), "localhost") require.Error(t, err, "a name resolving to loopback must be refused, not just a literal address") assert.Contains(t, err.Error(), "non-public") } @@ -530,3 +532,120 @@ func TestBedrockProfilesFromAnyGeographyArrivePriced(t *testing.T) { // only form that works at invoke time. assert.Equal(t, "jp.anthropic.claude-sonnet-5-20260514-v1:0", models[0].ID) } + +// TestFetch_AHostThatWillNotResolveIsUnreachable closes a gap the live suite +// found. The SSRF guard resolves the host before any request is built, so a +// name that does not resolve fails there rather than at the transport — and +// that error used to reach the caller unclassified. A wrong hostname is the +// commonest way for an upstream to be wrong, so it has to arrive as +// "unreachable" and not as an unrecognised fault. +func TestFetch_AHostThatWillNotResolveIsUnreachable(t *testing.T) { + // A resolver whose dial always fails, so the lookup errors without the + // test depending on real DNS. + refusing := &net.Resolver{ + PreferGo: true, + Dial: func(ctx context.Context, network, address string) (net.Conn, error) { + return nil, errors.New("resolver unavailable") + }, + } + client := &Client{Resolver: refusing} + + _, err := client.Fetch(context.Background(), Request{ + CatalogID: "openai_api", + UpstreamURL: "https://not-a-real-vendor-host.example.invalid", + APIKey: "sk-test", + }) + + require.Error(t, err) + var unreachable *UnreachableError + require.ErrorAs(t, err, &unreachable, "a host that will not resolve must classify as unreachable") + require.NotErrorIs(t, err, ErrPrivateHost, "it is not a host we declined to dial") +} + +// TestFetch_AProxyInThePathDoesNotSilentlyDisableTheCheck pins a fail-open the +// dial-time guard can produce. checkPublicHost clears the target before +// anything is dialled, so a private address refused at the socket is never the +// operator's upstream — it is a rebinding attempt, or an HTTP proxy the +// management server egresses through. Reporting either as ErrPrivateHost would +// read as "this provider cannot be checked" and let every save through +// unchecked, which is how a proxied deployment would install this feature and +// have it quietly do nothing. +func TestFetch_AProxyInThePathDoesNotSilentlyDisableTheCheck(t *testing.T) { + // A transport that refuses at the socket exactly as the guard does, with a + // loopback address standing in for the proxy the dial went to. + // AllowPrivateHosts short-circuits the resolve-stage check only; the + // injected transport below is still what the request goes through. Without + // it this test resolves api.openai.com for real, and on a runner with no + // egress that lookup fails as an UnreachableError too — so it would pass + // while never reaching the socket guard it is named for. + client := &Client{AllowPrivateHosts: true, HTTPClient: &http.Client{ + Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { + return nil, guardDialAddress("127.0.0.1:38599") + }), + CheckRedirect: refuseRedirect, + }} + + _, err := client.Fetch(context.Background(), Request{ + CatalogID: "openai_api", + UpstreamURL: "https://api.openai.com", + APIKey: "sk-test", + }) + + require.Error(t, err) + require.NotErrorIs(t, err, ErrPrivateHost, + "a refusal at the socket must not read as an upstream we cannot check") + var unreachable *UnreachableError + require.ErrorAs(t, err, &unreachable, "it is the vendor we failed to reach") +} + +// roundTripFunc adapts a function to http.RoundTripper. +type roundTripFunc func(*http.Request) (*http.Response, error) + +func (f roundTripFunc) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) } + +// TestFetch_TheUpstreamIsCheckedWhenTheListingCannotVouchForIt covers the hole +// a separate listing host leaves. Bedrock lists from the control plane, so a +// record whose runtime upstream does not exist reaches a perfectly good +// listing and saves — the requests it then serves go nowhere. +// +// Both halves matter. A runtime host that cannot be resolved is the record +// being wrong, and blocks. A proxied one resolves and only leaves the region +// underivable, which stays the unverifiable outcome it already was. +func TestFetch_TheUpstreamIsCheckedWhenTheListingCannotVouchForIt(t *testing.T) { + refusing := &net.Resolver{ + PreferGo: true, + Dial: func(ctx context.Context, network, address string) (net.Conn, error) { + return nil, errors.New("resolver unavailable") + }, + } + client := &Client{Resolver: refusing} + + _, err := client.Fetch(context.Background(), Request{ + CatalogID: "bedrock_api", + // Matches no catalog template, so nothing here reaches the control + // plane the listing comes from: without its own check this upstream + // was never contacted at all. + UpstreamURL: "https://bedrock.typo.example.invalid", + APIKey: "aws-bearer", + }) + + require.Error(t, err) + var unreachable *UnreachableError + require.ErrorAs(t, err, &unreachable, "a runtime host that will not resolve must block the save") +} + +// TestFetch_AListingHostOfItsOwnDoesNotReachThroughTheUpstream keeps the check +// above from reading the operator's upstream as the place to list from. +func TestFetch_AListingHostOfItsOwnDoesNotReachThroughTheUpstream(t *testing.T) { + cl, tr := newStubClient(http.StatusOK, bedrockListing) + + _, err := cl.Fetch(context.Background(), Request{ + CatalogID: "bedrock_api", + UpstreamURL: "https://bedrock-runtime.eu-central-1.amazonaws.com", + APIKey: "aws-bearer", + }) + require.NoError(t, err) + + assert.Equal(t, "bedrock.eu-central-1.amazonaws.com", tr.got.URL.Host, + "checking the runtime host must not turn it into the listing host") +} diff --git a/management/internals/modules/agentnetwork/modeldiscovery/failure.go b/management/internals/modules/agentnetwork/modeldiscovery/failure.go new file mode 100644 index 00000000000..41e4bfb56d5 --- /dev/null +++ b/management/internals/modules/agentnetwork/modeldiscovery/failure.go @@ -0,0 +1,114 @@ +package modeldiscovery + +import ( + "context" + "crypto/tls" + "errors" + "fmt" + "net" + "os" + "syscall" +) + +// Fetch serves two callers with different needs: the model picker, which only +// needs to know it failed, and the provider credential check, which has to +// tell an operator whether the URL or the key is at fault. Each failure +// carries a type so the second does not have to branch on a message. + +// VendorStatusError reports a listing answered with something other than 200. +// Only the vendor's own code separates a refused credential (401, 403) from a +// URL that does not serve this API (404, 405) from an unwell vendor (5xx). +type VendorStatusError struct { + Provider string + Status int +} + +func (e *VendorStatusError) Error() string { + return fmt.Sprintf("%s returned %d for its model listing", e.Provider, e.Status) +} + +// UnreachableError reports that the request never reached the vendor: the +// name did not resolve, the connection was refused, TLS failed, or it timed +// out. Nothing was authenticated, so only the URL is implicated. +type UnreachableError struct { + Provider string + Err error +} + +func (e *UnreachableError) Error() string { + return fmt.Sprintf("reach %s: %v", e.Provider, e.Err) +} + +func (e *UnreachableError) Unwrap() error { return e.Err } + +// Reason names the transport failure in words an operator can act on: a wrong +// port and a wrong hostname fail differently and are worth telling apart. +// Empty means unrecognised, and the caller should say only that the host could +// not be reached rather than paste a Go error into the UI. +func (e *UnreachableError) Reason() string { + err := e.Err + + var dns *net.DNSError + if errors.As(err, &dns) { + if dns.IsNotFound { + return "no such host" + } + // Named apart from the dial timeout below. A resolver that never + // answered and an upstream that never answered send an operator to + // different places, and the generic "connection timed out" would + // describe a connection that was never attempted. + if dns.IsTimeout { + return "dns lookup timed out" + } + return "dns lookup failed" + } + + // Timeouts are checked before the syscall cases: a dial that times out is + // reported as a net.OpError wrapping a timeout, and the operator needs to + // hear "timed out" rather than the syscall underneath it. + if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, os.ErrDeadlineExceeded) { + return "connection timed out" + } + var netErr net.Error + if errors.As(err, &netErr) && netErr.Timeout() { + return "connection timed out" + } + + if errors.Is(err, syscall.ECONNREFUSED) { + return "connection refused" + } + if errors.Is(err, syscall.EHOSTUNREACH) || errors.Is(err, syscall.ENETUNREACH) { + return "host unreachable" + } + + var certErr *tls.CertificateVerificationError + if errors.As(err, &certErr) { + return "tls certificate not trusted" + } + var recordErr tls.RecordHeaderError + if errors.As(err, &recordErr) { + return "not a tls endpoint" + } + + return "" +} + +// ErrUnparseableListing marks a 200 whose body is not a listing in the shape +// the catalog declared. Distinct from a status refusal: the host answered and +// authenticated fine, it is just not the API — a login page, say. +var ErrUnparseableListing = errors.New("response is not a model listing") + +// ErrNoDiscoveryHost marks a provider whose listing host cannot be derived +// from the record: Bedrock's control-plane host comes from the region in the +// upstream, so a proxied endpoint leaves nowhere to send it, and inventing one +// would spend the credential somewhere never configured. +// +// Wraps ErrInvalidRequest so the discovery endpoint still answers 400, while a +// credential check can read it as "cannot be checked" rather than "broken". +var ErrNoDiscoveryHost = errors.New("provider has no derivable discovery host") + +// ErrPrivateHost marks an upstream resolving somewhere management will not +// dial. A self-hosted endpoint on a private network is a legitimate provider +// the proxy reaches through the tunnel, so this means the check cannot run, +// not that the record is wrong. +var ErrPrivateHost = errors.New("discovery host is not publicly routable") diff --git a/management/internals/modules/agentnetwork/modeldiscovery/parse.go b/management/internals/modules/agentnetwork/modeldiscovery/parse.go index 83048cb8a90..67a10bf3642 100644 --- a/management/internals/modules/agentnetwork/modeldiscovery/parse.go +++ b/management/internals/modules/agentnetwork/modeldiscovery/parse.go @@ -43,7 +43,7 @@ func parseOpenAIData(body []byte) ([]listedModel, error) { } `json:"data"` } if err := json.Unmarshal(body, &doc); err != nil { - return nil, fmt.Errorf("decode model listing: %w", err) + return nil, fmt.Errorf("%w: decode model listing: %w", ErrUnparseableListing, err) } out := make([]listedModel, 0, len(doc.Data)) for _, entry := range doc.Data { @@ -71,7 +71,7 @@ func parseBedrockInferenceProfiles(body []byte) ([]listedModel, error) { } `json:"inferenceProfileSummaries"` } if err := json.Unmarshal(body, &doc); err != nil { - return nil, fmt.Errorf("decode inference-profile listing: %w", err) + return nil, fmt.Errorf("%w: decode inference-profile listing: %w", ErrUnparseableListing, err) } out := make([]listedModel, 0, len(doc.Summaries)) for _, entry := range doc.Summaries { @@ -98,7 +98,7 @@ func parseVertexPublisherModels(body []byte) ([]listedModel, error) { } `json:"publisherModels"` } if err := json.Unmarshal(body, &doc); err != nil { - return nil, fmt.Errorf("decode publisher-model listing: %w", err) + return nil, fmt.Errorf("%w: decode publisher-model listing: %w", ErrUnparseableListing, err) } out := make([]listedModel, 0, len(doc.Models)) for _, entry := range doc.Models { diff --git a/management/internals/modules/agentnetwork/settings_bootstrap_test.go b/management/internals/modules/agentnetwork/settings_bootstrap_test.go index fc6fd8b82df..fea62353ea0 100644 --- a/management/internals/modules/agentnetwork/settings_bootstrap_test.go +++ b/management/internals/modules/agentnetwork/settings_bootstrap_test.go @@ -26,6 +26,10 @@ type bootstrapFixture struct { manager Manager store store.Store perms *permissions.MockManager + // vendor stands in for the provider credential check's vendor call, which + // runs on every provider write. Without it these tests would reach a real + // vendor to save a record. + vendor *stubLister } func newBootstrapFixture(t *testing.T) *bootstrapFixture { @@ -47,10 +51,12 @@ func newBootstrapFixture(t *testing.T) *bootstrapFixture { accounts.EXPECT().UpdateAccountPeers(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes() accounts.EXPECT().BufferUpdateAccountPeers(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes() + vendor := &stubLister{} return &bootstrapFixture{ - manager: NewManager(st, perms, accounts, nil), + manager: NewManager(st, perms, accounts, nil, WithModelLister(vendor)), store: st, perms: perms, + vendor: vendor, } } @@ -211,6 +217,7 @@ func TestCreateProviderHasNoSettingsSideEffects(t *testing.T) { f.expectPermission("account1", "user1", modules.AgentNetworkProviders, operations.Create, true) provider := types.NewProvider("account1") + provider.ProviderID = "openai_api" provider.Name = "openai" provider.UpstreamURL = "https://api.openai.com" provider.APIKey = "sk-test" diff --git a/management/server/agentnetwork_budgetrule_realstack_test.go b/management/server/agentnetwork_budgetrule_realstack_test.go index 95f9c35dce3..b046581e816 100644 --- a/management/server/agentnetwork_budgetrule_realstack_test.go +++ b/management/server/agentnetwork_budgetrule_realstack_test.go @@ -96,10 +96,14 @@ func TestAgentNetwork_UpdateSettings_PreservesImmutableAndTogglesCollection(t *t assert.False(t, before.EnablePromptCollection, "prompt collection defaults off") _, err = mgr.CreateProvider(ctx, adminUserID, &agenttypes.Provider{ - AccountID: accountID, - ProviderID: "openai_api", - Name: "openai", - UpstreamURL: "https://api.openai.com", + AccountID: accountID, + ProviderID: "openai_api", + Name: "openai", + // A private address: the save-time credential check leaves it + // unchecked rather than spending a dummy key against the real + // api.openai.com, which the vendor refuses and which would make + // this test depend on the runner having egress. + UpstreamURL: "https://10.255.255.1", APIKey: "sk-test", Enabled: true, Models: []agenttypes.ProviderModel{{ID: "gpt-5.4"}}, diff --git a/management/server/agentnetwork_realstack_test.go b/management/server/agentnetwork_realstack_test.go index d4efb1607c8..d438ffbdd3c 100644 --- a/management/server/agentnetwork_realstack_test.go +++ b/management/server/agentnetwork_realstack_test.go @@ -101,10 +101,14 @@ func TestAgentNetwork_ProviderCRUD_FansOutToProxyAndClientPeers(t *testing.T) { drain(proxyCh) provider, err := agentMgr.CreateProvider(ctx, adminUserID, &agenttypes.Provider{ - AccountID: accountID, - ProviderID: "openai_api", - Name: "openai-test", - UpstreamURL: "https://api.openai.com", + AccountID: accountID, + ProviderID: "openai_api", + Name: "openai-test", + // A private address: the save-time credential check leaves it + // unchecked rather than spending a dummy key against the real + // api.openai.com, which the vendor refuses and which would make + // this test depend on the runner having egress. + UpstreamURL: "https://10.255.255.1", APIKey: "sk-test-key", Enabled: true, Models: []agenttypes.ProviderModel{{ID: "gpt-5.4"}}, diff --git a/shared/management/http/api/openapi.yml b/shared/management/http/api/openapi.yml index 3ab5a2e428e..1ad9c672a81 100644 --- a/shared/management/http/api/openapi.yml +++ b/shared/management/http/api/openapi.yml @@ -5345,7 +5345,7 @@ components: upstream_url: type: string description: | - The upstream being configured. Used to reach vendors that serve their listing from the same host as inference, and to read back the region for those whose host embeds one. Ignored when provider_id is supplied. + The upstream being configured. Used to reach vendors that serve their listing from the same host as inference, and to read back the region for those whose host embeds one. Sent alongside provider_id, it overrides the stored upstream, so an edit can be listed against the URL on the form before it is saved. example: "https://bedrock-runtime.eu-central-1.amazonaws.com" api_key: type: string @@ -5353,7 +5353,7 @@ components: example: "sk-..." provider_id: type: string - description: Existing Agent Network provider record whose stored credential and upstream should be used. Lets the form refresh the list without the client holding the key. + description: Existing Agent Network provider record to query with. Its stored credential is used, and its upstream unless upstream_url overrides it, so the form can refresh the list without the client holding the key. example: "ch8i4ug6lnn4g9hqv7m0" required: - catalog_provider_id @@ -14143,7 +14143,14 @@ 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 credential is checked against the vendor's model listing before the provider is stored, so a record the vendor will not accept is refused rather than saved. A rejected credential, a listing endpoint that does not resolve or answer, a vendor outage, and a timeout all block the write and return 422. + + What that proves about the upstream URL is narrower than the URL itself. Only its host is used: the listing is requested over HTTPS at the path the catalog entry declares, so a configured scheme or path is neither used nor validated here. Where the catalog entry has a listing host of its own — Bedrock, whose listing comes from the control plane — even the host is only resolved, never contacted, so a public host that does not answer is still stored. + + Only what cannot be checked at all is exempt and stored unverified: a catalog provider with no listing endpoint, one with no host to derive a listing from, an upstream resolving to a private address the management service will not dial, and a provider configured to skip TLS verification. tags: [ Agent Network ] security: - BearerAuth: [ ] @@ -14169,6 +14176,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 +14214,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, the API key or the catalog provider changes, the record is checked against the vendor before the change is stored, and a refusal returns 422 without replacing what was there. Switching TLS verification back on is the fourth trigger: a provider exempt from the check was stored unverified, so the edit that ends the exemption is the first opportunity to check it. Where one of the four does fire, an update that omits the API key is checked against the stored one. Edits touching none of them — a rename, model rows, price edits — are stored without a check, as are the cases the create description lists as unverifiable. tags: [ Agent Network ] security: - BearerAuth: [ ] @@ -14240,6 +14252,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: diff --git a/shared/management/http/api/types.gen.go b/shared/management/http/api/types.gen.go index db5b2e18e2b..cdd7702ad1a 100644 --- a/shared/management/http/api/types.gen.go +++ b/shared/management/http/api/types.gen.go @@ -2202,10 +2202,10 @@ type AgentNetworkModelDiscoveryRequest struct { // CatalogProviderId Catalog provider to query (AgentNetworkCatalogProvider.id). Determines the listing endpoint, the auth header and the response shape. CatalogProviderId string `json:"catalog_provider_id"` - // ProviderId Existing Agent Network provider record whose stored credential and upstream should be used. Lets the form refresh the list without the client holding the key. + // ProviderId Existing Agent Network provider record to query with. Its stored credential is used, and its upstream unless upstream_url overrides it, so the form can refresh the list without the client holding the key. ProviderId *string `json:"provider_id,omitempty"` - // UpstreamUrl The upstream being configured. Used to reach vendors that serve their listing from the same host as inference, and to read back the region for those whose host embeds one. Ignored when provider_id is supplied. + // UpstreamUrl The upstream being configured. Used to reach vendors that serve their listing from the same host as inference, and to read back the region for those whose host embeds one. Sent alongside provider_id, it overrides the stored upstream, so an edit can be listed against the URL on the form before it is saved. UpstreamUrl *string `json:"upstream_url,omitempty"` }