diff --git a/management/internals/shared/grpc/conversion.go b/management/internals/shared/grpc/conversion.go index 40b1f792e..353559538 100644 --- a/management/internals/shared/grpc/conversion.go +++ b/management/internals/shared/grpc/conversion.go @@ -2,7 +2,6 @@ package grpc import ( "context" - "encoding/base64" "fmt" "net/netip" "net/url" @@ -10,8 +9,6 @@ import ( "time" "github.com/hashicorp/go-version" - log "github.com/sirupsen/logrus" - nbversion "github.com/netbirdio/netbird/version" "google.golang.org/protobuf/types/known/timestamppb" @@ -27,7 +24,6 @@ import ( "github.com/netbirdio/netbird/shared/management/networkmap" "github.com/netbirdio/netbird/shared/management/proto" "github.com/netbirdio/netbird/shared/netiputil" - "github.com/netbirdio/netbird/shared/sshauth" ) const ( @@ -217,16 +213,16 @@ func ToSyncResponse(ctx context.Context, config *nbconfig.Config, httpConfig *nb response.NetworkMap.SshAuth = &proto.SSHAuth{AuthorizedUsers: hashedUsers, MachineUsers: machineUsers, UserIDClaim: userIDClaim} } - if networkMap.VNCAuthorizedUsers != nil || len(networkMap.VNCSessionPubKeys) > 0 { + if len(networkMap.VNCAuthorizedUsers) > 0 || len(networkMap.VNCSessionPubKeys) > 0 { var hashedUsers [][]byte var machineUsers map[string]*proto.MachineUserIndexes - if networkMap.VNCAuthorizedUsers != nil { + if len(networkMap.VNCAuthorizedUsers) > 0 { hashedUsers, machineUsers = networkmap.BuildAuthorizedUsersProto(ctx, networkMap.VNCAuthorizedUsers) } response.NetworkMap.VncAuth = &proto.VNCAuth{ AuthorizedUsers: hashedUsers, MachineUsers: machineUsers, - SessionPubKeys: buildSessionPubKeysProto(ctx, networkMap.VNCSessionPubKeys), + SessionPubKeys: networkmap.BuildSessionPubKeysProto(ctx, networkMap.VNCSessionPubKeys), } } @@ -243,38 +239,6 @@ func ToSyncResponse(ctx context.Context, config *nbconfig.Config, httpConfig *nb return response } -// buildSessionPubKeysProto decodes base64 X25519 session pubkeys and -// hashes the user IDs they belong to, emitting the proto entries the -// daemon's authorizer indexes by pubkey. -func buildSessionPubKeysProto(ctx context.Context, in []types.VNCSessionPubKey) []*proto.SessionPubKey { - if len(in) == 0 { - return nil - } - out := make([]*proto.SessionPubKey, 0, len(in)) - for _, e := range in { - pub, err := base64.StdEncoding.DecodeString(e.PubKey) - if err != nil { - log.WithContext(ctx).Warnf("decode VNC session pubkey: %v", err) - continue - } - if len(pub) != 32 { - log.WithContext(ctx).Warnf("VNC session pubkey wrong length: %d", len(pub)) - continue - } - hash, err := sshauth.HashUserID(e.UserID) - if err != nil { - log.WithContext(ctx).Warnf("hash VNC session user id: %v", err) - continue - } - out = append(out, &proto.SessionPubKey{ - PubKey: pub, - UserIdHash: hash[:], - DisplayName: e.DisplayName, - }) - } - return out -} - // encodeSessionExpiresAt encodes a server-side deadline into the 3-state wire // representation used on LoginResponse, SyncResponse and // ExtendAuthSessionResponse. See the proto comments on those messages. diff --git a/shared/management/networkmap/encode.go b/shared/management/networkmap/encode.go index 7d1573d62..dfc6ac98c 100644 --- a/shared/management/networkmap/encode.go +++ b/shared/management/networkmap/encode.go @@ -13,6 +13,7 @@ package networkmap import ( "context" + "encoding/base64" log "github.com/sirupsen/logrus" goproto "google.golang.org/protobuf/proto" @@ -292,6 +293,41 @@ func AppendRemotePeerConfig(dst []*proto.RemotePeerConfig, peers []*types.Compon return dst } +// BuildSessionPubKeysProto decodes base64 X25519 session pubkeys and hashes +// the user IDs they belong to, emitting the proto entries the daemon's VNC +// authorizer indexes by pubkey. An entry that cannot be decoded, is not a +// 32-byte key, or whose user id will not hash is dropped: the authorizer +// would never match it, and shipping it would only widen what the daemon +// trusts on the strength of a malformed value. +func BuildSessionPubKeysProto(ctx context.Context, in []types.VNCSessionPubKey) []*proto.SessionPubKey { + if len(in) == 0 { + return nil + } + out := make([]*proto.SessionPubKey, 0, len(in)) + for _, e := range in { + pub, err := base64.StdEncoding.DecodeString(e.PubKey) + if err != nil { + log.WithContext(ctx).Warnf("decode VNC session pubkey: %v", err) + continue + } + if len(pub) != 32 { + log.WithContext(ctx).Warnf("VNC session pubkey wrong length: %d", len(pub)) + continue + } + hash, err := sshauth.HashUserID(e.UserID) + if err != nil { + log.WithContext(ctx).Warnf("hash VNC session user id: %v", err) + continue + } + out = append(out, &proto.SessionPubKey{ + PubKey: pub, + UserIdHash: hash[:], + DisplayName: e.DisplayName, + }) + } + return out +} + // BuildAuthorizedUsersProto deduplicates user-IDs into a hashed list and // builds per-machine-user index maps. Returns (hashedUsers, machineUsers). // Errors from individual hash failures are logged via the provided context; diff --git a/shared/management/networkmap/envelope.go b/shared/management/networkmap/envelope.go index a928c5059..d0b1e01d6 100644 --- a/shared/management/networkmap/envelope.go +++ b/shared/management/networkmap/envelope.go @@ -101,6 +101,27 @@ func EnvelopeToNetworkMap(ctx context.Context, env *proto.NetworkMapEnvelope, lo } } + // Mirrors the server-side VncAuth build in the legacy path: the daemon's VNC + // authorizer reads only this field, so a components-path peer without it + // refuses every VNC connection. + // + // Emitted only when there is something to authorize. The resolve state always + // allocates VNCAuthorizedUsers, so testing it for nil would attach an empty + // VncAuth to every sync of every peer. Either spelling denies: a missing + // VncAuth becomes an empty config in updateVNCServerAuth. + if len(typedNM.VNCAuthorizedUsers) > 0 || len(typedNM.VNCSessionPubKeys) > 0 { + var hashedUsers [][]byte + var machineUsers map[string]*proto.MachineUserIndexes + if len(typedNM.VNCAuthorizedUsers) > 0 { + hashedUsers, machineUsers = BuildAuthorizedUsersProto(ctx, typedNM.VNCAuthorizedUsers) + } + protoNM.VncAuth = &proto.VNCAuth{ + AuthorizedUsers: hashedUsers, + MachineUsers: machineUsers, + SessionPubKeys: BuildSessionPubKeysProto(ctx, typedNM.VNCSessionPubKeys), + } + } + if typedNM.ForwardingRules != nil { forwardingRules := make([]*proto.ForwardingRule, 0, len(typedNM.ForwardingRules)) for _, rule := range typedNM.ForwardingRules { diff --git a/shared/management/networkmap/envelope_test.go b/shared/management/networkmap/envelope_test.go index a81478aff..e1469d0b2 100644 --- a/shared/management/networkmap/envelope_test.go +++ b/shared/management/networkmap/envelope_test.go @@ -86,6 +86,77 @@ func TestCalculate_FirewallRuleProtocol_NeverNetbirdSSH(t *testing.T) { } } +// A netbird-vnc policy must survive the components round trip and come out as +// VncAuth on the client side. The daemon's VNC authorizer reads only that +// field, so a components-path peer without it refuses every VNC connection — +// the feature would be silently dead for capability-advertising peers while +// working fine on the legacy path. +func TestEnvelopeToNetworkMap_VNCPolicyProducesVncAuth(t *testing.T) { + c, localPeerKey := buildSmokeComponents(t) + c.GroupIDToUserIDs = map[string][]string{"1": {"user-1"}} + c.Policies = []*types.Policy{{ + ID: "pol-vnc", PublicID: "2", Enabled: true, + Rules: []*types.PolicyRule{{ + ID: "rule-vnc", + Enabled: true, + Action: types.PolicyTrafficActionAccept, + Protocol: types.PolicyRuleProtocolNetbirdVNC, + Bidirectional: true, + Sources: []string{"group-all"}, + Destinations: []string{"group-all"}, + AuthorizedUser: "user-1", + SessionPubKey: base64.StdEncoding.EncodeToString(make([]byte, 32)), + SessionDisplayName: "Alice", + }}, + }} + + result := roundTripComponents(t, c, localPeerKey) + + require.NotNil(t, result.NetworkMap.VncAuth, "netbird-vnc policy must produce VncAuth") + require.NotEmpty(t, result.NetworkMap.VncAuth.AuthorizedUsers, "authorized users must survive the round trip") + require.Len(t, result.NetworkMap.VncAuth.SessionPubKeys, 1, "the session pubkey must survive the round trip") + pk := result.NetworkMap.VncAuth.SessionPubKeys[0] + require.Len(t, pk.PubKey, 32, "pubkey must be decoded to raw 32 bytes") + require.NotEmpty(t, pk.UserIdHash, "user id must be hashed for the authorizer") + require.Equal(t, "Alice", pk.DisplayName) + + // The VNC marker protocol must not reach the firewall rules, for the same + // reason NetbirdSSH must not: agents fall into UNKNOWN-protocol handling. + for i, fr := range result.NetworkMap.FirewallRules { + require.NotEqualf(t, proto.RuleProtocol_NETBIRD_VNC, fr.Protocol, + "FirewallRules[%d].Protocol must be the rewritten TCP, not NETBIRD_VNC", i) + } +} + +// A policy that authorizes nothing VNC-related must leave VncAuth unset, so the +// authorizer keeps refusing rather than being handed an empty allow-list to +// interpret. +func TestEnvelopeToNetworkMap_NoVNCPolicyLeavesVncAuthUnset(t *testing.T) { + c, localPeerKey := buildSmokeComponents(t) + result := roundTripComponents(t, c, localPeerKey) + require.Nil(t, result.NetworkMap.VncAuth, "a non-VNC policy set must not produce VncAuth") +} + +// roundTripComponents encodes c as an envelope, pushes it through the wire, and +// decodes it the way a client does. +func roundTripComponents(t *testing.T, c *types.NetworkMapComponents, localPeerKey string) *nbnetworkmap.EnvelopeResult { + t.Helper() + envelope := mgmtgrpc.EncodeNetworkMapEnvelope(mgmtgrpc.ComponentsEnvelopeInput{ + Components: c, + DNSDomain: "netbird.cloud", + }) + wire, err := goproto.Marshal(envelope) + require.NoError(t, err, "marshal envelope") + var decoded proto.NetworkMapEnvelope + require.NoError(t, goproto.Unmarshal(wire, &decoded), "unmarshal envelope") + + result, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), &decoded, localPeerKey, "netbird.cloud") + require.NoError(t, err, "EnvelopeToNetworkMap") + require.NotNil(t, result) + require.NotNil(t, result.NetworkMap) + return result +} + func TestEnvelopeToNetworkMap_NilEnvelope(t *testing.T) { _, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), nil, "key", "netbird.cloud") require.Error(t, err, "nil envelope must produce an error rather than panic")