Merge branch 'main' into ui-refactor

client/internal/dns/resutil/resolve.go

@@ -14,6 +14,10 @@ import (
 	log "github.com/sirupsen/logrus"
 )
 
+// errNoSuitableAddress mirrors the unexported error string the net package
+// uses when a resolved host has no addresses of the requested family.
+const errNoSuitableAddress = "no suitable address found"
+
 // GenerateRequestID creates a random 8-character hex string for request tracing.
 func GenerateRequestID() string {
 	bytes := make([]byte, 4)
@@ -126,6 +130,14 @@ func LookupIP(ctx context.Context, r resolver, network, host string, qtype uint1
 }
 
 func getRcodeForError(ctx context.Context, r resolver, host string, qtype uint16, err error) int {
+	// The net package returns this AddrError when the host resolves but has
+	// no addresses of the requested family. The domain exists, so answer
+	// NODATA instead of SERVFAIL.
+	var addrErr *net.AddrError
+	if errors.As(err, &addrErr) && addrErr.Err == errNoSuitableAddress {
+		return dns.RcodeSuccess
+	}
+
 	var dnsErr *net.DNSError
 	if !errors.As(err, &dnsErr) {
 		return dns.RcodeServerFailure

client/internal/dns/resutil/resolve_test.go

@@ -0,0 +1,122 @@
+package resutil
+
+import (
+	"context"
+	"errors"
+	"net"
+	"net/netip"
+	"testing"
+
+	"github.com/miekg/dns"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+type mockResolver struct {
+	// results maps network ("ip4"/"ip6") to the lookup outcome.
+	results map[string]mockLookup
+}
+
+type mockLookup struct {
+	ips []netip.Addr
+	err error
+}
+
+func (m *mockResolver) LookupNetIP(_ context.Context, network, _ string) ([]netip.Addr, error) {
+	res, ok := m.results[network]
+	if !ok {
+		return nil, errors.New("unexpected network: " + network)
+	}
+	return res.ips, res.err
+}
+
+func TestLookupIP_Success(t *testing.T) {
+	r := &mockResolver{results: map[string]mockLookup{
+		"ip4": {ips: []netip.Addr{netip.MustParseAddr("::ffff:192.0.2.1")}},
+	}}
+
+	result := LookupIP(context.Background(), r, "ip4", "example.com.", dns.TypeA)
+
+	assert.Equal(t, dns.RcodeSuccess, result.Rcode, "successful lookup should return NOERROR")
+	require.Len(t, result.IPs, 1, "should return the resolved address")
+	assert.Equal(t, netip.MustParseAddr("192.0.2.1"), result.IPs[0], "v4-mapped address should be unmapped")
+}
+
+func TestLookupIP_NoSuitableAddress(t *testing.T) {
+	// The net package returns this AddrError when the host resolves but has
+	// no addresses of the requested family (e.g. AAAA query for a v4-only
+	// hosts file entry). The domain exists, so this is NODATA, not SERVFAIL.
+	r := &mockResolver{results: map[string]mockLookup{
+		"ip6": {err: &net.AddrError{Err: "no suitable address found", Addr: "example.com."}},
+	}}
+
+	result := LookupIP(context.Background(), r, "ip6", "example.com.", dns.TypeAAAA)
+
+	assert.Equal(t, dns.RcodeSuccess, result.Rcode, "no suitable address should map to NODATA")
+	assert.Empty(t, result.IPs, "NODATA response should carry no addresses")
+}
+
+// TestErrNoSuitableAddressMatchesNetPackage pins our copy of the error string
+// to what the net package actually emits. A literal IP of the wrong family
+// takes the same filterAddrList path as a resolved hostname, without network
+// access.
+func TestErrNoSuitableAddressMatchesNetPackage(t *testing.T) {
+	_, err := (&net.Resolver{}).LookupNetIP(context.Background(), "ip6", "192.0.2.1")
+	require.Error(t, err)
+
+	var addrErr *net.AddrError
+	require.ErrorAs(t, err, &addrErr, "wrong-family lookup should return AddrError")
+	assert.Equal(t, errNoSuitableAddress, addrErr.Err, "net package error string should match our constant")
+}
+
+func TestLookupIP_OtherAddrError(t *testing.T) {
+	r := &mockResolver{results: map[string]mockLookup{
+		"ip4": {err: &net.AddrError{Err: "some other address problem", Addr: "example.com."}},
+	}}
+
+	result := LookupIP(context.Background(), r, "ip4", "example.com.", dns.TypeA)
+
+	assert.Equal(t, dns.RcodeServerFailure, result.Rcode, "unrecognized AddrError should map to SERVFAIL")
+}
+
+func TestLookupIP_NotFoundNXDomain(t *testing.T) {
+	r := &mockResolver{results: map[string]mockLookup{
+		"ip4": {err: &net.DNSError{Err: "no such host", Name: "example.com.", IsNotFound: true}},
+		"ip6": {err: &net.DNSError{Err: "no such host", Name: "example.com.", IsNotFound: true}},
+	}}
+
+	result := LookupIP(context.Background(), r, "ip4", "example.com.", dns.TypeA)
+
+	assert.Equal(t, dns.RcodeNameError, result.Rcode, "not found for both families should map to NXDOMAIN")
+}
+
+func TestLookupIP_NotFoundNoData(t *testing.T) {
+	r := &mockResolver{results: map[string]mockLookup{
+		"ip6": {err: &net.DNSError{Err: "no such host", Name: "example.com.", IsNotFound: true}},
+		"ip4": {ips: []netip.Addr{netip.MustParseAddr("192.0.2.1")}},
+	}}
+
+	result := LookupIP(context.Background(), r, "ip6", "example.com.", dns.TypeAAAA)
+
+	assert.Equal(t, dns.RcodeSuccess, result.Rcode, "not found with the other family present should map to NODATA")
+}
+
+func TestLookupIP_GenericError(t *testing.T) {
+	r := &mockResolver{results: map[string]mockLookup{
+		"ip4": {err: errors.New("connection refused")},
+	}}
+
+	result := LookupIP(context.Background(), r, "ip4", "example.com.", dns.TypeA)
+
+	assert.Equal(t, dns.RcodeServerFailure, result.Rcode, "generic error should map to SERVFAIL")
+}
+
+func TestLookupIP_DNSErrorNotIsNotFound(t *testing.T) {
+	r := &mockResolver{results: map[string]mockLookup{
+		"ip4": {err: &net.DNSError{Err: "server misbehaving", Name: "example.com.", IsTemporary: true}},
+	}}
+
+	result := LookupIP(context.Background(), r, "ip4", "example.com.", dns.TypeA)
+
+	assert.Equal(t, dns.RcodeServerFailure, result.Rcode, "upstream failure should map to SERVFAIL")
+}

idp/dex/provider.go

@@ -41,6 +41,8 @@ type Config struct {
 	GRPCAddr string
 }
 
+const localConnectorID = "local"
+
 // Provider wraps a Dex server
 type Provider struct {
 	config       *Config
@@ -544,7 +546,7 @@ func (p *Provider) CreateUser(ctx context.Context, email, username, password str
 
 	// Encode the user ID in Dex's format: base64(protobuf{user_id, connector_id})
 	// This matches the format Dex uses in JWT tokens
-	encodedID := EncodeDexUserID(userID, "local")
+	encodedID := EncodeDexUserID(userID, localConnectorID)
 	return encodedID, nil
 }
 
@@ -619,6 +621,13 @@ func DecodeDexUserID(encodedID string) (userID, connectorID string, err error) {
 	return userID, connectorID, nil
 }
 
+// IsLocalUserID reports whether encodedID is a Dex subject for the built-in
+// local password connector.
+func IsLocalUserID(encodedID string) bool {
+	_, connectorID, err := DecodeDexUserID(encodedID)
+	return err == nil && connectorID == localConnectorID
+}
+
 // GetUser returns a user by email
 func (p *Provider) GetUser(ctx context.Context, email string) (storage.Password, error) {
 	return p.storage.GetPassword(ctx, email)

idp/dex/provider_test.go

@@ -115,6 +115,26 @@ func TestDecodeDexUserID(t *testing.T) {
 	}
 }
 
+func TestIsLocalUserID(t *testing.T) {
+	tests := []struct {
+		name      string
+		encodedID string
+		want      bool
+	}{
+		{name: "local connector", encodedID: EncodeDexUserID("7aad8c05-3287-473f-b42a-365504bf25e7", "local"), want: true},
+		{name: "federated connector", encodedID: EncodeDexUserID("entra-user", "entra"), want: false},
+		{name: "non-dex external IdP id", encodedID: "google-oauth2|1234567890", want: false},
+		{name: "invalid base64", encodedID: "not-valid-base64!!!", want: false},
+		{name: "empty", encodedID: "", want: false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, IsLocalUserID(tt.encodedID))
+		})
+	}
+}
+
 func TestEncodeDexUserID(t *testing.T) {
 	userID := "7aad8c05-3287-473f-b42a-365504bf25e7"
 	connectorID := "local"

management/server/account.go

@@ -28,6 +28,7 @@ import (
 
 	nbdns "github.com/netbirdio/netbird/dns"
 	"github.com/netbirdio/netbird/formatter/hook"
+	"github.com/netbirdio/netbird/idp/dex"
 	"github.com/netbirdio/netbird/management/internals/controllers/network_map"
 	nbconfig "github.com/netbirdio/netbird/management/internals/server/config"
 	"github.com/netbirdio/netbird/management/server/account"
@@ -1588,7 +1589,10 @@ func (am *DefaultAccountManager) updateUserAuthWithSingleMode(ctx context.Contex
 // and propagates changes to peers if group propagation is enabled.
 // requires userAuth to have been ValidateAndParseToken and EnsureUserAccessByJWTGroups by the AuthManager
 func (am *DefaultAccountManager) SyncUserJWTGroups(ctx context.Context, userAuth auth.UserAuth) error {
-	if userAuth.IsChild || userAuth.IsPAT {
+	// Child accounts and PAT-authenticated requests do not sync JWT groups.
+	// Embedded-Dex local users also skip sync because local password authentication
+	// does not provide external IdP group claims.
+	if userAuth.IsChild || userAuth.IsPAT || dex.IsLocalUserID(userAuth.UserId) {
 		return nil
 	}
 

management/server/account_test.go

@@ -26,6 +26,7 @@ import (
 	"github.com/netbirdio/netbird/shared/management/status"
 
 	nbdns "github.com/netbirdio/netbird/dns"
+	"github.com/netbirdio/netbird/idp/dex"
 	"github.com/netbirdio/netbird/management/internals/controllers/network_map"
 	"github.com/netbirdio/netbird/management/internals/controllers/network_map/controller"
 	"github.com/netbirdio/netbird/management/internals/controllers/network_map/update_channel"
@@ -723,6 +724,28 @@ func TestDefaultAccountManager_SyncUserJWTGroups(t *testing.T) {
 		require.Equal(t, g2.Name, "group2", "group2 name should match")
 		require.Equal(t, g2.Issued, types.GroupIssuedJWT, "group2 issued should match")
 	})
+	t.Run("local embedded-Dex user is skipped", func(t *testing.T) {
+		initAccount.Settings.JWTGroupsEnabled = true
+		initAccount.Settings.JWTGroupsClaimName = "idp-groups"
+		err := manager.Store.SaveAccount(context.Background(), initAccount)
+		require.NoError(t, err, "save account failed")
+
+		localClaims := auth.UserAuth{
+			AccountId: accountID,
+			Domain:    domain,
+			UserId:    dex.EncodeDexUserID("local-owner", "local"),
+			Groups:    []string{"group3", "group4"},
+		}
+		err = manager.SyncUserJWTGroups(context.Background(), localClaims)
+		require.NoError(t, err, "sync should be a no-op for local users")
+
+		account, err := manager.Store.GetAccount(context.Background(), accountID)
+		require.NoError(t, err, "get account failed")
+		for _, g := range account.Groups {
+			require.NotEqual(t, "group3", g.Name, "local user JWT groups must not be synced")
+			require.NotEqual(t, "group4", g.Name, "local user JWT groups must not be synced")
+		}
+	})
 }
 
 func TestAccountManager_PrivateAccount(t *testing.T) {

management/server/auth/manager.go

@@ -12,6 +12,7 @@ import (
 	"github.com/netbirdio/netbird/shared/auth"
 
 	"github.com/netbirdio/netbird/base62"
+	"github.com/netbirdio/netbird/idp/dex"
 	"github.com/netbirdio/netbird/management/server/store"
 	"github.com/netbirdio/netbird/management/server/types"
 	nbjwt "github.com/netbirdio/netbird/shared/auth/jwt"
@@ -74,7 +75,10 @@ func (m *manager) ValidateAndParseToken(ctx context.Context, value string) (auth
 }
 
 func (m *manager) EnsureUserAccessByJWTGroups(ctx context.Context, userAuth auth.UserAuth, token *jwt.Token) (auth.UserAuth, error) {
-	if userAuth.IsChild || userAuth.IsPAT {
+	// Child accounts and PAT-authenticated requests do not use JWT group access checks.
+	// Embedded-Dex local users also skip them because local password authentication
+	// does not provide external IdP group claims.
+	if userAuth.IsChild || userAuth.IsPAT || dex.IsLocalUserID(userAuth.UserId) {
 		return userAuth, nil
 	}
 

management/server/auth/manager_test.go

@@ -16,6 +16,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/netbirdio/netbird/idp/dex"
 	"github.com/netbirdio/netbird/management/server/auth"
 	"github.com/netbirdio/netbird/management/server/store"
 	"github.com/netbirdio/netbird/management/server/types"
@@ -206,6 +207,43 @@ func TestAuthManager_EnsureUserAccessByJWTGroups(t *testing.T) {
 		_, err = manager.EnsureUserAccessByJWTGroups(context.Background(), userAuth, token)
 		require.Error(t, err, "ensure user access is not in allowed groups")
 	})
+
+	t.Run("Local embedded-Dex user is exempt from JWT allow-groups", func(t *testing.T) {
+		account.Settings.JWTGroupsEnabled = true
+		account.Settings.JWTGroupsClaimName = "idp-groups"
+		account.Settings.JWTAllowGroups = []string{"not-a-group"}
+		err := store.SaveAccount(context.Background(), account)
+		require.NoError(t, err, "save account failed")
+
+		// Local Dex users have a "local" connector encoded in their user ID.
+		localUserAuth := nbauth.UserAuth{
+			AccountId: account.Id,
+			Domain:    domain,
+			UserId:    dex.EncodeDexUserID("local-owner", "local"),
+		}
+
+		localUserAuth, err = manager.EnsureUserAccessByJWTGroups(context.Background(), localUserAuth, token)
+		require.NoError(t, err, "local user must not be locked out by JWT allow-groups (issue #5337)")
+		require.Len(t, localUserAuth.Groups, 0, "JWT groups must not be evaluated for local users")
+	})
+
+	t.Run("Federated embedded-Dex user is still subject to JWT allow-groups", func(t *testing.T) {
+		account.Settings.JWTGroupsEnabled = true
+		account.Settings.JWTGroupsClaimName = "idp-groups"
+		account.Settings.JWTAllowGroups = []string{"not-a-group"}
+		err := store.SaveAccount(context.Background(), account)
+		require.NoError(t, err, "save account failed")
+
+		// A federated user (non-"local" connector) must remain restricted.
+		fedUserAuth := nbauth.UserAuth{
+			AccountId: account.Id,
+			Domain:    domain,
+			UserId:    dex.EncodeDexUserID("entra-user", "entra"),
+		}
+
+		_, err = manager.EnsureUserAccessByJWTGroups(context.Background(), fedUserAuth, token)
+		require.Error(t, err, "federated user must still be restricted by JWT allow-groups")
+	})
 }
 
 func TestAuthManager_ValidateAndParseToken(t *testing.T) {