stages: ensure we do not allow \n in the ssh key or anaconda breaks

[mvo5]

There was a bug found in
osbuild/bootc-image-builder#363

where a stray newlnine in the ssh key broke the kickstart file.

[achilleas-k]

We've had the key option for a while now (887e1bd) so I don't know if it's good to restrict it.
Newlines probably work when escaped, right? Does this regex block that?

[mvo5]

Closing for now based on the comment from achilleas

[mvo5]

We've had the key option for a while now (887e1bd) so I don't know if it's good to restrict it. Newlines probably work when escaped, right? Does this regex block that?

I looked into this a bit now, if a user passed a config that contains a "\n" in the json it would result in the following. Here is a test that shows what happens (note the \n at the end of "some-ssh-key" in the test input):

$ git diff
diff --git a/stages/test/test_kickstart.py b/stages/test/test_kickstart.py
index 9bd65ef6..6aaab4ee 100644
--- a/stages/test/test_kickstart.py
+++ b/stages/test/test_kickstart.py
@@ -241,6 +241,8 @@ TEST_INPUT = [
     ({"ostreecontainer": {"transport": "dir", "url": "/run/install/repo/container", }, },
      "ostreecontainer --url=/run/install/repo/container --transport=dir",),
     ({"bootloader": {"append": "karg1 karg2=0"}}, "bootloader --append='karg1 karg2=0'"),
+    ({"users": {"foo": {"key": "some-ssh-key\n"}}},
+     'sshkey --username foo "some-ssh-key"'),
 ]

$ PYTHONPATH=. pytest  ./stages/test/test_kickstart.py::test_kickstart_valid
============================= test session starts ==============================
platform linux -- Python 3.9.18, pytest-6.2.2, py-1.10.0, pluggy-0.13.1
rootdir: /home/mvogt/devel/osbuild/osbuild
collected 83 items                                                             

stages/test/test_kickstart.py .......................................... [ 50%]
........................................F                                [100%]

=================================== FAILURES ===================================
___ test_kickstart_valid[test_input82-sshkey --username foo "some-ssh-key"] ____
...
----------------------------- Captured stdout call -----------------------------
created kickstarted at: kickstart/kfs.cfg

user --name foo
sshkey --username foo "some-ssh-key
"

General error in input file:  No closing quotation
=========================== short test summary info ============================
FAILED stages/test/test_kickstart.py::test_kickstart_valid[test_input82-sshkey --username foo "some-ssh-key"]

so this was never valid input and would always break the kickstart file.

I have no strong opinion here fwiw, I am happy to also fix this implicitly via eg.

diff --git a/stages/org.osbuild.kickstart b/stages/org.osbuild.kickstart
index 21d03ace..3a84c4b5 100755
--- a/stages/org.osbuild.kickstart
+++ b/stages/org.osbuild.kickstart
@@ -62,7 +62,7 @@ def make_users(users: Dict) -> List[str]:
 
         key = opts.get("key")
         if key:
-            res.append(f'sshkey --username {name} "{key}"')
+            res.append(f'sshkey --username {name} "{key.strip()}"')
 
     return res
 
diff --git a/stages/test/test_kickstart.py b/stages/test/test_kickstart.py
index 9bd65ef6..6aaab4ee 100644
--- a/stages/test/test_kickstart.py
+++ b/stages/test/test_kickstart.py
@@ -241,6 +241,8 @@ TEST_INPUT = [
     ({"ostreecontainer": {"transport": "dir", "url": "/run/install/repo/container", }, },
      "ostreecontainer --url=/run/install/repo/container --transport=dir",),
     ({"bootloader": {"append": "karg1 karg2=0"}}, "bootloader --append='karg1 karg2=0'"),
+    ({"users": {"foo": {"key": "some-ssh-key\n"}}},
+     'sshkey --username foo "some-ssh-key"'),
 ]
 
 

[achilleas-k]

Stripping is a good idea, I think.

I'm curious, does sshkey --username foo "key1\\nkey2" create an authorized_keys file with multiple lines? I think it wont, but I'm wondering if it's one of those things that accidentally works and we've been supporting it all along.

[supakeen]

Without passing value judgement I just want to point out that it is OK to narrow the schema if the previous schema could not lead to any valid image (which I think is what @achilleas-k is also trying to figure out) :)

[achilleas-k]

Without passing value judgement I just want to point out that it is OK to narrow the schema if the previous schema could not lead to any valid image (which I think is what @achilleas-k is also trying to figure out) :)

Yes, exactly :)

[mvo5]

I did a bit of research, so pykickstart is really about single lines (unless % is used), c.f. https://github.com/pykickstart/pykickstart/blob/master/pykickstart/parser.py#L725 and the lineno = self._readSection(lineIter, lineno) in there. The sshkey command seems to not handle \n (c.f. https://github.com/pykickstart/pykickstart/blob/master/pykickstart/commands/sshkey.py#L93).

But maybe a different idea is more fruitful, I opened #1789 with the idea to just run ksvalidator which knows best about this kind of validations :) (it does require a slightly bigger buildroot though)

[mvo5]

Closing for now as there seems to be no clear consensus that this is the right fix (and #1789 may actually be a nicer way to solve the general issue even if only at build time instead of statically).