Re: pasta does not correctly handle bind errors with port ranges

[Stefano Brivio <sbrivio@redhat.com>]

On Mon, 12 Feb 2024 15:43:00 +0100
Paul Holzinger <pholzing@redhat.com> wrote:

> On 12/02/2024 15:13, Stefano Brivio wrote:
> > On Mon, 12 Feb 2024 12:45:00 +0100
> > Paul Holzinger <pholzing@redhat.com> wrote:
> >  
> >> Hi Stefano,
> >>
> >> On 09/02/2024 22:09, Stefano Brivio wrote:  
> >>> Hi Paul,
> >>>
> >>> On Fri, 9 Feb 2024 17:57:05 +0100
> >>> Paul Holzinger <pholzing@redhat.com> wrote:
> >>>     
> >>>> Hi all,
> >>>> I found some issues with the pasta port binding logic, it does not
> >>>> correctly handle errors when trying to bind a port range.
> >>>> Let's first bind a port so we can force an error condition it:
> >>>> $ nc -l -p 8080 &
> >>>> $ pasta -t 8080  true
> >>>> Failed to bind any port for '-t 8080', exiting <-- fails as expected
> >>>> $ pasta -t 8081 -t 8080  true
> >>>> Failed to bind any port for '-t 8080', exiting <-- here it also fails
> >>>> correctly
> >>>> $ pasta -t 8080-8081  true
> >>>> <-- no error even though pasta could not bind 8080  
> >>> This is actually intended: it only fails if it can't bind *any* port in
> >>> a given range, so that users don't have to explicitly exclude ports
> >>> from ranges in case some are already taken, knowingly or not. That's
> >>> why the error message says "any port".  
> >> Ok but this results in a very inconsistent behavior. I argue this is not
> >> what a normal user expects at all.  
> > This really depends on the user. The main usage behind the current
> > behaviour is the "all" forwarding mode, as originally designed for
> > KubeVirt, now available via libvirt in two forms: -t all / -u all, or
> > giving excluded ranges only (from the man page: "Specifying excluded
> > ranges only implies that all other ports are forwarded.").
> >
> > At least this part, specifically, is an established behaviour and we
> > can't break it.
> >
> > But that's not a substantial conflict with what you're asking for
> > Podman: we can keep this behaviour only if the user asks to forward
> > (almost) "all" the ports (in whatever way). And Podman never asks for
> > that, and probably never will.
> >  
> >> It is not even documented in the man page.  
> > Right, yes, we'll need to describe it.
> >  
> >>> For two ports it probably makes no sense, but for larger ranges
> >>> excluding dozens of ports can get quite annoying for the user. And
> >>> warnings on failed bind() calls could get quite noisy, too.  
> >> At the same time this can result in a lot of unexpected problems for
> >> users as it just hides potential problems, assuming 8001-9000 are
> >> already in use then -t 8000-9000 would work and then one might
> >> reasonably expect that if they connect to any of the given ports they
> >> talk to the pasta namespace and not something else, this could be a
> >> security concern. I would say in such case yes I want to see a logged
> >> error for each individual port.  
> > I see, but again, if just port 8005 is not available, you might say
> > that having pasta succeed is a security concern because somebody can
> > "steal" the port, or that having pasta fail is a nuisance because the
> > user probably just wanted to forward the remaining 1000 ports, and
> > graceful degradation would be desirable instead.
> >
> > It's pretty impossible to draw a line in a different way that's not
> > considered inconsistent or inconvenient by some. So:  
>
> Yeah I understand that, I guess I just am missing the use case of 
> forwarding large port ranges in general as I don't understand how it is 
> used in practice. As an application inside the VM/container that just 
> binds a port it has no knowledge whenever passt/pasta forwarded the port 
> so if it happens to use one that was skipped because it was already used 
> on the host it just won't get external connections.

The "host" could be a very controlled environment, such as a dedicated
network namespace, say a Kubernetes pod. One example I have fresh in
mind, KubeVirt with Istio:

  https://kubevirt.io/user-guide/virtual_machines/istio_service_mesh/

Service meshes are usually implemented by / designed for containers. If
you move some parts (implementing a number of servers) to a virtual
machine, they don't expect to get any connection to the ports that are
normally taken on the host, but anything else is legitimate.

> I guess the answer for that is user error/problem.

In some sense, yes. Mind that we have automatic and somewhat abstracted
users, too, which is not so much the focus for Podman.

> >>> If it's a problem for Podman, I can think of two solutions. One would
> >>> be an option such as --strict-bind or suchlike (better names warmly
> >>> welcome).  
> >> It is a big problem for podman as it does not do what users want us to
> >> do. Also, because podman is smart enough to combine several ports into
> >> ranges internally for performance reasons, something like -p 80:80 -p
> >> 81:81 is always a range for podman thus there is no way for podman users
> >> to avoid hitting this problem. An opt-in option works for me but then we
> >> need to bump the minimum pasta requirement version for podman which is a
> >> bit annoying as I would need to wait until the versions lands in our CI.  
> > ...would probing for the option be a possibility? That is, run 'pasta
> > --strict-bind', and if it fails, log a warning and continue without the
> > option.
> >
> > If that's too cumbersome, then I would skip the new option and change
> > it to simply fail if we can't bind a single port (and report the single
> > error right away), unless the user requests to forward "almost all" the
> > ports, in which case we could probably warn() with a list of the ports
> > that we couldn't bind.  
> 
> I am fine with an opt in option, it just means that I will need to wait 
> a bit until the passt package landed in fedora and our CI images before 
> I can implement it. Retrying without that option is a good idea but I 
> rather just make a clean requirement of version xxx to not complicate 
> the podman code. We are in process of finalizing Podman 5.0 right now so 
> if such option could be implemented within the next 2-3 weeks that would 
> be great and I can add a note the we require passt version XXX now as 
> part of 5.0 and make it the packagers responsibility to provide that.

Okay, great. I'll send a patch in a bit.

-- 
Stefano