luks: Adds tpm2_pcrs config option for LUKS-encrypted devices - #488
Conversation
| def main(): | ||
| worker_count, args = service.get_worker_count_from_args(sys.argv) | ||
| CONF(args[1:], project='coriolis', version="1.0.0") | ||
| luks_mixin.validate_config() |
There was a problem hiding this comment.
Do we have to fail the whole service for this?
There was a problem hiding this comment.
Yes. It's a misconfiguration. It's better to fail fast rather than fail later on, when a LUKS VM has to be migrated.
There was a problem hiding this comment.
Usually we validate options only when they're actually being used. No other option is being validated in cmd module. But then again, you would have to specifically edit the option and then restart. If it's invalid, you'll know straight away, but failing the service as a whole seems a bit drastic. I'm ok with this as long as worker logs can still be downloaded and the validation errors are actually contained in the coriolis-worker.log. Can you please double-check?
There was a problem hiding this comment.
Also, users will have no tell if the options are valid or not unless they start migrating and notice a bunch of 500/503 errors. If you keep this validation in the osmorphing task alone, you can redirect the error message to the task as well and it'll be nicely output in the UI.
| Called at worker service startup, so a misconfigured [luks] config section | ||
| fails fast, rather than surfacing into an actual LUKS migration. | ||
| """ | ||
| if not CONF.luks.tpm2_pcrs: |
There was a problem hiding this comment.
I don't feel like we should force any PCRs. The only reason I asked for a validation method was to cover for user errors when building dracut/clevis strings, but if the int value is all the user has to pass and everything else is handled by _render_tpm2_pcrs, I guess we could also validate whether the pcr values are valid and restrict to only values that we can support (maybe 0-7, or whatever else you might have tested with or looks like it could work).
There was a problem hiding this comment.
We are not forcing any specific PCR, we just erroring out if there is no PCR configured. But yeah, we can further validate the values as well, making sure they are valid (0-15).
63c4fbf to
61054b6
Compare
Adds the mentioned config option. The default value for it is 7, which is the default value used by systemd-cryptenroll.
61054b6 to
1e3036f
Compare
2 participants
Adds the mentioned config option. The default value for it is 7, which is the default value used by
systemd-cryptenroll.