Support ECDSA with secp256k1 curve in AwsLcRsAlgorithm

[mkmks]

This is a follow-up PR to rustls/pki-types#96 that is meant to upstream @zama-ai 's work on enabling the use of TLS certificates signed with ECDSA+secp256k1 curve in applications based on rustls.

The discussion in rustls/pki-types#96 had a valid argument that it's not strictly speaking necessary to upstream all these changes because an application could define its custom CryptoProvider with its own list of allowed SignatureVerificationAlgorithms. However, there's still the question of the best way to define a SignatureVerificationAlgorithm in this particular case.

Since we're not adding some completely novel algorithm but ECDSA+secp256k1 that is already implemented in aws_lc_rs, writing an implementation of SignatureVerificationAlgorithm from scratch feels excessive given that the rustls-webpki crate already provides the AwsLcRsAlgorithm struct that implements SignatureVerificationAlgorithm for many algorithms implemented in aws_lc_rs.

Writing and maintaining a new implementation would require an effort comparable with forking and maintaining rustls-webpki. Rewriting/copying the test suite alone is, perhaps, a good argument to not follow that path.

With this in mind, we'd like to propose these small changes to AwsLcRsAlgorithm that do no more than add support for another well-known ECDSA curve. If accepted, a custom CryptoProvider supporting ECDSA+secp256k1 would, indeed, become an easy affair.

[djc]

The crate is named webpki for a reason.

Writing and maintaining a new implementation would require an effort comparable with forking and maintaining rustls-webpki. Rewriting/copying the test suite alone is, perhaps, a good argument to not follow that path.

This doesn't hold up for me.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.04%. Comparing base (23963ce) to head (5cfa16a).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #427   +/-   ##
=======================================
  Coverage   97.04%   97.04%           
=======================================
  Files          20       20           
  Lines        4230     4230           
=======================================
  Hits         4105     4105           
  Misses        125      125           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[djc]

@ctz wrote in rustls/pki-types#96:

I don't think I would accept changes for webpki or rustls in this direction, as it is much too esoteric.

I think this is a polite way of saying "we will not be merging this into mainline webpki or rustls", so I'll close this PR.

[mkmks]

Names don't define the nature of their referents, the referents provide meaning to names. Sometimes, old names stick without a clear reason.

Despite the name, the webpki crate implements X.509 certificate validation that can be used and is used in many contexts, not only on the "web". PKI and X.509 certificates aren't only used by TLS servers and clients.

For example, one can see here that the crate called webpki (it's the original one, not the rustls fork) is used to validate a certificate chain used to sign runtime measurements of a executing virtual machine, despite its name. An X.509 certificate is just that, an X.509 certificate. Validating one follows the same logic, whether it's used for TLS, S/MIME or for signing executables.

I can see the point that there are established practices on the "web", however the "web" is a also a fluid and constantly developing thing. Standards don't always keep up with practice. The TLS 1.3 standard came out in 2018, seven years ago, which is a long time for the "web".

Yes, TLS 1.3 (as defined in RFC8446) doesn't mention the secp256k1 curve but it also doesn't say that only the listed signature schemes are ever allowed. The listed schemes are mandatory to implement if one wants to claim TLS 1.3 compatibility, of course. This PR isn't breaking that.

If we forget for a moment about names and published standards and look at the pure technical matter, what would be a technical argument against merging this PR?

Can it be about increasing the maintenance burden? I'd say not. Let's take a look at what this PR does in detail:

1. It recognizes the ECDSA_P256K1 identifier (that was added in Add algorithm id for ECDSA with secp256k1 curve  pki-types#96) in the key encoding check in the verify_signature() implementation for AwsLcRsAlgorithm. A small but necessary and easily maintainable change that enables the following change.
2. It defines the ECDSA_P256K1_SHA256 value that can be used to represent a SignatureVerificationAlgorithm in a custom CryptoProvider. One could argue that having one too many of these values is increasing the maintenance burden: what if everyone decides to add one for their favourite combination of curves and hashes? That would be madness! So maybe this definition could be put not in the webpki crate but in the application that needs its custom CryptoProvider? That would be an option, if not for the last change.
3. It adds tests for the new AwsLcRsAlgorithm value which make the bulk of this PR's volume. These tests are rather repetitive but so are the tests for each other AwsLcRsAlgortihm. Perhaps, a good way forward here is to refactor the signature tests to avoid the code duplication in the first place? They aren't even hand-written but generated by a script.

I could clarify my argument for putting these changes into webpki though. The TLS stack is a critical part of the application but also a low-level part that isn't specific to the application. Any customisations to it are a potential security issue which is why it's best to not deviate from its upstream development.

This holds even more for the choice of the EC curve in the signature algorithm. Each signature algorithm definition in webpki comes with tests, so a custom one of a comparable quality would require writing the same tests. But the application built with rustls isn't a good place to maintain these tests when webpki has plenty of tests that look almost the same.

Would the maintainers be more accepting of this PR if it also refactors the hundreds of lines of repetitive tests in tests/signatures.rs into something much more compact and more maintainable that just happens to test the ECDSA+secp256k1 algorithm definition as well?

These tests are actually generated with a script, so maintanability isn't really an issue with these tests.

[djc]

I could clarify my argument for putting these changes into webpki though. The TLS stack is a critical part of the application but also a low-level part that isn't specific to the application. Any customisations to it are a potential security issue which is why it's best to not deviate from its upstream development.

I would not count this as a customization. On the other hand, I'm also quite happy to remain not responsible for the added security risks of cryptography that is irrelevant to all of my use cases.

[mkmks]

added security risks of cryptography

That would be a valid argument if this PR implemented any new cryptography. It doesn't!

This PR only enables the use of cryptography already implemented in aws_lc_rs that webpki already depends on. Moreover, it does so through the existing interfaces (AwsLcRsAlgorithm) and test generation scripts (tests/generate.py) without any innovation. I only used existing plumbing.

I can understand that one might want the default CryptoProvider in rustls to only support what TLS 1.3 explicitly mandates, and thank you again for that pointer. However, having to maintain a custom SignatureVerificationAlgorithm implementation when there's already a great mechanism for blanket implementation in webpki seems suboptimal.