Have a single ActivateVolumeOptions struct for the ActivateVolumeWith* family #116
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…* funcs Also rename PIN in its definition to UserPassprase to move toward better and also more abstract naming. Same in the corresponding RecoveryKeyUsageReason code. Se also issue #113
bboozzoo
approved these changes
Sep 25, 2020
| r := pinReader | ||
| pinReader = nil | ||
| r := passphraseReader | ||
| passphraseReader = nil |
There was a problem hiding this comment.
Unrelated to renames, is this a precaution that the reader wouldn't be used again by accident?
There was a problem hiding this comment.
I'm not quite sure what was the original intention here. It's meant to fall back to use systemd-ask-password after consuming one passphrase. But it's very specific behavior that's another reason to have some more control later via the Prompter if needed.
| // If the TPM sealed key object has a PIN defined, then this function will use systemd-ask-password to request it. If pinReader is not | ||
| // nil, then an attempt to read the PIN from this will be made instead by reading all characters until the first newline. The PINTries | ||
| // field of options defines how many attempts should be made to obtain the correct PIN before failing. | ||
| // If the TPM sealed key object has a user passphrase/PIN defined, then this function will use systemd-ask-password to request it. If passphraseReader is not |
There was a problem hiding this comment.
That's a long comment, but I see it's consistent with the rest of the comments in the package. Nevertheless, it does not fit into a file view in a github review page, making it a bit hard to follow. Could we unify the comments style with snapd at some point?
crypt.go
Outdated
Comment on lines
468
to
470
| // set. With a TPM attempts to unseal with a user passphrase | ||
| // will stop if the TPM enters dictionary attack lockout mode | ||
| // before this limit is reached. |
There was a problem hiding this comment.
Suggested change
| // set. With a TPM attempts to unseal with a user passphrase | |
| // will stop if the TPM enters dictionary attack lockout mode | |
| // before this limit is reached. | |
| // set. With a TPM, subsequent attempts to unseal with a user passphrase | |
| // will be ceased if the TPM enters dictionary attack lockout mode | |
| // before the tries limit is reached. |
crypt.go
Outdated
| // will stop if the TPM enters dictionary attack lockout mode | ||
| // before this limit is reached. | ||
| // It is unused by ActivateWithRecoveryKey. | ||
| UserPassphraseTries int |
| // ActivateWithRecoveryKey and when falling in the other | ||
| // cases, for example failed TPM unsealing. Setting this to | ||
| // zero will disable attempts to activate with the fallback | ||
| // recovery key. | ||
| RecoveryKeyTries int |
crypt.go
Outdated
Comment on lines
464
to
465
| // with the recovery key if RecoveryKeyTries is greater than | ||
| // zero. Setting this to zero disables unsealing with a user |
There was a problem hiding this comment.
Suggested change
| // with the recovery key if RecoveryKeyTries is greater than | |
| // zero. Setting this to zero disables unsealing with a user | |
| // with the recovery key (see RecoveryKeyTries). | |
| // Setting this to zero disables unsealing with a user |
crypt.go
Outdated
| // LockSealedKeyAccess controls whether LockAccessToSealedKeys should be called after unsealing the TPM sealed key. It is called if | ||
| // this is set to true, and not called if this is set to false. | ||
| LockSealedKeyAccess bool | ||
| // LockSealedKeys controls whether access to the keys locked |
There was a problem hiding this comment.
Suggested change
| // LockSealedKeys controls whether access to the keys locked | |
| // LockSealedKeys controls whether access to the keys is locked |
anonymouse64
approved these changes
Sep 25, 2020
crypt.go
Outdated
Comment on lines
468
to
470
| // set. With a TPM attempts to unseal with a user passphrase | ||
| // will stop if the TPM enters dictionary attack lockout mode | ||
| // before this limit is reached. |
There was a problem hiding this comment.
Suggested change
| // set. With a TPM attempts to unseal with a user passphrase | |
| // will stop if the TPM enters dictionary attack lockout mode | |
| // before this limit is reached. | |
| // set. With a TPM, attempts to unseal will stop if the TPM enters | |
| // dictionary attack lockout mode before this limit is reached. |
crypt.go
Outdated
Comment on lines
476
to
478
| // attempted. It is considered directly by | ||
| // ActivateWithRecoveryKey and when falling in the other | ||
| // cases, for example failed TPM unsealing. Setting this to |
There was a problem hiding this comment.
Suggested change
| // attempted. It is considered directly by | |
| // ActivateWithRecoveryKey and when falling in the other | |
| // cases, for example failed TPM unsealing. Setting this to | |
| // attempted. It is used to directly with | |
| // ActivateWithRecoveryKey and indirectly with other | |
| // methods upon failure, for example failed TPM | |
| // unsealing. Setting this to |
| RecoveryKeyTries int | ||
|
|
||
| // ActivateOptions provides a mechanism to pass additional options to systemd-cryptsetup. | ||
| // ActivateOptions provides a mechanism to pass additional | ||
| // options to systemd-cryptsetup. | ||
| ActivateOptions []string |
There was a problem hiding this comment.
Suggested change
| ActivateOptions []string | |
| AdditionalActivateOptions []string |
or maybe
Suggested change
| ActivateOptions []string | |
| AdditionalActivationOptions []string |
or maybe even
Suggested change
| ActivateOptions []string | |
| AdditionalSystemdCryptSetupActivationOptions []string |
(I'm not familiar enough with the pkg to know how specific we ought to be about using systemd-cryptsetup references)
crypt.go
Outdated
Comment on lines
488
to
489
| // after unsealing. That means calling LockAccessToSealedKeys | ||
| // in TPM case. |
There was a problem hiding this comment.
Suggested change
| // after unsealing. That means calling LockAccessToSealedKeys | |
| // in TPM case. | |
| // after unsealing by calling LockAccessToSealedKeys in the TPM case | |
| // for example |
UserPassphrase is fairly long and it's unclear whether it always be strictly from a user thanks @bboozzoo, @anonymouse64
pedronis
force-pushed
the
activate-volume-options
branch
from
3fe87bd
to
8995bba
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Also rename PIN in its definition to Passphrase to move toward
better and also more abstract naming.
Same in the corresponding RecoveryKeyUsageReason code.
See also issue #113