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