Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add kernel-install plugin that calls ukify #27262

Merged
merged 20 commits into from May 6, 2023
Merged

Conversation

keszybz
Copy link
Member

@keszybz keszybz commented Apr 13, 2023

Alternative for #27119.

Basic functionality works, and I'm making a draft PR for early comments. Signing is not done yet.

Also early microcode is not done at all. I think that #27111 is a better approach: let a separate plugin build a prefix initrd, instead of duplicating the logic in multiple places.

@github-actions github-actions bot added meson tests uki please-review PR is ready for (re-)review by a maintainer labels Apr 13, 2023
src/ukify/ukify.py Outdated Show resolved Hide resolved
src/kernel-install/meson.build Show resolved Hide resolved
src/kernel-install/60-ukify.install.in Show resolved Hide resolved
@yuwata yuwata added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks kernel-install and removed please-review PR is ready for (re-)review by a maintainer labels Apr 14, 2023
@poettering

This comment was marked as resolved.

@github-actions github-actions bot added sd-boot/sd-stub/bootctl please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Apr 14, 2023
@keszybz
Copy link
Member Author

keszybz commented Apr 14, 2023

Updated. I still didn't get around to signing, but I want to see if the CI passes.
Changes:

  • some tweaks that should make the tests pass
  • adjustments to meson config
  • ukify is imported, instead of being executed

meson.build Show resolved Hide resolved
src/kernel-install/60-ukify.install.in Outdated Show resolved Hide resolved
Comment on lines 188 to 199
def main():
opts = parse_args()
if opts.command != 'add':
return
if not we_are_wanted():
return

cmdline = kernel_cmdline()
if cmdline is None:
sys.exit('No cmdline configuration found')

make_uki(opts, cmdline)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now raised errors are never caught and will thus produce tracebacks if something goes wrong, the exit code will also ever be 1.

Adding a custom error and wrapping the body of main in a try block, could be used to suppress that

Suggested change
def main():
opts = parse_args()
if opts.command != 'add':
return
if not we_are_wanted():
return
cmdline = kernel_cmdline()
if cmdline is None:
sys.exit('No cmdline configuration found')
make_uki(opts, cmdline)
class UkifyInstallError(Exception):
def __init__(self, msg, rc=1):
self.msg = msg
self.rc = rc
def main():
try:
opts = parse_args()
if opts.command != 'add':
return
if not we_are_wanted():
return
cmdline = kernel_cmdline()
if cmdline is None:
sys.exit('No cmdline configuration found')
make_uki(opts, cmdline)
except UkifyInstallError as e:
print(e.msg, file=sys.stderr)
sys.exit(e.rc)

This way some of the errors could then also get an exit code of 77 and kernel_cmdline() could throw the error instead of returning None. We would also see whether an exit was from an error we already anticipated or was a bug. This would also leave the door open to add some debug environment variable to always print the stack trace.

When going this route, the raises should look like, e.g.

except ValueError as e:
    raise UkifyInstallError("Kernel options must not start with '@'", 77) from e

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think it makes sense to suppress tracebacks. But I'm leaving that for later… As the discussion in mkosi shows, getting this just right is quite hard, and generally during early development its convenient to rather have too many tracebacks than too few.


def log(*args, **kwargs):
if VERBOSE:
print(*args, **kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cpython's output is always buffered, maybe to be more shell like this should be

Suggested change
print(*args, **kwargs)
print(*args, **kwargs, flush=True)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that output is flushed on each newline, at least when writing to a terminal. So I don't think we need to do explicit flushing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not sure it's strictly necessary, I just remember running into this problem in the past, but that was code using multi processing, so it had a lot of children.

An alternative to the explicit flush in the function, would be using cpython's -u option

Force the stdout and stderr streams to be unbuffered. This option has no effect on the stdin stream.

This might be a bit hard, since using /usr/bin/env this would need the -S option, which should be now be available everywhere, though I'm not sure about.

On that note, I noticed that this plugin uses env. The other kernel-install plugins use /bin/sh, e.g. force the default system shell. Should this plugin maybe also default to the system python path?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not sure it's strictly necessary, I just remember running into this problem in the past, but that was code using multi processing, so it had a lot of children.

An alternative to the explicit flush in the function, would be using cpython's -u option

Force the stdout and stderr streams to be unbuffered. This option has no effect on the stdin stream.

This might be a bit hard, since using /usr/bin/env this would need the -S option, which should be now be available everywhere, though I'm not sure about.

Is there some problem that this is intended to solve? If not, I would keep default behaviour.

On that note, I noticed that this plugin uses env. The other kernel-install plugins use /bin/sh, e.g. force the default system shell. Should this plugin maybe also default to the system python path?

We use env for python code. /bin/bash and /bin/sh are standarized locations, but the place where python3 is installed varies. Some distros will replace the path in scripts to some desired system path. It is better leave it to them. For non-distro installations, using env is fine, it "just works".

Comment on lines 174 to 184
# Punish me harder.
# We want this:
# ukify = importlib.machinery.SourceFileLoader('ukify', UKIFY).load_module()
# but it throws a DeprecationWarning.
# https://stackoverflow.com/questions/67631/how-can-i-import-a-module-dynamically-given-the-full-path
# https://github.com/python/cpython/issues/65635
# offer "explanations", but to actually load a python file without a .py extension,
# the "solution" is 4+ incomprehensible lines.
# The solution with runpy gives a dictionary, which isn't great, but will do.
ukify = runpy.run_path(UKIFY, run_name='ukify')
ukify['main'](args)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't be so hard on yourself, I actually think using runpy is a good and readable solution! :)

I do think though, that maybe this comment doesn't need to be shipped where everybody can read it. While I agree, that the one-liner is preferable and something like it should be added back to the stdlib, the alternative they provide as a recipe is not that inscrutable. The reason is that the Python devs wanted to split getting information about a module from loading it, which is why they added the module specs so now initialising a module is this three step process of getting the spec, making the module from the spec and then executing it in a clean namespace. Hooking the module up to sys.modules would probably not be necessary for ukify, since that seems to be needed to make relative imports work, but ukify is a single file.

That being said, I think I would have gone with your first solution of shelling out to ukify, as I don't really see an advantage in this dance, no matter which version.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first comment on the stackoverflow question has 138139 upvotes, and it expresses my sentiment very well. I don't understand why Python developers are trying so hard to make this hard and not listening to people telling them that the new "solution" is not great.

Not shelling out has advantages. For the next version of this PR I reworked the ukify code to skip the option-parsing step. This avoids pitfalls with syntax and is generally nicer. It's certainly more work, but I hope it'll be worth it in the long run.


def we_are_wanted() -> bool:
KERNEL_INSTALL_LAYOUT = os.getenv('KERNEL_INSTALL_LAYOUT')

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if KERNEL_INSTALL_UKI_GENERATOR is None or KERNEL_INSTALL_UKI_GENERATOR == '':
log('KERNEL_INSTALL_UKI_GENERATOR is not defined, using ukify as the default.')
return True

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ukify should be the default KERNEL_INSTALL_UKI_GENERATOR if none is specified.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that's the best choice, since there's a a few initrd generators out there, that already build UKIs but might not handle KERNEL_INSTALL_UKI_GENERATOR yet.

Since I think most initrd generators have settled for the 50- prefix and this is at 60-, a better solution might be to default to ukify if the variable is unset and uki.efi does not exist in the staging directory.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to change this, I think the right place would be kernel-install. Then all the plugins get a consistent view.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point there was, that kernel-install cannot know whether anything down the road will be able to handle KERNEL_INSTALL_UKI_GENERATOR, so a fallback would have to be local to 60-ukify.install, but thinking some more about this, I don't think this is worth it. This is something distros need to decide. Packagers can set the default for KERNEL_INSTALL_UKI_GENERATOR to whatever makes sense together with their initrd generator that might or might not handle building UKIs and tinkerers can be expected to add their own plugins for kernel-install.

@keszybz keszybz added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Apr 20, 2023
@poettering
Copy link
Member

(btw, iirc dracut currently optoinally supports the rescue image concept, i.e. spitting out two images instead of one that are registered with the boot loader. Not sure this makes too much sense, but can we cover that somehow?)

@behrmann
Copy link
Contributor

(btw, iirc dracut currently optoinally supports the rescue image concept, i.e. spitting out two images instead of one that are registered with the boot loader. Not sure this makes too much sense, but can we cover that somehow?)

This has come up before in discussions with Debian and Arch. I'm not entirely sold on the concept of a rescue image - I think boot counting and falling back to what booted previously, but generating multiple output for a single kernel version seems useful. Specifically for Arch this came up for mkinitcpio which has a concept of generating multiple profiles of configs (called presets) for a given kernel. This is currently not not a natural fit for kernel-install, but it would make kernel-install more useful to more people.

@keszybz
Copy link
Member Author

keszybz commented Apr 21, 2023

I'm leaving the question of rescue images on the side for now. I think it'd make sense to support this, because some people will want it. But I think it's best supported as a separate plugin that loads a separate configuration file. Since this is python, we can easily create a second module that imports this one, reconfigures a few paths and some other details, and then builds a second kernel. But let's leave that for later.


Update. I addressed the comments above, or I commented why not (or why not yet).

I found a very strange thing: test_ukify.py is executed in the CI, and reported as passing, but the tests were broken. I don't understand what's going on here. Let's see if the tests pass now.

ukify.py is now imported as a module and the relevant parts of the code are invoked as functions. There are some uglinesses here and there, but nothing too major.

A config file is supported for ukify, so pretty much everything can be configured for 60-ukify.install via this config file.

I also added some more tests (and found some bugs on the way…).

@bluca bluca added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed tests sd-boot/sd-stub/bootctl good-to-merge/with-minor-suggestions labels May 4, 2023
for section_name, section in cp.items():
idx = section_name.find(':')
if idx > 0:
section_name, group = section_name[:idx+1], section_name[idx+1:]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure: The inclusion of ":" at the end of section_name here is for the config examples?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The section is identified via config_key, which includes :. E.g. config_key = 'PCRSignature:/PCRPrivateKey'. It is also shown in the output.

src/ukify/ukify.py Outdated Show resolved Hide resolved

def kernel_cmdline_base() -> typing.Optional[typing.List[str]]:
if root := os.getenv('KERNEL_INSTALL_CONF_ROOT'):
return Path(root).joinpath('cmdline').read_text().split()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's reasonable, but I'm pretty sure that might be one of the errors that will be hit a bit more often, so I think catching it and printing a clear error would be helpful.

except FileNotFoundError:
continue

options = Path('/proc/cmdline').read_text().split()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could /proc be missing, e.g. when kernel-install starts working on images? One can expect the others to be there, but an error message will be nicer than a stack trace.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kernel-install itself might support --root, but if plugins are invoked, they'll most likely be invoked with the API filesystems mounted. So it's quite unlikely that this script can be invoked without /proc.

man/ukify.xml Outdated Show resolved Hide resolved
man/ukify.xml Outdated Show resolved Hide resolved
keszybz added 13 commits May 5, 2023 18:42
In some ways this is similar to mkosi: we have a argparse.ArgumentParser()
with a bunch of options, and a configparser.ConfigParser() with an
overlapping set of options. Many options are settable in both places, but
not all. In mkosi, we define this in three places (a dataclass, and a
function for argparse, and a function for configparser). Here, we have one
huge list of ConfigItem instances. Each instance specifies the full metadata
for both parsers. Argparse generates a --help string for all the options,
and we also append a config file sample to --help based on the ConfigItem
data:

$ python src/ukify/ukify.py --help|tail -n 25
config file:
  [UKI]
  Linux = LINUX
  Initrd = INITRD…
  Cmdline = TEXT|@path
  OSRelease = TEXT|@path
  DeviceTree = PATH
  Splash = BMP
  PCRPKey = KEY
  Uname = VERSION
  EFIArch = ia32|x64|arm|aa64|riscv64
  Stub = STUB
  PCRBanks = BANK…
  SigningEngine = ENGINE
  SecureBootPrivateKey = SB_KEY
  SecureBootCertificate = SB_CERT
  SignKernel = SIGN_KERNEL

  [PCRSignature:NAME]
  PCRPrivateKey = PATH
  PCRPublicKey = PATH
  Phases = PHASE-PATH…

While writing this I needed to check the argument parsing, so I added
a --summary switch. It just pretty-prints the resulting option dictionary:

$ python src/ukify/ukify.py /efi//3a9d668b4db749398a4a5e78a03bffa5/6.2.11-300.fc38.x86_64/linux /efi//3a9d668b4db749398a4a5e78a03bffa5/6.2.11-300.fc38.x86_64/initrd --pcr-private-key=PRIV.key --pcr-public-key=PUB.key --config=man/ukify-example.conf --summary
Host arch 'x86_64', EFI arch 'x64'
{'_groups': [0, 'initrd', 'system'],
 'cmdline': 'A1 B2 C3',
 'config': 'man/ukify-example.conf',
 'devicetree': None,
 'efi_arch': 'x64',
 'initrd': [PosixPath('initrd1'),
            PosixPath('initrd2'),
            PosixPath('initrd3'),
            PosixPath('/efi/3a9d668b4db749398a4a5e78a03bffa5/6.2.11-300.fc38.x86_64/initrd')],
 'linux': PosixPath('/efi/3a9d668b4db749398a4a5e78a03bffa5/6.2.11-300.fc38.x86_64/linux'),
 'measure': None,
 'os_release': PosixPath('/etc/os-release'),
 'output': 'linux.efi',
 'pcr_banks': ['sha1', 'sha384'],
 'pcr_private_keys': [PosixPath('PRIV.key'),
                      PosixPath('pcr-private-initrd-key.pem'),
                      PosixPath('pcr-private-system-key.pem')],
 'pcr_public_keys': [PosixPath('PUB.key'),
                     PosixPath('pcr-public-initrd-key.pem'),
                     PosixPath('pcr-public-system-key.pem')],
 'pcrpkey': None,
 'phase_path_groups': [None,
                       ['enter-initrd'],
                       ['enter-initrd:leave-initrd',
                        'enter-initrd:leave-initrd:sysinit',
                        'enter-initrd:leave-initrd:sysinit:ready']],
 'sb_cert': PosixPath('mkosi.secure-boot.crt'),
 'sb_key': PosixPath('mkosi.secure-boot.key'),
 'sections': [],
 'sign_kernel': None,
 'signing_engine': None,
 'splash': None,
 'stub': PosixPath('/usr/lib/systemd/boot/efi/linuxx64.efi.stub'),
 'summary': True,
 'tools': None,
 'uname': None}

With --summary, existence of input paths is not checked. I think we'll
want to show them, instead of throwing an error, but in red, similarly to
'bootctl list'.

This also fixes tests which were failing with e.g.
E       FileNotFoundError: [Errno 2] No such file or directory: '/ARG1'
=========================== short test summary info ============================
FAILED ../src/ukify/test/test_ukify.py::test_parse_args_minimal - FileNotFoun...
FAILED ../src/ukify/test/test_ukify.py::test_parse_args_many - FileNotFoundEr...
FAILED ../src/ukify/test/test_ukify.py::test_parse_sections - FileNotFoundErr...
=================== 3 failed, 10 passed, 3 skipped in 1.51s ====================
We don't lowercase acronyms in systemd usually.
Remove unnused f'' prefix to avoid a pylint warning.
60-ukify.install calls ukify with a config file, so singing and policies and
splash will be done through the ukify config file, without 60-ukify.install
knowing anything directly.

In meson.py, the variable for loaderentry.install.in is used just once, let's
drop it. (I guess this approach was copied from kernel_install_in, which is
used in another file.)

The general idea is based on cvlc12's systemd#27119, but now in Python instead of
bash.
We install a kernel with layout=uki and uki_generator=ukify, and test
that a UKI gets installed in the expected place. The two plugins cooperate,
so it's easiest to test them together.
Without this, build would fail if the stub is not available in /usr/lib/.
0ccfd35 implemented one of the items, and this
pull requests handles the other one.
Note to self: PEP 585 introduced using collection types as types,
and is available since 3.9. PEP 604 allows writing unions with "|",
but is only available since 3.10, so not yet here because we maintain
compat with 3.9.
Oops. This explains why the tests were "passing" in CI even
though a direct pytest invocation would fail.
The usual approach is to put 'addopts = --flakes' in setup.cfg. Unfortunately
this fails badly when pytest-flakes is not installed:
  ERROR: usage: test_ukify.py [options] [file_or_dir] [file_or_dir] [...]
  test_ukify.py: error: unrecognized arguments: --flakes

pytest-flakes is not packaged everywhere, and this test is not very important,
so let's just do it only if pytest-flakes is available. We now detect if
pytest-flakes is available and only add '--flakes' conditionally. This
unfortunately means that when invoked via 'pytest' or directly as
'src/ukify/test/test_ukify.py', '--flakes' will not be appended automatically.
But I don't see a nice way to achieve previous automatic behaviour.

(I first considered making 'setup.cfg' templated. But then it is created
in the build directory, but we would need it in the source directory for
pytest to load it automatically. So to load the file, we'd need to give an
argument to pytest anyway, so we don't gain anything with this more complex
approach.)
Some web searches say that it's packaged for those distros and not the others…

v2:
- drop arch. https://aur.archlinux.org/packages/python-pytest-flakes exists,
  but installation fails in CI.
As in mkosi(1), let's describe the config file and commandline options
together. This is nice for us, because we don't need to duplicate descriptions
and we're less likely to forget to update one place or the other. This is also
nice for users, because they can easily figure out what can be configured
where.

The options are now ordered by config file section.

--summary was not described before.

More examples are added.
@keszybz
Copy link
Member Author

keszybz commented May 5, 2023

Updated with the smaller changes requested. I didn't add the handling of errors. This will be quite a lot of work, and I'd like to get this version finally merged.

@keszybz
Copy link
Member Author

keszybz commented May 6, 2023

Processing triggers for initramfs-tools (0.140ubuntu13.1) ...
update-initramfs: Generating /boot/initrd.img-5.15.0-71-generic
No lz4 in /usr/bin:/sbin:/bin, using gzip
Errors were encountered while processing:
 udev
W: --force-yes is deprecated, use one of the options starting with --allow instead.
E: Sub-process /usr/bin/dpkg returned an error code (1)
autopkgtest [17:43:42]: ERROR: erroneous package: installation of basic binaries failed, exit code 100
blame: https://salsa.debian.org/systemd-team/systemd.git#upstream-ci
badpkg: installation of basic binaries failed, exit code 100

Whatever. Not related.

@keszybz keszybz added ci-failure-appears-unrelated and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels May 6, 2023
@keszybz keszybz merged commit 9dfed0d into systemd:main May 6, 2023
46 of 47 checks passed
@keszybz keszybz deleted the ukify-install branch May 6, 2023 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

7 participants