-
Notifications
You must be signed in to change notification settings - Fork 157
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New API design #34
Comments
Just to be clear, the above is intended as something concrete we can stare at and work on, not as the final proposal (though I think the above is a reasonable design). |
This looks good at first glance, and is more or less how the Something else to consider is incorporating more of the parameters for how the signature is computed in the signature type itself, e.g. making I'd probably recommend avoiding that initially and circling back on it later. |
Updated draft to use |
The api looks pretty good to me. The main question for me is how do we integrate this with the encryption part, as that is kinda needed for getting it integrated into the crate |
My suggestion (which I'll write up next) is to have equivalent
|
I worry a bit about this api becoming quite complex, having to create multiple structs for each operation seems like quite a lot of mental overhead for the user of the api, when all these operations could just be straightforwad function calls like they are now. |
Not saying the current API is perfect, not even great, but I am a big advocate of making cryptographic primitives easier to use, not harder. |
The struct-based API would be used something like this: use rsa::{pss::Signer, RsaPrivateKey};
use sha2::{Digest, Sha256};
use signature::DigestSigner;
let privkey = RsaPrivateKey::from_wherever();
let signer = Signer::unblinded(&privkey);
let digest = Sha256::digest(msg);
let signature = signer.sign_digest(digest); This is where the APIs on use rsa::RsaPrivateKey;
use sha2::{Digest, Sha256};
use signature::DigestSigner;
let privkey = RsaPrivateKey::from_wherever();
let signer = privkey.sign_pss();
let digest = Sha256::digest(msg);
let signature = signer.sign_digest(digest); This only saves an import, as the separate use rsa::key::RsaPrivateKey;
use sha2::{Digest, Sha256};
use signature::DigestSigner;
let signer = RsaPrivateKey::from_wherever().sign_pss();
let digest = Sha256::digest(msg);
let signature = signer.sign_digest(digest); The trade-off here is that this isn't as usable for HSM backends (where you probably only have a single reference to the connection that you are passing around) or for use-cases where the same key is used for both encryption and signing in the same logic (does this happen?) |
Writing the above three more compactly (for more options to stare at): use rsa::{pss, RsaPrivateKey};
use sha2::{Digest, Sha256};
use signature::DigestSigner;
let privkey = RsaPrivateKey::from_wherever();
let digest = Sha256::digest(msg);
let signature = pss::Signer::unblinded(&privkey).sign_digest(digest); use rsa::RsaPrivateKey;
use signature::DigestSigner;
use sha2::{Digest, Sha256};
let privkey = RsaPrivateKey::from_wherever();
let digest = Sha256::digest(msg);
let signature = privkey.sign_pss().sign_digest(digest); use rsa::RsaPrivateKey;
use signature::DigestSigner;
use sha2::{Digest, Sha256};
let signer = RsaPrivateKey::from_wherever().sign_pss();
let digest = Sha256::digest(msg);
let signature = signer.sign_digest(digest); |
Note that you can use the https://github.com/tendermint/signatory/blob/develop/signatory-secp256k1/src/lib.rs#L25 That would reduce this: use signature::DigestSigner;
let signer = PrivateKey::from_wherever().sign_pss();
let digest = Sha256::digest(msg);
let signature = signer.sign_digest(digest); to: use signature::Signer;
let signer = PrivateKey::from_wherever().sign_pss();
let signature = signer.sign(msg); |
And the current API for comparison: use rsa::{
hash::Hashes,
PaddingScheme, RSAPrivateKey,
};
use sha2::{Digest, Sha256};
let privkey = RsaPrivateKey::from_wherever();
let digest = Sha256::digest(msg);
let signature = privkey.sign(Padding::PKCS1v15, Some(Hashes::SHA2_256), digest.as_bytes()); |
I considered adding that to my example, but I wondered whether that might result in confusing behaviour, given that for PKCS#1 v1.5 we'd be using the |
it does happen sometimes for RSA in my experience |
Ugh yes RSASSA-PKCS#1v1.5, it is definitely a wart there, but I'd also argue it doesn't quite fit the (perhaps underdocumented)
It absolutely happens in pre-1.3 TLS when the same RSA key is permitted to be used for both key encipherment and signing, and both (EC)DHE and RSA key transport ciphersuites are enabled. |
Maybe we could provide wrapper methods, that hide the struct generation? use signature::DigestSigner;
let digest = Sha256::digest(msg);
let signature = PrivateKey::from_wherever().sign_pss(digest);
fn sign_pss(&self, digest) -> Signature {
let signer = self.create_pss_signer(); // this is what is currently named sign_pss
signer.sign(digest)
} |
That seems like a reasonable approach. The underlying structs allow for higher-level protocols to implement more descriptive typed APIs (e.g. an outer protocol struct that stores an |
Ideally library-level code which wants to target multiple RSA signing backends (so far we have the This isn't orthogonal to your suggestion, but if that's the "blessed" API, would probably discourage (e.g. protocol) library authors from writing code in a multi-provider-friendly way. |
couldn't we have something like fn sign_pss<S: SignatureBackend = DefaultBackend>(&self, digest) -> Signature { .. } |
What type are you proposing that's implemented on? Note that in the case of an HSM/KMS/SEP-backed signer, by design there is no However, something like this defined as fn sign_pss<S: SignatureBackend = DefaultBackend>(signer: &S, digest) -> pss::Signature { .. } |
My idea was this would be implemented for the
Couldn't those still have some kind of struct that implements a |
@tarcieri in my sketch proposal, In my mind, |
So we could actually have something like let hsm: HSM = get_hsm();
let signature = hsm.sign_pss(digest);
// somewhere
impl PrivateKey for HSM {} |
That seems like the same intended usage as the Any reason why you can't have blanket impls of Generally for supporting HSMs, I think it'd be better to split things apart into separate traits, as they may or may not support specific algorithms, and many by design do not support unpadded RSA, which it seems like |
The problem AIUI with that is how you specify all the necessary algorithm parameters. This is why I proposed an e.g. You could probably get away with directly implementing |
Per my earlier suggestion, encode them all in the signature type, potentially making "what everyone uses by convention" default generic parameters. I think this is where I'd like to go with the Doing so would help prevent the hash substitution attacks against RSASSA-PSS raised by Peter Gutmann via type safety. |
One issue I see with making it generic over the digest type, is that whatever hashing library you choose, gets blessed, and it is really hard to use a different one. E.g. I use blake2b_simd in all my code, but if we were to use the Digest trait from rustcrypto, I suddenly have to copy and dance around, in order to use the different hashing crate. |
How else would you propose supporting multiple digest backends other than having things be generic over a |
This would definitely require a Mocking this out as an API: mod pkcs1v15 {
use digest::Digest;
use signature::{RandomizedDigestSigner, DigestSigner};
pub struct Signature<D: Digest> {
bytes: Vec<u8>,
_digest: PhantomData<D>,
}
// For blinded signing
impl<R: Rng, D: Digest> RandomizedDigestSigner<R, Signature<D>> for RsaPrivateKey {
fn try_sign_digest_randomized(
&self, rng: R, digest: D,
) -> Result<Signature<D>, Error> { ... }
}
// For unblinded signing
impl<D: Digest> DigestSigner<Signature<D>> for RsaPrivateKey {
fn try_sign_digest(&self, digest: D) -> Result<Signature<D>, Error> { ... }
}
}
mod pss {
use digest::Digest;
use generic_array::ArrayLength;
use signature::RandomizedSigner;
pub trait MaskGenFunc<D: Digest> { ... }
// Define an ArrayLength of zero as the None case?
pub struct Signature<D: Digest, MGF: MaskGenFunc<D>, SALT_LEN: ArrayLength> {
bytes: Vec<u8>,
_digest: PhantomData<D>,
_salt_len: PhantomData<SALT_LEN>,
}
impl<R, D, MGF, SALT_LEN> RandomizedDigestSigner<R, Signature<D, MGF, SALT_LEN>>
for RsaPrivateKey
where
R: Rng,
D: Digest,
MGF: MaskGenFunc<D>,
SALT_LEN: ArrayLength {
fn try_sign_digest_randomized(
&self, rng: R, digest: D,
) -> Result<Signature<D, MGF, SALT_LEN>, Error> { ... }
}
impl<R, D, MGF, SALT_LEN> DigestSigner<R, Signature<D, MGF, SALT_LEN>>
for RsaPrivateKey
where
R: Rng,
D: Digest,
MGF: MaskGenFunc<D>,
SALT_LEN: ArrayLength {
fn try_sign_digest(&self, rng: R, digest: D) -> Result<Signature<D, MGF, SALT_LEN>, Error> { ... }
}
}
The concrete hashing libraries don't get blessed, but the |
Great question, which I unfortunately don't have a great answer to, but maybe sth like |
I'd be fine with adding that. |
For a bit of precedent, the https://docs.rs/yubihsm/0.30.0/yubihsm/rsa/mgf/enum.Algorithm.html The digests are encoded as part of the OAEP and PSS algorithm identifiers (Yubico's choice, not mine): https://docs.rs/yubihsm/0.30.0/yubihsm/rsa/oaep/enum.Algorithm.html If you want to be generic across more digest backends than what are available in RustCrypto, but still be type safe, you could have something like a |
@tarcieri and those are then simply used as parameters? |
AFAICT this doesn't work with using type signatures for parameters; we'd then still need a per-algo |
That's also true... these are presently passed as parameters to the signing operation (which doesn't use the |
I'm going to try making branches that mock out the two different approaches:
The latter will depend on having a |
One issue I've come across while implementing this: I don't have a good way to connect the ASN.1 DER prefixes to particular
|
any update to the draft? |
@dignifiedquire @tarcieri I think this issue can be now closed, can't it? |
I think the ideas proposed here have either been implemented or obsoleted by other work /cc @str4d |
👍 |
Prompted by RustCrypto/signatures#25 (comment) and my desire to get #18 and #26 merged 😄
The main design difference compared to the current traits is that instead of making the "padding scheme" a parameter of the pubkey sign/verify functions, we make each scheme a first-class primitive that wraps the common public or private key. It needs to be an explicit choice by the user anyway.
Current draft proposal (as of 2020-03-07) (without changes to the
signature
crate, and without handling the encryption cases):I'll update the proposal in this post as we discuss it.
The text was updated successfully, but these errors were encountered: