[pbs-devel] [PATCH proxmox 01/12] auth-api: move signing into the private key

[Stefan Sterz]

On Tue Feb 27, 2024 at 7:13 PM CET, Esi Y wrote:
> 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).

sorry if that came across as harsh, that wasn't intentional.

> 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.

i guess the file could be called `keyring.rs` instead, but that's bike
shedding about a decision that was already made. not sure this really
helps anyone as `auth_key` is not a public module so users don't really
see this detail anyway. in any case, a keyring handles the actual auth
key, which can be a asymmetric key with a public or private key or a
symmetric hmac key.

> 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.

before i get to the rest of your argument: yes, the digest could also be
hidden from the users of these types. one may argue, though, at least in
the case of hmac, users may want to be able to upgrade the hashing
function without considering all down-stream users of this crate.

also users that are part of the same crate, like ticket.rs, don't really
create much of a maintenance burden as changing function signatures will
be caught by rustc anyway and are easily adapted in this case.

> 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.

i think i can see what you are getting at, but there is one problem with
completelly abstracting away public, private and hmac keys into one
trait: users of this create do need to know what kind of key they are
using and providing to the keyring. for example, in pbs we have two
deamons that need to be able to verify tickets, but only one is allowed
to sign them.

in this case, you can't use HMAC keys as that would break the security
parameters for such a setup (both would be able to verify *and* sign).
so here you need to be able to have a public key type, as the daemon
that only verifies tickets has nothing else available.

so you could have an `AnyKey` trait here that offers a generic `sign`
and `verify`. however, the `sign` would need to fail if `AnyKey` is
backed by a public key, which makes it useless for the `signing_key`
member, as we'd lose type safety there. so you'd need an additional
`SigningKey` trait at a minimum that offers a `sign` method.

this would make the code more convoluted with little benefit in my
opionion. if we need to switch to a different scheme such as, and this
is just an example, ElGamal, we could just extend the `PrivateKey` and
`PublicKey` implementation fairly easily. all we'd need to do is adapt
the `sign` and verify functions and maybe add another generation
function.

with the `AnyKey` approach we would then have a a struct `ElGamal` that
implements `AnyKey` to verify stuff, and a second one that is called
`ElGamalSign` that implements `SigningKey`. you can then have an `Into`
implementation, that turns `ElGamalSign` into an `AnyKey` or implement
`AnyKey` for `ElGamalSign` so you can verify with that to.

now you basically have two new types that implement two traits to have
more or less the same type safety and utility. i am not sure this
actually helps.

> >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() { ...

well as the comment here states, rsa needs to be non-pkcs#8 for legacy
reasons (pbs used this format for rsa keys up until now). the commit
that itroduces support for ed25519 also removes the bit with
`self.key.ec_key()`. all of this is basically due to how openssl works.
which is why we wrap it and abstract it away. this isn't exposed to
users of `PublicKey` and `PrivateKey`, so while this may not look
pretty, it isn't a real issue.

> No love for polymorphism, but also convoluted:
>
> pub fn generate_rsa()
> pub fn generate_ec()

i'll grant you that from a "software engineering" perspective it might
be "cleaner" to use generics here or something similar. however, that
would break the public api at this point and the upside would be fairly
slim. where i agree with you is that we could have an additional generic
`generate` function that would always generate the currently up-to-date
assymmetric key of choice. maybe also have a `generate_symmetric` or
similar as an alternative.

however that would need to be in addition, as some users may still
require specific crypto schemes. note that overly abstract and
"over-engineered" code has a maintenance burden as well. it is harder to
understand where which code path is taken if everything is an abstract
`AnyKey`.

and none of this code makes it hard to move to a new crypto scheme, we
just need to adapt some parts of `PrivateKey` which can be completelly
transparent to the users and that's it. all the users then neeed to do
is call the "newer" `generate` funciton.

once users create a new key, they can update the keyring's `signing_key`
with the newly generate `PrivateKey` and leave the old key's `PublicKey`
in the `public_keys` vector. this effectively switches the encryption
scheme for new tickets while still being able to verify older tickets
until the old `PublicKey` is also removed. and users have to worry about
nothing other then "is it safe to use a symmetric scheme yes/no?"

> ... 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.

as stated above, some if it needs to be worried about by the users of
the crate, though. symmetric and asymmetric keys are non-interchangeable
and their suitability depends on the scenario.

> >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.

thanks, i guess :)

> >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

works well, not sure that 66 is ideal, but alright, you do you.