[PATCH] qemurunner.py: support setting slirp host ip address

[PATCH] qemurunner.py: support setting slirp host ip address

[Mikko Rapeli]

By default host side IP address is not set and qemu listens
on all IP addresses on the host machine which is not a good
idea when images have root login enabled without password.
It make sense to listen only on localhost IP address 127.0.0.1 using
config:

QB_SLIRP_OPT = "-netdev user,id=net0,hostfwd=tcp:127.0.0.1:2222-:22"

Support detecting port number from this too.

Signed-off-by: Mikko Rapeli <mikko.rapeli@linaro.org>
---
 meta/lib/oeqa/utils/qemurunner.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/meta/lib/oeqa/utils/qemurunner.py b/meta/lib/oeqa/utils/qemurunner.py
index e602399232..f175f8a1de 100644
--- a/meta/lib/oeqa/utils/qemurunner.py
+++ b/meta/lib/oeqa/utils/qemurunner.py
@@ -401,7 +401,8 @@ class QemuRunner:
                 cmdline = re_control_char.sub(' ', cmdline)
             try:
                 if self.use_slirp:
-                    tcp_ports = cmdline.split("hostfwd=tcp::")[1]
+                    tcp_ports = cmdline.split("hostfwd=tcp:")[1]
+                    tcp_ports = tcp_ports.split(":")[1]
                     host_port = tcp_ports[:tcp_ports.find('-')]
                     self.ip = "localhost:%s" % host_port
                 else:
-- 
2.34.1

Re: [OE-core] [PATCH] qemurunner.py: support setting slirp host ip address

[Quentin Schulz]

Hi Mikko,

On 11/14/22 11:19, Mikko Rapeli wrote:
> By default host side IP address is not set and qemu listens
> on all IP addresses on the host machine which is not a good
> idea when images have root login enabled without password.
> It make sense to listen only on localhost IP address 127.0.0.1 using
> config:
> 
> QB_SLIRP_OPT = "-netdev user,id=net0,hostfwd=tcp:127.0.0.1:2222-:22"
> 
> Support detecting port number from this too.
> 
> Signed-off-by: Mikko Rapeli <mikko.rapeli@linaro.org>
> ---
>   meta/lib/oeqa/utils/qemurunner.py | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/meta/lib/oeqa/utils/qemurunner.py b/meta/lib/oeqa/utils/qemurunner.py
> index e602399232..f175f8a1de 100644
> --- a/meta/lib/oeqa/utils/qemurunner.py
> +++ b/meta/lib/oeqa/utils/qemurunner.py
> @@ -401,7 +401,8 @@ class QemuRunner:
>                   cmdline = re_control_char.sub(' ', cmdline)
>               try:
>                   if self.use_slirp:
> -                    tcp_ports = cmdline.split("hostfwd=tcp::")[1]
> +                    tcp_ports = cmdline.split("hostfwd=tcp:")[1]
> +                    tcp_ports = tcp_ports.split(":")[1]
>                       host_port = tcp_ports[:tcp_ports.find('-')]
>                       self.ip = "localhost:%s" % host_port

But localhost is enforced here?

This patch basically allows to pass
hostfwd=tcp:127.0.0.1:2222-:22
instead of
hostfwd=tcp::2222-:22
but with the exact same result (which is, localhost:2222 will be used?)

Also, this could be migrated to using re instead of doing manual lookups.

I'm not sure the commit log matches what the commit is actually doing?

Cheers,
Quentin

Re: [OE-core] [PATCH] qemurunner.py: support setting slirp host ip address

[Mikko Rapeli]

Hi,

On Mon, Nov 14, 2022 at 11:31:11AM +0100, Quentin Schulz wrote:
> Hi Mikko,
> 
> On 11/14/22 11:19, Mikko Rapeli wrote:
> > By default host side IP address is not set and qemu listens
> > on all IP addresses on the host machine which is not a good
> > idea when images have root login enabled without password.
> > It make sense to listen only on localhost IP address 127.0.0.1 using
> > config:
> > 
> > QB_SLIRP_OPT = "-netdev user,id=net0,hostfwd=tcp:127.0.0.1:2222-:22"
> > 
> > Support detecting port number from this too.
> > 
> > Signed-off-by: Mikko Rapeli <mikko.rapeli@linaro.org>
> > ---
> >   meta/lib/oeqa/utils/qemurunner.py | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/meta/lib/oeqa/utils/qemurunner.py b/meta/lib/oeqa/utils/qemurunner.py
> > index e602399232..f175f8a1de 100644
> > --- a/meta/lib/oeqa/utils/qemurunner.py
> > +++ b/meta/lib/oeqa/utils/qemurunner.py
> > @@ -401,7 +401,8 @@ class QemuRunner:
> >                   cmdline = re_control_char.sub(' ', cmdline)
> >               try:
> >                   if self.use_slirp:
> > -                    tcp_ports = cmdline.split("hostfwd=tcp::")[1]
> > +                    tcp_ports = cmdline.split("hostfwd=tcp:")[1]
> > +                    tcp_ports = tcp_ports.split(":")[1]
> >                       host_port = tcp_ports[:tcp_ports.find('-')]
> >                       self.ip = "localhost:%s" % host_port
> 
> But localhost is enforced here?

Is it really? Where?

With default "-netdev user,id=net0,hostfwd=tcp::2222-:22" I am able
to login using all local IP addresses:

$ nc 192.168.1.103 2222
SSH-2.0-OpenSSH_8.9
^C

$ nc -v -v -v 127.0.0.1 2222
Connection to 127.0.0.1 2222 port [tcp/*] succeeded!
SSH-2.0-OpenSSH_8.9
^C

The open port 2222 show on the build machine with:

$ lsof -i|grep qemu | grep 2222
qemu-syst  170445 builder   12u  IPv4 45057952      0t0  TCP *:2222 (LISTEN)

By using "hostfwd=tcp:127.0.0.1:2222-:22" this reduces to the more safe:

$ lsof -i|grep qemu
qemu-syst  127592 builder   12u  IPv4 44993375      0t0  TCP localhost:2222 (LISTEN)

I don't dare to make that the new default so just enabling
runqemu to work when user configures the host IP address like this.

> This patch basically allows to pass
> hostfwd=tcp:127.0.0.1:2222-:22
> instead of
> hostfwd=tcp::2222-:22
> but with the exact same result (which is, localhost:2222 will be used?)

Nope, now the other non-local IP addresses are not open for the port
2222 on the machine running qemu. The localhost:2222 works in both
cases.

> Also, this could be migrated to using re instead of doing manual lookups.

Yes, but re is more resource consuming.

> I'm not sure the commit log matches what the commit is actually doing?

How could I improve that? If I manually start the qemu machine with
hostfwd=tcp:127.0.0.1:2222-:22 the machine works but with runqemu it
fails due to a confusing error about detecting IP address. I could add that
if it helps. With this patch runqemu doesn't care how the IP address
is configured when looking for the port number, and also doesn't fail
when it's set.

Cheers,

-Mikko

Re: [OE-core] [PATCH] qemurunner.py: support setting slirp host ip address

[Quentin Schulz]

Hi Mikko,

On 11/14/22 11:59, Mikko Rapeli wrote:
> Hi,
> 
> On Mon, Nov 14, 2022 at 11:31:11AM +0100, Quentin Schulz wrote:
>> Hi Mikko,
>>
>> On 11/14/22 11:19, Mikko Rapeli wrote:
>>> By default host side IP address is not set and qemu listens
>>> on all IP addresses on the host machine which is not a good
>>> idea when images have root login enabled without password.
>>> It make sense to listen only on localhost IP address 127.0.0.1 using
>>> config:
>>>
>>> QB_SLIRP_OPT = "-netdev user,id=net0,hostfwd=tcp:127.0.0.1:2222-:22"
>>>
>>> Support detecting port number from this too.
>>>
>>> Signed-off-by: Mikko Rapeli <mikko.rapeli@linaro.org>
>>> ---
>>>    meta/lib/oeqa/utils/qemurunner.py | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/meta/lib/oeqa/utils/qemurunner.py b/meta/lib/oeqa/utils/qemurunner.py
>>> index e602399232..f175f8a1de 100644
>>> --- a/meta/lib/oeqa/utils/qemurunner.py
>>> +++ b/meta/lib/oeqa/utils/qemurunner.py
>>> @@ -401,7 +401,8 @@ class QemuRunner:
>>>                    cmdline = re_control_char.sub(' ', cmdline)
>>>                try:
>>>                    if self.use_slirp:
>>> -                    tcp_ports = cmdline.split("hostfwd=tcp::")[1]
>>> +                    tcp_ports = cmdline.split("hostfwd=tcp:")[1]
>>> +                    tcp_ports = tcp_ports.split(":")[1]
>>>                        host_port = tcp_ports[:tcp_ports.find('-')]
>>>                        self.ip = "localhost:%s" % host_port
>>
>> But localhost is enforced here?
> 
> Is it really? Where?
> 

I clearly misunderstood the code only glancing at it. QB_SLIRP_OPT will 
be passed to QEMU before that, and then we read the kernel cli from QEMU 
PID to print something nice.

Therefore, the printed message will always mention localhost:<host_port> 
as being the "Target IP" (see a few lines below in the code), which, if 
I'm not mistaken, might be incorrect depending on what you pick in the 
hostaddr field, but that's another topic.

> With default "-netdev user,id=net0,hostfwd=tcp::2222-:22" I am able
> to login using all local IP addresses:
> 
> $ nc 192.168.1.103 2222
> SSH-2.0-OpenSSH_8.9
> ^C
> 
> $ nc -v -v -v 127.0.0.1 2222
> Connection to 127.0.0.1 2222 port [tcp/*] succeeded!
> SSH-2.0-OpenSSH_8.9
> ^C
> 
> The open port 2222 show on the build machine with:
> 
> $ lsof -i|grep qemu | grep 2222
> qemu-syst  170445 builder   12u  IPv4 45057952      0t0  TCP *:2222 (LISTEN)
> 
> By using "hostfwd=tcp:127.0.0.1:2222-:22" this reduces to the more safe:
> 
> $ lsof -i|grep qemu
> qemu-syst  127592 builder   12u  IPv4 44993375      0t0  TCP localhost:2222 (LISTEN)
> 
> I don't dare to make that the new default so just enabling
> runqemu to work when user configures the host IP address like this.
> 

Maybe we could make it the default if it's more secure but that would be 
a different commit anyways :)

>> This patch basically allows to pass
>> hostfwd=tcp:127.0.0.1:2222-:22
>> instead of
>> hostfwd=tcp::2222-:22
>> but with the exact same result (which is, localhost:2222 will be used?)
> 
> Nope, now the other non-local IP addresses are not open for the port
> 2222 on the machine running qemu. The localhost:2222 works in both
> cases.
> 
>> Also, this could be migrated to using re instead of doing manual lookups.
> 
> Yes, but re is more resource consuming.
> 
>> I'm not sure the commit log matches what the commit is actually doing?
> 
> How could I improve that? If I manually start the qemu machine with
> hostfwd=tcp:127.0.0.1:2222-:22 the machine works but with runqemu it
> fails due to a confusing error about detecting IP address. I could add that
> if it helps. With this patch runqemu doesn't care how the IP address
> is configured when looking for the port number, and also doesn't fail
> when it's set.
> 

Yes that was exactly what was missing for me to understand this patch. I 
thought the code section I was looking at was the one handling your 
parameters, and there you can see that the hostaddr isn't actually being 
read, directly assumed to be localhost, hence my confusion.

The issue is not that it's not possible, since QEMU actually supports 
that, like you rightfully mentioned. It's that our code parsing this 
QEMU kernel cli argument fails.

I think the important part here is something like:
"""
hostfwd QEMU option in QB_SLIRP_OPT currently only supports *not* 
passing any hostaddr, otherwise failing to parse the option and thus 
fails runqemu.

While the default is to not pass any hostaddr, binding all interfaces on 
the host, it is not secure so it should be allowed to pass a hostaddr in 
hostfwd via QB_SLIRP_OPT QEMU option. e.g. QB_SLIRP_OPT = "-netdev 
user,id=net0,hostfwd=tcp:127.0.0.1:2222-:22" to bind to the loopback 
interface only.
"""

but this is just how *I* would have written it and I guess there will 
always be someone for whom a git commit log won't make much sense on 
first read.

I realize this is because I didn't take the time to understand the code 
properly, sorry to have voiced my doubts on this without properly 
checking first.

That being said, it does what it says it does, so:
Reviewed-by: Quentin Schulz <foss+yocto@0leil.net>

Thanks,
Quentin