[pbs-devel] [PATCH proxmox-backup 1/2] fix #3853: api: add force option to tape key change-passphrase
Stefan Sterz
s.sterz at proxmox.com
Mon Feb 7 17:14:51 CET 2022
On 2/7/22 16:57, Stefan Sterz wrote:
> On 2/7/22 15:58, Thomas Lamprecht wrote:
>> On 07.02.22 13:48, Stefan Sterz wrote:
>>> When force is used, the current passphrase is not required. Instead
>>> it will be read from the file pointed to by TAPE_KEYS_FILENAME and
>>> the old key configuration will be overwritten using the new
>>> passphrase.
>>>
>> looks quite ok, some nits/suggestions in line.
>>
>>> Signed-off-by: Stefan Sterz <s.sterz at proxmox.com>
>>> ---
>>> src/api2/config/tape_encryption_keys.rs | 36
>>> ++++++++++++++++++++++---
>>> 1 file changed, 33 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/api2/config/tape_encryption_keys.rs
>>> b/src/api2/config/tape_encryption_keys.rs
>>> index 1ad99377..b31f741d 100644
>>> --- a/src/api2/config/tape_encryption_keys.rs
>>> +++ b/src/api2/config/tape_encryption_keys.rs
>>> @@ -70,6 +70,7 @@ pub fn list_keys(
>>> password: {
>>> description: "The current password.",
>>> min_length: 5,
>>> + optional: true,
>>> },
>>> "new-password": {
>>> description: "The new password.",
>>> @@ -78,6 +79,12 @@ pub fn list_keys(
>>> hint: {
>>> schema: PASSWORD_HINT_SCHEMA,
>>> },
>>> + force: {
>>> + optional: true,
>>> + type: bool,
>>> + description: "Don't ask for the old passphrase and
>>> overwrite it. Root only.",
>> Maybe we can better hint that we reset the password by
>> restoring/re-using the
>> original key, which is naturally only possible if the key available,
>> e.g.:
>>
>> "Reset password for tape key-copy using original, root-only
>> accessible key"
>>
>>> + default: false,
>>> + },
>>> digest: {
>>> optional: true,
>>> schema: PROXMOX_CONFIG_DIGEST_SCHEMA,
>>> @@ -91,9 +98,10 @@ pub fn list_keys(
>>> /// Change the encryption key's password (and password hint).
>>> pub fn change_passphrase(
>>> kdf: Option<Kdf>,
>>> - password: String,
>>> + password: Option<String>,
>>> new_password: String,
>>> hint: String,
>>> + force: bool,
>>> fingerprint: Fingerprint,
>>> digest: Option<String>,
>>> _rpcenv: &mut dyn RpcEnvironment
>>> @@ -116,10 +124,32 @@ pub fn change_passphrase(
>>> let key_config = match config_map.get(&fingerprint) {
>>> Some(key_config) => key_config,
>>> - None => bail!("tape encryption key '{}' does not exist.",
>>> fingerprint),
>>> + None => bail!("tape encryption key configuration '{}' does
>>> not exist.", fingerprint),
>>> + };
>>> +
>>> + // sanity checks for "password xor --force"
>>> + if force && password.is_some() {
>>> + bail!("password is not allowed when using force")
>>> + }
>>> +
>>> + if !force && password.is_none() {
>>> + bail!("missing parameter: password")
>>> + }
>> Above two if's could be written slightly shorter while IMO even
>> improving readability
>>
>> match (force, password) {
>> (true, Some(_)) => bail!("password is not allowed when using
>> force"),
>> (false, None) => bail!("missing parameter: password"),
>> _ => (), // OK
>> }
> This does not work, because here password is moved into the match
> expression. The borrow checker will complain about it being used later
> on when trying to decrypt the key configuration. You could clone
> password here, but this solution strikes me as rather "inelegant".
>> We probably could even extend this over the "decrypt old key or force
>> loading old key from plaintext
>> file" part below, so that the whole things looks something like
>> (untested):
>>
>>
>> let (key, created, fingerprint) = match (force, password) {
>> (true, Some(_)) => bail!("password is not allowed when using
>> force"),
>> (false, None) => bail!("missing parameter: password"),
>> (true, Some(pass)) => key_config.decrypt(&||
>> Ok(pass.as_bytes().to_vec()))?,
>> (false, None) => {
>> let key = load_keys()?.map(|keys, _|
>> key.get(&fingerprint)).unwrap_or_else(|| bail!("..."));
>> (key, key_config.created, fingerprint)
>> }
>> }
>>
>> but not to hard feeling there, may get seen as a little bit too
>> condensed...
>
> I considered this in a previous version, but dismissed it for the same
> reason. It would, however, solve the borrow checker issue from before.
> Note that this code doesn't work either due to trait and type
> constraints/mismatches. You could do something like this:
>
> let key = match load_keys()?.0.get(&fingerprint) {
> Some(k) => k.key,
> None => bail!("failed to reset passphrase, could not find key
> '{}'", fingerprint)
> };
>
> Please tell me your preference and I'll be happy to submit an updated
> patch.
>
>>> +
>>> + // decrypt old key or force loading old key from plaintext file
>>> + let (key, created, fingerprint) = if let Some(pass) = password {
>>> + key_config.decrypt(&|| Ok(pass.as_bytes().to_vec()))?
>>> + } else {
>>> + let (key_map, _) = load_keys()?;
>>> +
>>> + let key = match key_map.get(&fingerprint) {
>>> + Some(k) => k.key,
>>> + None => bail!("tape encryption key '{}' does not
>>> exist.", fingerprint)
>> error message could be slightly improved with context:
>> "failed to reset key password, original tape enc..."
>>
>>> + };
>>> +
>>> + (key, key_config.created, fingerprint)
>>> };
>>> - let (key, created, fingerprint) = key_config.decrypt(&||
>>> Ok(password.as_bytes().to_vec()))?;
>>> let mut new_key_config = KeyConfig::with_key(&key,
>>> new_password.as_bytes(), kdf)?;
>>> new_key_config.created = created; // keep original value
>>> new_key_config.hint = Some(hint);
>
>
Sorry forgot to hit reply-all.
More information about the pbs-devel
mailing list