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

[Dominik Csapak <d.csapak@proxmox.com>]

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);