From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <m.carrara@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 355ABB8272
 for <pbs-devel@lists.proxmox.com>; Thu,  7 Mar 2024 11:12:12 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
 by firstgate.proxmox.com (Proxmox) with ESMTP id 1CED23446F
 for <pbs-devel@lists.proxmox.com>; Thu,  7 Mar 2024 11:12:12 +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))
 (No client certificate requested)
 by firstgate.proxmox.com (Proxmox) with ESMTPS
 for <pbs-devel@lists.proxmox.com>; Thu,  7 Mar 2024 11:12:11 +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 5996048849
 for <pbs-devel@lists.proxmox.com>; Thu,  7 Mar 2024 11:12:11 +0100 (CET)
Message-ID: <8abb8e05-fe1d-4c5a-94cc-a3913967c58d@proxmox.com>
Date: Thu, 7 Mar 2024 11:12:10 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Content-Language: en-US
To: Proxmox Backup Server development discussion
 <pbs-devel@lists.proxmox.com>, Stefan Sterz <s.sterz@proxmox.com>
References: <20240306123609.164021-1-s.sterz@proxmox.com>
From: Max Carrara <m.carrara@proxmox.com>
In-Reply-To: <20240306123609.164021-1-s.sterz@proxmox.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
X-SPAM-LEVEL: Spam detection results:  0
 AWL 0.003 Adjusted score from AWL reputation of From: address
 BAYES_00                 -1.9 Bayes spam probability is 0 to 1%
 DMARC_MISSING             0.1 Missing DMARC policy
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 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} v2 00/12] auth-api clean up and improvements
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: Thu, 07 Mar 2024 10:12:12 -0000

On 3/6/24 13:35, Stefan Sterz wrote:
> this series adds some more functionality to `proxmox-auth-api` and tries
> to clean it up a little. the first commit moves signing into the keyring
> itself, instead of exposing openssl's `Signer` further.
> 
> the second commit replaces our old P-256 ec signatures with the Ed25519
> scheme which offers similar security but is a bit more modern and tries
> to avoid common implementation pitfalls.
> 
> the third commit adds hmac signing to `proxmox-auth-api`'s `Keyring`
> which is useful for applications where only one daemon is issueing and
> verifying tickets. hmac uses symmetric keys and is much more efficient
> than asymetric signature schemes. the downside being that the verifier
> and the signer need to be the exact same party, as the verification key
> can also be used to issue new signatures.
> 
> the fourth commit uses a constant time comparison for our csrf tokens to
> dimnish the chance of side-channel attack there. the fifth commit uses
> the hmac functionality to sign csrf tokens. here we previously used a
> self-rolled potentially insecure method of generating these tokens. hmac
> avoids common pitfalls here. the commit also provides a fallback to
> avoid compatability issues.
> 
> the next two commits move our password hashing scheme to yescrypt and
> implement a constant time comparison for password hashes. the final
> commit for `proxmox-auth-api` cleans up some test cases that were
> failing for the wrong reasons.
> 
> the four commits on the proxmox backup server side do the following:
> 
> - use hmac keys when generating new csrf tokens
> - upgrade password hashes on log in if they are not using the latest
>   password hash function already
> - use the auth-api's wrapper types to load authkeys
> - use Ed25519 keys when generating new auth keys
> 
> the first and the last commit here will require a bump of
> `proxmox-auth-api`. the second commit also needs a bump to
> `proxmox-sys`.
> 

I like all the changes you made since v1!

Regarding the Code
------------------

* All of the changes are easy to follow; IMO you explained your reasoning
  very well
* Patches apply cleanly
* `cargo fmt` only changed a single line - honestly a non-issue, can just
  be fixed when applying the patch IMO (see comment on patch)
* `cargo clippy` doesn't complain (about your changes)

My initial confusion regarding the hash upgrading on login was also
resolved (thanks for your answers off list).


Testing
-------

Note: I shamelessly commented out some lines in proxmox-schema while
the GUI thing is being worked on

* Made a snapshot of my testing VM and did a clean build of master
* Created a user called "test" - checked their hash in /etc/proxmox-backup/shadow.json
* Applied and built your patches, deployed everything
* Logged in and out as "test" user
* Checked hash again - hash was upgraded successfully
* New CSRF token also seems to work (didn't notice anything complaining)


Conclusion
----------

LGTM; everything just worked. Maybe somebody else can also chime in and
test things, just in case.

Reviewed-by: Max Carrara <m.carrara@proxmox.com>
Tested-by: Max Carrara <m.carrara@proxmox.com>