From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <s.sterz@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits))
 (No client certificate requested)
 by lists.proxmox.com (Postfix) with ESMTPS id C3B2A60E9D
 for <pbs-devel@lists.proxmox.com>; Mon,  7 Feb 2022 17:15:24 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
 by firstgate.proxmox.com (Proxmox) with ESMTP id BB3342717B
 for <pbs-devel@lists.proxmox.com>; Mon,  7 Feb 2022 17:14:54 +0100 (CET)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
 [94.136.29.106])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256)
 (No client certificate requested)
 by firstgate.proxmox.com (Proxmox) with ESMTPS id 36BBA2716F
 for <pbs-devel@lists.proxmox.com>; Mon,  7 Feb 2022 17:14:53 +0100 (CET)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
 by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 0893F46208
 for <pbs-devel@lists.proxmox.com>; Mon,  7 Feb 2022 17:14:53 +0100 (CET)
Message-ID: <10c2b794-7815-30b8-9957-a89acdafcab1@proxmox.com>
Date: Mon, 7 Feb 2022 17:14:51 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Content-Language: de-AT
To: Thomas Lamprecht <t.lamprecht@proxmox.com>,
 Proxmox Backup Server development discussion <pbs-devel@lists.proxmox.com>
References: <20220207124825.1116194-1-s.sterz@proxmox.com>
 <a6a5daee-5e44-3fa1-f832-1903bce75f59@proxmox.com>
From: Stefan Sterz <s.sterz@proxmox.com>
In-Reply-To: <a6a5daee-5e44-3fa1-f832-1903bce75f59@proxmox.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
X-SPAM-LEVEL: Spam detection results:  0
 AWL 0.000 Adjusted score from AWL reputation of From: address
 BAYES_00                 -1.9 Bayes spam probability is 0 to 1%
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 NICE_REPLY_A           -0.001 Looks like a legit reply (A)
 SPF_HELO_NONE           0.001 SPF: HELO does not publish an SPF Record
 SPF_PASS               -0.001 SPF: sender matches SPF record
 T_SCC_BODY_TEXT_LINE    -0.01 -
Subject: Re: [pbs-devel] [PATCH proxmox-backup 1/2] fix #3853: api: add
 force option to tape key change-passphrase
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
 <pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>, 
 <mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>, 
 <mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Mon, 07 Feb 2022 16:15:24 -0000

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.