Skip to content

DSA: Verifier trait not implemented #520

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

Closed
Geobert opened this issue Sep 5, 2022 · 17 comments
Closed

DSA: Verifier trait not implemented #520

Geobert opened this issue Sep 5, 2022 · 17 comments

Comments

@Geobert
Copy link

Geobert commented Sep 5, 2022

Hi,

I'm in a case where I need to verify a signature done on a whole &[u8] rather than a digest and I see that the relevant method is in the Verifier trait but it is not implemented anywhere in the dsa crate.

Is that an oversight or it is something just waiting for a contribution? Or am I missing something?

Regards,

@aumetra
Copy link
Contributor

aumetra commented Sep 5, 2022

If you have a while message aggregated inside a byte slice and you want to verify it, you can do the following (replace Sha1 with whatever hashing algorithm you use):

let is_valid = verifying_key.verify_digest(Sha1::new_with_prefix(data)).is_ok();

This could maybe be added as to the documentation as reference.

An implementation of the Verifier trait would do roughly the same thing but default to the preferred hashing algorithm for the signature scheme. DigestVerifier is more flexible and lets you choose the hashing algorithm you want to use.

@Geobert
Copy link
Author

Geobert commented Sep 5, 2022

Fair enough, I have to add dsa support after I've done ed25519 and the latter had verify so that confused me.

Thanks a lot for your answer!

@Geobert Geobert closed this as completed Sep 5, 2022
@tarcieri tarcieri reopened this Sep 5, 2022
@tarcieri
Copy link
Member

tarcieri commented Sep 5, 2022

Both sets of traits can be implemented. We should do both unless there’s a complication I’m not thinking of.

@aumetra
Copy link
Contributor

aumetra commented Sep 5, 2022

There shouldn't be any complications. The preferred hashing algorithm for DSA is SHA-1, if I'm not mistaken?

@tarcieri
Copy link
Member

tarcieri commented Sep 6, 2022

Ugh, well at the very least that should be feature-gated due to the insecurity of SHA-1.

Looking for relevant RFCs, I see a lot of Internet-Drafts to update the default hash algorithm for DSA, but none of them actually accepted as standards-track RFCs, e.g. draft-rsa-dsa-sha2-256 which became draft-ietf-curdle-rsa-sha2(RSA-only) and eventually RFC8332.

@lumag
Copy link
Contributor

lumag commented Sep 12, 2022

RFC 5754 defines DSA with SHA-224 and SHA256.

@cobratbq
Copy link
Contributor

cobratbq commented Oct 23, 2022

I had a quick look at this one. It seems RFC 5754 assumes an identifier is included to indicate what hash algorithm was used. So for a plain signature as used for Signer and Verifier this would be an issue. If DSA is used with SHA-1 by default, but then this is disallowed, considered unsafe for many cases. So any choice for the implementation of Signer and Verifier is rather arbitrary IIUC. (And, if the default changes at some later point, implementations that upgrade will suddenly find themselves operating with different parameters, which may be a source of problems in itself, especially with interop.)
update So, I guess the question is, is such an implementation desirable?
update I am rereading, because I feel like I am missing something. Is the idea to have the generic type as part of the Signature-type, such that Signer and Verifier can derive the hashing algorithm automatically?

@lumag
Copy link
Contributor

lumag commented Oct 23, 2022

I'd follow what was done for the RSA crate: turn the Digest into the generic type argument for the Signer and Verifier implementations:

pub struct Signature {...}
impl signature::Signature for Signature {....}
pub struct SigningKey<D> where D: Digest {...}
impl<D> RandomizedSigner<Signature> for SigningKey<D> where D: Digest {....}
impl<D> RandomizedDigestSigner<Signature> for SigningKey<D> where D: Digest {....}
#[cfg(feature = "hazmat")]
impl<D> RandomizedPrehashSigner<Signature> for SigningKey<D> where D: Digest {....}

@lumag
Copy link
Contributor

lumag commented Oct 23, 2022

In other words: let the caller code determine used digest algo and pass it as a generic argument.

@tarcieri
Copy link
Member

@lumag yeah exactly, if I'm reading RFC 5754 correctly it uses a similar OID-based approach to RSA, so we can do the exact same thing and query the OID via AssociatedOid.

I would recommend SHA-256 as the default, as SHA-1 is insecure.

@cobratbq
Copy link
Contributor

cobratbq commented Oct 25, 2022

'd follow what was done for the RSA crate: turn the Digest into the generic type argument for the Signer and Verifier implementations: [..]

This does mean that, within the same application, a produced Signature can be handled incorrectly by lack of type-safety. Acknowledged Granted, this is not the most common use-case, still.

@tarcieri
Copy link
Member

@cobratbq I don't follow your complaint. You're arguing that using a generic parameter somehow reduces type safety? Safety around what, and compared to what alternative?

@cobratbq
Copy link
Contributor

@tarcieri If I use some Signer -- based on SHA-1 -- to produce a Signature, then use another Verifier -- based on SHA-256 -- to verify the signature, it will only fail at run-time for not being able to verify the signature. It could have failed syntax checking (type consistency) if a generic parameter would be included. (My reasoning is based on the code sample above, if it is missing some detail, then I am making wrong assumptions.)
NOTE: I am not arguing for or against either solution, just want to make sure this consideration is taken into account.

@tarcieri
Copy link
Member

It could have failed syntax checking (type consistency) if a generic parameter would be included. (My reasoning is based on the code sample above, if it is missing some detail, then I am making wrong assumptions.)

But that example is using a generic parameter? I still don't follow.

One complication in generic code is there are potentially multiple implementations of the same digest which should be treated the same even if they aren't the same type, such as wrappers for cryptographic accelerators.

But the AssociatedOid trait would still let you bound on a particular OID constant at the type level, e.g.

where D: AssociatedOid<OID = sha2::Sha256::OID>

@cobratbq
Copy link
Contributor

@tarcieri hmmm, I think it does. I get it now.

@cobratbq
Copy link
Contributor

PR #559 is a minimal implementation of Signer and Verifier. The changes to the types discussed above are still possible, but not covered. The PR shows the implementation needed to make it work within the current code-base. I have no urgent need for this contribution myself, but it is a logical next incremental addition.
If you consider the type-system changes deal-breaking, let me know and I'll close the request.

@cobratbq
Copy link
Contributor

cobratbq commented Nov 1, 2022

I think this could be closed, as further changes depend on RustCrypto/traits#1141.

@tarcieri tarcieri closed this as completed Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants