From 32e12a8e5ab25c54ffe5366825e420a30ea4c3fe Mon Sep 17 00:00:00 2001 From: Marco Iorio Date: Thu, 12 Oct 2023 09:07:56 +0200 Subject: [PATCH] connectivity: add sanity checks for encryption tests Currently, encryption checks are run only when encryption (either IPSec or WireGuard) is enabled. While this approach is sensible, it also led to cases in which the tests were broken in certain circumstances, and we didn't notice [1,2,3,4]. Let's instead always run these tests, asserting that unencrypted packets are either seen or not based on the feature set. Hence, we can immediately notice if there's a regression, and no unencrypted packets are detected even if encryption is not enabled. [1]: f512df45538a ("connectivity: fix node-to-node encryption tests") [2]: HEAD~3 ("connectivity: fix pod-to-pod encryption validation") [3]: HEAD~2 ("connectivity: fix encryption validation when running in ENI mode") [4]: HEAD~1 ("connectivity: fix encryption validation when host firewall is enabled") Signed-off-by: Marco Iorio --- connectivity/suite.go | 12 ++++++---- connectivity/tests/encryption.go | 41 ++++++++++++++++++++++---------- 2 files changed, 36 insertions(+), 17 deletions(-) diff --git a/connectivity/suite.go b/connectivity/suite.go index 743bec4980..f1567e1b45 100644 --- a/connectivity/suite.go +++ b/connectivity/suite.go @@ -745,16 +745,18 @@ func Run(ctx context.Context, ct *check.ConnectivityTest, addExtraTests func(*ch tests.OutsideToNodePort(), ) + // Encryption checks are always executed as a sanity check, asserting whether + // unencrypted packets shall, or shall not, be observed based on the feature set. ct.NewTest("pod-to-pod-encryption"). - WithFeatureRequirements(features.RequireEnabled(features.EncryptionPod)). WithScenarios( - tests.PodToPodEncryption(), + tests.PodToPodEncryption(features.RequireEnabled(features.EncryptionPod)), ) ct.NewTest("node-to-node-encryption"). - WithFeatureRequirements(features.RequireEnabled(features.EncryptionPod), - features.RequireEnabled(features.EncryptionNode)). WithScenarios( - tests.NodeToNodeEncryption(), + tests.NodeToNodeEncryption( + features.RequireEnabled(features.EncryptionPod), + features.RequireEnabled(features.EncryptionNode), + ), ) if ct.Params().IncludeUnsafeTests { diff --git a/connectivity/tests/encryption.go b/connectivity/tests/encryption.go index 6eb89e0144..d012f6d1e5 100644 --- a/connectivity/tests/encryption.go +++ b/connectivity/tests/encryption.go @@ -107,15 +107,17 @@ func getSourceAddressFilter(ctx context.Context, t *check.Test, client, // PodToPodEncryption is a test case which checks the following: // - There is a connectivity between pods on different nodes when any // encryption mode is on (either WireGuard or IPsec). -// - No unencrypted packet is leaked. +// - No unencrypted packet is leaked. As a sanity check, we additionally +// run the same test also when encryption is disabled, asserting that +// we effectively observe unencrypted packets. // // The checks are implemented by curl'ing a server pod from a client pod, and // then inspecting tcpdump captures from the client pod's node. -func PodToPodEncryption() check.Scenario { - return &podToPodEncryption{} +func PodToPodEncryption(reqs ...features.Requirement) check.Scenario { + return &podToPodEncryption{reqs} } -type podToPodEncryption struct{} +type podToPodEncryption struct{ reqs []features.Requirement } func (s *podToPodEncryption) Name() string { return "pod-to-pod-encryption" @@ -137,14 +139,20 @@ func (s *podToPodEncryption) Run(ctx context.Context, t *check.Test) { // clientHost is a pod running on the same node as the client pod, just in // the host netns. clientHost := ct.HostNetNSPodsByNode()[client.Pod.Spec.NodeName] + assertNoLeaks, _ := ct.Features.MatchRequirements(s.reqs...) + + if !assertNoLeaks { + t.Debugf("%s test running in sanity mode, expecting unencrypted packets", s.Name()) + } t.ForEachIPFamily(func(ipFam features.IPFamily) { - testNoTrafficLeak(ctx, t, s, client, &server, &clientHost, requestHTTP, ipFam) + testNoTrafficLeak(ctx, t, s, client, &server, &clientHost, requestHTTP, ipFam, assertNoLeaks) }) } func testNoTrafficLeak(ctx context.Context, t *check.Test, s check.Scenario, client, server, clientHost *check.Pod, reqType requestType, ipFam features.IPFamily, + assertNoLeaks bool, ) { dstAddr := server.Address(ipFam) iface := getInterNodeIface(ctx, t, clientHost, ipFam, client.Address(ipFam), dstAddr) @@ -233,7 +241,7 @@ func testNoTrafficLeak(ctx context.Context, t *check.Test, s check.Scenario, if err != nil { t.Fatalf("Failed to retrieve tcpdump pkt count: %s", err) } - if !strings.HasPrefix(count.String(), "0 packets") { + if !strings.HasPrefix(count.String(), "0 packets") && assertNoLeaks { t.Failf("Captured unencrypted pkt (count=%s)", strings.TrimRight(count.String(), "\n\r")) // If debug mode is enabled, dump the captured pkts @@ -246,6 +254,10 @@ func testNoTrafficLeak(ctx context.Context, t *check.Test, s check.Scenario, t.Debugf("Captured pkts:\n%s", out.String()) } } + + if strings.HasPrefix(count.String(), "0 packets") && !assertNoLeaks { + t.Failf("Expected to see unencrypted packets, but none found. This check might be broken") + } } // bytes.Buffer from the stdlib is non-thread safe, thus our custom @@ -281,11 +293,11 @@ func (b *safeBuffer) ReadString(d byte) (string, error) { return b.b.ReadString(d) } -func NodeToNodeEncryption() check.Scenario { - return &nodeToNodeEncryption{} +func NodeToNodeEncryption(reqs ...features.Requirement) check.Scenario { + return &nodeToNodeEncryption{reqs} } -type nodeToNodeEncryption struct{} +type nodeToNodeEncryption struct{ reqs []features.Requirement } func (s *nodeToNodeEncryption) Name() string { return "node-to-node-encryption" @@ -308,14 +320,19 @@ func (s *nodeToNodeEncryption) Run(ctx context.Context, t *check.Test) { clientHost := t.Context().HostNetNSPodsByNode()[client.Pod.Spec.NodeName] // serverHost is a pod running in a remote node's host netns. serverHost := t.Context().HostNetNSPodsByNode()[server.Pod.Spec.NodeName] + assertNoLeaks, _ := t.Context().Features.MatchRequirements(s.reqs...) + + if !assertNoLeaks { + t.Debugf("%s test running in sanity mode, expecting unencrypted packets", s.Name()) + } t.ForEachIPFamily(func(ipFam features.IPFamily) { // Test pod-to-remote-host (ICMP Echo instead of HTTP because a remote host // does not have a HTTP server running) - testNoTrafficLeak(ctx, t, s, client, &serverHost, &clientHost, requestICMPEcho, ipFam) + testNoTrafficLeak(ctx, t, s, client, &serverHost, &clientHost, requestICMPEcho, ipFam, assertNoLeaks) // Test host-to-remote-host - testNoTrafficLeak(ctx, t, s, &clientHost, &serverHost, &clientHost, requestICMPEcho, ipFam) + testNoTrafficLeak(ctx, t, s, &clientHost, &serverHost, &clientHost, requestICMPEcho, ipFam, assertNoLeaks) // Test host-to-remote-pod - testNoTrafficLeak(ctx, t, s, &clientHost, &server, &clientHost, requestHTTP, ipFam) + testNoTrafficLeak(ctx, t, s, &clientHost, &server, &clientHost, requestHTTP, ipFam, assertNoLeaks) }) }