* [pbs-devel] [PATCH proxmox-backup 1/2] fix #3853: api: add force option to tape key change-passphrase @ 2022-02-07 12:48 Stefan Sterz 2022-02-07 12:48 ` [pbs-devel] [PATCH proxmox-backup 2/2] fix #3853: tape cli: add force flag to " Stefan Sterz ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Stefan Sterz @ 2022-02-07 12:48 UTC (permalink / raw) To: pbs-devel 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. Signed-off-by: Stefan Sterz <s.sterz@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.", + 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") + } + + // 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) + }; + + (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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 2/2] fix #3853: tape cli: add force flag to key change-passphrase 2022-02-07 12:48 [pbs-devel] [PATCH proxmox-backup 1/2] fix #3853: api: add force option to tape key change-passphrase Stefan Sterz @ 2022-02-07 12:48 ` Stefan Sterz 2022-02-09 13:56 ` Wolfgang Bumiller 2022-02-07 14:58 ` [pbs-devel] [PATCH proxmox-backup 1/2] fix #3853: api: add force option to tape " Thomas Lamprecht 2022-02-09 13:52 ` Wolfgang Bumiller 2 siblings, 1 reply; 9+ messages in thread From: Stefan Sterz @ 2022-02-07 12:48 UTC (permalink / raw) To: pbs-devel 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 | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/src/bin/proxmox_tape/encryption_key.rs b/src/bin/proxmox_tape/encryption_key.rs index 156295fd..3dd9b58b 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: "Don't ask for the old passphrase and overwrite it. Root only.", + 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,23 @@ fn change_passphrase( bail!("unable to change passphrase - no tty"); } - let password = tty::read_password("Current Tape Encryption Key Password: ")?; + let effective_uid = nix::unistd::Uid::effective(); + let running_uid = nix::unistd::Uid::current(); + + // if the `--force` flag is provided check if we are root + if force && !effective_uid.is_root() && !running_uid.is_root() { + bail!("the force flag requires root privileges"); + } + + 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 2/2] fix #3853: tape cli: add force flag to key change-passphrase 2022-02-07 12:48 ` [pbs-devel] [PATCH proxmox-backup 2/2] fix #3853: tape cli: add force flag to " Stefan Sterz @ 2022-02-09 13:56 ` Wolfgang Bumiller 0 siblings, 0 replies; 9+ messages in thread From: Wolfgang Bumiller @ 2022-02-09 13:56 UTC (permalink / raw) To: Stefan Sterz; +Cc: pbs-devel On Mon, Feb 07, 2022 at 01:48:25PM +0100, Stefan Sterz wrote: > 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 | 23 +++++++++++++++++++++-- > 1 file changed, 21 insertions(+), 2 deletions(-) > > diff --git a/src/bin/proxmox_tape/encryption_key.rs b/src/bin/proxmox_tape/encryption_key.rs > index 156295fd..3dd9b58b 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: "Don't ask for the old passphrase and overwrite it. Root only.", > + 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,23 @@ fn change_passphrase( > bail!("unable to change passphrase - no tty"); > } > > - let password = tty::read_password("Current Tape Encryption Key Password: ")?; > + let effective_uid = nix::unistd::Uid::effective(); > + let running_uid = nix::unistd::Uid::current(); > + > + // if the `--force` flag is provided check if we are root > + if force && !effective_uid.is_root() && !running_uid.is_root() { > + bail!("the force flag requires root privileges"); > + } ^ Wrong place for this. Also, as a general note, checks such as this one should simply not be done by CLI tools unless they're actually installed as `setuid-root` since it's the OS' job to do permission checks. I may be running an unprivileged root (dropped capabilities, entered some restrictive AppArmor label), or I may actually be a *privileged* non-zero-uid for some reason as well... ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 1/2] fix #3853: api: add force option to tape key change-passphrase 2022-02-07 12:48 [pbs-devel] [PATCH proxmox-backup 1/2] fix #3853: api: add force option to tape key change-passphrase Stefan Sterz 2022-02-07 12:48 ` [pbs-devel] [PATCH proxmox-backup 2/2] fix #3853: tape cli: add force flag to " Stefan Sterz @ 2022-02-07 14:58 ` Thomas Lamprecht 2022-02-07 16:14 ` Stefan Sterz 2022-02-09 13:52 ` Wolfgang Bumiller 2 siblings, 1 reply; 9+ messages in thread From: Thomas Lamprecht @ 2022-02-07 14:58 UTC (permalink / raw) To: Proxmox Backup Server development discussion, Stefan Sterz 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@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 } 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... > + > + // 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); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 1/2] fix #3853: api: add force option to tape key change-passphrase 2022-02-07 14:58 ` [pbs-devel] [PATCH proxmox-backup 1/2] fix #3853: api: add force option to tape " Thomas Lamprecht @ 2022-02-07 16:14 ` Stefan Sterz 2022-02-08 15:26 ` Dominik Csapak 0 siblings, 1 reply; 9+ messages in thread From: Stefan Sterz @ 2022-02-07 16:14 UTC (permalink / raw) To: Thomas Lamprecht, Proxmox Backup Server development discussion 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@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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 1/2] fix #3853: api: add force option to tape key change-passphrase 2022-02-07 16:14 ` Stefan Sterz @ 2022-02-08 15:26 ` Dominik Csapak 2022-02-08 15:30 ` Stefan Sterz 0 siblings, 1 reply; 9+ messages in thread From: Dominik Csapak @ 2022-02-08 15:26 UTC (permalink / raw) To: Proxmox Backup Server development discussion, Stefan Sterz, Thomas Lamprecht On 2/7/22 17:14, Stefan Sterz wrote: > 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: [snip] >>>> + // 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". did not look at the rest of the patch really, but i think thats wrong.. couldn't you simply use the reference instead? then the value will not be moved? match (force, &password) { ... } ? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 1/2] fix #3853: api: add force option to tape key change-passphrase 2022-02-08 15:26 ` Dominik Csapak @ 2022-02-08 15:30 ` Stefan Sterz 2022-02-09 15:54 ` Thomas Lamprecht 0 siblings, 1 reply; 9+ messages in thread From: Stefan Sterz @ 2022-02-08 15:30 UTC (permalink / raw) To: Dominik Csapak, Proxmox Backup Server development discussion, Thomas Lamprecht On 2/8/22 16:26, Dominik Csapak wrote: > On 2/7/22 17:14, Stefan Sterz wrote: >> 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: > [snip] >>>>> + // 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". > > did not look at the rest of the patch really, but i think thats wrong.. > > couldn't you simply use the reference instead? then the value > will not be moved? > > match (force, &password) { > ... > } > > > ? Yes sorry, using a reference works too. The value is moved when the tuple is constructed is what I meant. This can be avoided by cloning it or as you mentioned using a reference. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 1/2] fix #3853: api: add force option to tape key change-passphrase 2022-02-08 15:30 ` Stefan Sterz @ 2022-02-09 15:54 ` Thomas Lamprecht 0 siblings, 0 replies; 9+ messages in thread From: Thomas Lamprecht @ 2022-02-09 15:54 UTC (permalink / raw) To: Stefan Sterz, Dominik Csapak, Proxmox Backup Server development discussion On 08.02.22 16:30, Stefan Sterz wrote: > On 2/8/22 16:26, Dominik Csapak wrote: >> On 2/7/22 17:14, Stefan Sterz wrote: >>> 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: >> [snip] >>>>>> + // 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". Yeah I just wrote that in the MTA from top of my head so that you get the rough idea, using a reference here like Dominik also suggests is the obvious way to make it work as intended.. ;-) The whole thing, actually working, meaning it compiles ^^: 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 (keys, _) = load_keys()?; let key_info = keys.get(&fingerprint).ok_or_else(|| format_err!("..."))?; (key_info.key, key_config.created, fingerprint) } }; IMO having this as match where we know we got all cases covered has it's value and it's not that bad to read, but I also do not want to spent to much time nitpicking on this one, so your call if it's the whole match or just the checks in the match. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 1/2] fix #3853: api: add force option to tape key change-passphrase 2022-02-07 12:48 [pbs-devel] [PATCH proxmox-backup 1/2] fix #3853: api: add force option to tape key change-passphrase Stefan Sterz 2022-02-07 12:48 ` [pbs-devel] [PATCH proxmox-backup 2/2] fix #3853: tape cli: add force flag to " Stefan Sterz 2022-02-07 14:58 ` [pbs-devel] [PATCH proxmox-backup 1/2] fix #3853: api: add force option to tape " Thomas Lamprecht @ 2022-02-09 13:52 ` Wolfgang Bumiller 2 siblings, 0 replies; 9+ messages in thread From: Wolfgang Bumiller @ 2022-02-09 13:52 UTC (permalink / raw) To: Stefan Sterz; +Cc: pbs-devel On Mon, Feb 07, 2022 at 01:48:24PM +0100, 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. > > Signed-off-by: Stefan Sterz <s.sterz@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.", "Root only.", but this seems to lack the actual permission check. You seem to be doing this on the CLI side in the other patch, but that's the wrong place for it, the API is what's reachable from the outside. Also, as an HTTP API endpoint, "don't ask for the old passphrase" is a bit of a weird description ;-) ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-02-09 15:55 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-02-07 12:48 [pbs-devel] [PATCH proxmox-backup 1/2] fix #3853: api: add force option to tape key change-passphrase Stefan Sterz 2022-02-07 12:48 ` [pbs-devel] [PATCH proxmox-backup 2/2] fix #3853: tape cli: add force flag to " Stefan Sterz 2022-02-09 13:56 ` Wolfgang Bumiller 2022-02-07 14:58 ` [pbs-devel] [PATCH proxmox-backup 1/2] fix #3853: api: add force option to tape " Thomas Lamprecht 2022-02-07 16:14 ` Stefan Sterz 2022-02-08 15:26 ` Dominik Csapak 2022-02-08 15:30 ` Stefan Sterz 2022-02-09 15:54 ` Thomas Lamprecht 2022-02-09 13:52 ` Wolfgang Bumiller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox