From b3ee6c9af2b114683d3582a355bfb2ea039acddf Mon Sep 17 00:00:00 2001 From: riccardom Date: Fri, 25 Sep 2026 13:34:17 +0200 Subject: [PATCH] [client] Move the MDM precedence into ResolveUploadURL Three of the four bundle paths ordered the destinations themselves, each with its own copy of "MDM first, then what was asked for". Give the resolver the MDM value as its first argument so the order lives in one place and a call site cannot express a different one. Pure refactor: the daemon passes "" for now, matching what it does today, and the remote job and both mobile SDKs resolve exactly as before. --- client/android/client.go | 2 +- client/internal/debug/destination.go | 38 ++++++++++++++++------- client/internal/debug/destination_test.go | 19 +++++++++++- client/internal/engine.go | 13 +++----- client/ios/NetBirdSDK/client.go | 2 +- client/server/debug.go | 2 +- 6 files changed, 52 insertions(+), 24 deletions(-) diff --git a/client/android/client.go b/client/android/client.go index 5619fbe44..e75bfc227 100644 --- a/client/android/client.go +++ b/client/android/client.go @@ -382,7 +382,7 @@ func (c *Client) DebugBundle(platformFiles PlatformFiles, anonymize bool, anonym // An MDM override wins; otherwise the destination this deployment publishes // is used, and failing that the service NetBird runs. - uploadURL := debug.ResolveUploadURL(cfg.DebugBundleUploadURL, publishedUploadURL) + uploadURL := debug.ResolveUploadURL(cfg.DebugBundleUploadURL, "", publishedUploadURL) path, err := bundleGenerator.Generate() if err != nil { diff --git a/client/internal/debug/destination.go b/client/internal/debug/destination.go index c503e2c4e..7d1f3962c 100644 --- a/client/internal/debug/destination.go +++ b/client/internal/debug/destination.go @@ -4,19 +4,33 @@ import ( "github.com/netbirdio/netbird/upload-server/types" ) -// ResolveUploadURL decides where a debug bundle is uploaded. +// ResolveUploadURL decides where a debug bundle is uploaded, taking the first +// destination that is set: // -// requested is a destination a caller named explicitly — an MDM override, the -// CLI's --upload-bundle-url, a remote job's upload_url; it always wins, and the -// callers that accept one gate it separately (see requirePrivilegeForUploadURL: -// any host other than the default needs a privileged caller). published is what -// the management server of this deployment advertises, which the engine holds -// (Engine.DebugUploadURL). With neither, the upload service NetBird runs is the -// default, for a self-hosted deployment as much as for a cloud one: an operator -// who needs the bundles to stay inside their own infrastructure points either -// knob at their own upload service, and until they do the everyday -// "collect a bundle and send it to support" flow keeps working. -func ResolveUploadURL(requested, published string) string { +// - mdm is the debugBundleUploadURL policy on this device. It outranks +// everything, including a URL the caller named: pinning the destination on a +// managed device is pointless if the person at the keyboard can send the +// bundle elsewhere. +// - requested is what this particular bundle asked for — the CLI's +// --upload-bundle-url, or a remote job's upload_url. The callers that accept +// one gate it separately (see requirePrivilegeForUploadURL: any host other +// than the default needs a privileged caller). +// - published is what the management server of this deployment advertises, +// which the engine holds (Engine.DebugUploadURL). +// +// With none of them, the upload service NetBird runs is the default, for a +// self-hosted deployment as much as for a cloud one: an operator who needs the +// bundles to stay inside their own infrastructure points one of the knobs at +// their own upload service, and until they do the everyday "collect a bundle and +// send it to support" flow keeps working. +// +// Every caller goes through here rather than ordering the sources itself, so a +// path cannot quietly skip one of them. +func ResolveUploadURL(mdm, requested, published string) string { + if mdm != "" { + return mdm + } + if requested != "" { return requested } diff --git a/client/internal/debug/destination_test.go b/client/internal/debug/destination_test.go index cd2d27d0f..f4a153ad0 100644 --- a/client/internal/debug/destination_test.go +++ b/client/internal/debug/destination_test.go @@ -14,12 +14,29 @@ func TestResolveUploadURL(t *testing.T) { requestedURL = "https://requested.example.com/upload-url" ) + const mdmURL = "https://mdm.example.com/upload-url" + tests := []struct { name string + mdm string requested string published string want string }{ + { + // Pinning the destination on a managed device is pointless if the + // person at the keyboard can name another one. + name: "MDM outranks a URL the caller named", + mdm: mdmURL, + requested: requestedURL, + published: operatorURL, + want: mdmURL, + }, + { + name: "MDM alone wins", + mdm: mdmURL, + want: mdmURL, + }, { name: "requested wins over published", requested: requestedURL, @@ -47,7 +64,7 @@ func TestResolveUploadURL(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - assert.Equal(t, tc.want, ResolveUploadURL(tc.requested, tc.published)) + assert.Equal(t, tc.want, ResolveUploadURL(tc.mdm, tc.requested, tc.published)) }) } } diff --git a/client/internal/engine.go b/client/internal/engine.go index c33442b8d..bb2c4b319 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -1479,16 +1479,13 @@ func (e *Engine) handleBundle(params *mgmProto.BundleParameters) (*mgmProto.JobR log.Warnf("get latest sync response: %v", err) } - // Resolve the upload destination: an MDM override, when set, takes - // precedence over the job's URL. Both are validated the same way. With - // neither, the destination this deployment publishes is used, and failing - // that the service NetBird runs. - uploadURL := params.GetUploadUrl() - if override := e.config.ProfileConfig.DebugBundleUploadURL; override != "" { + // Resolve the upload destination: the MDM policy, then the job's URL, then + // what this deployment publishes, then the service NetBird runs. + mdmUploadURL := e.config.ProfileConfig.DebugBundleUploadURL + if mdmUploadURL != "" && params.GetUploadUrl() != "" && mdmUploadURL != params.GetUploadUrl() { log.Infof("using MDM debug bundle upload URL override instead of the management-supplied value") - uploadURL = override } - uploadURL = debug.ResolveUploadURL(uploadURL, e.DebugUploadURL()) + uploadURL := debug.ResolveUploadURL(mdmUploadURL, params.GetUploadUrl(), e.DebugUploadURL()) // Validated after resolution, so the destination this deployment published // meets the same rule as one named in the job. Management validates it at diff --git a/client/ios/NetBirdSDK/client.go b/client/ios/NetBirdSDK/client.go index dedd74ab0..f9f04e23a 100644 --- a/client/ios/NetBirdSDK/client.go +++ b/client/ios/NetBirdSDK/client.go @@ -331,7 +331,7 @@ func (c *Client) DebugBundle(anonymize bool, anonymizeLevel string) (string, err // An MDM override wins; otherwise the destination this deployment publishes // is used, and failing that the service NetBird runs. - uploadURL := debug.ResolveUploadURL(cfg.DebugBundleUploadURL, publishedUploadURL) + uploadURL := debug.ResolveUploadURL(cfg.DebugBundleUploadURL, "", publishedUploadURL) path, err := bundleGenerator.Generate() if err != nil { diff --git a/client/server/debug.go b/client/server/debug.go index d1315d3d3..f9b6c68f8 100644 --- a/client/server/debug.go +++ b/client/server/debug.go @@ -49,7 +49,7 @@ func (s *Server) DebugBundle(callerCtx context.Context, req *proto.DebugBundleRe // it is the operator of this deployment naming their own upload service, and // the peer already trusts that server for its whole configuration. Only a // URL the local caller named goes through requirePrivilegeForUploadURL above. - uploadURL := debug.ResolveUploadURL(req.GetUploadURL(), publishedUploadURL) + uploadURL := debug.ResolveUploadURL("", req.GetUploadURL(), publishedUploadURL) // The upload runs without s.mutex held: it does network I/O to a possibly // slow destination and must not block the other RPCs that take the lock. The