W.I.P Exposure analysis with anp #486

shireenf-ibm · 2025-01-21T19:32:18Z

No description provided.

…rt_admin_netpolicy

…ll but: udp 5353 instead of SCTP 1-65535,TCP 1-65535,UDP 1-5352,5354-65535)

…t) for connectionSet

…licies in the input resources

…h_anp

…cluding some peers (no peer resolution)

…onnection. In case of representative-peer adding this "no conns" to the output to see the exception

adisos

initial comments/questions, did not review all files yet

adisos · 2025-01-26T18:18:44Z

pkg/netpol/connlist/connlist.go

+			// skip empty connections unless one of the peers is representative
+			if allowedConnections.IsEmpty() && !(pe.IsRepresentativePeer(srcPeer) || pe.IsRepresentativePeer(dstPeer)) {


i added it so i can handle the following case:
assume we have an ANP that denies all conns from a -> b ("b" is a representative peer)
and another (with lower priority) rule that allows all conns from a to entire-cluster

i.e "a" is exposed to entire-cluster on all conns except to b (conns are empty to b)
this was added so I can add the entry a->b in the exposure map (with "no conns" ) and display it in the output
see example: "tests/exposure_test_with_anp_9"

ok, please add comment in code explaining this

adisos · 2025-01-26T18:30:22Z

pkg/netpol/connlist/exposure_map.go

-	// check if the connection is contained in an entire cluster connection; if yes skip; if not store the connection data
-	contained, err := connectionContainedInEntireClusterConn(pe, peer, allowedConnSet, isIngress)
+	// check if the peer is exposed to entire-cluster and if connection equals the entire cluster connection;
+	equals, isExposed, err := connectionEqualsEntireClusterConn(pe, peer, allowedConnSet, isIngress)


contained-in instead of equals?

opposite, equals instead of contained-in (it was containedIn, where actually if it was not larger it was equals)
if there is a rule that allows any connection to entire-cluster when we compute allowed conns between peers, the result contains this connection ( so if it is same as entire-cluster connection, there is no need to add it as "separate" exposure).
if the connection is larger (i.e same conn to entire-cluster + other conn - it will be added as another exposure (was and yet))
the new case - with ANP we have deny rules that may be prior to the "allow to entire-cluster" :
so if a real-peer is exposed to entire-cluster on all-TCP
but there is a rule "deny to new-ns on TCP90" --- the exposure to "new-ns" is all TCP but TCP 90 which is different than the exposure to entire-cluster even it is containedIn it.

please add example/explanation in code

adisos · 2025-01-27T06:16:43Z

pkg/netpol/connlist/exposure_map.go

+	// 2. if the connection is empty (no conns) and the peer is exposed to entire-cluster - add this empty connection to the exposure map
+	// to tell that this representative peer is excluded from `entire-cluster` connection


how is it going to be marked as excluded in the output?

see example tests/exposure_test_with_anp_9 and its document in connlist_test.go

there is also an example with real-pod tests/exposure_test_with_anp_10_with_real_pod (when a real is exposed to entire-cluster, but the connection to another real-pod is blocked);
with real pods connlist-output gives as the indication
(related also to this comment

pkg/netpol/eval/exposure.go

adisos · 2025-01-27T06:27:28Z

pkg/netpol/eval/exposure.go

 // - if the rule's namespaceSelector is not nil, no representative namespace will be generated (representative pod has empty namespace name)
 // anyway, the representative pod will store the namespace data.
 func (pe *PolicyEngine) generateRepresentativePeers(selectors []k8s.SingleRuleSelectors, policyNs string) (err error) {
 	for i := range selectors {
 		podNs := "" // by default: representative peer has no namespace; (don't generate representative namespaces)
-		if selectors[i].NsSelector == nil {
+		// note that policyNs is "" (empty) for cluster-scoped policies (ANP and BANP) - means podNs kept empty in this case
+		if selectors[i].NsSelector == nil && policyNs != "" {


policyNs == "" does not necessarily indicate that the policy is cluster-scoped?
better rely on the policy type ?

for all current cases, it does (as the code is written ) and because an NP without namespace would raise error before getting here; we can also change to be more "accurate and sure"

better rely on policy type, so the code is more robust

pkg/netpol/eval/check.go

adisos · 2025-01-27T08:05:50Z

pkg/netpol/eval/check.go

+	anpConnsDeterminedAllFlag := false
+	// if all cluster-wide conns were determined by the ANP then update the selected peer's cluster wide exposure
+	if pe.exposureAnalysisFlag && anpExposure.layerConns.DeterminesAllConns() {
+		podExposureUpdatedFlag = true


isn't exposure to external ip-blocks determined by regular netpols ?

not only;
egress rules in (B)ANP can also determine conns to IPs (networks field)

can podExposureUpdatedFlag be set to true, if possibly more exposure should be inferred from regular netpols?

i think this is related to the other comment
the anser is no, if the flag is true means ANP determined all conns between the peer and entire-cluster

it's not clear what do these flags mean? where is it documented?

what is exposure updated related to? exposure to entire cluster? exposure to entire cluster and external ip-blocks?
what does value true marks?

to entire cluster only.
we have IP-peers for external exposure
will update the document more ; also described above type policiesLayerXgressConns

consider removing "entire-world" fields from exposure analysis structs, and mention in documentation that exposure analysis only computes exposure to entire-cluster (for external ip-blocks just copying the conn-list results).

opened #488

adisos · 2025-01-27T08:06:51Z

pkg/netpol/eval/check.go

+			return anpConns.layerConns.AllowedConns, nil
+		}
+		// else : exposure-analysis is on and still not determined continue
+		anpConnsDeterminedAllFlag = true


how can get here?

because if a->b allows all (or at least determined all conns (ports) from ANPs) does not necessary mean that all exposure to entire cluster was also determined from ANPs .. so need to analyze also NPs / BANP for exposure

not sure I follow why

example:
ANP denies all conns from a -> b (ANP determined all conns to b; if exposure analysis is off no need to continue to NP/BANP)
But:
NP exposes A to entire-cluster on all-conns (if we don't continue - this would not be computed and will have wrong results as exposure to entire-cluster is computed once for each direction)

ok. might be useful to add this example in comment as documentation, so it is clear.

Co-authored-by: Adi Sosnovich <82078442+adisos@users.noreply.github.com>

adisos

for test tests/exposure_test_with_anp_1/ , why isn't workload b mentioned in the output as "Workloads not protected by network policies:" ?

…ol-analyzer into exposure_analysis_with_anp

shireenf-ibm · 2025-01-27T10:38:54Z

for test tests/exposure_test_with_anp_1/ , why isn't workload b mentioned in the output as "Workloads not protected by network policies:" ?

because the subject of the ANP is all hello-world pods
so it is protected and exposed to slytherin but since it is also exposed to entire-cluster on all connections (there is no deny rule/ any limiting policy selecting workload-b) then exposure to slytherin is actually contained in the exposure to entire-cluster

adisos · 2025-01-27T10:42:49Z

for test tests/exposure_test_with_anp_1/ , why isn't workload b mentioned in the output as "Workloads not protected by network policies:" ?

because the subject of the ANP is all hello-world pods so it is protected and exposed to slytherin but since it is also exposed to entire-cluster on all connections (there is no deny rule/ any limiting policy selecting workload-b) then exposure to slytherin is actually contained in the exposure to entire-cluster

I see.. it's a bit misleading because its connectivity has the same effect as "not protected".. can open issue with this example to consider enhancements later.

shireenf-ibm · 2025-01-27T11:49:49Z

exposure_test_with_anp_1/

done #489

shireenf-ibm added 30 commits April 30, 2024 13:01

adding ANP to parser.k8sobj

7286094

fixing gocritic rangeValCopy by indexing

cc14430

Merge github.com:np-guard/netpol-analyzer into support_admin_netpolicy

bb14609

Merge branch 'main' of github.com:np-guard/netpol-analyzer into suppo…

03af344

…rt_admin_netpolicy

Merge github.com:np-guard/netpol-analyzer into support_admin_netpolicy

47b8718

w.i.p. anp support - first commit

150f698

more examples (2 ANPs/ ANP+NP)

87243c2

fixing references

4adf709

new_test that ensures rule ordering in ANP is respected

d25107d

update the conn representation as complement in case it is shorter (a…

d72b159

…ll but: udp 5353 instead of SCTP 1-65535,TCP 1-65535,UDP 1-5352,5354-65535)

test with swapped rules from another test + diff test

4dfeb6d

more-tests

de0ff91

fixing conns computations and a test with multiple ANPs

3eacc3c

extending output formats of existing tests

0a85d43

tiny fix

3e9db6e

fixing a tinu bug in ruleConnections func

836ed4f

tiny doc updte

6641bfd

tiny doc update

619cb88

a @todo tbd while review

b082158

return error if ANPs are without name or not unique names

484d447

remove redundant lines

71031e8

reverting the changes adding complement string representation (all bu…

f672a0b

…t) for connectionSet

Merge github.com:np-guard/netpol-analyzer into support_admin_netpolicy

eb7a685

Merge remote-tracking branch 'origin/HEAD' into support_admin_netpolicy

634706f

minor updates to netpol_errors

2e2ca27

currently disabling exposure-analysis when there are admin-network-po…

bc29f3f

…licies in the input resources

some organizations (mainly comments updates)

56e9f5d

updating some todo messages

6391bbe

updating some todo messages/questions

9b93505

todo question

fb5a582

shireenf-ibm and others added 7 commits November 27, 2024 10:18

Merge branch 'main' into support_admin_netpolicy

b8b98c6

Merge remote-tracking branch 'origin/main' into exposure_analysis_wit…

3bc7db9

…h_anp

Merge remote-tracking branch 'origin/main' into exposure_analysis_wit…

589fdf2

…h_anp

Merge remote-tracking branch 'origin/main' into exposure_analysis_wit…

8ae7c68

…h_anp

Merge remote-tracking branch 'origin/main' into exposure_analysis_wit…

102bae8

…h_anp

Merge remote-tracking branch 'origin/main' into exposure_analysis_wit…

af53aeb

…h_anp

exposure analysis with (B)ANPs + some tests

1a291a3

pull-request-size bot added the size/XXL label Jan 21, 2025

shireenf-ibm marked this pull request as draft January 21, 2025 19:36

shireenf-ibm and others added 6 commits January 21, 2025 21:37

Merge branch 'main' into exposure_analysis_with_anp

8fbfe0a

new example with some clarification regarding entire-cluster conns ex…

da1fc03

…cluding some peers (no peer resolution)

adding two tests where there is a peer excluded from entire-cluster c…

9d9bc9f

…onnection. In case of representative-peer adding this "no conns" to the output to see the exception

test with named port

eb8eba2

Merge branch 'main' into exposure_analysis_with_anp

c60631d

more tests

96ef78b

shireenf-ibm marked this pull request as ready for review January 26, 2025 20:50

adisos reviewed Jan 27, 2025

View reviewed changes

Update pkg/netpol/eval/check.go

cc9aba7

Co-authored-by: Adi Sosnovich <82078442+adisos@users.noreply.github.com>

adisos reviewed Jan 27, 2025

View reviewed changes

shireenf-ibm added 2 commits January 27, 2025 12:35

some doc updates

877ff36

Merge branch 'exposure_analysis_with_anp' of github.com:np-guard/netp…

9704739

…ol-analyzer into exposure_analysis_with_anp

rely on policy scope

34b611f

adisos approved these changes Jan 27, 2025

View reviewed changes

shireenf-ibm linked an issue Jan 27, 2025 that may be closed by this pull request

Exposure-Analysis extend support with (Baseline)AdminNetworkPolicy #445

Closed

shireenf-ibm merged commit 1d93856 into main Jan 27, 2025
4 checks passed

shireenf-ibm deleted the exposure_analysis_with_anp branch January 27, 2025 12:37

		// skip empty connections unless one of the peers is representative
		if allowedConnections.IsEmpty() && !(pe.IsRepresentativePeer(srcPeer) \|\| pe.IsRepresentativePeer(dstPeer)) {

		// 2. if the connection is empty (no conns) and the peer is exposed to entire-cluster - add this empty connection to the exposure map
		// to tell that this representative peer is excluded from `entire-cluster` connection

W.I.P Exposure analysis with anp #486

W.I.P Exposure analysis with anp #486

Conversation

shireenf-ibm commented Jan 21, 2025 • edited Loading

adisos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shireenf-ibm Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adisos left a comment

Choose a reason for hiding this comment

shireenf-ibm commented Jan 27, 2025

adisos commented Jan 27, 2025

shireenf-ibm commented Jan 27, 2025

shireenf-ibm commented Jan 21, 2025 •

edited

Loading

shireenf-ibm Jan 27, 2025 •

edited

Loading