Repository navigation
signature v3: type inference regression #1831
Description
It seems after #1827 type inference is not working as well:
error[E0282]: type annotations needed
--> tests/pkcs1v15.rs:16:9
|
16 | let signature = signing_key.sign(bytes);
| ^^^^^^^^^
...
22 | assert_eq!(signature.to_string(), expected);
| --------- type must be known at this point
|
help: consider giving `signature` an explicit type
|
16 | let signature: /* Type */ = signing_key.sign(bytes);
| ++++++++++++
It can no longer automatically infer the signature type in the case there is only a single non-overlapping impl, which seems pretty bad from an ergonomic perspective.
Activity
tarcieri commentedon Apr 30, 2025
The specific case where this seems to occur is when there is an explicit
Signerimpl and the blanket impl isn't satisfied.Based on this, I'd suggest reverting #1827.
We can potentially still get rid of custom derive by replacing it with
macro_rules.newpavlov commentedon Apr 30, 2025
I am a bit out of context, but why can't we make the signature type
San associated type instead of a generic parameter on the trait? For example, in the RSA case the implementation looks like this:impl<D> DigestVerifier<D, Signature<D>> for VerifyingKey<D> { .. }. It does not use the ability to be generic over signature type at all and probably can be changed toimpl<D> DigestVerifier<D> for VerifyingKey<D> { type Signature = Signature<D>; ... }without any issues.If other crates rely on this feature, we could move this generality to the implementor side.
tarcieri commentedon Apr 30, 2025
@newpavlov overlapping impls. Some key types support many, many resulting signature types, e.g. https://docs.rs/ecdsa/latest/ecdsa/struct.SigningKey.html
newpavlov commentedon Apr 30, 2025
Why can't we change it to
SigningKey<C, S>, as I suggested at the end of my comment? I think it makes sense to parametrize the signer type instead of trait which it implements.tarcieri commentedon Apr 30, 2025
The ability of a single key to support multiple signature algorithms is important, for example if you're implementing something like a KMS system which may need to dynamically support multiple signature formats depending on what the client requests.
It's the reason why the
rsacrate maintains an entire duplicate/parallel API onRsaPrivateKeyfor such purposes.newpavlov commentedon May 1, 2025
Can you show a concrete code example for this? I would think it should not be different from supporting multiple hash functions or AEAD algorithms, i.e. your KMS could return a custom signture type which under the hood could be an enum over supported signature algorithms.
Your mean generality over
SignatureScheme?tarcieri commentedon May 1, 2025
Unfortunately not because everything that would be a good example I'm currently working on as a downstream consumer only supports one signature algorithm per key type. That's for want of features I eventually do plan on integrating in these systems though.
The problem comes up pretty much anywhere there's dynamism around signature algorithm choice where multiple signing algorithms are possible for a single asymmetric key type.
Hash functions aren't keyed.
I think it's probably less common to reuse symmetric keys with different AEAD algorithms. They don't have something analogous to a common unifying public key scheme. Where that reuse happens is probably upgrading legacy users who have a symmetric key from a legacy unauthenticated algorithm to an AEAD.
Public key schemes, particularly RSA, also have expensive setup. I guess you could first decode to an
RsaPublicKey(and perform precomputation), then convert to an e.g.rsa::pss::SigningKey<Sha256>, which would at least clone the private key material for each use, but IMO it's nice to be able to dispatch to a particular signature algorithm without having to clone the key material (or creating per-algorithm proxy objects that borrow the key material).Yes, and sidebar while that API should probably be revisited, this isn't the release to do it as there are major unreleased security fixes and we should make that upgrade process as smooth as possible.
Backing up, the inference problems go away if we revert the blanket impl of
SignerforDigestSigner, i.e. #1827. I wound up less happy with adding that blanket impl after I discovered we couldn't have symmetric blanket impls for theAsync*traits (which I had originally worried about in #1419)Revert "signature: replace `signature_derive` with blanket impls (#1827…
Revert "signature: replace `signature_derive` with blanket impls (Rus…