Add a new integration test suite checking network behaviors by akerouanton · Pull Request #45850 · moby/moby

akerouanton · 2023-06-29T20:32:16Z

- What I did

This PR introduces a new integration test suite aimed at testing networking features like inter-container communication, network isolation, port mapping, what happens when the userland proxy is enabled or disabled, etc... and how these features interact with daemon-level and network-level parameters.

So far, there's pretty much no tests making sure our networks are working as expected. For instance, there's a few tests for port mapping, but they don't cover all cases (eg. like IPv6, disabling the userland proxy, etc...); 2. there're a few tests that check if a specific iptables rule exists, but that doesn't prevent that specific rule from be wrong in the first place.

As we're planning to refactor how iptables rules are written and change some of them to fix known security issues, we need a way to test all parameter combinations (at least, the important ones). So far, this was done by hand which is a particularly painful and time consuming experience. As such, this new test suite is foundational to upcoming work.

This test suite should help:

Assert networking security boundaries are working properly, and prevent regressions on security patches ;
Discover what's missing to make the userland proxy disabled by default ;
Give a canonical test plan for new port mappers ;

Following tests are implemented in this PR:

Inter-container communication for internal and non-internal bridge networks, over IPv4 and IPv6 (using ULAs or link-local addresses, SLAAC-assigned or dynamically allocated) ;
Inter-network communication for internal and non-internal bridge networks, over IPv4 and IPv6, is disallowed ;
Connecting to published ports from the host, using either IPv4 or IPv6 loopback address or another local address ;
Connecting to published ports from another bridge, using the bridge gateway address with either the userland proxy enabled or disabled ;
Connecting to ports bound to loopback address, from another host running on a shared L2 segment ;
Reaching a container that didn't publish any port, from another host running on a shared L2 segment ;
Connecting to a published port bound to a specific IP address, from another host running on another L2 segment ;

I plan to write some more tests in the near future (eg. for CVE-2020-13401).

- How to verify it

CI is green.

- A picture of a cute animal (not mandatory but encouraged)

corhere

I would like to see the libnetwork testutils package moved under libnetwork/internal/ so we don't have to worry about some third party importing it.

libnetwork/testutils/prober.go

libnetwork/testutils/l3_segment_linux.go

akerouanton · 2023-07-24T12:45:59Z

I would like to see the libnetwork testutils package moved under libnetwork/internal/ so we don't have to worry about some third party importing it.

We can't move testutils into libnetwork/internal and use it from github.com/docker/docker/integration as internal packages can only be used by other packages in the parent directory they're rooted at. From https://dave.cheney.net/2019/10/06/use-internal-packages-to-reduce-your-public-api-surface:

For example, a package /a/b/c/internal/d/e/f can only be imported by code in the directory tree rooted at /a/b/c. It cannot be imported by code in /a/b/g or in any other repository.

integration/networking/port_mapping_linux_test.go

corhere · 2023-07-24T19:31:12Z

We can't move testutils into libnetwork/internal and use it from github.com/docker/docker/integration

Fine, github.com/docker/docker/internal/... then. I don't care how, I just want it so that external modules can't import the package so we can make breaking changes to it at any time.

internal/testutils/l2_segment_linux.go

This commit introduces a new integration test suite aimed at testing networking features like inter-container communication, network isolation, port mapping, etc... and how they interact with daemon-level and network-level parameters. So far, there's pretty much no tests making sure our networks are well configured: 1. there're a few tests for port mapping, but they don't cover all use cases ; 2. there're a few tests that check if a specific iptables rule exist, but that doesn't prevent that specific iptables rule to be wrong in the first place. As we're planning to refactor how iptables rules are written, and change some of them to fix known security issues, we need a way to test all combinations of parameters. So far, this was done by hand, which is particularly painful and time consuming. As such, this new test suite is foundational to upcoming work. Following tests are implemented in this specific commit: - Inter-container communications for internal and non-internal bridge networks, over IPv4 and IPv6. - Inter-container communications using IPv6 link-local addresses for internal and non-internal bridge networks. - Inter-network communications for internal and non-internal bridge networks, over IPv4 and IPv6, are disallowed. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

Signed-off-by: Albin Kerouanton <albinker@gmail.com>

This commit adds a test to the new networking test suite that makes sure we can connect to ports published on local host, from loopback address or another local address. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

Signed-off-by: Albin Kerouanton <albinker@gmail.com>

These tests use a SYN prober to check whether mapped ports are reachable when they should not. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

Signed-off-by: Albin Kerouanton <albinker@gmail.com>

The tests introduced in "integration: Test port mapping security" check victim hosts don't send a SYN-ACK when it should not to. This could happen for two reasons: 1. because the right security measures are effectively dropping the SYN packet ; or 2. because the SYN packet was never received by the victim host. The latter case could be a potential source of false positives, which are pretty bad since we're testing security features. To alleviate this risk, this commit adds the ability to capture packets on the victim's host interface and leverage that to make sure the SYN packet is well received. If it's not, the test fails. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

internal/testutils/networking/l2_segment_linux.go

thaJeztah

Just some comments after making a first pass / glance over the changes 😅

integration/internal/container/container.go

integration/internal/network/network.go

integration/networking/bridge_test.go

thaJeztah · 2023-07-31T15:13:32Z

integration/internal/container/container.go

+type RunResult struct {
+	ContainerID string
+	ExitCode    int
+	Stdout      *bytes.Buffer
+	Stderr      *bytes.Buffer
+}


Starting to think if we could use icmd.Result for these, but it looks like it would need some contributing to upstream because the outBuffer and errBuffer are not exported.

Could be nice though to use that type for this as well 🤔

moby/vendor/gotest.tools/v3/icmd/command.go

Lines 43 to 52 in 7baab8b

// Result stores the result of running a command

type Result struct {

Cmd *exec.Cmd

ExitCode int

Error error

// Timeout is true if the command was killed because it ran for too long

Timeout bool

outBuffer *lockedBuffer

errBuffer *lockedBuffer

}

I think some of this code also shows that we need to improve some of the client to not having to write utilities like this (e.g. the demultiplexing). Perhaps something to work on (separately) to move code from the CLI, and/or provide better utilities in the client code 🤔

I disagree on all counts.

Starting to think if we could use icmd.Result for these

While there are a lot of similarities, this is not the result of an *exec.Cmd run. The gotest.tools module is Apache2 licensed so there's nothing preventing us from copying that code and modifying it to fit the needs of asserting on the result of attaching to a container or running an exec.

I think some of this code also shows that we need to improve some of the client to not having to write utilities like this (e.g. the demultiplexing).

Demultiplex into what? Buffering the whole demuxed output into an in-memory buffer is rarely the right thing to do in general as the output of an arbitrary command may be unbounded. We shouldn't provide utilities in the client code which make it easier to do the wrong thing than to do the right thing. By only providing stdcopy the consumer of the client is forced to choose which io.Writer to demux the streams into, giving them the opportunity to consider which choice would best fit their use case.

integration/networking/port_mapping_test.go

thaJeztah · 2023-07-31T15:34:18Z

internal/testutils/networking/l2_segment_linux.go

+	if err != nil && !os.IsNotExist(err) {
+		assert.NilError(t, err)
+	}


Does this assert actually do anything? It would always be a non-nil error? Should this be a t.Fatal(err)?

thaJeztah · 2023-07-31T15:34:46Z

internal/testutils/networking/l2_segment_linux.go

+	if err == nil {
+		if reusePrevious {
+			return newNs
+		}
+		t.Fatalf("Network namespace %s already exists whereas TEST_REUSE_L2SEGMENT is empty.", newNs.name)
+	}


This part is a bit confusing ... no error, and producing a t.Fatal() 🤔

thaJeztah · 2023-07-31T15:36:31Z

internal/testutils/networking/l2_segment_linux.go

+		if err := netns.Set(orig.handle); err != nil {
+			t.Fatalf("could not switch back to the original netns: %v", err)
+		}
+		runtime.UnlockOSThread()


It's ok to skip the runtime.UnlockOSThread() if we fail? (I guess it is if we os.Exit()

Not only is it okay, it's mandatory. See #45850 (comment)

thaJeztah · 2023-07-31T15:37:52Z

internal/testutils/networking/l2_segment_linux.go

+	if err := syscall.Unmount(ns.Path(), 0); err != nil {
+		logger.Logf("failed to unmount netns %s: %v", ns, err)
+	}
+	if err := os.Remove(ns.Path()); err != nil {
+		logger.Logf("failed to remove netns mountpoint %s: %v", ns, err)
+	}


Just double checking; we should remove the path even if we failed to unmount?

thaJeztah · 2023-07-31T15:39:19Z

internal/testutils/networking/prober_linux.go

+	}
+
+	daddr := &packet.Addr{HardwareAddr: p.DstMAC}
+	fmt.Printf("Sending Ethernet frame to %s (%d bytes).\n%s\n", daddr.String(), len(buf.Bytes()), hex.Dump(buf.Bytes()))


Wondering if we need a context passed so allow us to past a test-logger (instead of fmt.Printf 🤔

corhere

Please break this up into smaller PRs; this is way too much to review at once.

integration/networking/bridge_test.go

corhere · 2023-07-31T16:14:48Z

integration/networking/bridge_test.go

+			if t.Failed() {
+				t.Logf("Logs from %s:\n%s", ctr2Name, logs)
+			}


The test runner is perfectly capable of suppressing log output on passing tests. All you're doing by making this log conditional is breaking the -test.v flag. Don't break the -test.v flag.

integration/networking/bridge_test.go

corhere · 2023-07-31T16:29:16Z

internal/testutils/networking/l2_segment_linux.go

+		if err := netns.Set(orig.handle); err != nil {
+			t.Fatalf("could not switch back to the original netns: %v", err)
+		}
+		runtime.UnlockOSThread()


Not only is it okay, it's mandatory. See #45850 (comment)

integration/networking/bridge_test.go

corhere · 2023-07-31T17:12:35Z

integration/internal/container/container.go

+type RunResult struct {
+	ContainerID string
+	ExitCode    int
+	Stdout      *bytes.Buffer
+	Stderr      *bytes.Buffer
+}


I disagree on all counts.

Starting to think if we could use icmd.Result for these

While there are a lot of similarities, this is not the result of an *exec.Cmd run. The gotest.tools module is Apache2 licensed so there's nothing preventing us from copying that code and modifying it to fit the needs of asserting on the result of attaching to a container or running an exec.

I think some of this code also shows that we need to improve some of the client to not having to write utilities like this (e.g. the demultiplexing).

Demultiplex into what? Buffering the whole demuxed output into an in-memory buffer is rarely the right thing to do in general as the output of an arbitrary command may be unbounded. We shouldn't provide utilities in the client code which make it easier to do the wrong thing than to do the right thing. By only providing stdcopy the consumer of the client is forced to choose which io.Writer to demux the streams into, giving them the opportunity to consider which choice would best fit their use case.

integration/internal/container/container.go

integration/networking/port_mapping_test.go

akerouanton · 2024-11-22T14:36:19Z

A similar L3Segment was introduced in #48545. It uses iproute2 commands instead of a full Go implementation. This makes it easier to manually reproduce the environment where a specific test is failing. Since the introduction of this L3Segment, we've merged various integration tests that simulates networking between multiple hosts on the same L2 / L3 semgnet.

Most / all the test cases in this PR should be covered by newer tests, so let me close it.

akerouanton force-pushed the networking-test-suite branch from 9dbbec0 to a477f71 Compare June 29, 2023 20:32

corhere reviewed Jun 30, 2023

View reviewed changes

libnetwork/testutils/prober.go Outdated Show resolved Hide resolved

libnetwork/testutils/l3_segment_linux.go Outdated Show resolved Hide resolved

libnetwork/testutils/l3_segment_linux.go Outdated Show resolved Hide resolved

akerouanton force-pushed the networking-test-suite branch 5 times, most recently from 59b4203 to 0fc8c2d Compare July 24, 2023 15:08

akerouanton commented Jul 24, 2023

View reviewed changes

integration/networking/port_mapping_linux_test.go Outdated Show resolved Hide resolved

akerouanton force-pushed the networking-test-suite branch 10 times, most recently from 8bdf860 to b26b1ce Compare July 24, 2023 19:27

akerouanton force-pushed the networking-test-suite branch 10 times, most recently from c20dadf to b28358c Compare July 26, 2023 14:51

akerouanton commented Jul 26, 2023

View reviewed changes

internal/testutils/l2_segment_linux.go Outdated Show resolved Hide resolved

akerouanton mentioned this pull request Jul 26, 2023

tests: Move libnetwork/testutils to internal/testutils/netnsutils #46083

Merged

akerouanton added 6 commits July 26, 2023 22:47

integration: Add RunAttach helper

e129aa0

Signed-off-by: Albin Kerouanton <albinker@gmail.com>

integration: Test ports published to lo

b83249a

This commit adds a test to the new networking test suite that makes sure we can connect to ports published on local host, from loopback address or another local address. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

integration: drop previous, non-exhaustive port mapping test

a118732

Signed-off-by: Albin Kerouanton <albinker@gmail.com>

vendor: add github.com/mdlayher/packet

51d711a

Signed-off-by: Albin Kerouanton <albinker@gmail.com>

vendor: Add github.com/google/gopacket

fb3151c

Signed-off-by: Albin Kerouanton <albinker@gmail.com>

akerouanton force-pushed the networking-test-suite branch 2 times, most recently from 903a916 to 593ec82 Compare July 27, 2023 00:54

akerouanton marked this pull request as ready for review July 27, 2023 08:32

akerouanton requested a review from tianon as a code owner July 27, 2023 08:32

akerouanton removed the request for review from tianon July 27, 2023 08:40

akerouanton mentioned this pull request Jul 27, 2023

libnet/d/bridge: Allow IPv6 ICC from any IP address #45649

Merged

akerouanton added 3 commits July 27, 2023 16:41

integration: Test port mapping security

030dded

These tests use a SYN prober to check whether mapped ports are reachable when they should not. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

hack: Add TEST_MANUAL_DEBUG & TEST_REUSE_L2SEGMENT

4fd5775

Signed-off-by: Albin Kerouanton <albinker@gmail.com>

akerouanton commented Jul 27, 2023

View reviewed changes

internal/testutils/networking/l2_segment_linux.go Outdated Show resolved Hide resolved

akerouanton force-pushed the networking-test-suite branch from 593ec82 to 397b812 Compare July 27, 2023 14:52

thaJeztah reviewed Jul 31, 2023

View reviewed changes

corhere requested changes Jul 31, 2023

View reviewed changes

akerouanton mentioned this pull request Aug 1, 2023

integration: Add RunAttach helper #46138

Merged

akerouanton mentioned this pull request Sep 21, 2023

integration: Add a new networking integration test suite #46531

Merged

akerouanton mentioned this pull request Sep 24, 2024

integration: Add tests for port mappings #48545

Merged

akerouanton closed this Nov 22, 2024

akerouanton deleted the networking-test-suite branch November 22, 2024 14:36

	// Result stores the result of running a command
	type Result struct {
	Cmd *exec.Cmd
	ExitCode int
	Error error
	// Timeout is true if the command was killed because it ran for too long
	Timeout bool
	outBuffer *lockedBuffer
	errBuffer *lockedBuffer
	}

Conversation

akerouanton commented Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corhere left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

akerouanton commented Jul 24, 2023

Uh oh!

Uh oh!

corhere commented Jul 24, 2023

Uh oh!

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corhere left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

akerouanton commented Nov 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

akerouanton commented Jun 29, 2023 •

edited

Loading