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

podman machine improve port forwarding #12283

Merged
merged 1 commit into from
Nov 16, 2021

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Nov 12, 2021

What this PR does / why we need it:

This commits adds port forwarding logic directly into podman. The
podman-machine cni plugin is no longer needed.

The following new features are supported:

  • works with cni, netavark and slirp4netns
  • ports can use the hostIP to bind instead of hard coding 0.0.0.0
  • gvproxy no longer listens on 0.0.0.0:7777 (requires a new gvproxy
    version), the API is only reachable from within the VM via gateway.containers.internal
  • support the udp protocol

With this we no longer need podman-machine-cni and should remove it from
the packaging. There is also a change to make sure we are backwards
compatible with old config which include this plugin.

How to verify it

We have no podman machine test at the moment.
Please test this manually on your system.
Make sure to copy the new podman and rootlessport binary into the vm
because the server needs the fix. Also make sure your gvproxy is new enough.

Which issue(s) this PR fixes:

Fixes #11528
Fixes #11728

Special notes for your reviewer:

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 12, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 12, 2021
@Luap99
Copy link
Member Author

Luap99 commented Nov 12, 2021

@baude @ashley-cui @guillaumerose PTAL

@guillaumerose
Copy link
Contributor

lgtm

removing the podman-machine-cni binary is a great idea!

@rhatdan
Copy link
Member

rhatdan commented Nov 12, 2021

@baude @mheon @flouthoc PTAL

@TomSweeneyRedHat
Copy link
Member

LGTM
but definitely want some of the others noted in prior comments to throw in an opinion, @jwhonce too

@guillaumerose
Copy link
Contributor

Is there a chance having an old VM with a recent podman on the host won't work?
For instance, the old machine will try to connect to port 7777.

If the VM image with newer podman is released after podman on the Mac, could it happen?

@afbjorklund
Copy link
Contributor

It's not so obvious how to tunnel an arbitrary port (e.g. 9090) over to the VM, like which podman command to use.

@Luap99
Copy link
Member Author

Luap99 commented Nov 13, 2021

It's not so obvious how to tunnel an arbitrary port (e.g. 9090) over to the VM, like which podman command to use.

Port forwarding is done automatically for your containers. There is no podman command for that. If you need to expose arbitrary ports you need to talk directly to gvproxy.

@Luap99
Copy link
Member Author

Luap99 commented Nov 13, 2021

Is there a chance having an old VM with a recent podman on the host won't work? For instance, the old machine will try to connect to port 7777.

If the VM image with newer podman is released after podman on the Mac, could it happen?

Yes this will be a problem. Given that this is a 4.0 change we have to update both at the same time otherwise you run into many issues, the libpod API is not compatible.

This commits adds port forwarding logic directly into podman. The
podman-machine cni plugin is no longer needed.

The following new features are supported:
 - works with cni, netavark and slirp4netns
 - ports can use the hostIP to bind instead of hard coding 0.0.0.0
 - gvproxy no longer listens on 0.0.0.0:7777 (requires a new gvproxy
   version)
 - support the udp protocol

With this we no longer need podman-machine-cni and should remove it from
the packaging. There is also a change to make sure we are backwards
compatible with old config which include this plugin.

Fixes containers#11528
Fixes containers#11728

[NO NEW TESTS NEEDED] We have no podman machine test at the moment.
Please test this manually on your system.

Signed-off-by: Paul Holzinger <[email protected]>
@flouthoc
Copy link
Collaborator

My two-cents

  • I think PR should have a label of breaking-change as we discussed and would be also nice if we could add a PR where support for :7777 was removed from gv_proxy and a small description of how do we communicate with API now.

  • Although i'd like to suggested @guillaumerose @Luap99 . Couldn't we check gvproxy version in podman using helper function and add support for :7777 for cases where gv_proxy is still pointing to older version while new logic for newer version.
    My only concern is podman is breaking compatibility with loosely handled component like gvproxy. But if we are going to do a force update with podman 4.0 then i guess its fine.

@Luap99 Luap99 added breaking-change A change that will require a full version bump, i.e. 3.* to 4.* 4.0 labels Nov 16, 2021
@Luap99
Copy link
Member Author

Luap99 commented Nov 16, 2021

I think PR should have a label of breaking-change as we discussed and would be also nice if we could add a PR where support for :7777 was removed from gv_proxy and a small description of how do we communicate with API now.

Added labels and more info to the PR description.

Although i'd like to suggested @guillaumerose @Luap99 . Couldn't we check gvproxy version in podman using helper function and add support for :7777 for cases where gv_proxy is still pointing to older version while new logic for newer version.
My only concern is podman is breaking compatibility with loosely handled component like gvproxy. But if we are going to do a force update with podman 4.0 then i guess its fine.

There is no point in supporting that, the 4.X client should/will refuse to work with an 3.X server. The API is not compatible so there is no point is supporting the the 7777 port for the old server. The latest gvproxy release already contains this feature so we should be good, podman 4.0 will not be released for another 2-3 months.

@rhatdan
Copy link
Member

rhatdan commented Nov 16, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 16, 2021
@openshift-merge-robot openshift-merge-robot merged commit 8430ffc into containers:main Nov 16, 2021
@Luap99 Luap99 deleted the machine-ports branch November 16, 2021 13:59
@p-rog
Copy link

p-rog commented Nov 29, 2021

This PR contains a fix to the CVE-2021-4024

Vulnerability description:
The gvproxy API is being accessible on port 7777 on all IP addresses on the host. If that port is open on the host's firewall, an attacker can potentially use gvproxy API to forward ports on the host to ports in the VM, making private services on the VM accessible to the network. This issue could be also used to interrupt the host's services by forwarding all ports to the VM.

@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 Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. breaking-change A change that will require a full version bump, i.e. 3.* to 4.* lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adjustable machine proxy port Port forwarding with podman machine for 127.0.0.1 should work
8 participants