ukify: Support multiple PCR signing keys to sign phases separately#4279
ukify: Support multiple PCR signing keys to sign phases separately#4279pothos wants to merge 1 commit into
Conversation
31bd2c6 to
0d61443
Compare
behrmann
left a comment
There was a problem hiding this comment.
Arghs, forgot to press the submit button
ba039eb to
1981f1e
Compare
To be able to bind a secret against a signed PCR policy so that it only unlocks in the initrd but not the final system one has to sign the policies with separate keys because that's what one actually binds to. This is supported in ukify as "phases" and ukify accepts the PCR signing keys specified multiple times together with the phase they are for. We can't extend the existing config setting for this scheme, so instead we have to add a new setting which can hold the configuration entries. Also, we need a new setting to specify which PCR pub key goes into the .pcrpkey UKI section because when we have many, ukify won't add any by default. Add SignExpectedPcrWithPhases= as alternative to SignExpectedPcrKey=/ SignExpectedPcrCertificate=/*Source= where we can now specify the phase signing configuration one line each. A line contains KEY CERT PHASES [KEYSRC [CERTSRC]] where KEYSRC/CERTSRC defaults to "file". One can also set "-" for PHASES to omit --phases for ukify so that it chooses the defaults. Also add SignExpectedPcrUKIPublicKey= to be able to specify the key for the .pcrpkey UKI section. Closes systemd#4109
|
The general feature looks ok, as long as the existing configs keep working. I'm not 100% convinced on the format of the configuration, giving semantic meaning to line breaks, but I'll leave that decision to @daandemeyer and @behrmann |
That is indeed a bit hairy, but the problem is that we're running out of separators at this interplay, since between mkosi and ukify we're already using whitespace, commas, and colons. Could this maybe be better served by an indirection putting this into the native config files for ukify and passing them to ukify from some well-known place, e.g. some |
Claude review of PR #4279 (f33f09e)Suggestions
|
| if signer.key_source.type == KeySourceType.engine: | ||
| arguments += ["--signing-engine", signer.key_source.source] |
There was a problem hiding this comment.
Claude: suggestion: Claude: When multiple signers have non-file key sources, --signing-engine/--signing-provider is emitted once per signer in the loop. Since ukify uses a single global engine/provider, passing it multiple times is redundant at best. More importantly, if different signers specify inconsistent key source types (e.g., one uses engine:pkcs11 and another uses provider:pkcs11), this would produce conflicting --signing-engine and --signing-provider arguments on the same ukify command line with no early validation error from mkosi.
The documentation notes that "it is the user's responsibility to keep sources consistent across signers," but a validation check in validate_pcr_signing (similar to the existing secure_boot_key_source consistency check) would catch misconfigurations early with a clear error message. Alternatively, emitting --signing-engine/--signing-provider only once (outside or after the loop) would make the "global" nature explicit in code.
To be able to bind a secret against a signed PCR policy so that it only unlocks in the initrd but not the final system one has to sign the policies with separate keys because that's what one actually binds to. This is supported in ukify as "phases" and ukify accepts the PCR signing keys specified multiple times together with the phase they are for. We can't extend the existing config setting for this scheme, so instead we have to add a new setting which can hold the configuration entries. Also, we need a new setting to specify which PCR pub key goes into the .pcrpkey UKI section because when we have many, ukify won't add any by default.
Add SignExpectedPcrWithPhases= as alternative to SignExpectedPcrKey=/ SignExpectedPcrCertificate=/*Source= where we can now specify the phase signing configuration one line each. A line contains KEY CERT PHASES [KEYSRC [CERTSRC]] where KEYSRC/CERTSRC defaults to "file". One can also set "-" for PHASES to omit --phases for ukify so that it chooses the defaults. Also add SignExpectedPcrUKIPublicKey= to be able to specify the key for the .pcrpkey UKI section.
Closes #4109