Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Disable automatic publishing of all ports for kube play #17028

Closed
Syquel opened this issue Jan 8, 2023 · 34 comments · Fixed by #19823
Closed

[Bug]: Disable automatic publishing of all ports for kube play #17028

Syquel opened this issue Jan 8, 2023 · 34 comments · Fixed by #19823
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. kube locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Comments

@Syquel
Copy link
Contributor

Syquel commented Jan 8, 2023

Feature request description

Currently podman kube play automatically publishes all ports which are defined in Kubernetes YAMLs as containerPort.

Kubernetes YAML example:

apiVersion: v1
kind: Pod
metadata:
  name: all-ports-published-reproducer
spec:
  containers:
    - name: all-ports-published-reproducer-nginx
      image: "docker.io/bitnami/nginx:1.23"
      env:
        - name: NGINX_HTTP_PORT_NUMBER
          value: "8082"
      ports:
        - name: http
          containerPort: 8082

Podman command: podman kube play all-ports-published-reproducer.yaml

Output of podman port --latest: 8082/tcp -> 0.0.0.0:8082

Users should be able to disable this behavior to prevent accidentally publishing ports to the outside world.

Suggest potential solution

Similiarly to #16880 a new flag --publish-all like the one in podman run should be added to podman kube play.
This flag should accept a boolean parameter which is true by default to preserve the current behavior.

If --publish-all=false is set ports should not be automatically published and only be published if the Kubernetes YAML explicitly defines the hostPort field.

Kubernetes YAML example with explicit hostPort set.

apiVersion: v1
kind: Pod
metadata:
  name: all-ports-published-reproducer
spec:
  containers:
    - name: all-ports-published-reproducer-nginx
      image: "docker.io/bitnami/nginx:1.23"
      env:
        - name: NGINX_HTTP_PORT_NUMBER
          value: "8082"
      ports:
        - name: http
          containerPort: 8082
          hostPort: 80

Have you considered any alternatives?

Podman Quadlets would be an alternative but would prevent users from using already existing Kubernetes YAMLs / Helm charts.

Additional context

#16766 introduces a change to publish ports on a random port instead of reusing the containerPort field of the Kubernetes YAML if hostPort is not set.
If the new flag --publish-all is set to false and the hostPort field is explicitly set to 0 the port should be published on a random port.

Even if firewalld is active on the server netavark currently allows these automatically published ports to be reachable from the outside world, which might be a security concern.

@Syquel Syquel added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 8, 2023
@github-actions
Copy link

github-actions bot commented Feb 8, 2023

A friendly reminder that this issue had no activity for 30 days.

@Syquel
Copy link
Contributor Author

Syquel commented Feb 8, 2023

Any thoughts on my proposal?

@rhatdan
Copy link
Member

rhatdan commented Feb 8, 2023

@ygalblum
Copy link
Collaborator

ygalblum commented Feb 9, 2023

SGTM
In K8S listing the ports is only informative and as a result, users tend to list them even if they are not meant to be exposed. Furthermore, while the example provided here uses 8082 as the containerPort, the issue becomes even more required when the containerPort is a privileged one. Not being able to not publish the port forces the user to explicitly set a different port when running as rootless.
So, a port should be published if:

  • The YAML file includes a hostPort
  • The port is published via --publish
  • publish-all is true (default behavior to maintain the current behavior)
    As a result, the user will be able to block the publishing of a port listed in the YAML file by not setting hostPort, not publishing via --publish and setting --publish-all=false

@haircommander
Copy link
Collaborator

this SGTM too

@saschagrunert
Copy link
Member

Agreed, SGTM

@github-actions
Copy link

A friendly reminder that this issue had no activity for 30 days.

@rhatdan
Copy link
Member

rhatdan commented Mar 27, 2023

@ygalblum @umohnani8 care to work on this?

@rhatdan rhatdan added the Good First Issue This issue would be a good issue for a first time contributor to undertake. label Mar 27, 2023
@MarkoH17
Copy link

MarkoH17 commented May 3, 2023

Just checking whether this feature will allow k8s deployments where containers expose the same containerPort?

Sample kube.yaml:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: test-app
spec:
  replicas: 1
  template:
    metadata:
      name: test-app
      labels:
        &a1
        app: test-app
    spec:
      containers:
        - name: nginx-web
          image: docker.io/library/nginx
          ports:
            - containerPort: 80
              hostPort: 8080
        - name: httpd-web
          image: docker.io/library/httpd
          ports:
            - containerPort: 80
              hostPort: 8081
  selector:
    matchLabels: *a1

Running podman kube play kube.yaml results in the creation of the pods, but since podman tries to bind both containerPort's to the host, one of the pods ends up throwing an Address already in use error.

Edit for more context:

Container with ID 0539dbd9daa6 will keep restarting, since it cannot bind to port 80.

$ podman ps
CONTAINER ID  IMAGE                                    COMMAND               CREATED        STATUS        PORTS                                       NAMES
728e64135f85  localhost/podman-pause:4.5.0-1681486942                        4 minutes ago  Up 4 minutes  0.0.0.0:8080->80/tcp, 0.0.0.0:8081->80/tcp  a830b7afed42-infra
0539dbd9daa6  docker.io/library/nginx:latest           nginx -g daemon o...  4 minutes ago  Up 1 second   0.0.0.0:8080->80/tcp, 0.0.0.0:8081->80/tcp  test-app-pod-nginx-web
07136a58cad6  docker.io/library/httpd:latest           httpd-foreground      4 minutes ago  Up 4 minutes  0.0.0.0:8080->80/tcp, 0.0.0.0:8081->80/tcp  test-app-pod-httpd-web
$ podman logs 0539dbd9daa6 | head -n 10
/docker-entrypoint.sh: /docker-entrypoint.d/ is not empty, will attempt to perform configuration
/docker-entrypoint.sh: Looking for shell scripts in /docker-entrypoint.d/
/docker-entrypoint.sh: Launching /docker-entrypoint.d/10-listen-on-ipv6-by-default.sh
10-listen-on-ipv6-by-default.sh: info: Getting the checksum of /etc/nginx/conf.d/default.conf
10-listen-on-ipv6-by-default.sh: info: Enabled listen on IPv6 in /etc/nginx/conf.d/default.conf
/docker-entrypoint.sh: Launching /docker-entrypoint.d/20-envsubst-on-templates.sh
/docker-entrypoint.sh: Launching /docker-entrypoint.d/30-tune-worker-processes.sh
/docker-entrypoint.sh: Configuration complete; ready for start up
2023/05/03 01:30:37 [emerg] 1#1: bind() to 0.0.0.0:80 failed (98: Address already in use)
nginx: [emerg] bind() to 0.0.0.0:80 failed (98: Address already in use)
2023/05/03 01:30:37 [emerg] 1#1: bind() to [::]:80 failed (98: Address already in use)

@Shikachuu
Copy link

@ygalblum @umohnani8 care to work on this?

@rhatdan is this still relevant?

If I understand everything correctly the sweetspot should be somewhere between discussion here and the comments below #16766.

To summarize, there should be a flag --publish-all which is true by default, but when it is false, only those ports should be exposed which were explicitly introduced in the config by setting the hostPort field in the yaml or by using the --publish override flag described in #16880.

Cause if I catch everything right, I would like to work on this.

@rhatdan
Copy link
Member

rhatdan commented May 5, 2023

I believe you are correct, but I am no expert in Kubernetes.
You are welcome to work on this.

@rhatdan
Copy link
Member

rhatdan commented May 5, 2023

@umohnani8 @ygalblum Is his assumption correct?

@ygalblum
Copy link
Collaborator

ygalblum commented May 7, 2023

@Shikachuu yes, your description is correct.

The current behavior is that all the ports are published. If the host is port is set via either --publish or through the hostPort field in the YAML file, Podman will use this configuration. If not, Podman will use the containerPort as the hostPort.

The --publish-all flag will control only the last part where true will maintain the current behavior (and therefore must be the default) and false will skip it.

@ygalblum
Copy link
Collaborator

ygalblum commented May 7, 2023

@MarkoH17 Are you sure this configuration is valid? AFAIK all containers in a single pod share the same network namespace. As a result, you should not be able to reuse a containerPort.

@joelpurra
Copy link
Contributor

joelpurra commented May 10, 2023

I suggest --publish-all=false should be the default.

#17028 (comment): In K8S listing the ports is only informative and as a result, users tend to list them even if they are not meant to be exposed.

(Emphasis mine.) Yes, this makes exposing containerPort unexpected kubernetes-behavior in podman kube play. Perhaps even classified as a security risk?

Sure, implementing kind: Service may be overkill for podman. The reasons kubernetes lists for not using hostPort as best practices sound like reasons for primarily using hostPort with podman kube play though:

Don't specify a hostPort for a Pod unless it is absolutely necessary. When you bind a Pod to a hostPort, it limits the number of places the Pod can be scheduled, because each <hostIP, hostPort, protocol> combination must be unique. If you don't specify the hostIP and protocol explicitly, Kubernetes will use 0.0.0.0 as the default hostIP and TCP as the default protocol.

Multiple podman kube play kube-N.yaml should be equivalent to multiple Pods on a single kubernetes Node/host, where hostPort is expected to collide. (But containerPorts are scoped to the Pod network namespaces and do not collide on the host.)

Basically, hostPort is already the kubernetes escape-hatch workaround for exposing ports directly on the current Node/host. Having podman kube play offer two other modes seems a bit much.

  • --publish-all=false (proposed default): expose only explicitly listed hostPort from kube.yaml.
  • --publish-all=true (current default): expose all informative containerPort from kube.yaml, which is unexpected behavior in kubernetes and a security risk. Also exposes all explicit hostPort I guess?
  • --publish: in addition to the above, ad-hoc containerPort-to-hostPort mapping(s) completely outside of kube.yaml.

To me this boils down to a documentation issue for podman kube play, with emphasis on using explicit hostPort as it is more compatible.

@MarkoH17
Copy link

@MarkoH17 Are you sure this configuration is valid? AFAIK all containers in a single pod share the same network namespace. As a result, you should not be able to reuse a containerPort.

@ygalblum You are definitely right - the containers share a pod and encounter a collision when a containerPort is reused. Without knowing how things work here under the hood, does it make sense to support a Deployment where containerPorts may overlap?

@ygalblum
Copy link
Collaborator

I suggest --publish-all=false should be the default.

@joelpurra While I agree with your analysis, the problem is with backward compatibility. The current behavior is the equivalent of --publish-all=true. So, presenting this parameter with default of false will break current implementations.

@ygalblum
Copy link
Collaborator

Without knowing how things work here under the hood, does it make sense to support a Deployment where containerPorts may overlap?

Sorry, but, the main problem lives "under the hood". This will just not work. The second application will fail to open the port inside of the container regardless to how it's exposed on the host.

@joelpurra
Copy link
Contributor

@ygalblum wrote:

While I agree with your analysis, the problem is with backward compatibility.

Then we agree that is a compatibility and security issue, when compared to "standard" Kubernetes YAML. I agree that it is a backward compatibility problem for podman kube play, but not that it shouldn't be fixed. When is the next opportunity to introduce a breaking change?

For reference, there are nearly half a million YAML files containing containerPort on Github alone (although including Helm charts/templates etcetera). Even if only one percent of those belong to popular projects, they may each have quite a few users. I would like to extend my concern to users podman kube playing them, without knowing the implications.

While recently moving servers/services to rootless podman kube play I set up a pod with a database server container, as usual documented with containerPort 3306 (default) and name mariadb. The pod is shared with an application container, accessing the database internally through port 3306 (again, default). I did not know, until reading this issue, that this would also expose the database to the host network, even if the public port number may be randomized.

This pod is running on a publicly exposed server, receiving thousands of port scan requests per day. There's no advanced layered protection, but at least I have a restrictive firewall set up. I still do not like the surprise "pod/container breakout" port exposure, from a database server port no less; my take-away is that it was not far away from an accidental data leak.

@ygalblum
Copy link
Collaborator

@joelpurra As I wrote, I completely understand your point. But, I know from previous cases that default behavior changes trigger quite the storm.
@vrothberg @rhatdan WDYT?

@vrothberg
Copy link
Member

A security issue is a good reason to change default behavior.

I like the idea of adding a --publish-all CLI flag. The default of this flag could be made configurable in a new field in man containers.conf such that it's easier to switch between the two behaviors.

@Luap99 WDYT?

@Luap99
Copy link
Member

Luap99 commented May 11, 2023

Well not binding a host port is exactly what podman did until #15946. I don't understand why it was changed like this when it is neither how k8s behaves (not binding) nor how podman run would behave (binding a random port). I would even argue that #15946 was already a breaking change.

Adding a --publish-all sounds good to me. As for breaking backwards compatibly the next possibility would be 5.0.

@Shikachuu
Copy link

So the flag should be false by default, but the defalt behaviour of this flag can be changed in the containers.conf right?

@rhatdan
Copy link
Member

rhatdan commented May 12, 2023

SGTM

@schewara
Copy link

I also just got bit by this issue after upgrading from EL 9.1 -> 9.2 with podman being upgraded from version 4.2 to 4.4, with the result, that all web containers refused to start with

rootlessport cannot expose privileged port 80

From my point of view that was already a breaking change which should be reverted sooner then later.

Also comparing it with the behavior of docker compose, where the ports also must be explicitly defined, users migrating from compose to podman will most likely also run into this issue.

In my testing setup here all pods are using a bridge network, where the pods should listen to and external access is only possible through an ingress pod communicating over the bridge network.

Running rootless without adjusting the net.ipv4.ip_unprivileged_port_start setting luckily saved me here, otherwise the services (ignoring the port conflicts), would have been automatically exposed to the wild with all security measures circumvented.

@vrothberg
Copy link
Member

@schewara, thanks for sharing. If you are using RHEL and desire a backport, please go through the Red Hat customer channels (e.g., Bugzilla).

@vrothberg vrothberg added kind/bug Categorizes issue or PR as related to a bug. and removed Good First Issue This issue would be a good issue for a first time contributor to undertake. kind/feature Categorizes issue or PR as related to a new feature. labels May 22, 2023
@vrothberg vrothberg changed the title [Feature]: Disable automatic publishing of all ports for kube play [Bug]: Disable automatic publishing of all ports for kube play May 22, 2023
Syquel added a commit to Syquel/podman that referenced this issue Jun 5, 2023
@void-spark
Copy link

Glad this is already reported, just ran into the same :)

@void-spark
Copy link

Is there a workaround, can I use --publish with a '0' port to prevent it from opening a port on the host?

@void-spark
Copy link

Is there a workaround, can I use --publish with a '0' port to prevent it from opening a port on the host?

Answered that one myself, no, it requires 1-65535 :)

@fsdrw08
Copy link

fsdrw08 commented Jul 24, 2023

I hope that podman do not expose container port to host port when there is no hostPort describe in k8s yaml file while running podman kube play

Backfighter added a commit to Backfighter/podman that referenced this issue Aug 31, 2023
Container ports defined with containerPort were exposed by default
even though kubernetes interprets them as mostly informative.
Fixes containers#17028
Backfighter added a commit to Backfighter/podman that referenced this issue Aug 31, 2023
Container ports defined with containerPort were exposed by default
even though kubernetes interprets them as mostly informative.
Fixes containers#17028

Signed-off-by: Peter Werner <wpw.peter@gmail.com>
Backfighter added a commit to Backfighter/podman that referenced this issue Aug 31, 2023
Container ports defined with containerPort were exposed by default
even though kubernetes interprets them as mostly informative.
Closes containers#17028

Signed-off-by: Peter Werner <wpw.peter@gmail.com>
Backfighter added a commit to Backfighter/podman that referenced this issue Aug 31, 2023
Container ports defined with containerPort were exposed by default
even though kubernetes interprets them as mostly informative.
Closes containers#17028

Signed-off-by: Peter Werner <wpw.peter@gmail.com>
Backfighter added a commit to Backfighter/podman that referenced this issue Aug 31, 2023
Container ports defined with containerPort were exposed by default
even though kubernetes interprets them as mostly informative.
Closes containers#17028

Signed-off-by: Peter Werner <wpw.peter@gmail.com>
Backfighter added a commit to Backfighter/podman that referenced this issue Sep 1, 2023
Container ports defined with containerPort were exposed by default
even though kubernetes interprets them as mostly informative.
Closes containers#17028

Signed-off-by: Peter Werner <wpw.peter@gmail.com>
Backfighter added a commit to Backfighter/podman that referenced this issue Sep 1, 2023
Container ports defined with containerPort were exposed by default
even though kubernetes interprets them as mostly informative.
Closes containers#17028

Signed-off-by: Peter Werner <wpw.peter@gmail.com>
Backfighter added a commit to Backfighter/podman that referenced this issue Sep 1, 2023
Container ports defined with containerPort were exposed by default
even though kubernetes interprets them as mostly informative.
Closes containers#17028

Signed-off-by: Peter Werner <wpw.peter@gmail.com>
Backfighter added a commit to Backfighter/podman that referenced this issue Sep 18, 2023
Container ports defined with containerPort were exposed by default
even though kubernetes interprets them as mostly informative.
Closes containers#17028

Signed-off-by: Peter Werner <wpw.peter@gmail.com>
Backfighter added a commit to Backfighter/podman that referenced this issue Sep 19, 2023
Container ports defined with containerPort were exposed by default
even though kubernetes interprets them as mostly informative.
Closes containers#17028

Signed-off-by: Peter Werner <wpw.peter@gmail.com>
Backfighter added a commit to Backfighter/podman that referenced this issue Sep 23, 2023
Container ports defined with containerPort were exposed by default
even though kubernetes interprets them as mostly informative.
Closes containers#17028

Signed-off-by: Peter Werner <wpw.peter@gmail.com>
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Dec 28, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/bug Categorizes issue or PR as related to a bug. kube locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet