Handle verification of keys using elliptic curve

[flavio]

This PR introduces support for handling certificates using elliptic curve public keys.

Only signature verification is currently implemented.
Update: Both signature creation and verification are supported.

The implementation relies on the ring crate, which currently is the only one capable of supporting both ECDSA p256 and ECDSA p384.

I haven't yet extended the unit tests. I'm looking forward to hear your feedback about this contribution. I think this is something you're interested about (see #100). I wonder if this is going in the direction you like.
Update: The unit tests have been extended too

[CBenoit]

Thank you very much 🎉

I haven't yet extended the unit tests. I'm looking forward to hear your feedback about this contribution. I think this is something you're interested about (see #100). I wonder if this is going in the direction you like.

This is going in the right direction! However, I wanted to use RustCrypto's crates to stay purely Rust only.
I think both ECDSA p256 and ECDSA p384 are supported:

• https://crates.io/crates/p256/
• https://crates.io/crates/p384/

Would that be acceptable on you side? (p384 crate might not be mature enough?)

I read the issue on sigstore-rs. Glad to see picky have been considered as a possible candidate for what you’re trying to do despite the limited popularity. I understand if you wish to switch to webpki in the future. In the meantime, I'll be happy to help and merge PR for functionalities you might need. 🙂

[flavio]

This is going in the right direction! However, I wanted to use RustCrypto's crates to stay purely Rust only. I think both ECDSA p256 and ECDSA p384 are supported:

I kinda imagined that :) I started to use ecdsa + p256 + p384 crates but then I realized the p384 crate isn't mature enough. It cannot be used yet, because some traits required by the ecdsa structs are not implemented yet. This is tracked inside of this issue: RustCrypto/elliptic-curves#240

If you want, I have another branch that builds on top of ring-compat. This however would bring ring as a dependency. Plus, the code using ring-compat looks a bit uglier compared to the one inside of this PR. That's because it's harder to abstract it.

I know ring is not written in pure Rust, but at least is not relying on openssl or some other library :)

Right now I need p384 support, because this what Fulcio (a project part of sigstore) uses.

If you are really against using ring, I could build EC P256 support on top of ecdsa + p256. While EC P384 could be hidden behind a feature flag, and be implemented via either ring or ring-compat. To be honest, this would be a bit painful and the final code would probably look uglier, but this could be a temporary solution until p384 matures.

What do you think?

[thenextman]

Hello @flavio; I resolved an issue in the CI that prevented checks from running on your branch. If you rebase on master the build should now run (tests are failing but I see from your OP that you haven't touched that side yet). Thank you for your contribution.

[CBenoit]

If you want, I have another branch that builds on top of ring-compat. This however would bring ring as a dependency. Plus, the code using ring-compat looks a bit uglier compared to the one inside of this PR. That's because it's harder to abstract it.

I know ring is not written in pure Rust, but at least is not relying on openssl or some other library :)

If you are really against using ring, I could build EC P256 support on top of ecdsa + p256. While EC P384 could be hidden behind a feature flag, and be implemented via either ring or ring-compat. To be honest, this would be a bit painful and the final code would probably look uglier, but this could be a temporary solution until p384 matures.

Thank you for the insight! I think it's good to use ring at least until p384 crate has arithmetic implemented. I agree, it's not worth to use ring-compat in our case either, we don't even rely that much on their traits in the first place.

[flavio]

Thank you for the insight! I think it's good to use ring at least until p384 crate has arithmetic implemented. I agree, it's not worth to use ring-compat in our case either, we don't even rely that much on their traits in the first place

Thanks, I'll go ahead with this PR and look into the unit tests

[flavio]

@thenextman, @CBenoit: I've forcefully pushed. The code now handles both signature creation and verification using ECDSA keys. The unit tests have been extended too, I hope you won't mind the new rstest dependency, I like how it can be used to reduce the boilerplate.

All the unit tests are green on my machine, clippy is happy too.

Some details:

• The whole implementation relies on ring, as discussed above
• Signature creation:
  • ECDSA P-256 keys can be used only with sha2 256 hashing algorithm.
  • ECDSA P-384 keys can be used only with sha2 384 hashing algorithm.
  • ECDSA P-512 keys cannot be used to produce signatures. This is a limitation of ring and all the other rust crates.
• Signature verification:
  • A certificate with a ECDSA P-256 public key assumes the sha2 256 algorithm has been used to produce the signature. This is what the official RFCs recommend (I don't remember the number of the RFC unfortunately, I got lost into the rabbit hole).
  • A certificate with a ECDSA P-384 public key assumes the sha2 384 algorithm has been used to produce the signature. This is what the official RFCs recommend (I don't remember the number of the RFC unfortunately, I got lost into the rabbit hole).
  • Signatures produced with a ECDSA P-512 keys cannot be verified. This is a limitation of ring and all the other rust crates.

Thanks again for your prompt and welcoming responses!

[CBenoit]

Very high quality pull request, thank you!
Introduction of rstest is much welcomed. I just didn't know about it yet. :)

CI shows that some code is still failing to compile behind a feature gate. It's not a big deal so I sent a patch directly since you allowed maintainer modifications.

I'll merge and release next version tomorrow 🎉

By the way, next version will be a candidate release, just to see if there is additional breaking changes before committing to picky 7.x. Would it cause any issue on your side?

[flavio]

CI shows that some code is still failing to compile behind a feature gate. It's not a big deal so I sent a patch directly since you allowed maintainer modifications.

Sorry, I didn't notice that while testing locally. Thanks for having fixed it!

I'll merge and release next version tomorrow 🎉

By the way, next version will be a candidate release, just to see if there is additional breaking changes before committing to picky 7.x. Would it cause any issue on your side?

That's fantastic news 👏
I have no worries about possible API breaking changes, go ahead with them if you need to.