diff --git a/libpod/container.go b/libpod/container.go index 3619b82c3f..e424afa9f6 100644 --- a/libpod/container.go +++ b/libpod/container.go @@ -118,6 +118,10 @@ type Container struct { rootlessPortSyncR *os.File rootlessPortSyncW *os.File + // reservedPorts contains the fds for the bound ports when using the + // bridge network mode as root. + reservedPorts []*os.File + // perNetworkOpts should be set when you want to use special network // options when calling network setup/teardown. This should be used for // container restore or network reload for example. Leave this nil if diff --git a/libpod/container_internal_freebsd.go b/libpod/container_internal_freebsd.go index 995d519299..ac9892ef2a 100644 --- a/libpod/container_internal_freebsd.go +++ b/libpod/container_internal_freebsd.go @@ -50,6 +50,10 @@ func (c *Container) prepare() error { // Set up network namespace if not already set up noNetNS := c.state.NetNS == "" if c.config.CreateNetNS && noNetNS && !c.config.PostConfigureNetNS { + c.reservedPorts, createNetNSErr = c.bindPorts() + if createNetNSErr != nil { + return + } ctrNS, networkStatus, createNetNSErr = c.runtime.createNetNS(c) if createNetNSErr != nil { return @@ -112,6 +116,11 @@ func (c *Container) prepare() error { logrus.Errorf("Preparing container %s: %v", c.ID(), createErr) createErr = fmt.Errorf("cleaning up container %s network after setup failure: %w", c.ID(), err) } + for _, f := range c.reservedPorts { + // make sure to close all ports again on errors + f.Close() + } + c.reservedPorts = nil return createErr } diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go index a7b16a7f0a..c479a0255d 100644 --- a/libpod/container_internal_linux.go +++ b/libpod/container_internal_linux.go @@ -82,6 +82,11 @@ func (c *Container) prepare() error { // Set up network namespace if not already set up noNetNS := c.state.NetNS == "" if c.config.CreateNetNS && noNetNS && !c.config.PostConfigureNetNS { + c.reservedPorts, createNetNSErr = c.bindPorts() + if createNetNSErr != nil { + return + } + netNS, networkStatus, createNetNSErr = c.runtime.createNetNS(c) if createNetNSErr != nil { return @@ -148,6 +153,11 @@ func (c *Container) prepare() error { } if createErr != nil { + for _, f := range c.reservedPorts { + // make sure to close all ports again on errors + f.Close() + } + c.reservedPorts = nil return createErr } diff --git a/libpod/networking_common.go b/libpod/networking_common.go index 6bd32a3d95..daf840df0f 100644 --- a/libpod/networking_common.go +++ b/libpod/networking_common.go @@ -5,6 +5,7 @@ package libpod import ( "errors" "fmt" + "os" "regexp" "slices" "sort" @@ -21,6 +22,16 @@ import ( "github.com/sirupsen/logrus" ) +// bindPorts ports to keep them open via conmon so no other process can use them and we can check if they are in use. +// Note in all cases it is important that we bind before setting up the network to avoid issues were we add firewall +// rules before we even "own" the port. +func (c *Container) bindPorts() ([]*os.File, error) { + if !c.runtime.config.Engine.EnablePortReservation || rootless.IsRootless() || !c.config.NetMode.IsBridge() { + return nil, nil + } + return bindPorts(c.convertPortMappings()) +} + // convertPortMappings will remove the HostIP part from the ports when running inside podman machine. // This is needed because a HostIP of 127.0.0.1 would now allow the gvproxy forwarder to reach to open ports. // For machine the HostIP must only be used by gvproxy and never in the VM. diff --git a/libpod/oci_conmon_common.go b/libpod/oci_conmon_common.go index c32fba46e2..a3e5a24b0d 100644 --- a/libpod/oci_conmon_common.go +++ b/libpod/oci_conmon_common.go @@ -1216,17 +1216,22 @@ func (r *ConmonOCIRuntime) createOCIContainer(ctr *Container, restoreOptions *Co cmd.Env = append(cmd.Env, conmonEnv...) cmd.ExtraFiles = append(cmd.ExtraFiles, childSyncPipe, childStartPipe) - if r.reservePorts && !rootless.IsRootless() && !ctr.config.NetMode.IsSlirp4netns() { - ports, err := bindPorts(ctr.convertPortMappings()) + if ctr.config.PostConfigureNetNS { + // netns was not setup yet but we have to bind ports now so we can leak the fd to conmon + ports, err := ctr.bindPorts() if err != nil { return 0, err } filesToClose = append(filesToClose, ports...) - // Leak the port we bound in the conmon process. These fd's won't be used // by the container and conmon will keep the ports busy so that another // process cannot use them. cmd.ExtraFiles = append(cmd.ExtraFiles, ports...) + } else { + // ports were bound in ctr.prepare() as we must do it before the netns setup + filesToClose = append(filesToClose, ctr.reservedPorts...) + cmd.ExtraFiles = append(cmd.ExtraFiles, ctr.reservedPorts...) + ctr.reservedPorts = nil } if ctr.config.NetMode.IsSlirp4netns() || rootless.IsRootless() { diff --git a/libpod/oci_util.go b/libpod/oci_util.go index 31d6b7fbe4..5d30c8b8f7 100644 --- a/libpod/oci_util.go +++ b/libpod/oci_util.go @@ -46,7 +46,12 @@ func bindPorts(ports []types.PortMapping) ([]*os.File, error) { for i := uint16(0); i < port.Range; i++ { f, err := bindPort(protocol, port.HostIP, port.HostPort+i, isV6, &sctpWarning) if err != nil { - return files, err + // close all open ports in case of early error so we do not + // rely garbage collector to close them + for _, f := range files { + f.Close() + } + return nil, err } if f != nil { files = append(files, f) diff --git a/test/system/500-networking.bats b/test/system/500-networking.bats index 0bd2257310..0d21391195 100644 --- a/test/system/500-networking.bats +++ b/test/system/500-networking.bats @@ -54,6 +54,15 @@ load helpers.network $IMAGE /bin/busybox-extras httpd -f -p 80 cid=$output + # Try to bind the same port again, this must fail. + # regression test for https://issues.redhat.com/browse/RHEL-50746 + # which caused this command to overwrite the firewall rules as root + # causing the curl commands below to fail + run_podman 126 run --rm -p "$HOST_PORT:80" $IMAGE true + # Note error messages differ between root/rootless, so only check port + # and the part of the error text that is common. + assert "$output" =~ "$HOST_PORT.*ddress already in use" "port in use" + # In that container, create a second file, using exec and redirection run_podman exec -i myweb sh -c "cat > index2.txt" <<<"$random_2" # ...verify its contents as seen from container. @@ -61,9 +70,9 @@ load helpers.network is "$output" "$random_2" "exec cat index2.txt" # Verify http contents: curl from localhost - run curl -s -S $SERVER/index.txt + run curl --max-time 3 -s -S $SERVER/index.txt is "$output" "$random_1" "curl 127.0.0.1:/index.txt" - run curl -s -S $SERVER/index2.txt + run curl --max-time 3 -s -S $SERVER/index2.txt is "$output" "$random_2" "curl 127.0.0.1:/index2.txt" # Verify http contents: wget from a second container