diff --git a/libpod/container.go b/libpod/container.go index edd825c637..acfd5031fc 100644 --- a/libpod/container.go +++ b/libpod/container.go @@ -115,6 +115,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_linux.go b/libpod/container_internal_linux.go index ddba432652..ed11f0aa52 100644 --- a/libpod/container_internal_linux.go +++ b/libpod/container_internal_linux.go @@ -70,6 +70,11 @@ func (c *Container) prepare() error { // Set up network namespace if not already set up noNetNS := c.state.NetNS == nil 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 @@ -136,6 +141,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 6806ed360a..bd851034c4 100644 --- a/libpod/networking_common.go +++ b/libpod/networking_common.go @@ -6,6 +6,7 @@ package libpod import ( "errors" "fmt" + "os" "regexp" "sort" @@ -41,6 +42,16 @@ func getNetNSPathCommon(ctr *Container) string { return fmt.Sprintf("%v", ctr.state.NetNS) } +// 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 need 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 59cdcef8d4..3f63c03f2a 100644 --- a/libpod/oci_conmon_common.go +++ b/libpod/oci_conmon_common.go @@ -1161,17 +1161,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 e118348fa3..81a19275a2 100644 --- a/libpod/oci_util.go +++ b/libpod/oci_util.go @@ -44,7 +44,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 3e8cbe7b3d..4e581f9408 100644 --- a/test/system/500-networking.bats +++ b/test/system/500-networking.bats @@ -49,6 +49,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. @@ -56,9 +65,9 @@ load helpers.network is "$output" "$random_2" "exec cat index2.txt" # Verify http contents: curl from localhost - run curl -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 $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