Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Have a single ActivateVolumeOptions struct for the ActivateVolumeWith* family #116

Merged
merged 2 commits into from
Sep 25, 2020
Merged

Have a single ActivateVolumeOptions struct for the ActivateVolumeWith* family #116

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fbec76e
Have a single ActivateVolumeOptions struct for the ActivateVolumeWith…
pedronis Sep 25, 2020
8995bba
improve comments based on feedback, drop User from UserPassphrase
pedronis Sep 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 59 additions & 56 deletions crypt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,9 +275,10 @@ const (
// is also not correctly provisioned.
RecoveryKeyUsageReasonInvalidKeyFile

// RecoveryKeyUsageReasonPINFail indicates that a volume had to be activated with the fallback recovery key because the correct PIN
// was not provided.
RecoveryKeyUsageReasonPINFail
// RecoveryKeyUsageReasonUserPassphraseFail indicates that a
// volume had to be activated with the fallback recovery key
// because the correct user passphrase/PIN was not provided.
RecoveryKeyUsageReasonUserPassphraseFail
)

func activateWithRecoveryKey(volumeName, sourceDevicePath string, keyReader io.Reader, tries int, reason RecoveryKeyUsageReason, activateOptions []string, keyringPrefix string) error {
Expand Down Expand Up @@ -367,7 +368,7 @@ func isLockAccessError(err error) bool {
return xerrors.As(err, &e)
}

func activateWithTPMKey(tpm *TPMConnection, volumeName, sourceDevicePath, keyPath string, pinReader io.Reader, pinTries int, lock bool, activateOptions []string, keyringPrefix string) error {
func activateWithTPMKey(tpm *TPMConnection, volumeName, sourceDevicePath, keyPath string, passphraseReader io.Reader, passphraseTries int, lock bool, activateOptions []string, keyringPrefix string) error {
var lockErr error
sealedKey, authPrivateKey, err := func() ([]byte, TPMPolicyAuthKey, error) {
defer func() {
Expand All @@ -383,20 +384,20 @@ func activateWithTPMKey(tpm *TPMConnection, volumeName, sourceDevicePath, keyPat
}

switch {
case pinTries == 0 && k.AuthMode2F() == AuthModePIN:
case passphraseTries == 0 && k.AuthMode2F() == AuthModePIN:
return nil, nil, requiresPinErr
case pinTries == 0:
pinTries = 1
case passphraseTries == 0:
passphraseTries = 1
}

var sealedKey []byte
var authPrivateKey []byte

for ; pinTries > 0; pinTries-- {
for ; passphraseTries > 0; passphraseTries-- {
var pin string
if k.AuthMode2F() == AuthModePIN {
r := pinReader
pinReader = nil
r := passphraseReader
passphraseReader = nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to renames, is this a precaution that the reader wouldn't be used again by accident?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

pin, err = getPassword(sourceDevicePath, "PIN", r)
if err != nil {
return nil, nil, xerrors.Errorf("cannot obtain PIN: %w", err)
Expand Down Expand Up @@ -454,40 +455,55 @@ func makeActivateOptions(in []string) ([]string, error) {
return append(out, "tries=1"), nil
}

// ActivateWithTPMSealedKeyOptions provides options to ActivateVolumeWtthTPMSealedKey.
type ActivateWithTPMSealedKeyOptions struct {
// PINTries specifies the maximum number of times that unsealing with a PIN should be attempted before failing with an error and
// falling back to activating with the recovery key if RecoveryKeyTries is greater than zero. Setting this to zero disables unsealing
// with a PIN - in this case, an error will be returned if the sealed key object indicates that a PIN has been set. Attempts to
// unseal with a PIN will stop if the TPM enters dictionary attack lockout mode before this limit is reached.
PINTries int

// RecoveryKeyTries specifies the maximum number of times that activation with the fallback recovery key should be attempted
// if activation with the TPM sealed key fails, before failing with an error. Setting this to zero will disable attempts to activate
// with the fallback recovery key.
// ActivateVolumeOptions provides options to the ActivateVolumeWith*
// family of functions.
type ActivateVolumeOptions struct {
// UserPassphraseTries specifies the maximum number of times
// that unsealing with a user passphrase should be attempted
// before failing with an error and falling back to activating
// with the recovery key if RecoveryKeyTries is greater than
// zero. Setting this to zero disables unsealing with a user
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// passphrase - in this case, an error will be returned if the
// sealed key object indicates that a user passphrase has been
// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// It is unused by ActivateWithRecoveryKey.
UserPassphraseTries int
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UserPassphraseMaxTries?


// RecoveryKeyTries specifies the maximum number of times that
// activation with the fallback recovery key should be
// attempted. It is considered directly by
// ActivateWithRecoveryKey and when falling in the other
// cases, for example failed TPM unsealing. Setting this to
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// zero will disable attempts to activate with the fallback
// recovery key.
RecoveryKeyTries int
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RecoveryKeyMaxTries?


// ActivateOptions provides a mechanism to pass additional options to systemd-cryptsetup.
// ActivateOptions provides a mechanism to pass additional
// options to systemd-cryptsetup.
ActivateOptions []string
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// LockSealedKeys controls whether access to the keys locked
// LockSealedKeys controls whether access to the keys is locked

// after unsealing. That means calling LockAccessToSealedKeys
// in TPM case.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// after unsealing. That means calling LockAccessToSealedKeys
// in TPM case.
// after unsealing by calling LockAccessToSealedKeys in the TPM case
// for example

LockSealedKeys bool

// KeyringPrefix is the prefix used for the description of any kernel keys created during activation.
// KeyringPrefix is the prefix used for the description of any
// kernel keys created during activation.
KeyringPrefix string
}

// ActivateVolumeWithTPMSealedKey attempts to activate the LUKS encrypted volume at sourceDevicePath and create a mapping with the
// name volumeName, using the TPM sealed key object at the specified keyPath. This makes use of systemd-cryptsetup.
//
// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

// nil, then an attempt to read the user passphrase/PIN from this will be made instead by reading all characters until the first newline. The UserPassphraseTries
// field of options defines how many attempts should be made to obtain the correct passphrase before failing.
//
// The ActivateOptions field of options can be used to specify additional options to pass to systemd-cryptsetup.
//
// If the LockSealedKeyAccess field of options is true, then this function will call LockAccessToSealedKeys after unsealing the key
// If the LockSealedKeys field of options is true, then this function will call LockAccessToSealedKeys after unsealing the key
// and before activating the LUKS volume.
//
// If activation with the TPM sealed key object fails, this function will attempt to activate it with the fallback recovery key
Expand All @@ -497,7 +513,7 @@ type ActivateWithTPMSealedKeyOptions struct {
// calling GetActivationDataFromKernel will return a *RecoveryActivationData containing the recovery key and the reason that the
// recovery key was requested.
//
// If either the PINTries or RecoveryKeyTries fields of options are less than zero, an error will be returned. If the ActivateOptions
// If either the UserPassphraseTries or RecoveryKeyTries fields of options are less than zero, an error will be returned. If the ActivateOptions
// field of options contains the "tries=" option, then an error will be returned. This option cannot be used with this function.
//
// If the LockSealedKeyAccess field of options is true and the call to LockAccessToSealedKeys fails, a LockAccessToSealedKeysError
Expand All @@ -514,9 +530,9 @@ type ActivateWithTPMSealedKeyOptions struct {
//
// If the volume is successfully activated, either with the TPM sealed key or the fallback recovery key, this function returns true.
// If it is not successfully activated, then this function returns false.
func ActivateVolumeWithTPMSealedKey(tpm *TPMConnection, volumeName, sourceDevicePath, keyPath string, pinReader io.Reader, options *ActivateWithTPMSealedKeyOptions) (bool, error) {
if options.PINTries < 0 {
return false, errors.New("invalid PINTries")
func ActivateVolumeWithTPMSealedKey(tpm *TPMConnection, volumeName, sourceDevicePath, keyPath string, passphraseReader io.Reader, options *ActivateVolumeOptions) (bool, error) {
if options.UserPassphraseTries < 0 {
return false, errors.New("invalid UserPassphraseTries")
}
if options.RecoveryKeyTries < 0 {
return false, errors.New("invalid RecoveryKeyTries")
Expand All @@ -527,7 +543,7 @@ func ActivateVolumeWithTPMSealedKey(tpm *TPMConnection, volumeName, sourceDevice
return false, err
}

if err := activateWithTPMKey(tpm, volumeName, sourceDevicePath, keyPath, pinReader, options.PINTries, options.LockSealedKeyAccess, activateOptions, options.KeyringPrefix); err != nil {
if err := activateWithTPMKey(tpm, volumeName, sourceDevicePath, keyPath, passphraseReader, options.UserPassphraseTries, options.LockSealedKeys, activateOptions, options.KeyringPrefix); err != nil {
reason := RecoveryKeyUsageReasonUnexpectedError
switch {
case isLockAccessError(err):
Expand All @@ -539,9 +555,9 @@ func ActivateVolumeWithTPMSealedKey(tpm *TPMConnection, volumeName, sourceDevice
case isInvalidKeyFileError(err):
reason = RecoveryKeyUsageReasonInvalidKeyFile
case xerrors.Is(err, requiresPinErr):
reason = RecoveryKeyUsageReasonPINFail
reason = RecoveryKeyUsageReasonUserPassphraseFail
case xerrors.Is(err, ErrPINFail):
reason = RecoveryKeyUsageReasonPINFail
reason = RecoveryKeyUsageReasonUserPassphraseFail
case isExecError(err, systemdCryptsetupPath):
// systemd-cryptsetup only provides 2 exit codes - success or fail - so we don't know the reason it failed yet. If activation
// with the recovery key is successful, then it's safe to assume that it failed because the key unsealed from the TPM is incorrect.
Expand All @@ -554,44 +570,31 @@ func ActivateVolumeWithTPMSealedKey(tpm *TPMConnection, volumeName, sourceDevice
return true, nil
}

// ActivateWithRecoveryKeyOptions provides options to ActivateVolumeWithRecoveryKey.
type ActivateWithRecoveryKeyOptions struct {
// Tries specifies the maximum number of times that activation with the fallback recovery key should be attempted before failing
// with an error.
Tries int

// ActivateOptions provides a mechanism to pass additional options to systemd-cryptsetup.
ActivateOptions []string

// KeyringPrefix is the prefix used for the description of any kernel keys created during activation.
KeyringPrefix string
}

// ActivateVolumeWithRecoveryKey attempts to activate the LUKS encrypted volume at sourceDevicePath and create a mapping with the
// name volumeName, using the fallback recovery key. This makes use of systemd-cryptsetup.
//
// This function will use systemd-ask-password to request the recovery key. If keyReader is not nil, then an attempt to read the key
// from this will be made instead by reading all characters until the first newline. The Tries field of options defines how many
// from this will be made instead by reading all characters until the first newline. The RecoveryKeyTries field of options defines how many
// attempts should be made to activate the volume with the recovery key before failing.
//
// The ActivateOptions field of options can be used to specify additional options to pass to systemd-cryptsetup.
//
// If activation with the recovery key is successful, calling GetActivationDataFromKernel will return a *RecoveryActivationData
// containing the recovery key and RecoveryKeyUsageReasonRequested as the recovery reason.
//
// If the Tries field of options is less than zero, an error will be returned. If the ActivateOptions field of options contains the
// If the RecoveryKeyTries field of options is less than zero, an error will be returned. If the ActivateOptions field of options contains the
// "tries=" option, then an error will be returned. This option cannot be used with this function.
func ActivateVolumeWithRecoveryKey(volumeName, sourceDevicePath string, keyReader io.Reader, options *ActivateWithRecoveryKeyOptions) error {
if options.Tries < 0 {
return errors.New("invalid Tries")
func ActivateVolumeWithRecoveryKey(volumeName, sourceDevicePath string, keyReader io.Reader, options *ActivateVolumeOptions) error {
if options.RecoveryKeyTries < 0 {
return errors.New("invalid RecoveryKeyTries")
}

activateOptions, err := makeActivateOptions(options.ActivateOptions)
if err != nil {
return err
}

return activateWithRecoveryKey(volumeName, sourceDevicePath, keyReader, options.Tries, RecoveryKeyUsageReasonRequested, activateOptions, options.KeyringPrefix)
return activateWithRecoveryKey(volumeName, sourceDevicePath, keyReader, options.RecoveryKeyTries, RecoveryKeyUsageReasonRequested, activateOptions, options.KeyringPrefix)
}

// ActivationData corresponds to some data added to the user keyring by one of the ActivateVolume functions.
Expand Down
Loading