crypto-common: add TryKeyInit trait
#2092
Conversation
Adds a trait providing fallible key initialization, similar to the existing `KeyInit` trait, but designed to handle the case that not all bytestrings of a given length represent valid keys. This is primarily useful in the context of public-key cryptography, e.g. scalars representing elliptic curve private keys. The API and method names are duplicated from `KeyInit`. It is assumed that `KeyInit` and `TryKeyInit` have an either-or relationship, i.e. types will not impl both `KeyInit` and `TryKeyInit`, and consumers of code which is generic over these traits will not be attempting to abstract over the `KeyInit`/`TryKeyInit` distinction, but one or the other will make sense in a given context (e.g. symmetric cryptography uses `KeyInit`, ECC uses `TryKeyInit`) Key generation methods are provided which generically implement rejection sampling as a reasonabe baseline for unbiased random sampling of potential keys. As a provided method, implementers of `TryKeyInit` can override it if they can provide more efficient means of generating keys.
There is a counterexample to this: the Ed25519 signature system uses a seed to derive the private scalar as the result of SHA-512 acting as a KDF, along with "clamping" to ensure the scalar is in range. So Ed25519 keys could use But for ECDSA keys, or secp256k1 Schnorr, we'd want (There is also a similar concern for X25519 static keys) |
| fn generate_key() -> Result<Key<Self>, getrandom::Error> { | ||
| // Use rejection sampling to find a key which initializes successfully in an unbiased manner | ||
| loop { | ||
| let mut key = Key::<Self>::default(); | ||
| getrandom::fill(&mut key)?; | ||
|
|
||
| if Self::new(&key).is_ok() { | ||
| return Ok(key); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
re: #2060 implementing random bytestring generation in hybrid-array (which I still think is an independently good idea) won't suffice for these use cases, since they need algorithm-specific logic to select valid keys.
The approach in this PR is able to provide a generic unbiased key generation implementation which can have algorithm-specific overrides to improve efficiency.
I think we could potentially drop the keygen for KeyInit and point to hybrid-array for such cases, but having generic keygen for TryKeyInit is still independently useful, IMO.
There was a problem hiding this comment.
On the first glance, it certainly could be useful to have unbiased generation as part of the trait. Especially for algorithms which do not use all most significant bits in the key. Using naive rejection sampling can be really inefficient in such cases.
But looking at the current code, I can not say I am happy with it... Not only it forces us to keep the rand_core/getrandom features in crypto-common, but it also could be inefficient. For example, let key = Alg::generate_key()?; let alg = Alg::new(&key).unwrap(); results in an unnecessary key validation as part of the new method (which is not guaranteed to be optimized out by the compiler), but also forces users to use unwrap or handle unreachable error.
How about introducing an associated type for "validated" key? Something like this:
pub trait TryKeyInit: KeySizeUser + Sized {
type ValidatedKey: Into<Key<Self>> + TryFrom<Key<Self>, Error = InvalidKey>;
fn new_from_validated(key: &Self::ValidatedKey) -> Self;
fn new(key: &Key<Self>) -> Result<Self, InvalidKey> {
key.try_into().map(Self::new_from_validated)
}
}This way we will be able to move key generation code to the validated key type without burdening the TryKeyInit trait. I haven't thought much about method names, so feel free to rename them.
There was a problem hiding this comment.
let key = Alg::generate_key()?; let alg = Alg::new(&key).unwrap(); results in an unnecessary key validation as part of the new method
This problem does not exist with the Generate/FromCryptoRng traits, it's inhereted by following the existing pattern of KeyInit, which is why I suggested separating the key generation traits out of KeyInit (#2060) and implemented an alternative which can generate random keys of any kind which you receive fully initialized (#2096), which fully addresses this concern.
There was a problem hiding this comment.
Are you sure that we do not need the TryKeyInit with "validated" type? It would be orthogonal to #2096 and we even could add Generate bound on the associated type.
There was a problem hiding this comment.
Except for micro-optimizing the case where keygen gives you back a "validated" bytestring rather than Self, I don't see a use case for it.
How else would you initialize a "validated" type other than fully initializing the Self and then throwing it away? And at that point, why not keep Self around, rather than the validated bytestring?
There was a problem hiding this comment.
Validated byte string may be smaller than Self. You also could have alternative ways for generating it (e.g. using deterministic rejection sampling).
|
With #2096 I think the RNG code in this PR can be substantially improved. It would be nice to retain a generic implementation of rejection sampling though. Perhaps it could work with a blanket impl and a marker trait, e.g. |
|
I'm just going to close this as it's very messy and I'm unhappy with it |
Adds a trait providing fallible key initialization, similar to the existing
KeyInittrait, but designed to handle the case that not all bytestrings of a given length represent valid keys.This is primarily useful in the context of public-key cryptography, e.g. scalars representing elliptic curve private keys.
The API and method names are duplicated from
KeyInit. It is assumed thatKeyInitandTryKeyInithave an either-or relationship, i.e. types will not impl bothKeyInitandTryKeyInit, and consumers of code which is generic over these traits will not be attempting to abstract over theKeyInit/TryKeyInitdistinction, but one or the other will make sense in a given context (e.g. symmetric cryptography usesKeyInit, ECC usesTryKeyInit)Key generation methods are provided which generically implement rejection sampling as a reasonabe baseline for unbiased random sampling of potential keys. As a provided method, implementers of
TryKeyInitcan override it if they can provide more efficient means of generating keys.Closes #1897