From: Max Carrara <m.carrara@proxmox.com>
To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>, Stefan Sterz <s.sterz@proxmox.com>
Subject: Re: [pbs-devel] [PATCH proxmox{, -backup} v2 00/12] auth-api clean up and improvements
Date: Thu, 7 Mar 2024 11:12:10 +0100 [thread overview]
Message-ID: <8abb8e05-fe1d-4c5a-94cc-a3913967c58d@proxmox.com> (raw)
In-Reply-To: <20240306123609.164021-1-s.sterz@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>
next prev parent reply other threads:[~2024-03-07 10:12 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-06 12:35 Stefan Sterz
2024-03-06 12:35 ` [pbs-devel] [PATCH proxmox v2 01/12] auth-api: move signing into the private key Stefan Sterz
2024-03-06 12:35 ` [pbs-devel] [PATCH proxmox v2 02/12] auth-api: move to Ed25519 signatures Stefan Sterz
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox v2 03/12] auth-api: add ability to use hmac singing in keyring Stefan Sterz
2024-03-07 10:11 ` Max Carrara
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox v2 04/12] auth-api: use constant time comparison for csrf tokens Stefan Sterz
2024-03-07 10:17 ` Max Carrara
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox v2 05/12] auth-api: move to hmac signing " Stefan Sterz
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox v2 06/12] sys: crypt: move to yescrypt for password hashing Stefan Sterz
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox v2 07/12] sys: crypt: use constant time comparison for password verification Stefan Sterz
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox v2 08/12] auth-api: fix types `compilefail` test Stefan Sterz
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox-backup v2 09/12] auth: move to hmac keys for csrf tokens Stefan Sterz
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox-backup v2 10/12] auth: upgrade hashes on user log in Stefan Sterz
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox-backup v2 11/12] auth: move to auth-api's private and public keys when loading keys Stefan Sterz
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox-backup v2 12/12] auth: use auth-api when generating keys and generate ec keys Stefan Sterz
2024-03-07 10:12 ` Max Carrara [this message]
2024-05-22 14:13 ` [pbs-devel] applied-series: [PATCH proxmox{, -backup} v2 00/12] auth-api clean up and improvements Wolfgang Bumiller
2024-05-24 8:45 ` Max Carrara
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8abb8e05-fe1d-4c5a-94cc-a3913967c58d@proxmox.com \
--to=m.carrara@proxmox.com \
--cc=pbs-devel@lists.proxmox.com \
--cc=s.sterz@proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox