mirror of
https://github.com/netbirdio/netbird.git
synced 2026-08-24 16:41:30 +02:00
[management] Stop a proxy in the egress path from disabling the check
The dial-time guard reported every non-public address as ErrPrivateHost, which the credential check reads as "this upstream cannot be reached from here, so save it unchecked". checkPublicHost has already cleared the target by the time anything is dialled, so an address refused at the socket is never the operator's upstream — it is a rebinding attempt, or an HTTP proxy the management server egresses through. A deployment behind such a proxy would install this feature and have it silently do nothing on every provider. It now reports an ordinary failure, which classifies as unreachable and blocks. Only the resolve-stage check still means "cannot be checked", and that one knows it is looking at the operator's own host. This is also why the three fixtures below passed locally and failed in CI: a sandbox that egresses through a loopback proxy skipped the check entirely, while CI reached the real api.openai.com and had the dummy key refused. They want a provider row rather than a working vendor, so they move to a private address and no longer depend on where a hostname resolves or whether the runner has egress.
This commit is contained in:
@@ -115,7 +115,7 @@ func TestCredentialCheckFailure_NamesTheTransportFault(t *testing.T) {
|
||||
}{
|
||||
{
|
||||
name: "unknown host",
|
||||
err: &net.DNSError{Err: "no such host", Name: "api.exmaple.com", IsNotFound: true},
|
||||
err: &net.DNSError{Err: "no such host", Name: "api.example.com", IsNotFound: true},
|
||||
want: "the upstream url could not be reached: no such host",
|
||||
},
|
||||
{
|
||||
@@ -191,14 +191,9 @@ func TestCredentialCheckFailure_AnAnsweringUrlThatIsNotTheApi(t *testing.T) {
|
||||
// providers. Blocking them would make the feature a lockout.
|
||||
func TestCredentialCheckFailure_WhatCannotBeCheckedIsNotAFailure(t *testing.T) {
|
||||
cases := map[string]error{
|
||||
"no listing endpoint": modeldiscovery.ErrNoDiscovery,
|
||||
"no derivable host": fmt.Errorf("%w: %w: bedrock", modeldiscovery.ErrInvalidRequest, modeldiscovery.ErrNoDiscoveryHost),
|
||||
"private upstream": fmt.Errorf("%w: %w: 10.0.0.5", modeldiscovery.ErrInvalidRequest, modeldiscovery.ErrPrivateHost),
|
||||
"private at dial time": fmt.Errorf("%w: discovery refused to dial non-public address 10.0.0.5", modeldiscovery.ErrPrivateHost),
|
||||
"wrapped by unreachable": &modeldiscovery.UnreachableError{
|
||||
Provider: "vLLM",
|
||||
Err: fmt.Errorf("%w: dial", modeldiscovery.ErrPrivateHost),
|
||||
},
|
||||
"no listing endpoint": modeldiscovery.ErrNoDiscovery,
|
||||
"no derivable host": fmt.Errorf("%w: %w: bedrock", modeldiscovery.ErrInvalidRequest, modeldiscovery.ErrNoDiscoveryHost),
|
||||
"private upstream": fmt.Errorf("%w: %w: 10.0.0.5", modeldiscovery.ErrInvalidRequest, modeldiscovery.ErrPrivateHost),
|
||||
}
|
||||
|
||||
for name, err := range cases {
|
||||
|
||||
@@ -64,10 +64,13 @@ func TestValidate_ModelRates(t *testing.T) {
|
||||
func TestProviderHandler_UpdateReplacesFullState(t *testing.T) {
|
||||
f := newAgentNetworkHandlerFixture(t)
|
||||
|
||||
// A private upstream: the save-time credential check leaves it unchecked
|
||||
// rather than spending "sk-test" against the real api.openai.com, which
|
||||
// the vendor refuses.
|
||||
create := `{
|
||||
"provider_id": "openai_api",
|
||||
"name": "openai",
|
||||
"upstream_url": "https://api.openai.com",
|
||||
"upstream_url": "https://10.255.255.1",
|
||||
"api_key": "sk-test",
|
||||
"enabled": true,
|
||||
"metadata_disabled": true,
|
||||
@@ -84,7 +87,7 @@ func TestProviderHandler_UpdateReplacesFullState(t *testing.T) {
|
||||
|
||||
// Minimal update: only the required fields, no api_key. Everything
|
||||
// optional must land as its zero value.
|
||||
update := `{"provider_id": "openai_api", "name": "openai-renamed", "upstream_url": "https://api.openai.com", "enabled": true}`
|
||||
update := `{"provider_id": "openai_api", "name": "openai-renamed", "upstream_url": "https://10.255.255.1", "enabled": true}`
|
||||
rec = f.do(t, nethttp.MethodPut, "/agent-network/providers/"+created.Id, update)
|
||||
require.Equal(t, nethttp.StatusOK, rec.Code, "update without api_key must succeed (key is preserved): %s", rec.Body.String())
|
||||
|
||||
|
||||
@@ -471,7 +471,14 @@ func guardDialAddress(address string) error {
|
||||
return fmt.Errorf("discovery dial address %q is not an IP", host)
|
||||
}
|
||||
if !isPublic(addr) {
|
||||
return fmt.Errorf("%w: discovery refused to dial non-public address %s", ErrPrivateHost, addr)
|
||||
// Deliberately not ErrPrivateHost, which means "this upstream is on a
|
||||
// private network, so we cannot check it" and lets a save through
|
||||
// unchecked. checkPublicHost has already cleared the target by the
|
||||
// time anything is dialled, so an address refused here is not the
|
||||
// operator's upstream: it is a rebinding attempt, or an HTTP proxy in
|
||||
// the path. Neither may quietly skip the check — one is hostile, and
|
||||
// the other would silently disable this on every provider.
|
||||
return fmt.Errorf("discovery refused to dial non-public address %s", addr)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
@@ -561,3 +561,39 @@ func TestFetch_AHostThatWillNotResolveIsUnreachable(t *testing.T) {
|
||||
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")
|
||||
}
|
||||
|
||||
// TestFetch_AProxyInThePathDoesNotSilentlyDisableTheCheck pins a fail-open the
|
||||
// dial-time guard can produce. checkPublicHost clears the target before
|
||||
// anything is dialled, so a private address refused at the socket is never the
|
||||
// operator's upstream — it is a rebinding attempt, or an HTTP proxy the
|
||||
// management server egresses through. Reporting either as ErrPrivateHost would
|
||||
// read as "this provider cannot be checked" and let every save through
|
||||
// unchecked, which is how a proxied deployment would install this feature and
|
||||
// have it quietly do nothing.
|
||||
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{
|
||||
Transport: roundTripFunc(func(*http.Request) (*http.Response, error) {
|
||||
return nil, guardDialAddress("127.0.0.1:38599")
|
||||
}),
|
||||
CheckRedirect: refuseRedirect,
|
||||
}}
|
||||
|
||||
_, err := client.Fetch(context.Background(), Request{
|
||||
CatalogID: "openai_api",
|
||||
UpstreamURL: "https://api.openai.com",
|
||||
APIKey: "sk-test",
|
||||
})
|
||||
|
||||
require.Error(t, err)
|
||||
require.NotErrorIs(t, err, ErrPrivateHost,
|
||||
"a refusal at the socket must not read as an upstream we cannot check")
|
||||
var unreachable *UnreachableError
|
||||
require.ErrorAs(t, err, &unreachable, "it is the vendor we failed to reach")
|
||||
}
|
||||
|
||||
// roundTripFunc adapts a function to http.RoundTripper.
|
||||
type roundTripFunc func(*http.Request) (*http.Response, error)
|
||||
|
||||
func (f roundTripFunc) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) }
|
||||
|
||||
@@ -96,10 +96,14 @@ func TestAgentNetwork_UpdateSettings_PreservesImmutableAndTogglesCollection(t *t
|
||||
assert.False(t, before.EnablePromptCollection, "prompt collection defaults off")
|
||||
|
||||
_, err = mgr.CreateProvider(ctx, adminUserID, &agenttypes.Provider{
|
||||
AccountID: accountID,
|
||||
ProviderID: "openai_api",
|
||||
Name: "openai",
|
||||
UpstreamURL: "https://api.openai.com",
|
||||
AccountID: accountID,
|
||||
ProviderID: "openai_api",
|
||||
Name: "openai",
|
||||
// A private address: the save-time credential check leaves it
|
||||
// unchecked rather than spending a dummy key against the real
|
||||
// api.openai.com, which the vendor refuses and which would make
|
||||
// this test depend on the runner having egress.
|
||||
UpstreamURL: "https://10.255.255.1",
|
||||
APIKey: "sk-test",
|
||||
Enabled: true,
|
||||
Models: []agenttypes.ProviderModel{{ID: "gpt-5.4"}},
|
||||
|
||||
@@ -101,10 +101,14 @@ func TestAgentNetwork_ProviderCRUD_FansOutToProxyAndClientPeers(t *testing.T) {
|
||||
drain(proxyCh)
|
||||
|
||||
provider, err := agentMgr.CreateProvider(ctx, adminUserID, &agenttypes.Provider{
|
||||
AccountID: accountID,
|
||||
ProviderID: "openai_api",
|
||||
Name: "openai-test",
|
||||
UpstreamURL: "https://api.openai.com",
|
||||
AccountID: accountID,
|
||||
ProviderID: "openai_api",
|
||||
Name: "openai-test",
|
||||
// A private address: the save-time credential check leaves it
|
||||
// unchecked rather than spending a dummy key against the real
|
||||
// api.openai.com, which the vendor refuses and which would make
|
||||
// this test depend on the runner having egress.
|
||||
UpstreamURL: "https://10.255.255.1",
|
||||
APIKey: "sk-test-key",
|
||||
Enabled: true,
|
||||
Models: []agenttypes.ProviderModel{{ID: "gpt-5.4"}},
|
||||
|
||||
Reference in New Issue
Block a user