Merge branch 'main' into ssh-config-tmp-cleanup

client/internal/debug/debug.go

@@ -21,7 +21,6 @@ import (
 	"time"
 
 	log "github.com/sirupsen/logrus"
-	"golang.zx2c4.com/wireguard/wgctrl/wgtypes"
 	"google.golang.org/protobuf/encoding/protojson"
 
 	"github.com/netbirdio/netbird/client/anonymize"
@@ -584,9 +583,6 @@ func isSensitiveEnvVar(key string) bool {
 func (g *BundleGenerator) addCommonConfigFields(configContent *strings.Builder) {
 	configContent.WriteString("NetBird Client Configuration:\n\n")
 
-	if key, err := wgtypes.ParseKey(g.internalConfig.PrivateKey); err == nil {
-		configContent.WriteString(fmt.Sprintf("PublicKey: %s\n", key.PublicKey().String()))
-	}
 	configContent.WriteString(fmt.Sprintf("WgIface: %s\n", g.internalConfig.WgIface))
 	configContent.WriteString(fmt.Sprintf("WgPort: %d\n", g.internalConfig.WgPort))
 	if g.internalConfig.NetworkMonitor != nil {
@@ -611,12 +607,6 @@ func (g *BundleGenerator) addCommonConfigFields(configContent *strings.Builder)
 	if g.internalConfig.EnableSSHRemotePortForwarding != nil {
 		configContent.WriteString(fmt.Sprintf("EnableSSHRemotePortForwarding: %v\n", *g.internalConfig.EnableSSHRemotePortForwarding))
 	}
-	if g.internalConfig.DisableSSHAuth != nil {
-		configContent.WriteString(fmt.Sprintf("DisableSSHAuth: %v\n", *g.internalConfig.DisableSSHAuth))
-	}
-	if g.internalConfig.SSHJWTCacheTTL != nil {
-		configContent.WriteString(fmt.Sprintf("SSHJWTCacheTTL: %d\n", *g.internalConfig.SSHJWTCacheTTL))
-	}
 
 	configContent.WriteString(fmt.Sprintf("DisableClientRoutes: %v\n", g.internalConfig.DisableClientRoutes))
 	configContent.WriteString(fmt.Sprintf("DisableServerRoutes: %v\n", g.internalConfig.DisableServerRoutes))
@@ -643,7 +633,6 @@ func (g *BundleGenerator) addCommonConfigFields(configContent *strings.Builder)
 	}
 
 	configContent.WriteString(fmt.Sprintf("LazyConnectionEnabled: %v\n", g.internalConfig.LazyConnectionEnabled))
-	configContent.WriteString(fmt.Sprintf("MTU: %d\n", g.internalConfig.MTU))
 }
 
 func (g *BundleGenerator) addProf() (err error) {

client/internal/debug/debug_test.go

@@ -5,21 +5,16 @@ import (
 	"bytes"
 	"encoding/json"
 	"net"
-	"net/url"
 	"os"
 	"path/filepath"
-	"reflect"
 	"strings"
 	"testing"
-	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/netbirdio/netbird/client/anonymize"
 	"github.com/netbirdio/netbird/client/configs"
-	"github.com/netbirdio/netbird/client/internal/profilemanager"
-	"github.com/netbirdio/netbird/shared/management/domain"
 	mgmProto "github.com/netbirdio/netbird/shared/management/proto"
 )
 
@@ -476,8 +471,8 @@ func TestSanitizeServiceEnvVars(t *testing.T) {
 			anonymize: false,
 			input: map[string]any{
 				jsonKeyServiceEnv: map[string]any{
-					"HOME":         "/root",
-					"PATH":         "/usr/bin",
+					"HOME":       "/root",
+					"PATH":       "/usr/bin",
 					"NB_LOG_LEVEL": "debug",
 				},
 			},
@@ -494,9 +489,9 @@ func TestSanitizeServiceEnvVars(t *testing.T) {
 			anonymize: false,
 			input: map[string]any{
 				jsonKeyServiceEnv: map[string]any{
-					"NB_SETUP_KEY": "abc123",
-					"NB_API_TOKEN": "tok_xyz",
-					"NB_LOG_LEVEL": "info",
+					"NB_SETUP_KEY":  "abc123",
+					"NB_API_TOKEN":  "tok_xyz",
+					"NB_LOG_LEVEL":  "info",
 				},
 			},
 			check: func(t *testing.T, params map[string]any) {
@@ -771,127 +766,3 @@ Chain OUTPUT (policy ACCEPT 0 packets, 0 bytes)
 	assert.Contains(t, anonNftables, "chain input {")
 	assert.Contains(t, anonNftables, "type filter hook input priority filter; policy accept;")
 }
-
-// TestAddConfig_AllFieldsCovered uses reflection to ensure every field in
-// profilemanager.Config is either rendered in the debug bundle or explicitly
-// excluded. When a new field is added to Config, this test fails until the
-// developer either dumps it in addConfig/addCommonConfigFields or adds it to
-// the excluded set with a justification.
-func TestAddConfig_AllFieldsCovered(t *testing.T) {
-	excluded := map[string]string{
-		"PrivateKey":        "sensitive: WireGuard private key",
-		"PreSharedKey":      "sensitive: WireGuard pre-shared key",
-		"SSHKey":            "sensitive: SSH private key",
-		"ClientCertKeyPair": "non-config: parsed cert pair, not serialized",
-	}
-
-	mURL, _ := url.Parse("https://api.example.com:443")
-	aURL, _ := url.Parse("https://admin.example.com:443")
-	bTrue := true
-	iVal := 42
-	cfg := &profilemanager.Config{
-		PrivateKey:                    "priv",
-		PreSharedKey:                  "psk",
-		ManagementURL:                 mURL,
-		AdminURL:                      aURL,
-		WgIface:                       "wt0",
-		WgPort:                        51820,
-		NetworkMonitor:                &bTrue,
-		IFaceBlackList:                []string{"eth0"},
-		DisableIPv6Discovery:          true,
-		RosenpassEnabled:              true,
-		RosenpassPermissive:           true,
-		ServerSSHAllowed:              &bTrue,
-		EnableSSHRoot:                 &bTrue,
-		EnableSSHSFTP:                 &bTrue,
-		EnableSSHLocalPortForwarding:  &bTrue,
-		EnableSSHRemotePortForwarding: &bTrue,
-		DisableSSHAuth:                &bTrue,
-		SSHJWTCacheTTL:                &iVal,
-		DisableClientRoutes:           true,
-		DisableServerRoutes:           true,
-		DisableDNS:                    true,
-		DisableFirewall:               true,
-		BlockLANAccess:                true,
-		BlockInbound:                  true,
-		DisableNotifications:          &bTrue,
-		DNSLabels:                     domain.List{},
-		SSHKey:                        "sshkey",
-		NATExternalIPs:                []string{"1.2.3.4"},
-		CustomDNSAddress:              "1.1.1.1:53",
-		DisableAutoConnect:            true,
-		DNSRouteInterval:              5 * time.Second,
-		ClientCertPath:                "/tmp/cert",
-		ClientCertKeyPath:             "/tmp/key",
-		LazyConnectionEnabled:         true,
-		MTU:                           1280,
-	}
-
-	for _, anonymize := range []bool{false, true} {
-		t.Run("anonymize="+map[bool]string{true: "true", false: "false"}[anonymize], func(t *testing.T) {
-			g := &BundleGenerator{
-				anonymizer:     newAnonymizerForTest(),
-				internalConfig: cfg,
-				anonymize:      anonymize,
-			}
-
-			var sb strings.Builder
-			g.addCommonConfigFields(&sb)
-			rendered := sb.String() + renderAddConfigSpecific(g)
-
-			val := reflect.ValueOf(cfg).Elem()
-			typ := val.Type()
-			var missing []string
-			for i := 0; i < typ.NumField(); i++ {
-				name := typ.Field(i).Name
-				if _, ok := excluded[name]; ok {
-					continue
-				}
-				if !strings.Contains(rendered, name+":") {
-					missing = append(missing, name)
-				}
-			}
-			if len(missing) > 0 {
-				t.Fatalf("Config field(s) not present in debug bundle output: %v\n"+
-					"Either render the field in addCommonConfigFields/addConfig, "+
-					"or add it to the excluded map with a justification.", missing)
-			}
-		})
-	}
-}
-
-// renderAddConfigSpecific renders the fields handled by the anonymize/non-anonymize
-// branches in addConfig (ManagementURL, AdminURL, NATExternalIPs, CustomDNSAddress).
-// addCommonConfigFields covers the rest. Keeping this in the test mirrors the
-// production shape without needing to write an actual zip.
-func renderAddConfigSpecific(g *BundleGenerator) string {
-	var sb strings.Builder
-	if g.anonymize {
-		if g.internalConfig.ManagementURL != nil {
-			sb.WriteString("ManagementURL: " + g.anonymizer.AnonymizeURI(g.internalConfig.ManagementURL.String()) + "\n")
-		}
-		if g.internalConfig.AdminURL != nil {
-			sb.WriteString("AdminURL: " + g.anonymizer.AnonymizeURI(g.internalConfig.AdminURL.String()) + "\n")
-		}
-		sb.WriteString("NATExternalIPs: x\n")
-		if g.internalConfig.CustomDNSAddress != "" {
-			sb.WriteString("CustomDNSAddress: " + g.anonymizer.AnonymizeString(g.internalConfig.CustomDNSAddress) + "\n")
-		}
-	} else {
-		if g.internalConfig.ManagementURL != nil {
-			sb.WriteString("ManagementURL: " + g.internalConfig.ManagementURL.String() + "\n")
-		}
-		if g.internalConfig.AdminURL != nil {
-			sb.WriteString("AdminURL: " + g.internalConfig.AdminURL.String() + "\n")
-		}
-		sb.WriteString("NATExternalIPs: x\n")
-		if g.internalConfig.CustomDNSAddress != "" {
-			sb.WriteString("CustomDNSAddress: " + g.internalConfig.CustomDNSAddress + "\n")
-		}
-	}
-	return sb.String()
-}
-
-func newAnonymizerForTest() *anonymize.Anonymizer {
-	return anonymize.NewAnonymizer(anonymize.DefaultAddresses())
-}

client/ssh/config/manager.go

@@ -224,18 +224,31 @@ func (m *Manager) buildHostPatterns(peer PeerSSHInfo) []string {
 
 func (m *Manager) writeSSHConfig(sshConfig string) error {
 	sshConfigPath := filepath.Join(m.sshConfigDir, m.sshConfigFile)
-	sshConfigPathTmp := sshConfigPath + ".tmp"
 
 	if err := os.MkdirAll(m.sshConfigDir, 0755); err != nil {
 		return fmt.Errorf("create SSH config directory %s: %w", m.sshConfigDir, err)
 	}
 
-	if err := writeFileWithTimeout(sshConfigPathTmp, []byte(sshConfig), 0644); err != nil {
-		return fmt.Errorf("write SSH config file %s: %w", sshConfigPath, err)
+	tmp, err := os.CreateTemp(m.sshConfigDir, m.sshConfigFile+".*.tmp")
+	if err != nil {
+		return fmt.Errorf("create temp SSH config: %w", err)
+	}
+	tmpPath := tmp.Name()
+	defer func() {
+		if err := os.Remove(tmpPath); err != nil && !os.IsNotExist(err) {
+			log.Debugf("remove temp SSH config %s: %v", tmpPath, err)
+		}
+	}()
+	if err := tmp.Close(); err != nil {
+		return fmt.Errorf("close temp SSH config %s: %w", tmpPath, err)
 	}
 
-	if err := os.Rename(sshConfigPathTmp, sshConfigPath); err != nil {
-		return fmt.Errorf("rename ssh config %s -> %s: %w", sshConfigPathTmp, sshConfigPath, err)
+	if err := writeFileWithTimeout(tmpPath, []byte(sshConfig), 0644); err != nil {
+		return fmt.Errorf("write SSH config file %s: %w", tmpPath, err)
+	}
+
+	if err := os.Rename(tmpPath, sshConfigPath); err != nil {
+		return fmt.Errorf("rename SSH config %s -> %s: %w", tmpPath, sshConfigPath, err)
 	}
 
 	log.Infof("Created NetBird SSH client config: %s", sshConfigPath)

management/server/account.go

@@ -1040,13 +1040,6 @@ func (am *DefaultAccountManager) lookupCache(ctx context.Context, accountUsers m
 
 	for attempt := 1; attempt <= maxAttempts; attempt++ {
 		if am.isCacheFresh(ctx, accountUsers, data) {
-			// Catch the silent vacuous-fresh case: empty accountUsers map + empty cache data
-			// → isCacheFresh returns true without iterating, skipping refreshCache,
-			// returning empty data. This matters when the account is mostly integration
-			// (SCIM) users and the InternalCache has been flushed.
-			if len(accountUsers) == 0 && len(data) == 0 {
-				log.WithContext(ctx).Warnf("lookupCache VACUOUS FRESH: accountUsers map is empty AND cache is empty for account %s — returning empty data without triggering loadAccount", accountID)
-			}
 			return data, nil
 		}
 

management/server/http/handlers/policies/geolocations_handler.go

@@ -7,8 +7,11 @@ import (
 	"github.com/gorilla/mux"
 
 	"github.com/netbirdio/netbird/management/server/account"
+	nbcontext "github.com/netbirdio/netbird/management/server/context"
 	"github.com/netbirdio/netbird/management/server/geolocation"
 	"github.com/netbirdio/netbird/management/server/permissions"
+	"github.com/netbirdio/netbird/management/server/permissions/modules"
+	"github.com/netbirdio/netbird/management/server/permissions/operations"
 	"github.com/netbirdio/netbird/shared/management/http/api"
 	"github.com/netbirdio/netbird/shared/management/http/util"
 	"github.com/netbirdio/netbird/shared/management/status"
@@ -42,6 +45,11 @@ func newGeolocationsHandlerHandler(accountManager account.Manager, geolocationMa
 
 // getAllCountries retrieves a list of all countries
 func (l *geolocationsHandler) getAllCountries(w http.ResponseWriter, r *http.Request) {
+	if err := l.authenticateUser(r); err != nil {
+		util.WriteError(r.Context(), err, w)
+		return
+	}
+
 	if l.geolocationManager == nil {
 		// TODO: update error message to include geo db self hosted doc link when ready
 		util.WriteError(r.Context(), status.Errorf(status.PreconditionFailed, "Geo location database is not initialized"), w)
@@ -63,6 +71,11 @@ func (l *geolocationsHandler) getAllCountries(w http.ResponseWriter, r *http.Req
 
 // getCitiesByCountry retrieves a list of cities based on the given country code
 func (l *geolocationsHandler) getCitiesByCountry(w http.ResponseWriter, r *http.Request) {
+	if err := l.authenticateUser(r); err != nil {
+		util.WriteError(r.Context(), err, w)
+		return
+	}
+
 	vars := mux.Vars(r)
 	countryCode := vars["country"]
 	if !countryCodeRegex.MatchString(countryCode) {
@@ -89,6 +102,27 @@ func (l *geolocationsHandler) getCitiesByCountry(w http.ResponseWriter, r *http.
 	util.WriteJSONObject(r.Context(), w, cities)
 }
 
+func (l *geolocationsHandler) authenticateUser(r *http.Request) error {
+	ctx := r.Context()
+
+	userAuth, err := nbcontext.GetUserAuthFromContext(ctx)
+	if err != nil {
+		return err
+	}
+
+	accountID, userID := userAuth.AccountId, userAuth.UserId
+
+	allowed, err := l.permissionsManager.ValidateUserPermissions(ctx, accountID, userID, modules.Policies, operations.Read)
+	if err != nil {
+		return status.NewPermissionValidationError(err)
+	}
+
+	if !allowed {
+		return status.NewPermissionDeniedError()
+	}
+	return nil
+}
+
 func toCountryResponse(country geolocation.Country) api.Country {
 	return api.Country{
 		CountryName: country.CountryName,

management/server/user.go

@@ -1061,28 +1061,15 @@ func (am *DefaultAccountManager) BuildUserInfosForAccount(ctx context.Context, a
 		queriedUsers = append(queriedUsers, usersFromIntegration...)
 	}
 
-	idpManagerNil := isNil(am.idpManager)
-	idpManagerEmbedded := !idpManagerNil && IsEmbeddedIdp(am.idpManager)
-
 	userInfosMap := make(map[string]*types.UserInfo)
 
 	// in case of self-hosted, or IDP doesn't return anything, we will return the locally stored userInfo
 	if len(queriedUsers) == 0 {
-		var earlyReturnEmpty int
-		var earlyReturnEmptySamples []string
 		for _, accountUser := range accountUsers {
 			info, err := accountUser.ToUserInfo(nil)
 			if err != nil {
 				return nil, err
 			}
-			if !accountUser.IsServiceUser && (info.Email == "" || info.Name == "") {
-				earlyReturnEmpty++
-				if len(earlyReturnEmptySamples) < 50 {
-					earlyReturnEmptySamples = append(earlyReturnEmptySamples,
-						fmt.Sprintf("%s(issued=%s,db.email=%q,db.name=%q)",
-							accountUser.Id, accountUser.Issued, accountUser.Email, accountUser.Name))
-				}
-			}
 			// Try to decode Dex user ID to extract the IdP ID (connector ID)
 			if _, connectorID, decodeErr := dex.DecodeDexUserID(accountUser.Id); decodeErr == nil && connectorID != "" {
 				info.IdPID = connectorID
@@ -1090,61 +1077,17 @@ func (am *DefaultAccountManager) BuildUserInfosForAccount(ctx context.Context, a
 			userInfosMap[accountUser.Id] = info
 		}
 
-		log.WithContext(ctx).Warnf("BuildUserInfosForAccount EARLY RETURN: queriedUsers empty, returning %d users with DB-only data (idpManagerNil=%v, idpManagerEmbedded=%v). %d non-service users have empty email/name in DB. Samples: %v",
-			len(accountUsers), idpManagerNil, idpManagerEmbedded, earlyReturnEmpty, earlyReturnEmptySamples)
-
-		// Same canonical-truth final scan, also on the early-return path
-		var finalEmptyEmail, finalEmptyName int
-		var finalEmptySamples []string
-		for id, info := range userInfosMap {
-			if info.IsServiceUser {
-				continue
-			}
-			if info.Email == "" {
-				finalEmptyEmail++
-			}
-			if info.Name == "" {
-				finalEmptyName++
-			}
-			if (info.Email == "" || info.Name == "") && len(finalEmptySamples) < 200 {
-				finalEmptySamples = append(finalEmptySamples,
-					fmt.Sprintf("%s(email=%q,name=%q,issued=%s)", id, info.Email, info.Name, info.Issued))
-			}
-		}
-		if finalEmptyEmail > 0 || finalEmptyName > 0 {
-			log.WithContext(ctx).Warnf("BuildUserInfosForAccount FINAL (early-return path): returning %d UserInfo entries — %d with empty email, %d with empty name. Samples: %v",
-				len(userInfosMap), finalEmptyEmail, finalEmptyName, finalEmptySamples)
-		}
-
 		return userInfosMap, nil
 	}
 
-	var cacheHitEmpty, fallbackMiss int
-	var cacheHitEmptySamples, fallbackMissSamples []string
 	for _, localUser := range accountUsers {
 		var info *types.UserInfo
 		if queriedUser, contains := findUserInIDPUserdata(localUser.Id, queriedUsers); contains {
-			if !localUser.IsServiceUser && (queriedUser.Email == "" || queriedUser.Name == "") {
-				cacheHitEmpty++
-				if len(cacheHitEmptySamples) < 50 {
-					cacheHitEmptySamples = append(cacheHitEmptySamples,
-						fmt.Sprintf("%s(cache.email=%q,cache.name=%q,db.email=%q,db.name=%q)",
-							localUser.Id, queriedUser.Email, queriedUser.Name, localUser.Email, localUser.Name))
-				}
-			}
 			info, err = localUser.ToUserInfo(queriedUser)
 			if err != nil {
 				return nil, err
 			}
 		} else {
-			if !localUser.IsServiceUser {
-				fallbackMiss++
-				if len(fallbackMissSamples) < 50 {
-					fallbackMissSamples = append(fallbackMissSamples,
-						fmt.Sprintf("%s(issued=%s,db.email=%q,db.name=%q)",
-							localUser.Id, localUser.Issued, localUser.Email, localUser.Name))
-				}
-			}
 			name := ""
 			if localUser.IsServiceUser {
 				name = localUser.ServiceUserName
@@ -1168,40 +1111,6 @@ func (am *DefaultAccountManager) BuildUserInfosForAccount(ctx context.Context, a
 		userInfosMap[info.ID] = info
 	}
 
-	if cacheHitEmpty > 0 {
-		log.WithContext(ctx).Warnf("BuildUserInfosForAccount: %d users found in cache with empty email or name (cache pollution). Samples: %v",
-			cacheHitEmpty, cacheHitEmptySamples)
-	}
-	if fallbackMiss > 0 {
-		log.WithContext(ctx).Warnf("BuildUserInfosForAccount: %d non-service users missed both caches (will get empty Name in API response from fallback). Samples: %v",
-			fallbackMiss, fallbackMissSamples)
-	}
-
-	// Canonical-truth log: scan what we are actually about to return to the handler.
-	// This catches empties from any code path (early-return, cache-hit-empty, fallback-miss,
-	// or anything we haven't identified yet).
-	var finalEmptyEmail, finalEmptyName int
-	var finalEmptySamples []string
-	for id, info := range userInfosMap {
-		if info.IsServiceUser {
-			continue
-		}
-		if info.Email == "" {
-			finalEmptyEmail++
-		}
-		if info.Name == "" {
-			finalEmptyName++
-		}
-		if (info.Email == "" || info.Name == "") && len(finalEmptySamples) < 200 {
-			finalEmptySamples = append(finalEmptySamples,
-				fmt.Sprintf("%s(email=%q,name=%q,issued=%s)", id, info.Email, info.Name, info.Issued))
-		}
-	}
-	if finalEmptyEmail > 0 || finalEmptyName > 0 {
-		log.WithContext(ctx).Warnf("BuildUserInfosForAccount FINAL: returning %d UserInfo entries — %d with empty email, %d with empty name. Samples: %v",
-			len(userInfosMap), finalEmptyEmail, finalEmptyName, finalEmptySamples)
-	}
-
 	return userInfosMap, nil
 }