From 8a716f12945ad47879d7ced3c5aa38b05fdfbb15 Mon Sep 17 00:00:00 2001 From: mlsmaycon Date: Thu, 27 Aug 2026 08:33:57 +0000 Subject: [PATCH] [management] Check a record when TLS verification is switched back on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The skip-TLS exemption added a hole of its own. Such a record is stored without being checked at all, and the re-check on update watched only the upstream, the key and the catalog entry — so turning verification back on left a provider that had never been checked now running as if it had. That transition is the first moment the record can be checked, and it now is. An update that preserves the stored key trims it on the way through, so a record saved before keys were normalised is repaired by the next edit rather than carrying whitespace the proxy still sends. That makes the comparison see a change, which is right: the key has never been tested in the form it is about to be sent in. The create description also claimed more of the upstream than the check reads. Only the host is used — the listing goes to the catalog's own path over HTTPS — so a configured scheme or path is neither used nor validated there. --- .../agentnetwork/credentialcheck_test.go | 27 +++++++++++++++++++ .../internals/modules/agentnetwork/manager.go | 15 +++++++++-- shared/management/http/api/openapi.yml | 4 +-- 3 files changed, 42 insertions(+), 4 deletions(-) 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 ]