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

[Paul Holzinger <pholzing@redhat.com>]

On 14/02/2024 10:15, Stefano Brivio wrote:
> On Mon, 12 Feb 2024 17:56:32 +0100
> Stefano Brivio <sbrivio@redhat.com> wrote:
>
>> 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.
> After thinking about it a bit further: adding yet a new option doesn't
> make a lot of sense if all we want to preserve is the established
> behaviour for -t / -u all, and while we can document everything, it
> makes things even more convoluted (and harder to document).
>
> In the 'all' case, or with excluded ranges only, we're very likely to
> fail to bind some ports, and the user can very reasonably expect it, so
> we can have a different behaviour for those types of port forwarding
> specifiers and document it. It actually makes the code shorter, perhaps
> simpler.
>
> In any other case, we'll fail as you suggested and report the error. If
> the user wants to have this same behaviour for -t all, they can also
> write it as -t 0-65535 -- that's a rather clear indication that they
> want to bind *all* the ports.
>
> With the patch I just posted:
>
>    $ cat /proc/sys/net/ipv4/ip_unprivileged_port_start
>    21
>    $ ./passt -t 20
>    Failed to bind port 20 (Permission denied) for option '-t 20', exiting
>    $ ./passt -t 20-5000
>    Failed to bind port 20 (Permission denied) for option '-t 20-5000', exiting
>    $ ./passt -t 23 -t 18
>    Failed to bind port 18 (Permission denied) for option '-t 18', exiting
>    $ ./passt -t 22-23
>    Failed to bind port 22 (Address already in use) for option '-t 22-23', exiting
>
> but:
>
>    $ ./passt -t ~0,~21-65535
>    Failed to bind any port for '-t ~0,~21-65535', exiting
>
> because this approximates the -t all behaviour, minus some ports.
>
> Let me know if this works for you.
Sure this works for me, thanks.