diff --git a/management/internals/modules/agentnetwork/credentialcheck_test.go b/management/internals/modules/agentnetwork/credentialcheck_test.go index 1d35c5858..1d6400ea8 100644 --- a/management/internals/modules/agentnetwork/credentialcheck_test.go +++ b/management/internals/modules/agentnetwork/credentialcheck_test.go @@ -503,3 +503,30 @@ func TestDiscoverProviderModels_FallsBackToTheStoredUrl(t *testing.T) { 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") +} diff --git a/management/internals/modules/agentnetwork/manager.go b/management/internals/modules/agentnetwork/manager.go index 9cf5a54e1..640ecf060 100644 --- a/management/internals/modules/agentnetwork/manager.go +++ b/management/internals/modules/agentnetwork/manager.go @@ -310,14 +310,20 @@ func (m *managerImpl) UpdateProvider(ctx context.Context, userID string, provide return nil, status.Errorf(status.InvalidArgument, "api_key must be non-blank when rotating an agent network provider") } - // Only the two fields the vendor would judge are worth a round-trip. This - // same call carries renames, model rows and price edits, and none of those + // 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. - if provider.UpstreamURL != existing.UpstreamURL || provider.APIKey != existing.APIKey { + if provider.UpstreamURL != existing.UpstreamURL || + provider.APIKey != existing.APIKey || + provider.ProviderID != existing.ProviderID { if err := m.checkProviderCredential(ctx, provider); err != nil { return nil, err } diff --git a/management/internals/modules/agentnetwork/modeldiscovery/discovery.go b/management/internals/modules/agentnetwork/modeldiscovery/discovery.go index c9af43919..63103858d 100644 --- a/management/internals/modules/agentnetwork/modeldiscovery/discovery.go +++ b/management/internals/modules/agentnetwork/modeldiscovery/discovery.go @@ -124,25 +124,28 @@ func (c *Client) Fetch(ctx context.Context, req Request) ([]Model, error) { return nil, ErrNoDiscovery } + // 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(entry, req.UpstreamURL); err != nil { + if err := c.checkUpstreamHost(ctx, entry, req.UpstreamURL); err != nil { return nil, err } } - endpoint, err := c.discoveryURL(entry, req) + 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) @@ -187,7 +190,7 @@ 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)) @@ -212,7 +215,7 @@ func (c *Client) discoveryURL(entry catalog.Provider, req Request) (string, erro } target := &url.URL{Scheme: "https", Host: host, Path: entry.Discovery.Path, RawQuery: entry.Discovery.Query} - if err := c.classifyHost(entry, target.Hostname()); err != nil { + if err := c.classifyHost(ctx, entry, target.Hostname()); err != nil { return "", err } return target.String(), nil @@ -225,12 +228,12 @@ func (c *Client) discoveryURL(entry catalog.Provider, req Request) (string, erro // 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(entry catalog.Provider, upstreamURL string) error { +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: provider upstream %q is not a usable URL", ErrInvalidRequest, upstreamURL) } - return c.classifyHost(entry, parsed.Hostname()) + return c.classifyHost(ctx, entry, parsed.Hostname()) } // classifyHost renders a failed host check as the two outcomes the caller @@ -238,8 +241,8 @@ func (c *Client) checkUpstreamHost(entry catalog.Provider, upstreamURL string) e // 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(entry catalog.Provider, host string) error { - err := c.checkPublicHost(host) +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 } @@ -283,7 +286,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 } @@ -294,9 +297,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) diff --git a/management/internals/modules/agentnetwork/modeldiscovery/discovery_test.go b/management/internals/modules/agentnetwork/modeldiscovery/discovery_test.go index 2d3921c3e..34b6f51cb 100644 --- a/management/internals/modules/agentnetwork/modeldiscovery/discovery_test.go +++ b/management/internals/modules/agentnetwork/modeldiscovery/discovery_test.go @@ -290,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") } diff --git a/shared/management/http/api/openapi.yml b/shared/management/http/api/openapi.yml index 281bb11ea..053462e1c 100644 --- a/shared/management/http/api/openapi.yml +++ b/shared/management/http/api/openapi.yml @@ -14146,7 +14146,7 @@ paths: 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. + 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 rejected credential, an upstream that does not resolve or answer, a vendor outage, and a timeout all block the write. 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, and an upstream resolving to a private address the management service will not dial. tags: [ Agent Network ] security: - BearerAuth: [ ] @@ -14213,7 +14213,7 @@ paths: 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. + 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. An update omitting the API key is checked against the stored one. Edits touching none of the three — 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: [ ]