| Message ID | 20221114101907.626251-1-mikko.rapeli@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | qemurunner.py: support setting slirp host ip address | expand |
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
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
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
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:
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(-)