From 6c7a6c3fb8d45d9d0c1c8d1718ac8e7dcbb63e0f Mon Sep 17 00:00:00 2001 From: mlsmaycon Date: Mon, 24 Aug 2026 07:22:20 +0000 Subject: [PATCH] [management] Classify a host that will not resolve as unreachable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 reached the caller unclassified — an operator with a typo in the hostname was told the provider could not be checked rather than that the url could not be reached. It is the commonest way for an upstream to be wrong. The live suite is what caught it: the unit tests construct the transport errors directly and so never went through the guard. The management fixture moves to a private upstream in the same change. It wants a provider row to hang a policy off, not a working vendor, and it was pointing a dummy key at the real api.openai.com — which the credential check now correctly refuses. A private address is left unchecked whether or not the run has vendor keys, and covers that path while it is there. --- e2e/agentnetwork/management_test.go | 12 +++++-- .../agentnetwork/modeldiscovery/discovery.go | 10 +++++- .../modeldiscovery/discovery_test.go | 31 +++++++++++++++++++ 3 files changed, 49 insertions(+), 4 deletions(-) diff --git a/e2e/agentnetwork/management_test.go b/e2e/agentnetwork/management_test.go index 9e3176d68..6868a9d4d 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/modeldiscovery/discovery.go b/management/internals/modules/agentnetwork/modeldiscovery/discovery.go index 42534eadb..b3f96c0a4 100644 --- a/management/internals/modules/agentnetwork/modeldiscovery/discovery.go +++ b/management/internals/modules/agentnetwork/modeldiscovery/discovery.go @@ -202,7 +202,15 @@ 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.checkPublicHost(target.Hostname()); err != nil { - return "", err + // A host that refuses to resolve fails here, before any request is + // built, and it is the commonest way for an upstream to be wrong. It + // has to reach the caller as unreachable rather than as an + // unclassified fault. ErrPrivateHost is the other outcome and means + // something else entirely — not a bad host, one we decline to dial. + if errors.Is(err, ErrPrivateHost) { + return "", err + } + return "", &UnreachableError{Provider: entry.Name, Err: err} } return target.String(), nil } diff --git a/management/internals/modules/agentnetwork/modeldiscovery/discovery_test.go b/management/internals/modules/agentnetwork/modeldiscovery/discovery_test.go index 133bd5148..9b39f387e 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" @@ -530,3 +532,32 @@ 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") +}