From 5404bf7842ac92df74e4a3381f56480172f1f75f Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Mon, 15 Jan 2024 10:52:18 +0300 Subject: [PATCH 01/12] Apply posture checks to network map generation --- management/server/policy.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/management/server/policy.go b/management/server/policy.go index 294d699c796..8fa1363925a 100644 --- a/management/server/policy.go +++ b/management/server/policy.go @@ -11,6 +11,7 @@ import ( "github.com/netbirdio/netbird/management/proto" "github.com/netbirdio/netbird/management/server/activity" nbpeer "github.com/netbirdio/netbird/management/server/peer" + "github.com/netbirdio/netbird/management/server/posture" "github.com/netbirdio/netbird/management/server/status" ) @@ -219,6 +220,25 @@ func (a *Account) getPeerConnectionResources(peerID string) ([]*nbpeer.Peer, []* continue } + peer, ok := a.Peers[peerID] + if !ok && peer == nil { + continue + } + + for _, postureChecksID := range policy.SourcePostureChecks { + postureChecks := getPostureCheck(a, postureChecksID) + if postureChecks == nil { + continue + } + + for _, check := range postureChecks.Checks { + if err := check.Check(*peer); err != nil { + log.Debugf("an error occurred on check %s: %s", check.Name(), err.Error()) + continue + } + } + } + for _, rule := range policy.Rules { if !rule.Enabled { continue @@ -512,3 +532,12 @@ func getAllPeersFromGroups(account *Account, groups []string, peerID string) ([] } return filteredPeers, peerInGroups } + +func getPostureCheck(account *Account, postureChecksID string) *posture.Checks { + for _, postureChecks := range account.PostureChecks { + if postureChecks.ID == postureChecksID { + return postureChecks + } + } + return nil +} From 1f18292d6771a4db73744a305a06124e4c1f4ed0 Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Tue, 16 Jan 2024 13:26:25 +0300 Subject: [PATCH 02/12] run policy posture checks on peers to connect --- management/server/policy.go | 55 +++++++++++++++++++++++++------------ 1 file changed, 37 insertions(+), 18 deletions(-) diff --git a/management/server/policy.go b/management/server/policy.go index 8fa1363925a..1e1374a2017 100644 --- a/management/server/policy.go +++ b/management/server/policy.go @@ -2,6 +2,7 @@ package server import ( _ "embed" + "fmt" "strconv" "strings" @@ -220,23 +221,11 @@ func (a *Account) getPeerConnectionResources(peerID string) ([]*nbpeer.Peer, []* continue } - peer, ok := a.Peers[peerID] - if !ok && peer == nil { - continue - } - - for _, postureChecksID := range policy.SourcePostureChecks { - postureChecks := getPostureCheck(a, postureChecksID) - if postureChecks == nil { - continue - } - - for _, check := range postureChecks.Checks { - if err := check.Check(*peer); err != nil { - log.Debugf("an error occurred on check %s: %s", check.Name(), err.Error()) - continue - } - } + // if peer validation fails, the peer should not be able to connect to the policy peer's + // we return an empty list of peers and firewall rule for that policy + err := a.validatePostureChecksOnPeer(policy.SourcePostureChecks, peerID) + if err != nil { + return nil, nil } for _, rule := range policy.Rules { @@ -294,6 +283,14 @@ func (a *Account) connResourcesGenerator() (func(*PolicyRule, []*nbpeer.Peer, in if peer == nil { continue } + + for _, policy := range a.Policies { + err := a.validatePostureChecksOnPeer(policy.SourcePostureChecks, peer.ID) + if err != nil { + continue + } + } + if _, ok := peersExists[peer.ID]; !ok { peers = append(peers, peer) peersExists[peer.ID] = struct{}{} @@ -533,7 +530,29 @@ func getAllPeersFromGroups(account *Account, groups []string, peerID string) ([] return filteredPeers, peerInGroups } -func getPostureCheck(account *Account, postureChecksID string) *posture.Checks { +func (a *Account) validatePostureChecksOnPeer(sourcePostureChecksID []string, peerID string) error { + peer, ok := a.Peers[peerID] + if !ok && peer == nil { + return fmt.Errorf("peer %s does not exists", peerID) + } + + for _, postureChecksID := range sourcePostureChecksID { + postureChecks := getPostureChecks(a, postureChecksID) + if postureChecks == nil { + continue + } + + for _, check := range postureChecks.Checks { + if err := check.Check(*peer); err != nil { + return fmt.Errorf("an error occurred on check %s: %s", check.Name(), err.Error()) + } + } + } + + return nil +} + +func getPostureChecks(account *Account, postureChecksID string) *posture.Checks { for _, postureChecks := range account.PostureChecks { if postureChecks.ID == postureChecksID { return postureChecks From 3c88ef543c62092aaed0dfc220e8808ea91056e0 Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Tue, 16 Jan 2024 14:35:32 +0300 Subject: [PATCH 03/12] Refactor and streamline policy posture check process for peers to connect. --- management/server/policy.go | 38 ++++++++++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/management/server/policy.go b/management/server/policy.go index 1e1374a2017..20232d1f5c9 100644 --- a/management/server/policy.go +++ b/management/server/policy.go @@ -278,19 +278,14 @@ func (a *Account) connResourcesGenerator() (func(*PolicyRule, []*nbpeer.Peer, in } return func(rule *PolicyRule, groupPeers []*nbpeer.Peer, direction int) { - isAll := (len(all.Peers) - 1) == len(groupPeers) - for _, peer := range groupPeers { + validGroupPeers := a.getValidatedPeersByPostureChecks(groupPeers) + + isAll := (len(all.Peers) - 1) == len(validGroupPeers) + for _, peer := range validGroupPeers { if peer == nil { continue } - for _, policy := range a.Policies { - err := a.validatePostureChecksOnPeer(policy.SourcePostureChecks, peer.ID) - if err != nil { - continue - } - } - if _, ok := peersExists[peer.ID]; !ok { peers = append(peers, peer) peersExists[peer.ID] = struct{}{} @@ -530,6 +525,7 @@ func getAllPeersFromGroups(account *Account, groups []string, peerID string) ([] return filteredPeers, peerInGroups } +// validatePostureChecksOnPeer validates the posture checks on a peer func (a *Account) validatePostureChecksOnPeer(sourcePostureChecksID []string, peerID string) error { peer, ok := a.Peers[peerID] if !ok && peer == nil { @@ -552,6 +548,30 @@ func (a *Account) validatePostureChecksOnPeer(sourcePostureChecksID []string, pe return nil } +// getValidatedPeersByPostureChecks returns a slice of valid peers based on applied policy posture checks +func (a *Account) getValidatedPeersByPostureChecks(groupPeers []*nbpeer.Peer) []*nbpeer.Peer { + validPeers := make([]*nbpeer.Peer, 0) + for _, peer := range groupPeers { + if peer == nil { + continue + } + + isValidPeer := true + for _, policy := range a.Policies { + err := a.validatePostureChecksOnPeer(policy.SourcePostureChecks, peer.ID) + if err != nil { + isValidPeer = false + break + } + } + + if isValidPeer && peer != nil { + validPeers = append(validPeers, peer) + } + } + return validPeers +} + func getPostureChecks(account *Account, postureChecksID string) *posture.Checks { for _, postureChecks := range account.PostureChecks { if postureChecks.ID == postureChecksID { From 5c924e5e50765c12efda459be2b452ee1e4c817d Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Tue, 16 Jan 2024 14:36:23 +0300 Subject: [PATCH 04/12] Add posture checks testing in a network map --- management/server/policy_test.go | 207 +++++++++++++++++++++++++++++++ 1 file changed, 207 insertions(+) diff --git a/management/server/policy_test.go b/management/server/policy_test.go index 715e2a8614e..053354ddb95 100644 --- a/management/server/policy_test.go +++ b/management/server/policy_test.go @@ -9,6 +9,7 @@ import ( "golang.org/x/exp/slices" nbpeer "github.com/netbirdio/netbird/management/server/peer" + "github.com/netbirdio/netbird/management/server/posture" ) func TestAccount_getPeersByPolicy(t *testing.T) { @@ -443,6 +444,212 @@ func TestAccount_getPeersByPolicyDirect(t *testing.T) { }) } +func TestAccount_getPeersByPolicyPostureChecks(t *testing.T) { + account := &Account{ + Peers: map[string]*nbpeer.Peer{ + "peerA": { + ID: "peerA", + IP: net.ParseIP("100.65.14.88"), + Status: &nbpeer.PeerStatus{}, + Meta: nbpeer.PeerSystemMeta{ + WtVersion: "0.25.9", + }, + }, + "peerB": { + ID: "peerB", + IP: net.ParseIP("100.65.80.39"), + Status: &nbpeer.PeerStatus{}, + Meta: nbpeer.PeerSystemMeta{ + WtVersion: "0.23.0", + }, + }, + "peerC": { + ID: "peerC", + IP: net.ParseIP("100.65.254.139"), + Status: &nbpeer.PeerStatus{}, + Meta: nbpeer.PeerSystemMeta{ + WtVersion: "0.25.8", + }, + }, + "peerD": { + ID: "peerD", + IP: net.ParseIP("100.65.62.5"), + Status: &nbpeer.PeerStatus{}, + Meta: nbpeer.PeerSystemMeta{ + WtVersion: "0.25.9", + }, + }, + "peerE": { + ID: "peerE", + IP: net.ParseIP("100.65.32.206"), + Status: &nbpeer.PeerStatus{}, + Meta: nbpeer.PeerSystemMeta{ + WtVersion: "0.24.0", + }, + }, + "peerF": { + ID: "peerF", + IP: net.ParseIP("100.65.250.202"), + Status: &nbpeer.PeerStatus{}, + Meta: nbpeer.PeerSystemMeta{ + WtVersion: "0.25.9", + }, + }, + "peerG": { + ID: "peerG", + IP: net.ParseIP("100.65.13.186"), + Status: &nbpeer.PeerStatus{}, + Meta: nbpeer.PeerSystemMeta{ + WtVersion: "0.23.2", + }, + }, + "peerH": { + ID: "peerH", + IP: net.ParseIP("100.65.29.55"), + Status: &nbpeer.PeerStatus{}, + Meta: nbpeer.PeerSystemMeta{ + WtVersion: "0.25.5", + }, + }, + }, + Groups: map[string]*Group{ + "GroupAll": { + ID: "GroupAll", + Name: "All", + Peers: []string{ + "peerB", + "peerA", + "peerD", + "peerC", + "peerE", + "peerF", + "peerG", + "peerH", + }, + }, + }, + Rules: map[string]*Rule{ + "RuleDefault": { + ID: "RuleDefault", + Name: "Default", + Description: "This is a default rule that allows connections between all the resources", + Source: []string{ + "GroupAll", + }, + Destination: []string{ + "GroupAll", + }, + }, + }, + PostureChecks: []*posture.Checks{ + { + ID: "PostureChecksDefault", + Name: "Default", + Description: "This is a posture checks that check if peer is running required version", + Checks: []posture.Check{ + &posture.NBVersionCheck{ + MinVersion: "0.25", + }, + }, + }, + }, + } + + policy, err := RuleToPolicy(account.Rules["RuleDefault"]) + assert.NoError(t, err) + policy.SourcePostureChecks = []string{"PostureChecksDefault"} + + account.Policies = append(account.Policies, policy) + + account.Policies = append(account.Policies, &Policy{ + ID: "PolicyPostureChecks", + Name: "", + Description: "This is the policy with posture checks applied", + Enabled: true, + SourcePostureChecks: []string{ + "PostureChecksDefault", + }, + }) + + t.Run("check peer's map details with posture checks", func(t *testing.T) { + peers, firewallRules := account.getPeerConnectionResources("peerB") + assert.Len(t, peers, 0) + assert.Len(t, firewallRules, 0) + + peers, firewallRules = account.getPeerConnectionResources("peerA") + assert.Len(t, peers, 4) + assert.Contains(t, peers, account.Peers["peerC"]) + assert.Contains(t, peers, account.Peers["peerD"]) + assert.Contains(t, peers, account.Peers["peerF"]) + assert.Contains(t, peers, account.Peers["peerH"]) + + expectedFirewallRules := []*FirewallRule{ + { + PeerIP: "100.65.254.139", + Direction: firewallRuleDirectionOUT, + Action: "accept", + Protocol: "all", + Port: "", + }, + { + PeerIP: "100.65.254.139", + Direction: firewallRuleDirectionIN, + Action: "accept", + Protocol: "all", + Port: "", + }, + { + PeerIP: "100.65.62.5", + Direction: firewallRuleDirectionOUT, + Action: "accept", + Protocol: "all", + Port: "", + }, + { + PeerIP: "100.65.62.5", + Direction: firewallRuleDirectionIN, + Action: "accept", + Protocol: "all", + Port: "", + }, + { + PeerIP: "100.65.250.202", + Direction: firewallRuleDirectionOUT, + Action: "accept", + Protocol: "all", + Port: "", + }, + { + PeerIP: "100.65.250.202", + Direction: firewallRuleDirectionIN, + Action: "accept", + Protocol: "all", + Port: "", + }, + { + PeerIP: "100.65.29.55", + Direction: firewallRuleDirectionOUT, + Action: "accept", + Protocol: "all", + Port: "", + }, + { + PeerIP: "100.65.29.55", + Direction: firewallRuleDirectionIN, + Action: "accept", + Protocol: "all", + Port: "", + }, + } + assert.Len(t, firewallRules, len(expectedFirewallRules)) + slices.SortFunc(expectedFirewallRules, sortFunc()) + slices.SortFunc(firewallRules, sortFunc()) + for i := range firewallRules { + assert.Equal(t, expectedFirewallRules[i], firewallRules[i]) + } + }) +} + func sortFunc() func(a *FirewallRule, b *FirewallRule) int { return func(a, b *FirewallRule) int { // Concatenate PeerIP and Direction as string for comparison From 524ed6a7c5dc3a0b92833a18fb9defb830bfe3a3 Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Tue, 16 Jan 2024 14:37:58 +0300 Subject: [PATCH 05/12] Remove redundant nil check in policy.go --- management/server/policy.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/management/server/policy.go b/management/server/policy.go index 20232d1f5c9..7f623496e92 100644 --- a/management/server/policy.go +++ b/management/server/policy.go @@ -282,9 +282,6 @@ func (a *Account) connResourcesGenerator() (func(*PolicyRule, []*nbpeer.Peer, in isAll := (len(all.Peers) - 1) == len(validGroupPeers) for _, peer := range validGroupPeers { - if peer == nil { - continue - } if _, ok := peersExists[peer.ID]; !ok { peers = append(peers, peer) From 45297b9df031d295523ee3a2ce15c023497b6d9d Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Tue, 16 Jan 2024 14:46:14 +0300 Subject: [PATCH 06/12] Refactor peer validation check in policy.go --- management/server/policy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/management/server/policy.go b/management/server/policy.go index 7f623496e92..ce09db08e11 100644 --- a/management/server/policy.go +++ b/management/server/policy.go @@ -562,7 +562,7 @@ func (a *Account) getValidatedPeersByPostureChecks(groupPeers []*nbpeer.Peer) [] } } - if isValidPeer && peer != nil { + if isValidPeer { validPeers = append(validPeers, peer) } } From d0e47434374e01f078cdb5c700858e75a209aa00 Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Wed, 17 Jan 2024 20:12:51 +0300 Subject: [PATCH 07/12] Update 'Check' function signature and use logger for version check --- management/server/posture/checks.go | 2 +- management/server/posture/version.go | 15 ++++++++------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/management/server/posture/checks.go b/management/server/posture/checks.go index a585836d16b..f9722594466 100644 --- a/management/server/posture/checks.go +++ b/management/server/posture/checks.go @@ -12,7 +12,7 @@ const ( // Check represents an interface for performing a check on a peer. type Check interface { - Check(peer nbpeer.Peer) error + Check(peer nbpeer.Peer) (bool, error) Name() string } diff --git a/management/server/posture/version.go b/management/server/posture/version.go index 46672a967df..1fe0aa57389 100644 --- a/management/server/posture/version.go +++ b/management/server/posture/version.go @@ -1,9 +1,8 @@ package posture import ( - "fmt" - "github.com/hashicorp/go-version" + log "github.com/sirupsen/logrus" nbpeer "github.com/netbirdio/netbird/management/server/peer" ) @@ -14,25 +13,27 @@ type NBVersionCheck struct { var _ Check = (*NBVersionCheck)(nil) -func (n *NBVersionCheck) Check(peer nbpeer.Peer) error { +func (n *NBVersionCheck) Check(peer nbpeer.Peer) (bool, error) { peerNBVersion, err := version.NewVersion(peer.Meta.WtVersion) if err != nil { - return err + return false, err } constraints, err := version.NewConstraint(">= " + n.MinVersion) if err != nil { - return err + return false, err } if constraints.Check(peerNBVersion) { - return nil + return true, nil } - return fmt.Errorf("peer NB version %s is older than minimum allowed version %s", + log.Debugf("peer %s NB version %s is older than minimum allowed version %s", + peer.ID, peer.Meta.UIVersion, n.MinVersion, ) + return false, nil } func (n *NBVersionCheck) Name() string { From 21134024883ca16740392ebae6bf6b97757ff4ed Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Wed, 17 Jan 2024 20:13:42 +0300 Subject: [PATCH 08/12] Refactor posture checks run on sources and updated the validation func --- management/server/policy.go | 70 +++++++++++++------------------------ 1 file changed, 25 insertions(+), 45 deletions(-) diff --git a/management/server/policy.go b/management/server/policy.go index ce09db08e11..92d1e11802c 100644 --- a/management/server/policy.go +++ b/management/server/policy.go @@ -2,7 +2,6 @@ package server import ( _ "embed" - "fmt" "strconv" "strings" @@ -221,20 +220,13 @@ func (a *Account) getPeerConnectionResources(peerID string) ([]*nbpeer.Peer, []* continue } - // if peer validation fails, the peer should not be able to connect to the policy peer's - // we return an empty list of peers and firewall rule for that policy - err := a.validatePostureChecksOnPeer(policy.SourcePostureChecks, peerID) - if err != nil { - return nil, nil - } - for _, rule := range policy.Rules { if !rule.Enabled { continue } - sourcePeers, peerInSources := getAllPeersFromGroups(a, rule.Sources, peerID) - destinationPeers, peerInDestinations := getAllPeersFromGroups(a, rule.Destinations, peerID) + sourcePeers, peerInSources := getAllPeersFromGroups(a, rule.Sources, peerID, policy.SourcePostureChecks) + destinationPeers, peerInDestinations := getAllPeersFromGroups(a, rule.Destinations, peerID, nil) sourcePeers = additions.ValidatePeers(sourcePeers) destinationPeers = additions.ValidatePeers(destinationPeers) @@ -278,10 +270,11 @@ func (a *Account) connResourcesGenerator() (func(*PolicyRule, []*nbpeer.Peer, in } return func(rule *PolicyRule, groupPeers []*nbpeer.Peer, direction int) { - validGroupPeers := a.getValidatedPeersByPostureChecks(groupPeers) - - isAll := (len(all.Peers) - 1) == len(validGroupPeers) - for _, peer := range validGroupPeers { + isAll := (len(all.Peers) - 1) == len(groupPeers) + for _, peer := range groupPeers { + if peer == nil { + continue + } if _, ok := peersExists[peer.ID]; !ok { peers = append(peers, peer) @@ -495,8 +488,9 @@ func toProtocolFirewallRules(update []*FirewallRule) []*proto.FirewallRule { // getAllPeersFromGroups for given peer ID and list of groups // -// Returns list of peers and boolean indicating if peer is in any of the groups -func getAllPeersFromGroups(account *Account, groups []string, peerID string) ([]*nbpeer.Peer, bool) { +// Returns list of peers from the provided groups that pass the posture checks +// if the sourcePostureChecksIDs is set. +func getAllPeersFromGroups(account *Account, groups []string, peerID string, sourcePostureChecksIDs []string) ([]*nbpeer.Peer, bool) { peerInGroups := false filteredPeers := make([]*nbpeer.Peer, 0, len(groups)) for _, g := range groups { @@ -511,6 +505,12 @@ func getAllPeersFromGroups(account *Account, groups []string, peerID string) ([] continue } + // validate the peer + isValid := account.validatePostureChecksOnPeer(sourcePostureChecksIDs, peer.ID) + if !isValid { + continue + } + if peer.ID == peerID { peerInGroups = true continue @@ -523,10 +523,10 @@ func getAllPeersFromGroups(account *Account, groups []string, peerID string) ([] } // validatePostureChecksOnPeer validates the posture checks on a peer -func (a *Account) validatePostureChecksOnPeer(sourcePostureChecksID []string, peerID string) error { +func (a *Account) validatePostureChecksOnPeer(sourcePostureChecksID []string, peerID string) bool { peer, ok := a.Peers[peerID] if !ok && peer == nil { - return fmt.Errorf("peer %s does not exists", peerID) + return false } for _, postureChecksID := range sourcePostureChecksID { @@ -536,37 +536,17 @@ func (a *Account) validatePostureChecksOnPeer(sourcePostureChecksID []string, pe } for _, check := range postureChecks.Checks { - if err := check.Check(*peer); err != nil { - return fmt.Errorf("an error occurred on check %s: %s", check.Name(), err.Error()) - } - } - } - - return nil -} - -// getValidatedPeersByPostureChecks returns a slice of valid peers based on applied policy posture checks -func (a *Account) getValidatedPeersByPostureChecks(groupPeers []*nbpeer.Peer) []*nbpeer.Peer { - validPeers := make([]*nbpeer.Peer, 0) - for _, peer := range groupPeers { - if peer == nil { - continue - } - - isValidPeer := true - for _, policy := range a.Policies { - err := a.validatePostureChecksOnPeer(policy.SourcePostureChecks, peer.ID) - if err != nil { - isValidPeer = false - break + isValid, err := check.Check(*peer) + if !isValid { + if err != nil { + log.Debugf("an error occured check %s: on peer: %s :%s", check.Name(), peer.ID, err.Error()) + } + return false } - } - if isValidPeer { - validPeers = append(validPeers, peer) } } - return validPeers + return true } func getPostureChecks(account *Account, postureChecksID string) *posture.Checks { From 1d6fe405e12f028d4dfa4f77d3ff0bf8ce048971 Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Wed, 17 Jan 2024 20:37:04 +0300 Subject: [PATCH 09/12] Update peer validation --- management/server/policy.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/management/server/policy.go b/management/server/policy.go index 92d1e11802c..cc463b0bbd8 100644 --- a/management/server/policy.go +++ b/management/server/policy.go @@ -488,8 +488,11 @@ func toProtocolFirewallRules(update []*FirewallRule) []*proto.FirewallRule { // getAllPeersFromGroups for given peer ID and list of groups // -// Returns list of peers from the provided groups that pass the posture checks -// if the sourcePostureChecksIDs is set. +// Returns a list of peers from specified groups that pass specified posture checks +// and a boolean indicating if the supplied peer ID exists within these groups. +// +// Important: Posture checks are applicable only to source group peers, +// for destination group peers, call this method with an empty list of sourcePostureChecksIDs func getAllPeersFromGroups(account *Account, groups []string, peerID string, sourcePostureChecksIDs []string) ([]*nbpeer.Peer, bool) { peerInGroups := false filteredPeers := make([]*nbpeer.Peer, 0, len(groups)) @@ -505,7 +508,7 @@ func getAllPeersFromGroups(account *Account, groups []string, peerID string, sou continue } - // validate the peer + // validate the peer based on policy posture checks applied isValid := account.validatePostureChecksOnPeer(sourcePostureChecksIDs, peer.ID) if !isValid { continue From af37b46a2ace1162b2b0b7e3f299c0817d096b33 Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Wed, 17 Jan 2024 20:44:34 +0300 Subject: [PATCH 10/12] fix tests --- management/server/policy.go | 2 +- management/server/posture/version_test.go | 14 +++++++++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/management/server/policy.go b/management/server/policy.go index cc463b0bbd8..d3741539338 100644 --- a/management/server/policy.go +++ b/management/server/policy.go @@ -542,7 +542,7 @@ func (a *Account) validatePostureChecksOnPeer(sourcePostureChecksID []string, pe isValid, err := check.Check(*peer) if !isValid { if err != nil { - log.Debugf("an error occured check %s: on peer: %s :%s", check.Name(), peer.ID, err.Error()) + log.Debugf("an error occurred check %s: on peer: %s :%s", check.Name(), peer.ID, err.Error()) } return false } diff --git a/management/server/posture/version_test.go b/management/server/posture/version_test.go index c590fd2aeb3..de51c2283b1 100644 --- a/management/server/posture/version_test.go +++ b/management/server/posture/version_test.go @@ -14,6 +14,7 @@ func TestNBVersionCheck_Check(t *testing.T) { input peer.Peer check NBVersionCheck wantErr bool + isValid bool }{ { name: "Valid Peer NB version", @@ -26,6 +27,7 @@ func TestNBVersionCheck_Check(t *testing.T) { MinVersion: "1.0.0", }, wantErr: false, + isValid: true, }, { name: "Valid Peer NB version With No Patch Version 1", @@ -38,6 +40,7 @@ func TestNBVersionCheck_Check(t *testing.T) { MinVersion: "2.0", }, wantErr: false, + isValid: true, }, { name: "Valid Peer NB version With No Patch Version 2", @@ -50,6 +53,7 @@ func TestNBVersionCheck_Check(t *testing.T) { MinVersion: "2.0", }, wantErr: false, + isValid: true, }, { name: "Older Peer NB version", @@ -61,7 +65,8 @@ func TestNBVersionCheck_Check(t *testing.T) { check: NBVersionCheck{ MinVersion: "1.0.0", }, - wantErr: true, + wantErr: false, + isValid: false, }, { name: "Older Peer NB version With Patch Version", @@ -73,7 +78,8 @@ func TestNBVersionCheck_Check(t *testing.T) { check: NBVersionCheck{ MinVersion: "0.2", }, - wantErr: true, + wantErr: false, + isValid: false, }, { name: "Invalid Peer NB version", @@ -86,17 +92,19 @@ func TestNBVersionCheck_Check(t *testing.T) { MinVersion: "1.0.0", }, wantErr: true, + isValid: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := tt.check.Check(tt.input) + isValid, err := tt.check.Check(tt.input) if tt.wantErr { assert.Error(t, err) } else { assert.NoError(t, err) } + assert.Equal(t, tt.isValid, isValid) }) } } From 3f240539cd0b3661ebbf1222413789bb9735c910 Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Thu, 18 Jan 2024 13:44:26 +0300 Subject: [PATCH 11/12] improved test coverage for policy posture check --- management/server/policy_test.go | 179 +++++++++++++++++++++---------- 1 file changed, 123 insertions(+), 56 deletions(-) diff --git a/management/server/policy_test.go b/management/server/policy_test.go index 053354ddb95..8076e9242dd 100644 --- a/management/server/policy_test.go +++ b/management/server/policy_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "golang.org/x/exp/slices" nbpeer "github.com/netbirdio/netbird/management/server/peer" @@ -508,7 +509,7 @@ func TestAccount_getPeersByPolicyPostureChecks(t *testing.T) { IP: net.ParseIP("100.65.29.55"), Status: &nbpeer.PeerStatus{}, Meta: nbpeer.PeerSystemMeta{ - WtVersion: "0.25.5", + WtVersion: "0.23.1", }, }, }, @@ -521,23 +522,21 @@ func TestAccount_getPeersByPolicyPostureChecks(t *testing.T) { "peerA", "peerD", "peerC", - "peerE", "peerF", "peerG", "peerH", }, }, - }, - Rules: map[string]*Rule{ - "RuleDefault": { - ID: "RuleDefault", - Name: "Default", - Description: "This is a default rule that allows connections between all the resources", - Source: []string{ - "GroupAll", - }, - Destination: []string{ - "GroupAll", + "GroupSwarm": { + ID: "GroupSwarm", + Name: "swarm", + Peers: []string{ + "peerB", + "peerA", + "peerD", + "peerE", + "peerG", + "peerH", }, }, }, @@ -555,93 +554,161 @@ func TestAccount_getPeersByPolicyPostureChecks(t *testing.T) { }, } - policy, err := RuleToPolicy(account.Rules["RuleDefault"]) - assert.NoError(t, err) - policy.SourcePostureChecks = []string{"PostureChecksDefault"} - - account.Policies = append(account.Policies, policy) - account.Policies = append(account.Policies, &Policy{ ID: "PolicyPostureChecks", Name: "", Description: "This is the policy with posture checks applied", Enabled: true, + Rules: []*PolicyRule{ + { + ID: "RuleSwarm", + Name: "Swarm", + Enabled: true, + Action: PolicyTrafficActionAccept, + Destinations: []string{ + "GroupSwarm", + }, + Sources: []string{ + "GroupAll", + }, + Bidirectional: false, + Protocol: PolicyRuleProtocolTCP, + Ports: []string{"80"}, + }, + }, SourcePostureChecks: []string{ "PostureChecksDefault", }, }) - t.Run("check peer's map details with posture checks", func(t *testing.T) { + t.Run("verify peer's network map with default group peer list", func(t *testing.T) { + // peerB doesn't fulfill the NB posture check but is included in the destination group Swarm, + // will establish a connection with all source peers satisfying the NB posture check. peers, firewallRules := account.getPeerConnectionResources("peerB") - assert.Len(t, peers, 0) - assert.Len(t, firewallRules, 0) - - peers, firewallRules = account.getPeerConnectionResources("peerA") assert.Len(t, peers, 4) + assert.Len(t, firewallRules, 4) + assert.Contains(t, peers, account.Peers["peerA"]) assert.Contains(t, peers, account.Peers["peerC"]) assert.Contains(t, peers, account.Peers["peerD"]) assert.Contains(t, peers, account.Peers["peerF"]) - assert.Contains(t, peers, account.Peers["peerH"]) + // peerC satisfy the NB posture check, should establish connection to all destination group peer's + // We expect a single permissive firewall rule which all outgoing connections + peers, firewallRules = account.getPeerConnectionResources("peerC") + assert.Len(t, peers, len(account.Groups["GroupSwarm"].Peers)) + assert.Len(t, firewallRules, 1) expectedFirewallRules := []*FirewallRule{ { - PeerIP: "100.65.254.139", + PeerIP: "0.0.0.0", Direction: firewallRuleDirectionOUT, Action: "accept", - Protocol: "all", - Port: "", - }, - { - PeerIP: "100.65.254.139", - Direction: firewallRuleDirectionIN, - Action: "accept", - Protocol: "all", - Port: "", + Protocol: "tcp", + Port: "80", }, + } + assert.ElementsMatch(t, firewallRules, expectedFirewallRules) + + // peerE doesn't fulfill the NB posture check and exists in only destination group Swarm, + // all source group peers satisfying the NB posture check should establish connection + peers, firewallRules = account.getPeerConnectionResources("peerE") + assert.Len(t, peers, 4) + assert.Len(t, firewallRules, 4) + assert.Contains(t, peers, account.Peers["peerA"]) + assert.Contains(t, peers, account.Peers["peerC"]) + assert.Contains(t, peers, account.Peers["peerD"]) + assert.Contains(t, peers, account.Peers["peerF"]) + }) + + t.Run("verify peer's network map with modified group peer list", func(t *testing.T) { + // Removing peerB as the part of destination group Swarm + account.Groups["GroupSwarm"].Peers = []string{"peerA", "peerD", "peerE", "peerG", "peerH"} + + // peerB doesn't satisfy the NB posture check, and doesn't exist in destination group peer's + // no connection should be established to any peer of destination group + peers, firewallRules := account.getPeerConnectionResources("peerB") + assert.Len(t, peers, 0) + assert.Len(t, firewallRules, 0) + + // peerC satisfy the NB posture check, should establish connection to all destination group peer's + // We expect a single permissive firewall rule which all outgoing connections + peers, firewallRules = account.getPeerConnectionResources("peerC") + assert.Len(t, peers, len(account.Groups["GroupSwarm"].Peers)) + assert.Len(t, firewallRules, len(account.Groups["GroupSwarm"].Peers)) + + peerIDs := make([]string, 0, len(peers)) + for _, peer := range peers { + peerIDs = append(peerIDs, peer.ID) + } + assert.ElementsMatch(t, peerIDs, account.Groups["GroupSwarm"].Peers) + + // Removing peerF as the part of source group All + account.Groups["GroupAll"].Peers = []string{"peerB", "peerA", "peerD", "peerC", "peerG", "peerH"} + + // peerE doesn't fulfill the NB posture check and exists in only destination group Swarm, + // all source group peers satisfying the NB posture check should establish connection + peers, firewallRules = account.getPeerConnectionResources("peerE") + assert.Len(t, peers, 3) + assert.Len(t, firewallRules, 3) + assert.Contains(t, peers, account.Peers["peerA"]) + assert.Contains(t, peers, account.Peers["peerC"]) + assert.Contains(t, peers, account.Peers["peerD"]) + + peers, firewallRules = account.getPeerConnectionResources("peerA") + assert.Len(t, peers, 5) + // assert peers from Group Swarm + assert.Contains(t, peers, account.Peers["peerD"]) + assert.Contains(t, peers, account.Peers["peerE"]) + assert.Contains(t, peers, account.Peers["peerG"]) + assert.Contains(t, peers, account.Peers["peerH"]) + + // assert peers from Group All + assert.Contains(t, peers, account.Peers["peerC"]) + + expectedFirewallRules := []*FirewallRule{ { PeerIP: "100.65.62.5", Direction: firewallRuleDirectionOUT, Action: "accept", - Protocol: "all", - Port: "", + Protocol: "tcp", + Port: "80", }, { - PeerIP: "100.65.62.5", - Direction: firewallRuleDirectionIN, + PeerIP: "100.65.32.206", + Direction: firewallRuleDirectionOUT, Action: "accept", - Protocol: "all", - Port: "", + Protocol: "tcp", + Port: "80", }, { - PeerIP: "100.65.250.202", + PeerIP: "100.65.13.186", Direction: firewallRuleDirectionOUT, Action: "accept", - Protocol: "all", - Port: "", + Protocol: "tcp", + Port: "80", }, { - PeerIP: "100.65.250.202", - Direction: firewallRuleDirectionIN, + PeerIP: "100.65.29.55", + Direction: firewallRuleDirectionOUT, Action: "accept", - Protocol: "all", - Port: "", + Protocol: "tcp", + Port: "80", }, { - PeerIP: "100.65.29.55", - Direction: firewallRuleDirectionOUT, + PeerIP: "100.65.254.139", + Direction: firewallRuleDirectionIN, Action: "accept", - Protocol: "all", - Port: "", + Protocol: "tcp", + Port: "80", }, { - PeerIP: "100.65.29.55", + PeerIP: "100.65.62.5", Direction: firewallRuleDirectionIN, Action: "accept", - Protocol: "all", - Port: "", + Protocol: "tcp", + Port: "80", }, } - assert.Len(t, firewallRules, len(expectedFirewallRules)) + require.Len(t, firewallRules, len(expectedFirewallRules)) slices.SortFunc(expectedFirewallRules, sortFunc()) slices.SortFunc(firewallRules, sortFunc()) for i := range firewallRules { From f27e918b841e6fb55f465f9e0c005920308ae085 Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Thu, 18 Jan 2024 13:56:53 +0300 Subject: [PATCH 12/12] Refactoring --- management/server/policy.go | 7 +++---- management/server/policy_test.go | 9 ++------- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/management/server/policy.go b/management/server/policy.go index d3741539338..050e8d60d7f 100644 --- a/management/server/policy.go +++ b/management/server/policy.go @@ -540,13 +540,12 @@ func (a *Account) validatePostureChecksOnPeer(sourcePostureChecksID []string, pe for _, check := range postureChecks.Checks { isValid, err := check.Check(*peer) + if err != nil { + log.Debugf("an error occurred check %s: on peer: %s :%s", check.Name(), peer.ID, err.Error()) + } if !isValid { - if err != nil { - log.Debugf("an error occurred check %s: on peer: %s :%s", check.Name(), peer.ID, err.Error()) - } return false } - } } return true diff --git a/management/server/policy_test.go b/management/server/policy_test.go index 8076e9242dd..d80841bde96 100644 --- a/management/server/policy_test.go +++ b/management/server/policy_test.go @@ -6,7 +6,6 @@ import ( "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" "golang.org/x/exp/slices" nbpeer "github.com/netbirdio/netbird/management/server/peer" @@ -708,12 +707,8 @@ func TestAccount_getPeersByPolicyPostureChecks(t *testing.T) { Port: "80", }, } - require.Len(t, firewallRules, len(expectedFirewallRules)) - slices.SortFunc(expectedFirewallRules, sortFunc()) - slices.SortFunc(firewallRules, sortFunc()) - for i := range firewallRules { - assert.Equal(t, expectedFirewallRules[i], firewallRules[i]) - } + assert.Len(t, firewallRules, len(expectedFirewallRules)) + assert.ElementsMatch(t, firewallRules, expectedFirewallRules) }) }