From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 ; 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 ; 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 ; 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 ; 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 , Stefan Sterz References: <20240306123609.164021-1-s.sterz@proxmox.com> From: Max Carrara 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-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 Tested-by: Max Carrara