[management] Fix what a second review found in the credential check

Four defects and two pieces of wording, from cubic's pass over the branch.

A provider that asks the proxy to skip TLS verification was checked with a
client that verifies it, so a self-hosted endpoint behind a self-signed
certificate was refused for the one reason its operator had already declared
they accept. Those records now save unchecked, alongside the other cases this
cannot speak for. Sending the credential over a connection management declines
to verify was the other way out, and a worse one.

A key pasted with surrounding whitespace passed its check and then failed every
request: the vendor call trims before building the auth header, the synthesiser
substitutes the stored value verbatim. The key is now stored in the form it
will be sent, so what was checked is what runs.

The proxy-guard test resolved api.openai.com for real before reaching its own
transport, and on a runner with no egress that lookup failed as an
UnreachableError too — so it passed while never exercising the socket guard it
is named for.

A DNS timeout said only that the lookup failed. It now says so as a timeout,
without borrowing the wording of a connection that was never attempted.

The create description promised more than the check delivers: for Bedrock the
runtime host is resolved but never contacted, so a public host that does not
answer is still stored. It says that now, and names the TLS exemption above.
The invalid-upstream message no longer quotes the URL back, which was the one
path in this feature that echoed what the operator typed.

The live suite's single-vendor scope and its dependence on vendor availability
are now stated where the tests are, rather than left to be rediscovered.

e2e/agentnetwork/credential_check_live_test.go

@@ -67,6 +67,13 @@ func liveCredentialCases() []credentialCase {
 // 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 {
@@ -111,6 +118,12 @@ func TestLiveProviderCredentialCheck(t *testing.T) {
 // 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 {
@@ -147,6 +160,9 @@ func TestLiveProviderUrlCheck(t *testing.T) {
 // 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 {

management/internals/modules/agentnetwork/credentialcheck.go

@@ -27,6 +27,17 @@ type ModelLister interface {
 // 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,

management/internals/modules/agentnetwork/credentialcheck_test.go

@@ -530,3 +530,49 @@ func TestUpdateProvider_MovingARecordToAnotherVendorIsChecked(t *testing.T) {
 	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")
+}

management/internals/modules/agentnetwork/manager.go

@@ -269,6 +269,11 @@ 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
@@ -311,10 +316,15 @@ 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 == "" {
+	switch trimmed := strings.TrimSpace(provider.APIKey); {
+	case provider.APIKey == "":
 		provider.APIKey = existing.APIKey
-	} else if strings.TrimSpace(provider.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

management/internals/modules/agentnetwork/modeldiscovery/discovery.go

@@ -195,7 +195,11 @@ func (c *Client) discoveryURL(ctx context.Context, entry catalog.Provider, req R
 	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
 	}
@@ -231,7 +235,7 @@ func (c *Client) discoveryURL(ctx context.Context, entry catalog.Provider, req R
 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 fmt.Errorf("%w: the provider upstream is not a usable URL", ErrInvalidRequest)
 	}
 	return c.classifyHost(ctx, entry, parsed.Hostname())
 }

management/internals/modules/agentnetwork/modeldiscovery/discovery_test.go

@@ -573,7 +573,12 @@ func TestFetch_AHostThatWillNotResolveIsUnreachable(t *testing.T) {
 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.
-	client := &Client{HTTPClient: &http.Client{
+	// 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")
 		}),

management/internals/modules/agentnetwork/modeldiscovery/failure.go

@@ -53,6 +53,13 @@ func (e *UnreachableError) Reason() string {
 		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"
 	}
 

shared/management/http/api/openapi.yml

@@ -14146,7 +14146,11 @@ 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 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.
+        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. Returns 422 naming what is at fault — a rejected credential, a listing endpoint that does not resolve or answer, a vendor outage, and a timeout all block the write.
+
+        How much of the upstream URL that covers depends on the provider. Where the listing is served from the upstream itself, reaching it proves the URL. Where the catalog entry has a listing host of its own — Bedrock, whose listing comes from the control plane — the configured runtime host is resolved on its own account but 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: [ ]