From 2a17bf0d550ff92df1e9aa17c56d10b97f605e3d Mon Sep 17 00:00:00 2001 From: riccardom Date: Wed, 2 Sep 2026 14:31:38 +0200 Subject: [PATCH] [client] Compare service URLs as endpoints, not as strings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three places in one request path each had their own notion of "same management URL": the config layer compared the parsed URLs as strings, the privileged-change gate compared scheme + host + effective port, and the MDM conflict check compared strings after filling in the default port. Only the middle one was right. A string comparison answers the wrong question. "https://api.netbird.io", "https://api.netbird.io/" and "https://API.netbird.io:443" are one endpoint written three ways, so a client restating its own management URL with a trailing slash — a normal way to write it — was still read as a client asking to be repointed, and the update-settings gate refused it. The MDM check had the same flaw against the enforced value. profilemanager.SameServiceURL is now the single comparison: same scheme, same host case-insensitively as DNS names are, same effective port. The config layer, the privileged-change gate and the MDM conflict check all defer to it, so there is one answer to "did this URL change?" instead of three. --- client/internal/profilemanager/config.go | 52 +++++++++++++--- .../config_would_change_test.go | 60 +++++++++++++++++++ client/server/mdm.go | 35 +++++------ client/server/ssh_gate.go | 18 +----- client/server/update_settings_gate_test.go | 24 ++++++++ 5 files changed, 145 insertions(+), 44 deletions(-) diff --git a/client/internal/profilemanager/config.go b/client/internal/profilemanager/config.go index acd8f3110..d84a0edc3 100644 --- a/client/internal/profilemanager/config.go +++ b/client/internal/profilemanager/config.go @@ -328,16 +328,17 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) { return false, err } } - // The comparison is between parsed URLs, not raw strings: the same - // endpoint can be written differently (an implicit :443, say), and - // treating an equivalent URL as new would rewrite the config and report a - // settings change where the configuration does not actually change. + // The comparison is on the endpoint the URL addresses, not on its + // spelling: the same endpoint can be written several ways (an implicit + // :443, a trailing slash, a different host case), and treating an + // equivalent URL as new would rewrite the config and report a settings + // change where the configuration does not actually change. if input.ManagementURL != "" { URL, err := parseURL("Management URL", input.ManagementURL) if err != nil { return false, err } - if URL.String() != config.ManagementURL.String() { + if !SameServiceURL(URL, config.ManagementURL) { log.Infof("new Management URL provided, updated to %#v (old value %#v)", URL.String(), config.ManagementURL.String()) config.ManagementURL = URL @@ -352,13 +353,13 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) { return false, err } } - // Same parsed-form comparison as the Management URL above. + // Same endpoint comparison as the Management URL above. if input.AdminURL != "" { newURL, err := parseURL("Admin Panel URL", input.AdminURL) if err != nil { return updated, err } - if newURL.String() != config.AdminURL.String() { + if !SameServiceURL(newURL, config.AdminURL) { log.Infof("new Admin Panel URL provided, updated to %#v (old value %#v)", newURL.String(), config.AdminURL.String()) config.AdminURL = newURL @@ -879,6 +880,43 @@ func ParseServiceURL(serviceName, serviceURL string) (*url.URL, error) { return parseURL(serviceName, serviceURL) } +// SameServiceURL reports whether two service URLs address the same endpoint: +// same scheme, same host compared case-insensitively as DNS names are, and +// same effective port, where an absent port means the scheme's default. +// +// This is the one comparison every caller deciding "did this URL change?" must +// use. A string comparison answers a different question: "https://host", +// "https://host/" and "https://HOST:443" are one endpoint written three ways, +// and reading them as three values makes a client that restates its own +// management URL look like a client asking to be repointed. A nil operand +// matches only another nil one. +func SameServiceURL(a, b *url.URL) bool { + if a == nil || b == nil { + return a == b + } + + return a.Scheme == b.Scheme && + strings.EqualFold(a.Hostname(), b.Hostname()) && + ServiceURLPort(a) == ServiceURLPort(b) +} + +// ServiceURLPort returns the port a service URL addresses, resolving an absent +// one to the default of its scheme. +func ServiceURLPort(u *url.URL) string { + if port := u.Port(); port != "" { + return port + } + + switch u.Scheme { + case "https": + return "443" + case "http": + return "80" + default: + return "" + } +} + func parseURL(serviceName, serviceURL string) (*url.URL, error) { parsedMgmtURL, err := url.ParseRequestURI(serviceURL) if err != nil { diff --git a/client/internal/profilemanager/config_would_change_test.go b/client/internal/profilemanager/config_would_change_test.go index 0670528a5..f899923a7 100644 --- a/client/internal/profilemanager/config_would_change_test.go +++ b/client/internal/profilemanager/config_would_change_test.go @@ -133,3 +133,63 @@ func TestPeekConfigDoesNotWriteBack(t *testing.T) { require.NoError(t, err) require.NotEqual(t, string(denormalized), string(persisted), "GetConfig is the variant that normalizes on disk") } + +// One endpoint written several ways is one endpoint. A gate that compared +// spellings refused a client restating its own management URL with a trailing +// slash, which is a normal way to write it. +func TestSameServiceURL(t *testing.T) { + tests := []struct { + a, b string + want bool + }{ + {a: "https://mgmt.example.com", b: "https://mgmt.example.com:443", want: true}, + {a: "https://mgmt.example.com", b: "https://mgmt.example.com/", want: true}, + {a: "https://mgmt.example.com/", b: "https://mgmt.example.com:443/", want: true}, + {a: "https://MGMT.example.com", b: "https://mgmt.example.com", want: true}, + {a: "http://mgmt.example.com", b: "http://mgmt.example.com:80", want: true}, + {a: "https://mgmt.example.com", b: "http://mgmt.example.com", want: false}, + {a: "https://mgmt.example.com", b: "https://mgmt.example.com:8443", want: false}, + {a: "https://mgmt.example.com", b: "https://other.example.com", want: false}, + } + + for _, tt := range tests { + t.Run(tt.a+" vs "+tt.b, func(t *testing.T) { + a, err := ParseServiceURL("a", tt.a) + require.NoError(t, err) + b, err := ParseServiceURL("b", tt.b) + require.NoError(t, err) + + require.Equal(t, tt.want, SameServiceURL(a, b)) + require.Equal(t, tt.want, SameServiceURL(b, a), "the comparison must be symmetric") + }) + } +} + +// The same spellings, through the dry run the update-settings gate uses. +func TestWouldChangeIgnoresURLSpelling(t *testing.T) { + path := filepath.Join(t.TempDir(), "seeded.json") + _, err := UpdateOrCreateConfig(ConfigInput{ + ConfigPath: path, + ManagementURL: "https://mgmt.example.com", + }) + require.NoError(t, err) + + cfg, err := GetConfig(path) + require.NoError(t, err) + + for _, spelling := range []string{ + "https://mgmt.example.com", + "https://mgmt.example.com/", + "https://mgmt.example.com:443", + "https://mgmt.example.com:443/", + "https://MGMT.example.com", + } { + changed, err := cfg.WouldChange(ConfigInput{ManagementURL: spelling}) + require.NoError(t, err) + require.False(t, changed, "%q is the stored endpoint written differently", spelling) + } + + changed, err := cfg.WouldChange(ConfigInput{ManagementURL: "https://mgmt.example.com:8443"}) + require.NoError(t, err) + require.True(t, changed, "a different port is a different endpoint") +} diff --git a/client/server/mdm.go b/client/server/mdm.go index 8ef1ab5e1..982e9e20f 100644 --- a/client/server/mdm.go +++ b/client/server/mdm.go @@ -3,13 +3,13 @@ package server import ( "context" "fmt" - "net/url" "time" log "github.com/sirupsen/logrus" "google.golang.org/grpc/codes" gstatus "google.golang.org/grpc/status" + "github.com/netbirdio/netbird/client/internal/profilemanager" "github.com/netbirdio/netbird/client/mdm" "github.com/netbirdio/netbird/client/proto" ) @@ -185,24 +185,11 @@ func conflictBool(key string, p *bool) conflictCheck { } } -func canonicalURL(s string) string { - u, err := url.ParseRequestURI(s) - if err != nil { - return s - } - if u.Port() == "" { - switch u.Scheme { - case "https": - u.Host += ":443" - case "http": - u.Host += ":80" - } - } - return u.String() -} - -// conflictURL is conflictString for URL-typed keys: both sides are -// normalized via canonicalURL before comparison. +// conflictURL is conflictString for URL-typed keys: both sides are compared as +// endpoints (profilemanager.SameServiceURL), so an implicit default port, a +// trailing slash or a different host case is not read as a divergence from the +// policy. A value that does not parse as a URL falls back to string equality, +// which is the strictest thing left to do with it. func conflictURL(key, got string) conflictCheck { return conflictCheck{ key: key, @@ -211,7 +198,15 @@ func conflictURL(key, got string) conflictCheck { return true } want, ok := pol.GetString(key) - return ok && canonicalURL(want) == canonicalURL(got) + if !ok { + return false + } + wantURL, wantErr := profilemanager.ParseServiceURL(key, want) + gotURL, gotErr := profilemanager.ParseServiceURL(key, got) + if wantErr != nil || gotErr != nil { + return want == got + } + return profilemanager.SameServiceURL(wantURL, gotURL) }, } } diff --git a/client/server/ssh_gate.go b/client/server/ssh_gate.go index 01d24687e..40d66b7a5 100644 --- a/client/server/ssh_gate.go +++ b/client/server/ssh_gate.go @@ -331,21 +331,5 @@ func sameManagementURL(stored *url.URL, requested string) bool { return false } - return stored.Scheme == parsed.Scheme && - stored.Hostname() == parsed.Hostname() && - effectivePort(stored) == effectivePort(parsed) -} - -func effectivePort(u *url.URL) string { - if port := u.Port(); port != "" { - return port - } - switch u.Scheme { - case "https": - return "443" - case "http": - return "80" - default: - return "" - } + return profilemanager.SameServiceURL(stored, parsed) } diff --git a/client/server/update_settings_gate_test.go b/client/server/update_settings_gate_test.go index b9ca4623a..6c98c5e33 100644 --- a/client/server/update_settings_gate_test.go +++ b/client/server/update_settings_gate_test.go @@ -244,3 +244,27 @@ func TestSetConfig_RefusedRequestLeavesTheConfigFileUntouched(t *testing.T) { require.NoError(t, err) require.Equal(t, string(before), string(after), "the refused request rewrote the profile config") } + +// The container case that the string comparison still broke: the management URL +// supplied through the environment is the stored one, written with a trailing +// slash. +func TestSetConfig_ManagementURLSpellingsPassTheGate(t *testing.T) { + for _, spelling := range []string{ + "https://api.netbird.io", + "https://api.netbird.io/", + "https://api.netbird.io:443/", + "https://API.netbird.io:443", + } { + t.Run(spelling, func(t *testing.T) { + s, ctx, profName, username, _ := setupServerWithProfile(t) + s.updateSettingsDisabled = true + + _, err := s.SetConfig(ctx, &proto.SetConfigRequest{ + ProfileName: profName, + Username: username, + ManagementUrl: spelling, + }) + require.NoError(t, err, "%q is the stored management URL written differently", spelling) + }) + } +}