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/client/mdm/policy.go b/client/mdm/policy.go index c57c5303e..638fa0d80 100644 --- a/client/mdm/policy.go +++ b/client/mdm/policy.go @@ -235,6 +235,8 @@ func (p *Policy) GetBool(key string) (bool, bool) { return t != 0, true case int64: return t != 0, true + case float64: + return t != 0, true } return false, false } diff --git a/client/mdm/policy_test.go b/client/mdm/policy_test.go index 177fcd550..ea467f861 100644 --- a/client/mdm/policy_test.go +++ b/client/mdm/policy_test.go @@ -96,7 +96,8 @@ func TestPolicy_GetBool(t *testing.T) { {"int64 nonzero", int64(2), true, true}, {"int64 zero", int64(0), false, true}, {"string garbage", "maybe", false, false}, - {"float unsupported", 1.0, false, false}, + {"float nonzero", 1.0, true, true}, + {"float zero", 0.0, false, true}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { @@ -156,6 +157,20 @@ func TestPolicy_GetStringSlice(t *testing.T) { }) } +// encoding/json decodes every JSON number into float64, so the mobile +// loaders never see int. +func TestJSONLoader_BoolFromNumber(t *testing.T) { + p := NewJSONLoader(func() string { return `{"blockInbound":1,"disableProfiles":0}` }).Load() + + got, ok := p.GetBool(KeyBlockInbound) + assert.True(t, ok) + assert.True(t, got) + + got, ok = p.GetBool(KeyDisableProfiles) + assert.True(t, ok) + assert.False(t, got) +} + func TestLoader_NilFetcherReturnsEmpty(t *testing.T) { // Loader.Load with no fetcher (desktop construction) must degrade // gracefully and never return nil; on linux loadPlatform is a stub 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"))) +}