Add LUKS support via Clevis TPM 2 token #1200
Conversation
Thank you for contributing to the Leapp project!Please note that every PR needs to comply with the Leapp Guidelines and must pass all tests in order to be mergeable.
Packit will automatically schedule regression tests for this PR's build and latest upstream leapp build. If you need a different version of leapp, e.g. from PR#42, use It is possible to schedule specific on-demand tests as well. Currently 2 test sets are supported,
See other labels for particular jobs defined in the Please open ticket in case you experience technical problem with the CI. (RH internal only) Note: In case there are problems with tests not being triggered automatically on new PR/commit or pending for a long time, please contact leapp-infra. |
| CLEVIS_RHEL8_DOC_URL = 'https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/security_hardening/configuring-automated-unlocking-of-encrypted-volumes-using-policy-based-decryption_security-hardening#configuring-manual-enrollment-of-volumes-using-tpm2_configuring-automated-unlocking-of-encrypted-volumes-using-policy-based-decryption' # noqa: E501; pylint: disable=line-too-long | ||
| CLEVIS_RHEL9_DOC_URL = 'https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/9/html/security_hardening/configuring-automated-unlocking-of-encrypted-volumes-using-policy-based-decryption_security-hardening#configuring-manual-enrollment-of-volumes-using-tpm2_configuring-automated-unlocking-of-encrypted-volumes-using-policy-based-decryption' # noqa: E501; pylint: disable=line-too-long | ||
| LUKS2_CONVERT_RHEL8_DOC_URL = 'https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/security_hardening/encrypting-block-devices-using-luks_security-hardening#luks-versions-in-rhel_encrypting-block-devices-using-luks' # noqa: E501; pylint: disable=line-too-long | ||
| LUKS2_CONVERT_RHEL9_DOC_URL = 'https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/9/html/security_hardening/encrypting-block-devices-using-luks_security-hardening#luks-versions-in-rhel_encrypting-block-devices-using-luks' # noqa: E501; pylint: disable=line-too-long | ||
|
|
There was a problem hiding this comment.
candidates for shortened URLs
- shortened URLs allow us to react on possible changes in access.redhat.com which could result in 404 errors, etc. you do not need to care about shortened URLs now, we will create them later when wanted.
danzatt
force-pushed
the
add-luks-support
branch
4 times, most recently
from
9fa57ae
to
f8346e4
Compare
|
@danzatt Hi Dan \o most likely I will get to the review during early May or later June. We are dealing now with additional stuff. |
|
/packit test |
Modify the StorageInfo model to include path and name of the parent device. Use StorageScanner to collect this information. Morover fix lsblk test, there should be a full device path in "lsblk -pbnr" output (just names were used in the original test).
Add LuksScanner actor that runs 'cryptsetup luksDump' for all 'crypt' from lsblk output. The output is then parsed and filled into LuksDump and LuksToken models.
Consume LuksDump messages to decide whether the upgrade process should be inhibited. If all devices are LUKS2 with clevis TPM2 binding, don't inhibit.
| elif luks_dump.version == 2 and \ | ||
| not any([x.token_type == "clevis-tpm2" for x in luks_dump.tokens]): |
There was a problem hiding this comment.
omit please line breaks in the code. use rather parenthesis if needed or define function to do this one liner. yes, we have several line breaks in the existing code, but we are trying to get rid of it. it's not blocking the merge, but it's preferred change.
| reporting.Title('LUKS2 partition ({}) without clevis TPM2 binding detected' | ||
| .format(luks_dump.device_name)), |
There was a problem hiding this comment.
Titles needs to be static. dynamic titles is a clue that there is something wrong with the report idea. I expect to have only one report informing about existing LUKS2 partitions without clevis TPM2 binding for the whole machine. Information about what partitions we speak about is a parameter of the report, so it should be part of the summary.
Note the summary for this report is incorrect too. (see my comment about the loop)
| break | ||
| pass | ||
|
|
||
| for luks_dump in self.consume(LuksDump): |
There was a problem hiding this comment.
This loop needs to be redesigned. No report should be produced in the loop. The code should get list(s) of problematic partitions and then one report per specific problem should be created. IOW something like:
# pseudocode
list_luks1_partitions = []
list_no_tpm2_partitions = []
for .....:
if partition.luks1:
list_luks1_partitions.append(partition)
elif _has_tpm2_binding(partition):
list_no_tpm2_partitions.append(partition)
..
if list_luks1_partitions:
report_luks1_parittions_detected(...)
if list_no_tpm2_partitions:
report_partitions_without_tpm2_binding(...)To get some examples of such reports:
grep -r "FMT_LIST_SEPARATOR" repos/system_upgrade
| """ | ||
| Represents a single token associated with the LUKS device | ||
| """ |
There was a problem hiding this comment.
Nitpick, to make it clear that people should not try to produce/consume this model in actors, but they should look at LuksDump instead. Otherwise thanks for documenting all fields! I love it
| """ | |
| Represents a single token associated with the LUKS device | |
| """ | |
| """ | |
| Represents a single token associated with the LUKS device. | |
| Note this model is supposed to be used just as part of the LuksDump msg. | |
| """ |
| """ | ||
|
|
||
|
|
||
| class LuksDump(Model): |
There was a problem hiding this comment.
Checking the logic, I suggest to create one msg that will envelope all created LuksDump (list(Model(LuksDump)). It has some benefits:
- you can check that only one msg exists at all (e.g. no other actors, like custom/third-party, are produced the msg too)
- kind of easier debugging when checking leapp.db, as you find the msg and just go through the data; however for multiple msgs it's kind of more noise - you have more reading actually
It's not blocker, but I suggest to consider the change. You can find both principles used in the project, so no blocker at all. But from my experience having just one msg for "system facts" is little bit more comfortable. Keeping it on you what you is your preference.
|
@danzatt I haven't went through the whole code yet, but covered most of it. I found some things that could be changed, and some that needs to be changed. I do not expect I will find anything else in the rest of the code (and not sure when I will get to it), but i am letting you know about that in advance, in case you would like to wait for the full review. |
Add a
LuksScanneractor which scans all crypt devices usingcryptsetup luksDump. Don't inhibit, when all devices are LUKS2 with clevs TPM2 token./cc @pirat89