diff --git a/client/android/client.go b/client/android/client.go index 2e3300453..5619fbe44 100644 --- a/client/android/client.go +++ b/client/android/client.go @@ -349,8 +349,8 @@ func (c *Client) DebugBundle(platformFiles PlatformFiles, anonymize bool, anonym } // Empty unless an engine is running and has synced: a bundle generated with - // the client stopped has no management-published destination, so it uploads - // only when the peer is enrolled with NetBird's cloud. + // the client stopped has no management-published destination and goes to the + // service NetBird runs. var publishedUploadURL string if cc != nil { @@ -380,15 +380,9 @@ func (c *Client) DebugBundle(platformFiles PlatformFiles, anonymize bool, anonym }, ) - // Resolved before the bundle is generated: with no destination there is - // nothing to hand back to the app, and generating (then deleting) a bundle - // nobody can collect is wasted work on the device. An MDM override wins; - // otherwise the destination this deployment publishes is used, and only a - // peer enrolled with NetBird's cloud falls back to the service NetBird runs. - uploadURL, err := debug.ResolveUploadURL(cfg.DebugBundleUploadURL, publishedUploadURL, cfg.ManagementURL.String()) - if err != nil { - return "", 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) path, err := bundleGenerator.Generate() if err != nil { diff --git a/client/internal/debug/destination.go b/client/internal/debug/destination.go index 1f53816d7..c503e2c4e 100644 --- a/client/internal/debug/destination.go +++ b/client/internal/debug/destination.go @@ -1,41 +1,29 @@ package debug import ( - "errors" - - "github.com/netbirdio/netbird/client/internal/metrics" "github.com/netbirdio/netbird/upload-server/types" ) -// ErrNoUploadDestination reports that a bundle has nowhere to go: the -// management server of this deployment publishes no upload service, and the -// peer is not enrolled with NetBird's cloud either. A debug bundle carries the -// peer's logs, routes, DNS and firewall state, so the default is to keep it -// inside the operator's control sphere rather than fall back to the service -// NetBird runs. -var ErrNoUploadDestination = errors.New("this deployment publishes no debug bundle upload service; set it on the account settings or in the management server config, or pass an explicit upload URL") - -// ResolveUploadURL decides where a debug bundle may be uploaded. +// ResolveUploadURL decides where a debug bundle is uploaded. // -// requested is a destination a caller named explicitly (a CLI flag, the daemon -// request); it always wins, and the callers that accept one gate it separately. -// published is what the management server of this deployment advertises, which -// the engine holds (Engine.DebugUploadURL). With neither, only a peer enrolled -// with NetBird's cloud falls back to the service NetBird runs — for anyone else -// that would carry the bundle out of the deployment the operator controls, so it -// fails closed with ErrNoUploadDestination. -func ResolveUploadURL(requested, published, managementURL string) (string, error) { +// 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 { if requested != "" { - return requested, nil + return requested } if published != "" { - return published, nil + return published } - if metrics.DetermineDeploymentType(managementURL) == metrics.DeploymentTypeCloud { - return types.DefaultBundleURL, nil - } - - return "", ErrNoUploadDestination + return types.DefaultBundleURL } diff --git a/client/internal/debug/destination_test.go b/client/internal/debug/destination_test.go index de88a3cab..cd2d27d0f 100644 --- a/client/internal/debug/destination_test.go +++ b/client/internal/debug/destination_test.go @@ -4,83 +4,50 @@ import ( "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" "github.com/netbirdio/netbird/upload-server/types" ) func TestResolveUploadURL(t *testing.T) { const ( - cloudMgm = "https://api.netbird.io:443" - selfHostedMgm = "https://netbird.example.com:33073" - operatorURL = "https://upload.example.com/upload-url" - requestedURL = "https://requested.example.com/upload-url" + operatorURL = "https://upload.example.com/upload-url" + requestedURL = "https://requested.example.com/upload-url" ) tests := []struct { - name string - requested string - published string - managementURL string - want string - wantErr bool + name string + requested string + published string + want string }{ { - name: "requested wins over published", - requested: requestedURL, - published: operatorURL, - managementURL: selfHostedMgm, - want: requestedURL, + name: "requested wins over published", + requested: requestedURL, + published: operatorURL, + want: requestedURL, }, { - name: "requested wins on cloud too", - requested: requestedURL, - managementURL: cloudMgm, - want: requestedURL, + name: "requested wins with nothing published", + requested: requestedURL, + want: requestedURL, }, { - name: "published used when nothing requested", - published: operatorURL, - managementURL: selfHostedMgm, - want: operatorURL, + name: "published used when nothing requested", + published: operatorURL, + want: operatorURL, }, { - // A cloud deployment publishing its own destination must not be - // overridden by the compiled-in default. - name: "published wins over the cloud fallback", - published: operatorURL, - managementURL: cloudMgm, - want: operatorURL, - }, - { - name: "cloud falls back to the NetBird service", - managementURL: cloudMgm, - want: types.DefaultBundleURL, - }, - { - // The whole point of GHSA-hf99-43rj-h577: no silent hop to a - // vendor-controlled destination. - name: "self-hosted with no destination fails closed", - managementURL: selfHostedMgm, - wantErr: true, - }, - { - name: "unknown management URL fails closed", - managementURL: "", - wantErr: true, + // The default stays the service NetBird runs whatever the + // deployment: an operator who wants the bundles elsewhere says so, + // and until then collecting one and sending it to support works. + name: "nothing configured falls back to the NetBird service", + want: types.DefaultBundleURL, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - got, err := ResolveUploadURL(tc.requested, tc.published, tc.managementURL) - if tc.wantErr { - require.ErrorIs(t, err, ErrNoUploadDestination) - assert.Empty(t, got) - return - } - require.NoError(t, err) - assert.Equal(t, tc.want, got) + assert.Equal(t, tc.want, ResolveUploadURL(tc.requested, tc.published)) }) } } diff --git a/client/internal/engine.go b/client/internal/engine.go index 238715f37..7f3042b7c 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -230,9 +230,8 @@ type Engine struct { // debugUploadURL is the debug-bundle upload service the management server // publishes for this deployment, refreshed on every NetbirdConfig update. // Atomic because the bundle paths (remote job, daemon RPC, mobile SDK) read - // it off the engine loop. Empty when the deployment publishes none, which is - // what makes a self-hosted peer keep its bundle local instead of shipping it - // to the upload service NetBird runs. + // it off the engine loop. Empty when the deployment publishes none, in which + // case the callers fall back to the service NetBird runs. debugUploadURL atomic.Pointer[string] clientCtx context.Context @@ -1243,8 +1242,8 @@ func (e *Engine) handleDebugUploadUpdate(config *mgmProto.DebugConfig) { // DebugUploadURL returns the debug-bundle upload service the management server // published, or empty when it published none or the engine never synced. The -// callers treat empty as "no destination from this deployment" and fail closed -// unless the peer is enrolled with NetBird's cloud; see debug.ResolveUploadURL. +// callers treat empty as "this deployment names no destination" and fall back to +// the service NetBird runs; see debug.ResolveUploadURL. func (e *Engine) DebugUploadURL() string { if url := e.debugUploadURL.Load(); url != nil { return *url @@ -1466,10 +1465,8 @@ func (e *Engine) handleBundle(params *mgmProto.BundleParameters) (*mgmProto.JobR // 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 only a - // peer enrolled with NetBird's cloud falls back to the service NetBird runs - // — a self-hosted deployment that named no upload service gets no upload - // rather than one that leaves the operator's control sphere. + // 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 != "" { log.Infof("using MDM debug bundle upload URL override instead of the management-supplied value") @@ -1479,10 +1476,7 @@ func (e *Engine) handleBundle(params *mgmProto.BundleParameters) (*mgmProto.JobR return nil, err } - uploadURL, err = debug.ResolveUploadURL(uploadURL, e.DebugUploadURL(), e.config.ProfileConfig.ManagementURL.String()) - if err != nil { - return nil, err - } + uploadURL = debug.ResolveUploadURL(uploadURL, e.DebugUploadURL()) bundleDeps := debug.GeneratorDependencies{ InternalConfig: e.config.ProfileConfig, diff --git a/client/ios/NetBirdSDK/client.go b/client/ios/NetBirdSDK/client.go index 36d494f36..dedd74ab0 100644 --- a/client/ios/NetBirdSDK/client.go +++ b/client/ios/NetBirdSDK/client.go @@ -298,8 +298,8 @@ func (c *Client) DebugBundle(anonymize bool, anonymizeLevel string) (string, err } // Empty unless an engine is running and has synced: a bundle generated with - // the client stopped has no management-published destination, so it uploads - // only when the peer is enrolled with NetBird's cloud. + // the client stopped has no management-published destination and goes to the + // service NetBird runs. var publishedUploadURL string if cc != nil { @@ -329,15 +329,9 @@ func (c *Client) DebugBundle(anonymize bool, anonymizeLevel string) (string, err }, ) - // Resolved before the bundle is generated: with no destination there is - // nothing to hand back to the app, and generating (then deleting) a bundle - // nobody can collect is wasted work on the device. An MDM override wins; - // otherwise the destination this deployment publishes is used, and only a - // peer enrolled with NetBird's cloud falls back to the service NetBird runs. - uploadURL, err := debug.ResolveUploadURL(cfg.DebugBundleUploadURL, publishedUploadURL, cfg.ManagementURL.String()) - if err != nil { - return "", 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) path, err := bundleGenerator.Generate() if err != nil { diff --git a/client/jobexec/executor.go b/client/jobexec/executor.go index fbbfdb6b3..672297c74 100644 --- a/client/jobexec/executor.go +++ b/client/jobexec/executor.go @@ -29,12 +29,11 @@ func NewExecutor() *Executor { // BundleJob generates a debug bundle for a remote job and uploads it to // uploadURL, returning the key the management server hands back to whoever asked. -// The caller resolves uploadURL (see debug.ResolveUploadURL): a job whose -// deployment names no upload service never reaches here, so there is no -// fallback destination to pick locally. +// The caller resolves uploadURL (see debug.ResolveUploadURL), which never yields +// an empty one, so there is no fallback destination to pick locally. func (e *Executor) BundleJob(ctx context.Context, debugBundleDependencies debug.GeneratorDependencies, params debug.BundleConfig, waitForDuration time.Duration, mgmURL, uploadURL string) (string, error) { if uploadURL == "" { - return "", debug.ErrNoUploadDestination + return "", errors.New("no debug bundle upload destination resolved") } if waitForDuration > MaxBundleWaitTime { diff --git a/client/proto/daemon.pb.go b/client/proto/daemon.pb.go index 7b722d33a..d3efda6e1 100644 --- a/client/proto/daemon.pb.go +++ b/client/proto/daemon.pb.go @@ -2822,10 +2822,8 @@ type DebugBundleRequest struct { AnonymizeLevel string `protobuf:"bytes,8,opt,name=anonymizeLevel,proto3" json:"anonymizeLevel,omitempty"` // upload asks the daemon to upload the bundle. When uploadURL is empty the // daemon resolves the destination itself: the one the management server - // publishes, or the service NetBird runs when the peer is enrolled with - // NetBird's cloud. A self-hosted deployment that publishes no destination - // gets no upload, so a bundle never leaves the operator's control sphere by - // default. uploadURL still overrides it, for a privileged caller. + // publishes, or else the service NetBird runs. uploadURL still overrides it, + // and a host other than the default one requires a privileged caller. Upload bool `protobuf:"varint,9,opt,name=upload,proto3" json:"upload,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache diff --git a/client/proto/daemon.proto b/client/proto/daemon.proto index 745351aab..5b197213a 100644 --- a/client/proto/daemon.proto +++ b/client/proto/daemon.proto @@ -555,10 +555,8 @@ message DebugBundleRequest { string anonymizeLevel = 8; // upload asks the daemon to upload the bundle. When uploadURL is empty the // daemon resolves the destination itself: the one the management server - // publishes, or the service NetBird runs when the peer is enrolled with - // NetBird's cloud. A self-hosted deployment that publishes no destination - // gets no upload, so a bundle never leaves the operator's control sphere by - // default. uploadURL still overrides it, for a privileged caller. + // publishes, or else the service NetBird runs. uploadURL still overrides it, + // and a host other than the default one requires a privileged caller. bool upload = 9; } diff --git a/client/server/debug.go b/client/server/debug.go index 974fc186e..d7d1f388d 100644 --- a/client/server/debug.go +++ b/client/server/debug.go @@ -48,11 +48,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, err := debug.ResolveUploadURL(req.GetUploadURL(), publishedUploadURL, managementURL) - if err != nil { - log.Errorf("cannot upload debug bundle: %v", err) - return &proto.DebugBundleResponse{Path: path, UploadFailureReason: err.Error()}, nil - } + 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 diff --git a/management/internals/server/config/config.go b/management/internals/server/config/config.go index fdf4d88e7..50028660a 100644 --- a/management/internals/server/config/config.go +++ b/management/internals/server/config/config.go @@ -218,12 +218,11 @@ type AgentNetwork struct { // // The client paths that upload without a human picking a destination — the // remote debug-bundle job, the mobile clients and the desktop UI — take the -// destination from here. It exists so a self-hosted deployment keeps its -// bundles, which carry peer logs, routes, DNS and firewall state, inside the -// operator's own control sphere instead of reaching the upload service NetBird -// runs. Leaving it unset publishes no destination: a peer enrolled with -// NetBird's cloud still uses NetBird's service, a self-hosted peer keeps the -// bundle local. +// destination from here. It exists so an operator who needs the bundles, which +// carry peer logs, routes, DNS and firewall state, to stay inside their own +// infrastructure can say so once. Leaving it unset publishes no destination and +// the peers upload to the service NetBird runs, which keeps the everyday +// "collect a bundle and send it to support" flow working out of the box. // // Set URL to the upload service's get-URL endpoint, e.g. // https://upload.example.com/upload-url (see the upload-server component). diff --git a/management/internals/shared/grpc/conversion.go b/management/internals/shared/grpc/conversion.go index 4edff8e6e..7067fa787 100644 --- a/management/internals/shared/grpc/conversion.go +++ b/management/internals/shared/grpc/conversion.go @@ -115,9 +115,10 @@ func toNetbirdConfig(config *nbconfig.Config, turnCredentials *Token, relayToken } } - // The account setting wins, the server config is the deployment-wide default - // a self-hosted install can set once so a fresh account is not left with the - // vendor fallback. Both are https-validated where they are written. + // The account setting wins, the server config is the deployment-wide value a + // self-hosted install can set once for every account. Both are + // https-validated where they are written. Neither set publishes nothing, and + // the peers fall back to the service NetBird runs. debugUploadURL := config.DebugUpload.URL if settings != nil && settings.DebugBundleUploadURL != "" { debugUploadURL = settings.DebugBundleUploadURL diff --git a/management/server/types/settings.go b/management/server/types/settings.go index b88b3d0ec..1286cacdc 100644 --- a/management/server/types/settings.go +++ b/management/server/types/settings.go @@ -79,10 +79,9 @@ type Settings struct { // DebugBundleUploadURL is the debug-bundle upload service the peers of this // account send their bundles to. A bundle carries peer logs, routes, DNS and // firewall state, so the destination decides whose infrastructure that data - // lands on; setting it keeps a self-hosted account's bundles inside its own - // control sphere. Empty falls back to the deployment-wide DebugUpload.URL - // from the management server config, and with neither only a peer enrolled - // with NetBird's cloud uploads at all. Must be an https URL with a host. + // lands on. Empty falls back to the deployment-wide DebugUpload.URL from the + // management server config, and with neither to the service NetBird runs. + // Must be an https URL with a host. DebugBundleUploadURL string // AgentNetworkOnly limits the dashboard to the Agent Network surface for this account. diff --git a/shared/management/http/api/openapi.yml b/shared/management/http/api/openapi.yml index ff41f5c11..3987e92fd 100644 --- a/shared/management/http/api/openapi.yml +++ b/shared/management/http/api/openapi.yml @@ -386,9 +386,9 @@ components: debug_bundle_upload_url: description: | Upload service the peers of this account send debug bundles to. A bundle carries peer logs, routes, DNS and - firewall state, so setting this keeps that data inside infrastructure the account controls instead of the - upload service NetBird runs. Must be an https URL with a host. Empty falls back to the deployment-wide value - configured on the management server; with neither, only peers enrolled with NetBird's cloud upload at all. + firewall state, so setting this keeps that data inside infrastructure the account controls. Must be an https + URL with a host. Empty falls back to the deployment-wide value configured on the management server, and with + neither to the upload service NetBird runs. type: string example: "https://upload.example.com/upload-url" agent_network_only: diff --git a/shared/management/http/api/types.gen.go b/shared/management/http/api/types.gen.go index e3f3a7533..901793d0f 100644 --- a/shared/management/http/api/types.gen.go +++ b/shared/management/http/api/types.gen.go @@ -1687,9 +1687,9 @@ type AccountSettings struct { DashboardFeatures *AccountDashboardFeatures `json:"dashboard_features,omitempty"` // DebugBundleUploadUrl Upload service the peers of this account send debug bundles to. A bundle carries peer logs, routes, DNS and - // firewall state, so setting this keeps that data inside infrastructure the account controls instead of the - // upload service NetBird runs. Must be an https URL with a host. Empty falls back to the deployment-wide value - // configured on the management server; with neither, only peers enrolled with NetBird's cloud upload at all. + // firewall state, so setting this keeps that data inside infrastructure the account controls. Must be an https + // URL with a host. Empty falls back to the deployment-wide value configured on the management server, and with + // neither to the upload service NetBird runs. DebugBundleUploadUrl *string `json:"debug_bundle_upload_url,omitempty"` // DnsDomain Allows to define a custom dns domain for the account diff --git a/shared/management/proto/management.pb.go b/shared/management/proto/management.pb.go index 3d9f96be9..4c361e976 100644 --- a/shared/management/proto/management.pb.go +++ b/shared/management/proto/management.pb.go @@ -2403,11 +2403,10 @@ type DebugConfig struct { // upload_url is the debug-bundle upload service this peer's account uses, // taken from the account settings or, failing that, from the management - // server config. Publishing it keeps a self-hosted deployment's bundles inside - // its own control sphere instead of falling back to the upload service NetBird - // runs. An empty value means no destination is published: the peer then - // uploads only when it is enrolled with NetBird's cloud, and otherwise keeps - // the bundle local. + // server config. Setting it keeps a deployment's bundles — which carry peer + // logs, routes, DNS and firewall state — inside infrastructure the operator + // controls. An empty value means no destination is published, and the peer + // uploads to the service NetBird runs. UploadUrl string `protobuf:"bytes,1,opt,name=upload_url,json=uploadUrl,proto3" json:"upload_url,omitempty"` } diff --git a/shared/management/proto/management.proto b/shared/management/proto/management.proto index 4b4672392..dc66f736f 100644 --- a/shared/management/proto/management.proto +++ b/shared/management/proto/management.proto @@ -386,11 +386,10 @@ message MetricsConfig { message DebugConfig { // upload_url is the debug-bundle upload service this peer's account uses, // taken from the account settings or, failing that, from the management - // server config. Publishing it keeps a self-hosted deployment's bundles inside - // its own control sphere instead of falling back to the upload service NetBird - // runs. An empty value means no destination is published: the peer then - // uploads only when it is enrolled with NetBird's cloud, and otherwise keeps - // the bundle local. + // server config. Setting it keeps a deployment's bundles — which carry peer + // logs, routes, DNS and firewall state — inside infrastructure the operator + // controls. An empty value means no destination is published, and the peer + // uploads to the service NetBird runs. string upload_url = 1; }