diff --git a/management/internals/modules/agentnetwork/credentialcheck_test.go b/management/internals/modules/agentnetwork/credentialcheck_test.go index 4e31331a0..8bc8f0b58 100644 --- a/management/internals/modules/agentnetwork/credentialcheck_test.go +++ b/management/internals/modules/agentnetwork/credentialcheck_test.go @@ -576,3 +576,30 @@ func TestCreateProvider_TheStoredKeyIsTheOneThatWasChecked(t *testing.T) { 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/manager.go b/management/internals/modules/agentnetwork/manager.go index 173782e6c..c3d396b66 100644 --- a/management/internals/modules/agentnetwork/manager.go +++ b/management/internals/modules/agentnetwork/manager.go @@ -318,7 +318,12 @@ func (m *managerImpl) UpdateProvider(ctx context.Context, userID string, provide // real key, but it must not silently overwrite a valid stored key. switch trimmed := strings.TrimSpace(provider.APIKey); { case provider.APIKey == "": - provider.APIKey = existing.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: @@ -338,9 +343,15 @@ func (m *managerImpl) UpdateProvider(ctx context.Context, userID string, provide // 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 { + provider.ProviderID != existing.ProviderID || + (existing.SkipTLSVerification && !provider.SkipTLSVerification) { if err := m.checkProviderCredential(ctx, provider); err != nil { return nil, err } diff --git a/shared/management/http/api/openapi.yml b/shared/management/http/api/openapi.yml index c37ebf33a..f3807e15e 100644 --- a/shared/management/http/api/openapi.yml +++ b/shared/management/http/api/openapi.yml @@ -14146,9 +14146,9 @@ paths: 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. 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. + 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. - 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. + 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 ]