[pbs-devel] [PATCH proxmox-backup v2 1/2] fix #3853: api: add force option to tape key change-passphrase

[pbs-devel] [PATCH proxmox-backup v2 1/2] fix #3853: api: add force option to tape key change-passphrase

[Stefan Sterz]

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. Requires super user privileges.

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
v1->v2: check for root privileges moved into the api endpoint, better
descriptions and errors strings and incorporated some nitpicks.

Thanks for the feedback to Thomas Lamprecht, Dominik Csapak, and 
Wolfgang Bumiller. 

 src/api2/config/tape_encryption_keys.rs | 41 +++++++++++++++++++++----
 1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/src/api2/config/tape_encryption_keys.rs b/src/api2/config/tape_encryption_keys.rs
index 1ad99377..25cc6cc0 100644
--- a/src/api2/config/tape_encryption_keys.rs
+++ b/src/api2/config/tape_encryption_keys.rs
@@ -1,4 +1,4 @@
-use anyhow::{bail, Error};
+use anyhow::{format_err, bail, Error};
 use serde_json::Value;
 use hex::FromHex;
 
@@ -6,12 +6,14 @@ use proxmox_router::{ApiMethod, Router, RpcEnvironment, Permission};
 use proxmox_schema::api;
 
 use pbs_api_types::{
-    Fingerprint, KeyInfo, Kdf,
+    Authid, Fingerprint, KeyInfo, Kdf,
     TAPE_ENCRYPTION_KEY_FINGERPRINT_SCHEMA,
     PROXMOX_CONFIG_DIGEST_SCHEMA, PASSWORD_HINT_SCHEMA,
     PRIV_TAPE_AUDIT, PRIV_TAPE_MODIFY,
 };
 
+use pbs_config::CachedUserInfo;
+
 use pbs_config::key_config::KeyConfig;
 use pbs_config::open_backup_lockfile;
 use pbs_config::tape_encryption_keys::{
@@ -70,6 +72,7 @@ pub fn list_keys(
             password: {
                 description: "The current password.",
                 min_length: 5,
+                optional: true,
             },
             "new-password": {
                 description: "The new password.",
@@ -78,6 +81,12 @@ pub fn list_keys(
             hint: {
                 schema: PASSWORD_HINT_SCHEMA,
             },
+            force: {
+                optional: true,
+                type: bool,
+                description: "Reset the passphrase for a tape key, using the root-only accessible copy.",
+                default: false,
+            },
             digest: {
                 optional: true,
                 schema: PROXMOX_CONFIG_DIGEST_SCHEMA,
@@ -91,12 +100,13 @@ 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
+    rpcenv: &mut dyn RpcEnvironment
 ) -> Result<(), Error> {
 
     let kdf = kdf.unwrap_or_default();
@@ -116,10 +126,29 @@ 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),
+    };
+
+    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+    let user_info = CachedUserInfo::new()?;
+
+    if force && !user_info.is_superuser(&auth_id) {
+        bail!("resetting the key's passphrase requires root privileges")
+    }
+
+    let (key, created, fingerprint) = match (force, &password) {
+        (true, Some(_)) => bail!("password is not allowed when using force"),
+        (false, None) => bail!("missing parameter: password"),
+        (false, Some(pass)) => key_config.decrypt(&|| Ok(pass.as_bytes().to_vec()))?,
+        (true, None) => {
+                let key = load_keys()?.0.get(&fingerprint).ok_or_else(|| {
+                    format_err!("failed to reset passphrase, could not find key '{}'", fingerprint)
+                })?.key;
+
+                (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);
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v2 2/2] fix #3853: tape cli: add force flag to key change-passphrase

[Stefan Sterz]

Adds the '--force' flag to the proxmox-tape command allowing users
with root privileges to overwrite the passphrase of a given key.

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
 src/bin/proxmox_tape/encryption_key.rs | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/bin/proxmox_tape/encryption_key.rs b/src/bin/proxmox_tape/encryption_key.rs
index 156295fd..71df9ffa 100644
--- a/src/bin/proxmox_tape/encryption_key.rs
+++ b/src/bin/proxmox_tape/encryption_key.rs
@@ -146,11 +146,18 @@ fn show_key(
             hint: {
                 schema: PASSWORD_HINT_SCHEMA,
             },
+            force: {
+                optional: true,
+                type: bool,
+                description: "Reset the passphrase for a tape key, without asking for the old one.",
+                default: false,
+            },
         },
     },
 )]
 /// Change the encryption key's password.
 fn change_passphrase(
+    force: bool,
     mut param: Value,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<(), Error> {
@@ -159,11 +166,15 @@ fn change_passphrase(
         bail!("unable to change passphrase - no tty");
     }
 
-    let password = tty::read_password("Current Tape Encryption Key Password: ")?;
+    if force {
+        param["force"] = serde_json::Value::Bool(true);
+    } else {
+        let password = tty::read_password("Current Tape Encryption Key Password: ")?;
+        param["password"] = String::from_utf8(password)?.into();
+    }
 
     let new_password = tty::read_and_verify_password("New Tape Encryption Key Password: ")?;
 
-    param["password"] = String::from_utf8(password)?.into();
     param["new-password"] = String::from_utf8(new_password)?.into();
 
     let info = &api2::config::tape_encryption_keys::API_METHOD_CHANGE_PASSPHRASE;
-- 
2.30.2

[pbs-devel] applied-series: [PATCH proxmox-backup v2 1/2] fix #3853: api: add force option to tape key change-passphrase

[Thomas Lamprecht]

On 10.02.22 15:23, 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. Requires super user privileges.
> 
> Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
> ---
> v1->v2: check for root privileges moved into the api endpoint, better
> descriptions and errors strings and incorporated some nitpicks.
> 
> Thanks for the feedback to Thomas Lamprecht, Dominik Csapak, and 
> Wolfgang Bumiller. 
> 
>  src/api2/config/tape_encryption_keys.rs | 41 +++++++++++++++++++++----
>  1 file changed, 35 insertions(+), 6 deletions(-)
> 
>

applied series, thanks!

Re: [pbs-devel] [PATCH proxmox-backup v2 1/2] fix #3853: api: add force option to tape key change-passphrase

[Dominik Csapak]

one nit inline (could be done as a followup)

On 2/10/22 15:23, 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. Requires super user privileges.
> 
> Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
> ---
> v1->v2: check for root privileges moved into the api endpoint, better
> descriptions and errors strings and incorporated some nitpicks.
> 
> Thanks for the feedback to Thomas Lamprecht, Dominik Csapak, and
> Wolfgang Bumiller.
> 
>   src/api2/config/tape_encryption_keys.rs | 41 +++++++++++++++++++++----
>   1 file changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/src/api2/config/tape_encryption_keys.rs b/src/api2/config/tape_encryption_keys.rs
> index 1ad99377..25cc6cc0 100644
> --- a/src/api2/config/tape_encryption_keys.rs
> +++ b/src/api2/config/tape_encryption_keys.rs
> @@ -1,4 +1,4 @@
> -use anyhow::{bail, Error};
> +use anyhow::{format_err, bail, Error};
>   use serde_json::Value;
>   use hex::FromHex;
>   
> @@ -6,12 +6,14 @@ use proxmox_router::{ApiMethod, Router, RpcEnvironment, Permission};
>   use proxmox_schema::api;
>   
>   use pbs_api_types::{
> -    Fingerprint, KeyInfo, Kdf,
> +    Authid, Fingerprint, KeyInfo, Kdf,
>       TAPE_ENCRYPTION_KEY_FINGERPRINT_SCHEMA,
>       PROXMOX_CONFIG_DIGEST_SCHEMA, PASSWORD_HINT_SCHEMA,
>       PRIV_TAPE_AUDIT, PRIV_TAPE_MODIFY,
>   };
>   
> +use pbs_config::CachedUserInfo;
> +
>   use pbs_config::key_config::KeyConfig;
>   use pbs_config::open_backup_lockfile;
>   use pbs_config::tape_encryption_keys::{
> @@ -70,6 +72,7 @@ pub fn list_keys(
>               password: {
>                   description: "The current password.",
>                   min_length: 5,
> +                optional: true,
>               },
>               "new-password": {
>                   description: "The new password.",
> @@ -78,6 +81,12 @@ pub fn list_keys(
>               hint: {
>                   schema: PASSWORD_HINT_SCHEMA,
>               },
> +            force: {
> +                optional: true,
> +                type: bool,
> +                description: "Reset the passphrase for a tape key, using the root-only accessible copy.",
> +                default: false,
> +            },
>               digest: {
>                   optional: true,
>                   schema: PROXMOX_CONFIG_DIGEST_SCHEMA,
> @@ -91,12 +100,13 @@ 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
> +    rpcenv: &mut dyn RpcEnvironment
>   ) -> Result<(), Error> {
>   
>       let kdf = kdf.unwrap_or_default();
> @@ -116,10 +126,29 @@ 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),
> +    };
> +
> +    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> +    let user_info = CachedUserInfo::new()?;
> +
> +    if force && !user_info.is_superuser(&auth_id) {
> +        bail!("resetting the key's passphrase requires root privileges")
> +    }
> +
> +    let (key, created, fingerprint) = match (force, &password) {
> +        (true, Some(_)) => bail!("password is not allowed when using force"),
> +        (false, None) => bail!("missing parameter: password"),

those two errors could make use of 'ParameterError'
see proxmox-schema/src/schema.rs


> +        (false, Some(pass)) => key_config.decrypt(&|| Ok(pass.as_bytes().to_vec()))?,
> +        (true, None) => {
> +                let key = load_keys()?.0.get(&fingerprint).ok_or_else(|| {
> +                    format_err!("failed to reset passphrase, could not find key '{}'", fingerprint)
> +                })?.key;
> +
> +                (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);

Re: [pbs-devel] [PATCH proxmox-backup v2 1/2] fix #3853: api: add force option to tape key change-passphrase

[Thomas Lamprecht]

On 14.02.22 10:20, Dominik Csapak wrote:
>> @@ -116,10 +126,29 @@ 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),
>> +    };
>> +
>> +    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>> +    let user_info = CachedUserInfo::new()?;
>> +
>> +    if force && !user_info.is_superuser(&auth_id) {
>> +        bail!("resetting the key's passphrase requires root privileges")
>> +    }
>> +
>> +    let (key, created, fingerprint) = match (force, &password) {
>> +        (true, Some(_)) => bail!("password is not allowed when using force"),
>> +        (false, None) => bail!("missing parameter: password"),
> 
> those two errors could make use of 'ParameterError'
> see proxmox-schema/src/schema.rs

But as we do not have any `raise_param_exc` like helper switching to that is not really useful
FWICT, as the complete error message would need to be constructed manually anyway and would be
more verbose.

(btw. please trim context on reply)