From d2e62e358a07333462fa1e60587ef2964af85b1f Mon Sep 17 00:00:00 2001 From: Riccardo Manfrin <3090891+riccardomanfrin@users.noreply.github.com> Date: Tue, 8 Sep 2026 16:32:16 +0200 Subject: [PATCH] [client] Compare MDM-managed URLs as endpoints, not as strings (#7472) A policy that enforces a management URL refuses any SetConfig or Login whose URL differs from it. The comparison normalized only the default port, so three ways of writing the very endpoint the policy names were reported as conflicts: policy https://mgmt.example.com vs https://mgmt.example.com/ refused https://MGMT.example.com refused https://mgmt.example.com:0443 refused For an MDM-managed deployment whose stored or command-line URL is spelled differently from the policy's value, that means every settings update is refused with an MDMManagedFieldsViolation naming a field the caller did not change. `netbird up --management-url https://MGMT.example.com` reproduces it. The rules now live in util.SameServiceURL, and ConflictURL delegates: scheme and host compared case-insensitively, the effective port normalized numerically, a trailing slash ignored, and a path otherwise still part of the identity so /other remains a divergence. Unparseable input falls back to string equality. util rather than either caller, because comparing two service URLs is neither device management nor profile storage, and more than one place does it: an MDM-enforced management URL against a requested one here, a stored profile URL against a command-line one in profilemanager and the SSH gate. Every copy of these rules that drifts turns an equivalent URL into a refused request, which is how this one arose. CanonicalURL is left alone: besides comparison it is the canonical value handed to mdm.Restrictions and to the Android and iOS Preferences getters, and normalizing what those return is a separate decision. --- client/mdm/conflicts.go | 13 +++++-- client/mdm/conflicts_test.go | 40 ++++++++++++++++++++ util/serviceurl.go | 69 ++++++++++++++++++++++++++++++++++ util/serviceurl_test.go | 73 ++++++++++++++++++++++++++++++++++++ 4 files changed, 191 insertions(+), 4 deletions(-) create mode 100644 client/mdm/conflicts_test.go create mode 100644 util/serviceurl.go create mode 100644 util/serviceurl_test.go diff --git a/client/mdm/conflicts.go b/client/mdm/conflicts.go index a04cfb05c..160212afb 100644 --- a/client/mdm/conflicts.go +++ b/client/mdm/conflicts.go @@ -1,6 +1,10 @@ package mdm -import "net/url" +import ( + "net/url" + + "github.com/netbirdio/netbird/util" +) // PreSharedKeyRedactedSentinel is the redaction mask returned in place of a // real pre-shared key; an incoming value equal to it is a round-trip echo, @@ -44,8 +48,9 @@ func ConflictStringPtr(key string, p *string) ConflictCheck { } } -// ConflictURL builds a ConflictCheck for a URL-typed MDM key; both sides are -// normalized via CanonicalURL before comparison. +// ConflictURL builds a ConflictCheck for a URL-typed MDM key. The two sides are +// compared as the endpoints they address, not as strings: see +// util.SameServiceURL. func ConflictURL(key, got string) ConflictCheck { return ConflictCheck{ Key: key, @@ -54,7 +59,7 @@ func ConflictURL(key, got string) ConflictCheck { return true } want, ok := pol.GetString(key) - return ok && CanonicalURL(want) == CanonicalURL(got) + return ok && util.SameServiceURLStrings(want, got) }, } } diff --git a/client/mdm/conflicts_test.go b/client/mdm/conflicts_test.go new file mode 100644 index 000000000..d145ec103 --- /dev/null +++ b/client/mdm/conflicts_test.go @@ -0,0 +1,40 @@ +package mdm + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The same spellings, through the conflict check that decides whether a request +// is refused. An enforced URL restated in another spelling addresses the very +// server the policy names, so it must not be reported as a conflict. +func TestConflictURLComparesEndpoints(t *testing.T) { + policy := NewPolicy(map[string]any{KeyManagementURL: "https://mgmt.example.com"}) + require.True(t, policy.HasKey(KeyManagementURL)) + + for _, restated := range []string{ + "https://mgmt.example.com", + "https://mgmt.example.com:443", + "https://mgmt.example.com/", + "https://MGMT.example.com", + "https://mgmt.example.com:0443", + } { + conflicts := ResolveConflicts(policy, []ConflictCheck{ConflictURL(KeyManagementURL, restated)}) + assert.Empty(t, conflicts, "%q is the enforced endpoint written differently", restated) + } + + for _, diverging := range []string{ + "https://other.example.com", + "http://mgmt.example.com", + "https://mgmt.example.com:8443", + "https://mgmt.example.com/other", + } { + conflicts := ResolveConflicts(policy, []ConflictCheck{ConflictURL(KeyManagementURL, diverging)}) + assert.Equal(t, []string{KeyManagementURL}, conflicts, "%q addresses another endpoint", diverging) + } + + // An unset field is not a request to change anything. + assert.Empty(t, ResolveConflicts(policy, []ConflictCheck{ConflictURL(KeyManagementURL, "")})) +} diff --git a/util/serviceurl.go b/util/serviceurl.go new file mode 100644 index 000000000..ffd287df1 --- /dev/null +++ b/util/serviceurl.go @@ -0,0 +1,69 @@ +package util + +import ( + "net/url" + "strconv" + "strings" +) + +// SameServiceURL reports whether two service URLs address the same endpoint. +// One endpoint can be written several ways, and every spelling below reaches +// the same server, so none of them is a divergence from another: +// +// an implicit default port https://mgmt.example.com :443 +// a zero-padded port https://mgmt.example.com:0443 +// a different host case https://MGMT.example.com +// a trailing slash https://mgmt.example.com/ +// +// A path is otherwise part of the identity: https://mgmt.example.com and +// https://mgmt.example.com/other are two endpoints. +// +// It lives here rather than next to any one caller because several of them +// compare the same kind of URL — an MDM-enforced management URL against a +// requested one, a stored profile URL against a command-line one — and every +// copy of these rules that drifts turns an equivalent URL into a refused +// request. +func SameServiceURL(a, b *url.URL) bool { + if a == nil || b == nil { + return a == b + } + + return strings.EqualFold(a.Hostname(), b.Hostname()) && + strings.EqualFold(a.Scheme, b.Scheme) && + ServiceURLPort(a) == ServiceURLPort(b) && + strings.TrimSuffix(a.Path, "/") == strings.TrimSuffix(b.Path, "/") +} + +// SameServiceURLStrings is SameServiceURL for unparsed input. Input that does +// not parse falls back to string equality, which is the strictest thing left +// to do with it. +func SameServiceURLStrings(a, b string) bool { + ua, errA := url.ParseRequestURI(a) + ub, errB := url.ParseRequestURI(b) + if errA != nil || errB != nil { + return a == b + } + + return SameServiceURL(ua, ub) +} + +// ServiceURLPort is the port a URL addresses: the one it carries, normalized +// numerically so ":0443" and ":443" are one port, or the scheme's default. +func ServiceURLPort(u *url.URL) string { + port := u.Port() + if port == "" { + switch strings.ToLower(u.Scheme) { + case "https": + return "443" + case "http": + return "80" + default: + return "" + } + } + + if n, err := strconv.Atoi(port); err == nil { + return strconv.Itoa(n) + } + return port +} diff --git a/util/serviceurl_test.go b/util/serviceurl_test.go new file mode 100644 index 000000000..af32a7c29 --- /dev/null +++ b/util/serviceurl_test.go @@ -0,0 +1,73 @@ +package util + +import ( + "net/url" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSameServiceURLSpellings(t *testing.T) { + tests := []struct { + a, b string + want bool + }{ + // One endpoint, written several ways. + {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: "https://mgmt.example.com:0443", b: "https://mgmt.example.com:443", want: true}, + {a: "http://mgmt.example.com", b: "http://mgmt.example.com:80", want: true}, + {a: "HTTPS://mgmt.example.com", b: "https://mgmt.example.com", want: true}, + + // Different endpoints. + {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}, + {a: "https://mgmt.example.com", b: "https://mgmt.example.com/other", want: false}, + + // Unparseable input falls back to string equality. + {a: "mgmt.example.com", b: "mgmt.example.com", want: true}, + {a: "mgmt.example.com", b: "https://mgmt.example.com", want: false}, + } + + for _, tt := range tests { + t.Run(tt.a+" vs "+tt.b, func(t *testing.T) { + assert.Equal(t, tt.want, SameServiceURLStrings(tt.a, tt.b)) + assert.Equal(t, tt.want, SameServiceURLStrings(tt.b, tt.a), "the comparison must be symmetric") + }) + } +} + +// The parsed form is the primitive the string form delegates to, so it must +// answer the same for a spelling that only the parser can tell apart. +func TestSameServiceURLParsed(t *testing.T) { + parse := func(raw string) *url.URL { + t.Helper() + u, err := url.ParseRequestURI(raw) + require.NoError(t, err) + return u + } + + assert.True(t, SameServiceURL(parse("https://mgmt.example.com:0443/"), parse("https://MGMT.example.com"))) + assert.False(t, SameServiceURL(parse("https://mgmt.example.com"), parse("https://mgmt.example.com:8443"))) + + assert.True(t, SameServiceURL(nil, nil), "two absent URLs are the same absence") + assert.False(t, SameServiceURL(nil, parse("https://mgmt.example.com"))) +} + +func TestServiceURLPort(t *testing.T) { + parse := func(raw string) *url.URL { + t.Helper() + u, err := url.ParseRequestURI(raw) + require.NoError(t, err) + return u + } + + assert.Equal(t, "443", ServiceURLPort(parse("https://mgmt.example.com"))) + assert.Equal(t, "80", ServiceURLPort(parse("http://mgmt.example.com"))) + assert.Equal(t, "443", ServiceURLPort(parse("https://mgmt.example.com:0443"))) + assert.Equal(t, "8443", ServiceURLPort(parse("https://mgmt.example.com:8443"))) +}