mkosi: add support for PKCS7 signed images #798

poettering · 2021-09-08T07:42:58Z

This adds support for PKCS7 signed Verity disk images, compatible with the kernel's logic for that.

mkosi/__init__.py

keszybz

Looks very reasonable.

mkosi/__init__.py

poettering · 2021-09-09T19:39:05Z

systemd side for picking up these embedded signatures is in systemd/systemd#20691

poettering · 2021-09-10T20:16:42Z

this is no longer wip. Should be good for review/merge. PTAL!

mkosi/__init__.py

mkosi.md

behrmann

@poettering could you add types-openssl-python to ci-unit-test.yml, so that

    - name: Install
      run: |
-       python3 -m pip install pytest mypy isort pyflakes
+       python3 -m pip install pytest mypy types-openssl-python isort pyflakes
        npm install -g pyright

I've only quickly ran mypy with this and will probably not manage to have a further look at this before next week, but here are the easy to fix typing changes (I have never used the openssl API, though, so beware, I don't know whether the typing information is correct). Beyond that, mypy mostly stumbles over all the typing for internal functions that it doesn't have. For now the easiest solution would be to smother all of this with type ignore hints, but we can probably add some typing later and maybe upstream that to typeshed.

mkosi/__init__.py

keszybz · 2021-09-13T05:50:20Z

This needs another round…

behrmann · 2021-09-13T14:17:31Z

I've opened poettering#4, since this was a bit easier than doing this via the review interface. I think exchanging pyopenssl for cryptography is were the ecosystem is going and what upstream itself recommends. The backend is, as far as I understand, openssl, too, but the API seems a lot nicer and it is already fully typed.

With that the typing checks run through, but for some reason the unit tests are still borked.

poettering · 2021-09-13T14:35:38Z

I force pushed a new version, still using the OpenSSL thing, but with all issues supposedly fixed.

Using the OpenSSL module indeed seems a bit awkward, if there's now something more pythonic, that's great. I was wondering about that when I picked OpenSSL — which I picked simply for the reason it has the most hits on google.

(the "genkey" code should be ported over too btw)

poettering · 2021-09-13T14:37:28Z

not sure i grok the import ordering thing? is it that isort doesn't like imports inside of functions?

behrmann · 2021-09-13T15:10:15Z

not sure i grok the import ordering thing? is it that isort doesn't like imports inside of functions?

No, that is isort pedantically disliking the empty line (line 40)

behrmann · 2021-09-13T15:13:21Z

I force pushed a new version, still using the OpenSSL thing, but with all issues supposedly fixed.

There will be a few others. mypy will still stumble over the internals of OpenSSL (which could be silenced, with poettering#4 being the alternative), but there's also errors in the CLI unit tests I haven't yet looked at (and which are also present in poettering#4).

mkosi/__init__.py

poettering · 2021-09-13T19:04:42Z

Force pushed a new version now, incorporating @behrmann's work on python-cryptography. Alas, it doesn't work anymore. Something is broken with inserting the verity-sig partition after we put it together. I have the strong suspicion that #791 is at fault for that.

keszybz · 2021-09-13T19:06:12Z

I have the strong suspicion that #791 is at fault for that.

I'll take a look tomorrow.

keszybz · 2021-09-16T08:29:39Z

Sorry for the delay. I made a huge refactoring of the sfdisk code, but it still needs some debugging. I'm not working today, but hopefully it'll be ready on Monday.

lgtm-com · 2021-09-28T16:12:36Z

This pull request fixes 1 alert when merging f5eccf9 into f0f67d4 - view on LGTM.com

fixed alerts:

1 for Clear-text logging of sensitive information

poettering · 2021-09-30T09:39:19Z

Hmm, if I run this on the systemd main repo then it still doesn#t work: (i.e. mkosi -i -f -f boot)

‣ Running second (final) stage to generate cached copy…
Traceback (most recent call last):
  File "/usr/lib64/python3.9/runpy.py", line 197, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/lib64/python3.9/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/home/lennart/projects/mkosi/mkosi/__main__.py", line 32, in <module>
    main()
  File "/home/lennart/projects/mkosi/mkosi/__main__.py", line 26, in main
    run_verb(a)
  File "/home/lennart/projects/mkosi/mkosi/__init__.py", line 7204, in run_verb
    manifest = build_stuff(args)
  File "/home/lennart/projects/mkosi/mkosi/__init__.py", line 6619, in build_stuff
    image = build_image(args, root, do_run_build_script=False, for_cache=True)
  File "/home/lennart/projects/mkosi/mkosi/__init__.py", line 6339, in build_image
    raw = create_image(args, for_cache)
  File "/home/lennart/projects/mkosi/mkosi/__init__.py", line 608, in create_image
    assert args.partition_table is None
AssertionError

I think we should split out utils.py or something later on. But let's at least move it out of the main file.

As suggested in systemd#798 (comment).

lgtm-com · 2021-10-05T15:58:20Z

This pull request fixes 1 alert when merging 4774cf3 into 2e35cde - view on LGTM.com

fixed alerts:

1 for Clear-text logging of sensitive information

keszybz · 2021-10-05T16:49:01Z

Hmm, it now builds, but doesn't boot. I need to investigate more.

As suggested in systemd#798 (comment).

keszybz · 2021-10-05T19:47:26Z

I dropped a not in one of the conditionals… Let's see if it passes now.

lgtm-com · 2021-10-05T19:51:21Z

This pull request fixes 1 alert when merging dcc68e8 into 2e35cde - view on LGTM.com

fixed alerts:

1 for Clear-text logging of sensitive information

This adds support for creating signed GPT disk images. If Verity=signed is set this will not only generate and insert Verity data into the image, but then use the resulting root hash, sign it and include it in an additional partition. It will also write the resulting PKCS7 signature out into a new .roothash.p7s file. This scheme is compatible with kernel 5.4's PKCS7 signature logic for dm-verity: the resulting .p7s file can be passed as-is to the kernel (or systemd's RootHashSignature= setting). The partition this embedds contains a simple JSON object containing three fields. The verity root hash, the PKCS7 data (i.e. the same data as in the .p7s file, but in base64), and SHA256 fingerprint of the signing key. This partition is supposed to be read by the image dissection logic of systemd, to implement signed single-file images. (The corresponding PR for systemd I am still working on). This opens up two avenues for image verification: 1. Everything in one file: the single, "unified" GPT disk image contains three partitions, for payload data, verity data and verity signature. 2. Split out: root hash and its signature are stored in two "sidecar" files. (Of course I'd personally always go the "unified" way, but given the RootHashSignature= logic exists already, and it's easy to support, let's support it.) This uses the key already used for doing secureboot image signing. Conceptually this makes a ton of sense: we sign the same stuff here after all: the contents of the image, supporting two different entrypoints to the image: one via UEFI booting the image, and once for attaching directly to an image from a running system. Admittedly, the "mkosi.secure-boot.key" and "mkosi.secure-boot.certificate" monikers for this key pair might be a bit suprising though. Corresponding PR in systemd is: systemd/systemd#20691

@behrmann

This is mostly based on Jörg Behrmann's (@behrmann) work.

The general idea is that those closes collect attributes and information about the abstract partition table, but are independent of any underlying block device. The definitions exists indendependently of a block device and can be applied later on. Partitions are referred to by enum PartitionIdentifier. The calculation of the space necessary for those partitions is centralized in the class, so we don't have multiplace places where we arrive at slightly different formulas for the expected disk size.

We would have two conditions joined by 'and', and we would have the first operand and half of the second on the first line, and the remainder of the second operand on multiple lines.

As suggested in systemd#798 (comment).

lgtm-com · 2021-10-06T06:48:12Z

This pull request fixes 1 alert when merging e4996c9 into 2e35cde - view on LGTM.com

fixed alerts:

1 for Clear-text logging of sensitive information

keszybz · 2021-10-06T07:52:43Z

The only failure is:

Error: Failed to download metadata for repo 'AppStream': Cannot prepare internal mirrorlist: Status code: 503 for https://mirrors.rockylinux.org/mirrorlist?arch=x86_64&repo=AppStream-8 (IP: 151.101.186.132)

Yay!

keszybz · 2021-10-06T12:59:32Z

@poettering merge this now?

poettering · 2021-10-07T12:13:12Z

I think something is wrong with the partition size calculations. If I built an image with pkcs7 signature and look at the signature data it appears that it overlaps with the GPT footer, i.e. if you dump partition contents with xxd then you see something that looks a ton like the GPT partition list.

poettering · 2021-10-07T12:25:37Z

lgtm, otherwise

keszybz · 2021-10-07T13:10:35Z

(As discussed in the chat, we'll deal with the gpt header corruption later.)

We would create a file with 0 padding to force it to a certain size. This seems fairly ugly, in particular when we later want to use the blob in other contexts. Also, this embedded the assumption that padding to 4096 is always appropriate in the function that generates the signature. Let's just calculate the right amount when writing to the target partition. Incidentally, this seems to fix the issue where we'd the backup LBA header inserted over the output partition, as discussed in systemd#798 (comment). But I don't entirely understand the issue, so maybe this change is only hiding the bug.

poettering added the dont-merge label Sep 8, 2021

poettering force-pushed the signed-gpt branch from 963b33b to 901eda9 Compare September 8, 2021 07:54

behrmann reviewed Sep 8, 2021

View reviewed changes

mkosi/__init__.py Outdated Show resolved Hide resolved

mkosi/__init__.py Outdated Show resolved Hide resolved

mkosi/__init__.py Outdated Show resolved Hide resolved

bluca reviewed Sep 8, 2021

View reviewed changes

mkosi/__init__.py Outdated Show resolved Hide resolved

keszybz reviewed Sep 8, 2021

View reviewed changes

poettering force-pushed the signed-gpt branch 2 times, most recently from 135fd00 to 865ecc1 Compare September 8, 2021 14:37

poettering mentioned this pull request Sep 9, 2021

dissect: add support for GPT images with embedded dm-verity signatures systemd/systemd#20691

Merged

poettering force-pushed the signed-gpt branch from 865ecc1 to 71db4f3 Compare September 10, 2021 20:14

poettering removed the dont-merge label Sep 10, 2021

poettering changed the title ~~mkosi: add support for PKCS7 signed images (WIP)~~ mkosi: add support for PKCS7 signed images Sep 10, 2021

bluca reviewed Sep 10, 2021

View reviewed changes

mkosi/__init__.py Outdated Show resolved Hide resolved

keszybz reviewed Sep 10, 2021

View reviewed changes

behrmann reviewed Sep 11, 2021

View reviewed changes

mkosi/__init__.py Outdated Show resolved Hide resolved

mkosi/__init__.py Outdated Show resolved Hide resolved

keszybz added the reviewed/needs-rework label Sep 13, 2021

poettering force-pushed the signed-gpt branch from 71db4f3 to 37493f0 Compare September 13, 2021 14:30

behrmann reviewed Sep 13, 2021

View reviewed changes

mkosi/__init__.py Outdated Show resolved Hide resolved

poettering force-pushed the signed-gpt branch from 2f1bdf4 to 0104c11 Compare September 13, 2021 19:00

keszybz force-pushed the signed-gpt branch from 0104c11 to 53d2771 Compare September 20, 2021 17:33

Move PartitionTable to backend

5a1bf10

I think we should split out utils.py or something later on. But let's at least move it out of the main file.

keszybz added a commit to poettering/mkosi that referenced this pull request Oct 5, 2021

Rename Partition.name to Partition.description

4774cf3

As suggested in systemd#798 (comment).

keszybz force-pushed the signed-gpt branch from f5eccf9 to 4774cf3 Compare October 5, 2021 15:50

keszybz added a commit to poettering/mkosi that referenced this pull request Oct 5, 2021

Rename Partition.name to Partition.description

faa93e6

As suggested in systemd#798 (comment).

keszybz force-pushed the signed-gpt branch from 4774cf3 to dcc68e8 Compare October 5, 2021 19:44

poettering and others added 8 commits October 6, 2021 08:40

doc: document Verity=signed

bcc8700

mkosi: port to python-cryptography

77dc59f

This is mostly based on Jörg Behrmann's (@behrmann) work.

indentation: keep each condition on a separate line

25c1ef7

We would have two conditions joined by 'and', and we would have the first operand and half of the second on the first line, and the remainder of the second operand on multiple lines.

isort: apply isort

2f0b9bf

Rename Partition.name to Partition.description

0b750a3

As suggested in systemd#798 (comment).

ci: add types-cryptography to CI environment

e4996c9

keszybz force-pushed the signed-gpt branch from dcc68e8 to e4996c9 Compare October 6, 2021 06:40

keszybz removed the reviewed/needs-rework label Oct 6, 2021

keszybz merged commit af4b9f1 into systemd:main Oct 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mkosi: add support for PKCS7 signed images #798

mkosi: add support for PKCS7 signed images #798

poettering commented Sep 8, 2021

keszybz left a comment

poettering commented Sep 9, 2021 •

edited

Loading

poettering commented Sep 10, 2021

behrmann left a comment •

edited

Loading

keszybz commented Sep 13, 2021

behrmann commented Sep 13, 2021

poettering commented Sep 13, 2021

poettering commented Sep 13, 2021

behrmann commented Sep 13, 2021

behrmann commented Sep 13, 2021

poettering commented Sep 13, 2021

keszybz commented Sep 13, 2021

keszybz commented Sep 16, 2021

lgtm-com bot commented Sep 28, 2021

poettering commented Sep 30, 2021

lgtm-com bot commented Oct 5, 2021

keszybz commented Oct 5, 2021

keszybz commented Oct 5, 2021

lgtm-com bot commented Oct 5, 2021

lgtm-com bot commented Oct 6, 2021

keszybz commented Oct 6, 2021

keszybz commented Oct 6, 2021

poettering commented Oct 7, 2021

poettering commented Oct 7, 2021

keszybz commented Oct 7, 2021

mkosi: add support for PKCS7 signed images #798

mkosi: add support for PKCS7 signed images #798

Conversation

poettering commented Sep 8, 2021

keszybz left a comment

Choose a reason for hiding this comment

poettering commented Sep 9, 2021 • edited Loading

poettering commented Sep 10, 2021

behrmann left a comment • edited Loading

Choose a reason for hiding this comment

keszybz commented Sep 13, 2021

behrmann commented Sep 13, 2021

poettering commented Sep 13, 2021

poettering commented Sep 13, 2021

behrmann commented Sep 13, 2021

behrmann commented Sep 13, 2021

poettering commented Sep 13, 2021

keszybz commented Sep 13, 2021

keszybz commented Sep 16, 2021

lgtm-com bot commented Sep 28, 2021

poettering commented Sep 30, 2021

lgtm-com bot commented Oct 5, 2021

keszybz commented Oct 5, 2021

keszybz commented Oct 5, 2021

lgtm-com bot commented Oct 5, 2021

lgtm-com bot commented Oct 6, 2021

keszybz commented Oct 6, 2021

keszybz commented Oct 6, 2021

poettering commented Oct 7, 2021

poettering commented Oct 7, 2021

keszybz commented Oct 7, 2021

poettering commented Sep 9, 2021 •

edited

Loading

behrmann left a comment •

edited

Loading