Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions client/anonymize/anonymize.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ const anonTLD = ".domain"
type Anonymizer struct {
ipAnonymizer map[netip.Addr]netip.Addr
domainAnonymizer map[string]string
labelAnonymizer map[string]string
labelCounter int
currentAnonIPv4 netip.Addr
currentAnonIPv6 netip.Addr
startAnonIPv4 netip.Addr
Expand All @@ -34,6 +36,7 @@ func NewAnonymizer(startIPv4, startIPv6 netip.Addr) *Anonymizer {
return &Anonymizer{
ipAnonymizer: map[netip.Addr]netip.Addr{},
domainAnonymizer: map[string]string{},
labelAnonymizer: map[string]string{},
currentAnonIPv4: startIPv4,
currentAnonIPv6: startIPv6,
startAnonIPv4: startIPv4,
Expand Down Expand Up @@ -140,6 +143,21 @@ func (a *Anonymizer) AnonymizeDomain(domain string) string {
return result
}

// AnonymizeLabel anonymizes an arbitrary text label (e.g. group names) using a consistent mapping.
func (a *Anonymizer) AnonymizeLabel(label string) string {
if label == "" {
return label
}

anonymized, ok := a.labelAnonymizer[label]
if !ok {
a.labelCounter++
anonymized = fmt.Sprintf("anon-label-%d", a.labelCounter)
a.labelAnonymizer[label] = anonymized
}
return anonymized
}
Comment on lines +146 to +159
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Avoid generating an anonymized label that equals the original.

A real group name like anon-label-1 will pass through unchanged the first time it is seen, because the first generated token is also anon-label-1. That means --anonymize can still leak a user label. Please skip candidates that match the source label and add a regression test for labels already shaped like anon-label-N.

🛠️ Minimal fix
 func (a *Anonymizer) AnonymizeLabel(label string) string {
 	if label == "" {
 		return label
 	}
 
 	anonymized, ok := a.labelAnonymizer[label]
 	if !ok {
-		a.labelCounter++
-		anonymized = fmt.Sprintf("anon-label-%d", a.labelCounter)
+		for {
+			a.labelCounter++
+			anonymized = fmt.Sprintf("anon-label-%d", a.labelCounter)
+			if anonymized != label {
+				break
+			}
+		}
 		a.labelAnonymizer[label] = anonymized
 	}
 	return anonymized
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// AnonymizeLabel anonymizes an arbitrary text label (e.g. group names) using a consistent mapping.
func (a *Anonymizer) AnonymizeLabel(label string) string {
if label == "" {
return label
}
anonymized, ok := a.labelAnonymizer[label]
if !ok {
a.labelCounter++
anonymized = fmt.Sprintf("anon-label-%d", a.labelCounter)
a.labelAnonymizer[label] = anonymized
}
return anonymized
}
// AnonymizeLabel anonymizes an arbitrary text label (e.g. group names) using a consistent mapping.
func (a *Anonymizer) AnonymizeLabel(label string) string {
if label == "" {
return label
}
anonymized, ok := a.labelAnonymizer[label]
if !ok {
for {
a.labelCounter++
anonymized = fmt.Sprintf("anon-label-%d", a.labelCounter)
if anonymized != label {
break
}
}
a.labelAnonymizer[label] = anonymized
}
return anonymized
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@client/anonymize/anonymize.go` around lines 146 - 159, AnonymizeLabel
currently may return a generated token equal to the original label (e.g.
original "anon-label-1"); change AnonymizeLabel on type Anonymizer to ensure the
generated anonymized value is never equal to the source: when creating a new
anonymized value (using a.labelCounter and fmt.Sprintf("anon-label-%d", ...)),
loop/increment the counter until the candidate != label and is not already
present in a.labelAnonymizer, then assign it to a.labelAnonymizer[label]; also
add a regression test that calls AnonymizeLabel with inputs already shaped like
"anon-label-N" to assert the returned anonymized value differs from the original
and remains consistent across repeated calls.


func (a *Anonymizer) AnonymizeURI(uri string) string {
u, err := url.Parse(uri)
if err != nil {
Expand Down
27 changes: 27 additions & 0 deletions client/anonymize/anonymize_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -295,3 +295,30 @@ func TestAnonymizeString_IPAddresses(t *testing.T) {
})
}
}

func TestAnonymizeLabel(t *testing.T) {
anonymizer := anonymize.NewAnonymizer(anonymize.DefaultAddresses())

t.Run("consistent mapping", func(t *testing.T) {
result1 := anonymizer.AnonymizeLabel("Production")
result2 := anonymizer.AnonymizeLabel("Production")
assert.Equal(t, result1, result2, "same label should produce same anonymized value")
})

t.Run("different labels get different values", func(t *testing.T) {
result1 := anonymizer.AnonymizeLabel("All")
result2 := anonymizer.AnonymizeLabel("Staging")
assert.NotEqual(t, result1, result2, "different labels should produce different anonymized values")
})

t.Run("empty label unchanged", func(t *testing.T) {
result := anonymizer.AnonymizeLabel("")
assert.Equal(t, "", result)
})

t.Run("anonymized value hides original", func(t *testing.T) {
result := anonymizer.AnonymizeLabel("Finance-Team")
assert.NotContains(t, result, "Finance")
assert.Contains(t, result, "anon-label-")
})
}
8 changes: 8 additions & 0 deletions client/internal/engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -722,6 +722,10 @@ func (e *Engine) modifyPeers(peersUpdate []*mgmProto.RemotePeerConfig) error {
if err := e.statusRecorder.UpdatePeerFQDN(peerPubKey, p.GetFqdn()); err != nil {
log.Warnf("error updating peer's %s fqdn in the status recorder, got error: %v", peerPubKey, err)
}

if err := e.statusRecorder.UpdatePeerGroups(peerPubKey, p.GetGroupsNames()); err != nil {
log.Warnf("error updating peer's %s groups in the status recorder, got error: %v", peerPubKey, err)
}
}

// second, close all modified connections and remove them from the state map
Expand Down Expand Up @@ -1498,6 +1502,10 @@ func (e *Engine) addNewPeer(peerConfig *mgmProto.RemotePeerConfig) error {
log.Warnf("error adding peer %s to status recorder, got error: %v", peerKey, err)
}

if err := e.statusRecorder.UpdatePeerGroups(peerKey, peerConfig.GetGroupsNames()); err != nil {
log.Warnf("error updating peer %s groups in the status recorder, got error: %v", peerKey, err)
}

if exists := e.connMgr.AddPeerConn(e.ctx, peerKey, conn); exists {
conn.Close(false)
return fmt.Errorf("peer already exists: %s", peerKey)
Expand Down
18 changes: 18 additions & 0 deletions client/internal/peer/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ type State struct {
Latency time.Duration
RosenpassEnabled bool
SSHHostKey []byte
Groups []string
routes map[string]struct{}
}

Expand Down Expand Up @@ -575,6 +576,22 @@ func (d *Status) UpdatePeerFQDN(peerPubKey, fqdn string) error {
return nil
}

// UpdatePeerGroups updates peer's groups
func (d *Status) UpdatePeerGroups(peerPubKey string, groups []string) error {
d.mux.Lock()
defer d.mux.Unlock()

peerState, ok := d.peers[peerPubKey]
if !ok {
return errors.New("peer doesn't exist")
}

peerState.Groups = groups
d.peers[peerPubKey] = peerState

return nil
}

// UpdatePeerSSHHostKey updates peer's SSH host key
func (d *Status) UpdatePeerSSHHostKey(peerPubKey string, sshHostKey []byte) error {
d.mux.Lock()
Expand Down Expand Up @@ -1271,6 +1288,7 @@ func (fs FullStatus) ToProto() *proto.FullStatus {
Networks: networks,
Latency: durationpb.New(peerState.Latency),
SshHostKey: peerState.SSHHostKey,
Groups: peerState.Groups,
}
pbFullStatus.Peers = append(pbFullStatus.Peers, pbPeerState)
}
Expand Down
24 changes: 24 additions & 0 deletions client/internal/peer/status_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -247,3 +247,27 @@ func TestGetFullStatus(t *testing.T) {
assert.Equal(t, signalState, fullStatus.SignalState, "signal status should be equal")
assert.ElementsMatch(t, []State{peerState1, peerState2}, fullStatus.Peers, "peers states should match")
}

func TestUpdatePeerGroups(t *testing.T) {
key := "peerKey"
status := NewRecorder("https://mgm")
status.peers[key] = State{PubKey: key}

t.Run("update groups for existing peer", func(t *testing.T) {
groups := []string{"All", "Production"}
err := status.UpdatePeerGroups(key, groups)
assert.NoError(t, err)
assert.Equal(t, groups, status.peers[key].Groups)
})

t.Run("update groups to empty slice", func(t *testing.T) {
err := status.UpdatePeerGroups(key, []string{})
assert.NoError(t, err)
assert.Empty(t, status.peers[key].Groups)
})

t.Run("error on unknown peer", func(t *testing.T) {
err := status.UpdatePeerGroups("unknownKey", []string{"All"})
assert.Error(t, err)
})
}
13 changes: 11 additions & 2 deletions client/proto/daemon.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions client/proto/daemon.proto
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,7 @@ message PeerState {
google.protobuf.Duration latency = 17;
string relayAddress = 18;
bytes sshHostKey = 19;
repeated string groups = 20;
}

// LocalPeerState contains the latest state of the local peer
Expand Down
18 changes: 18 additions & 0 deletions client/status/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ type PeerStateDetailOutput struct {
Latency time.Duration `json:"latency" yaml:"latency"`
RosenpassEnabled bool `json:"quantumResistance" yaml:"quantumResistance"`
Networks []string `json:"networks" yaml:"networks"`
Groups []string `json:"groups" yaml:"groups"`
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

type PeersStateOutput struct {
Expand Down Expand Up @@ -337,7 +338,12 @@ func mapPeers(
Latency: pbPeerState.GetLatency().AsDuration(),
RosenpassEnabled: pbPeerState.GetRosenpassEnabled(),
Networks: pbPeerState.GetNetworks(),
Groups: pbPeerState.GetGroups(),
}
if peerState.Groups == nil {
peerState.Groups = []string{}
}
sort.Strings(peerState.Groups)

peersStateDetail = append(peersStateDetail, peerState)
}
Expand Down Expand Up @@ -645,6 +651,7 @@ func ToProtoFullStatus(fullStatus peer.FullStatus) *proto.FullStatus {
Networks: maps.Keys(peerState.GetRoutes()),
Latency: durationpb.New(peerState.Latency),
SshHostKey: peerState.SSHHostKey,
Groups: peerState.Groups,
}
pbFullStatus.Peers = append(pbFullStatus.Peers, pbPeerState)
}
Expand Down Expand Up @@ -733,6 +740,11 @@ func parsePeers(peers PeersStateOutput, rosenpassEnabled, rosenpassPermissive bo
networks = strings.Join(peerState.Networks, ", ")
}

groups := "-"
if len(peerState.Groups) > 0 {
groups = strings.Join(peerState.Groups, ", ")
}

peerString := fmt.Sprintf(
"\n %s:\n"+
" NetBird IP: %s\n"+
Expand All @@ -748,6 +760,7 @@ func parsePeers(peers PeersStateOutput, rosenpassEnabled, rosenpassPermissive bo
" Transfer status (received/sent) %s/%s\n"+
" Quantum resistance: %s\n"+
" Networks: %s\n"+
" Groups: %s\n"+
" Latency: %s\n",
domain.Domain(peerState.FQDN).SafeString(),
peerState.IP,
Expand All @@ -765,6 +778,7 @@ func parsePeers(peers PeersStateOutput, rosenpassEnabled, rosenpassPermissive bo
toIEC(peerState.TransferSent),
rosenpassEnabledStatus,
networks,
groups,
peerState.Latency.String(),
)

Expand Down Expand Up @@ -914,6 +928,10 @@ func anonymizePeerDetail(a *anonymize.Anonymizer, peer *PeerStateDetailOutput) {
for i, route := range peer.Networks {
peer.Networks[i] = a.AnonymizeRoute(route)
}

for i, group := range peer.Groups {
peer.Groups[i] = a.AnonymizeLabel(group)
}
}

func anonymizeOverview(a *anonymize.Anonymizer, overview *OutputOverview) {
Expand Down
17 changes: 15 additions & 2 deletions client/status/status_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ var resp = &proto.StatusResponse{
"10.1.0.0/24",
},
Latency: durationpb.New(time.Duration(10000000)),
Groups: []string{"Production", "All"},
},
{
IP: "192.168.178.102",
Expand All @@ -64,6 +65,7 @@ var resp = &proto.StatusResponse{
BytesRx: 2000,
BytesTx: 1000,
Latency: durationpb.New(time.Duration(10000000)),
Groups: []string{"Staging"},
},
},
ManagementState: &proto.ManagementState{
Expand Down Expand Up @@ -150,6 +152,7 @@ var overview = OutputOverview{
"10.1.0.0/24",
},
Latency: time.Duration(10000000),
Groups: []string{"All", "Production"},
},
{
IP: "192.168.178.102",
Expand All @@ -170,6 +173,7 @@ var overview = OutputOverview{
TransferReceived: 2000,
TransferSent: 1000,
Latency: time.Duration(10000000),
Groups: []string{"Staging"},
},
},
},
Expand Down Expand Up @@ -304,7 +308,8 @@ func TestParsingToJSON(t *testing.T) {
"quantumResistance": false,
"networks": [
"10.1.0.0/24"
]
],
"groups": ["All","Production"]
},
{
"fqdn": "peer-2.awesome-domain.com",
Expand All @@ -327,7 +332,8 @@ func TestParsingToJSON(t *testing.T) {
"transferSent": 1000,
"latency": 10000000,
"quantumResistance": false,
"networks": null
"networks": null,
"groups": ["Staging"]
}
]
},
Expand Down Expand Up @@ -436,6 +442,9 @@ func TestParsingToYAML(t *testing.T) {
quantumResistance: false
networks:
- 10.1.0.0/24
groups:
- All
- Production
- fqdn: peer-2.awesome-domain.com
netbirdIp: 192.168.178.102
publicKey: Pubkey2
Expand All @@ -455,6 +464,8 @@ func TestParsingToYAML(t *testing.T) {
latency: 10ms
quantumResistance: false
networks: []
groups:
- Staging
cliVersion: development
daemonVersion: 0.14.1
daemonStatus: Connected
Expand Down Expand Up @@ -535,6 +546,7 @@ func TestParsingToDetail(t *testing.T) {
Transfer status (received/sent) 200 B/100 B
Quantum resistance: false
Networks: 10.1.0.0/24
Groups: All, Production
Latency: 10ms

peer-2.awesome-domain.com:
Expand All @@ -551,6 +563,7 @@ func TestParsingToDetail(t *testing.T) {
Transfer status (received/sent) 2.0 KiB/1000 B
Quantum resistance: false
Networks: -
Groups: Staging
Latency: 10ms

Events: No events recorded
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,8 @@ func (c *Controller) sendUpdateAccountPeers(ctx context.Context, accountID strin
remotePeerNetworkMap.Merge(proxyNetworkMap)
}

remotePeerNetworkMap.PopulatePeerGroupsNames(account)

peerGroups := account.GetPeerGroups(p.ID)
start = time.Now()
update := grpc.ToSyncResponse(ctx, nil, c.config.HttpConfig, c.config.DeviceAuthorizationFlow, p, nil, nil, remotePeerNetworkMap, dnsDomain, postureChecks, dnsCache, account.Settings, extraSetting, maps.Keys(peerGroups), dnsFwdPort)
Expand Down Expand Up @@ -392,6 +394,8 @@ func (c *Controller) UpdateAccountPeer(ctx context.Context, accountId string, pe
return fmt.Errorf("failed to get extra settings: %v", err)
}

remotePeerNetworkMap.PopulatePeerGroupsNames(account)

peerGroups := account.GetPeerGroups(peerId)
dnsFwdPort := computeForwarderPort(maps.Values(account.Peers), network_map.DnsForwarderPortMinVersion)

Expand Down
Loading