Encrypted Pools #102
Encrypted Pools #102
Conversation
|
The obvious things missing from testing:
|
dwlehman
force-pushed
the
luks-pool
branch
3 times, most recently
from
b79af03 to
4640202
Compare
dwlehman
requested review from
tyll,
japokorn,
pcahyna,
richm and
sergio-correia
| @@ -145,15 +148,72 @@ class BlivetAnsibleError(Exception): | |||
| pass | |||
|
|
|||
|
|
|||
| class BlivetVolume(object): | |||
| class BlivetBase(object): | |||
There was a problem hiding this comment.
do you need unit tests for these changes? if so, they can be added later after the 15th
tests/tests_swap.yml
Outdated
| fs_type: ext3 | ||
| disks: "{{ unused_disks }}" | ||
|
|
||
| - include_tasks: verify-role-results.yml |
There was a problem hiding this comment.
Does verify-role-results.yml check for idempotence? Does it do something like assert: that: some_register_var is not changed?
There was a problem hiding this comment.
No, the only tests we have for idempotence are explicitly for that purpose -- basically running the role a second time and checking that things are the same afterwards. I don't think I did any of those for this changeset, though. Maybe I'll add one in the morning.
|
@sergio-correia the LUKS parameters are needlessly different from the ones used in your role. https://github.com/linux-system-roles/nbde_client/pull/3/files#diff-7eeda618087b49ae876084ab6c73fdbbR9 uses |
| This string specifies a passphrase used to unlock/open the LUKS volume(s). | ||
|
|
||
| ##### `encryption_key_file` | ||
| This string specifies the full path to the key file used to unlock the LUKS volume(s). |
There was a problem hiding this comment.
path on the control node or on the managed host?
There was a problem hiding this comment.
Everything is on the managed host AFAIK. Any operations to get the key file to the managed nodes is on the user. Maybe I should note that.
There was a problem hiding this comment.
@sergio-correia the LUKS parameters are needlessly different from the ones used in your role. https://github.com/linux-system-roles/nbde_client/pull/3/files#diff-7eeda618087b49ae876084ab6c73fdbbR9 uses
passandkeyfile. Here one usesencryption_passphraseandencryption_key_file. Those names should be consistent. (I don't care which one is chosen.)
I will adopt encryption_passphrase and encryption_key_file. These seem more explicit.
There was a problem hiding this comment.
I think you can use just passphrase and key_file. In your role it is obvious that it is encryption related (it is an encryption role). I was concerned about the gratuitous pass vs passphrase difference or keyfile vs. key_file.
There was a problem hiding this comment.
@dwlehman would it make sense to have key_file on the control node? nbde_client uses a keyfile on the control node: linux-system-roles/nbde_client#3 (comment) . Are the use cases different enough to justify this difference in semantics?
There was a problem hiding this comment.
I agree that it is more convenient for users and the consistency between roles is good.
Where on the managed node should we place the key files?
There was a problem hiding this comment.
Where on the managed node should we place the key files?
@sergio-correia what do you do in nbde_client?
There was a problem hiding this comment.
I define an internal variable __nbde_client_managed_dir and have a task to copy the key files there and do the cleanup later. I pass this variable also to the python module, so it knows where to find the key files. It's currently defined as /var/run/linux-system-roles.nbde_client
There was a problem hiding this comment.
For our purposes I think we need to keep the key file around for reboots, while I (think that I) see that you are removing them in the nbde_client role after setting up nbde. For us to specify that they be on the controller necessitates developing a scheme for storing them on the managed nodes.
There was a problem hiding this comment.
Yeah, for us it's simpler that we need it to do some operations, then we don't need it anymore and can remove it.
|
@pcahyna @sergio-correia we use the prefixes for a sort of namespacing, so all the encryption-specific parameters have the 'encryption_' prefix. Same for raid, fs, mount, etc. Not sure if there's a benefit for the clevis/tang roles to do something along the same lines. |
Right, I wont use the prefix. @pcahyna explained he was more concerned about pass/passphrase (we prefer passphrase, so I will update there) and keyfile/key_file |
This seems fairly complete
, but the tests are woefully inadequate. This is on top of #90 and #100. The relevant work begins with a3293e9.Edit: tests are reasonable now, although they don't check cipher, key size, or luks version.