sendnrw/netbird
2
0
Fork 0
mirror of https://github.com/netbirdio/netbird.git synced 2026-09-12 17:59:06 +02:00
Code Issues Packages Projects Releases Wiki Activity

[client] Compare service URLs as endpoints, not as strings

Browse Source 
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.
This commit is contained in:
riccardom
2026-09-02 14:31:38 +02:00
parent 0b969e2124
commit 2a17bf0d55
5 changed files with 145 additions and 44 deletions
Download Patch File Download Diff File Expand all files Collapse all files
client/internal/profilemanager/config.go
+45 -7
View File
@@ -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 {
client/internal/profilemanager/config_would_change_test.go
+60
View File
@@ -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")
}
client/server/mdm.go
+15 -20
View File
@@ -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)
},
}
}
client/server/ssh_gate.go
+1 -17
View File
@@ -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)
}
client/server/update_settings_gate_test.go
+24
View File
@@ -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)
})
}
}