public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Esi Y <esiy0676+proxmox@gmail.com>
To: Stefan Sterz <s.sterz@proxmox.com>
Cc: pbs-devel@lists.proxmox.com
Subject: Re: [pbs-devel] [PATCH proxmox 01/12] auth-api: move signing into the private key
Date: Tue, 27 Feb 2024 18:13:58 +0000	[thread overview]
Message-ID: <Zd4mZk/KtH5lmYSj@tema539532> (raw)
In-Reply-To: <CZFQUI78DNL3.3E2OA5W0YXN12@proxmox.com>

On Tue, Feb 27, 2024 at 10:12:10AM +0100, Stefan Sterz wrote:
>On Mon Feb 26, 2024 at 9:22 PM CET, Esi Y wrote:
>> This might be a bit nitpicky, but wouldn't this exactly be an
>> opportunity to properly introduce traits, so that in the future on the
>> same kind of swapout, it is more streamlined and less regression prone
>> going forward? Just wondering ... if there was a reason not to, on
>> such a rewrite.
>
>i am a bit uncertain as to what you mean here, as this series more or
>less uses such wrapper traits around the openssl api. `PrivateKey` and
>`PublicKey` wrap a `PKey<Private>` and `PKey<Public>` respectivelly.
>`HMACKey` wraps a `PKey<Private>` that is treated as an hmac key. this
>means that users of the auht-api crate don't have to use openssl to
>generate keys or handle them. it also means that the implementation of
>these traits is entirely private in the auth-api, so users don't need to
>adapt if we extend them with support for more cryptographic schemes
>(note that removing such schemes is always a breaking change).

This is a misundertanding indeed, what I was getting at was the 
state you already came to, so be easy on me in return please (too).

Wrapping PKey<T> is not really doing much when in fact auth_key.rs does 
not really encapsulate, an AuthKey. The whole thing still looks more 
like openssl helper of some universal kind with a keyring vector 
tossed in.

For instance there's no reason to follow the openssl in this even, 
there's no need to be splitting public and private key based on what is 
just a key processing tool logic. It could be AnyKey that can generate, 
derive, etc. This could have seperate impls, e.g. V1Key, V2Key, etc. 
The ticket.rs would ideally not even get to be passing on which 
MessageDigest it wants, it would just handle the formatting.

The AnyKey would be able (or not) to do import, export, sign, verify, 
(another trait could even do encrypt, decrypt). Unless I am missing 
something, the Keyring should not allow mixing key types and would 
generically handle AnyKeys' vector with a reference to current 
signing key.

>in the future, if you needed to switch out a key you have the keyring in
>the authcontext that can handle roll over (for the most part, but i am
>working on the missing links here). new keys would be added as the
>signing key (hence my introduction of the `SigningKey` and
>`VerificationKey` enums in patch 3) the old keys would stay as
>verification keys and be evicted after a given number of key rotations.

This is nice as for rotation, I am more looking at upgrading API 
situation, where I am surprised at duplicate original code elements 
such as:

if let Ok(rsa) = self.key.rsa() { ...
if let Ok(ec) = self.key.ec_key() { ...

No love for polymorphism, but also convoluted:

pub fn generate_rsa()
pub fn generate_ec()

... all in one struct. I know this was all there originally, but does 
it have to stay?

>using enums here was just simple, and unless a third type of crypto
>beside symmetric and asymmetric arises, this should be sufficient and
>not overcomplicate the code.

The beauty of not worrying about any of this, i.e. having it 
encapsulated would mean that e.g. one does not need to know what 
crypto or digest is in use, if it's (a)symmetric and one calls any 
function, AneKey would know best which key to use for that operation. 
And ticket.rs shouldn't really care.

>what kind of traits would you propose to make such transitions even
>easier?

I suspect I get not much sympathy for the above already. I do not 
think it's making it more complicated though, more the other way 
around. Not for someone who knows difference between one time pad 
and prime fields' differences in Edwards curves.

>ps: hard wrapping your emails would be nice :)

I tried now, I hope aerc can handle format=flowed. I did set it to 
66 LaTeX-ish syndrome, but could not discriminate any further. I 
mean, I do like to uphold the good old of being conservative in 
what I send out and liberal in what I receive. Unlike some others:
https://lkml.org/lkml/2020/5/29/1038

>-- >8 snip 8< --
>
>



  reply	other threads:[~2024-02-27 18:14 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-15 15:19 [pbs-devel] [PATCH proxmox{, -backup} 00/12] authentication cleanup and Stefan Sterz
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 01/12] auth-api: move signing into the private key Stefan Sterz
2024-02-26 20:22   ` Esi Y
2024-02-27  9:12     ` Stefan Sterz
2024-02-27 18:13       ` Esi Y [this message]
2024-02-29 16:07         ` Stefan Sterz
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 02/12] auth-api: move to Ed25519 signatures Stefan Sterz
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 03/12] auth-api: add ability to use hmac singing in keyring Stefan Sterz
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 04/12] auth-api: move to hmac signing for csrf tokens Stefan Sterz
2024-02-19 16:02   ` Max Carrara
2024-02-20 12:54     ` Max Carrara
2024-02-23  9:26       ` Stefan Sterz
2024-02-23 10:48         ` Thomas Lamprecht
2024-02-23 10:52           ` Stefan Sterz
2024-02-23 13:06         ` Wolfgang Bumiller
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 05/12] sys: crypt: move to yescrypt for password hashing Stefan Sterz
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 06/12] sys: crypt: use constant time comparison for password verification Stefan Sterz
2024-02-19 16:11   ` Max Carrara
2024-02-23  9:26     ` Stefan Sterz
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 07/12] sys: crypt: add helper to allow upgrading hashes Stefan Sterz
2024-02-19 18:50   ` Max Carrara
2024-02-23  9:26     ` Stefan Sterz
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 08/12] auth-api: fix types `compilefail` test Stefan Sterz
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox-backup 09/12] auth: move to hmac keys for csrf tokens Stefan Sterz
2024-02-19 18:55   ` Max Carrara
2024-02-23  9:26     ` Stefan Sterz
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox-backup 10/12] auth: upgrade hashes on user log in Stefan Sterz
2024-02-19 18:58   ` Max Carrara
2024-02-23  9:26     ` Stefan Sterz
2024-02-15 15:20 ` [pbs-devel] [PATCH proxmox-backup 11/12] auth/manager: add manager command to upgrade hashes Stefan Sterz
2024-02-19 19:06   ` Max Carrara
2024-02-23  9:26     ` Stefan Sterz
2024-02-15 15:20 ` [pbs-devel] [PATCH proxmox-backup 12/12] auth: us ec keys as auth keys Stefan Sterz
2024-02-19 19:10   ` Max Carrara
2024-02-23  9:26     ` Stefan Sterz

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=Zd4mZk/KtH5lmYSj@tema539532 \
    --to=esiy0676+proxmox@gmail.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
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal