Re: [pbs-devel] [PATCH proxmox{, -backup} v2 00/12] auth-api clean up and improvements

[Max Carrara <m.carrara@proxmox.com>]

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>