Expose PKCS_RSA_PSS_SHA256 for CSR generation #272
Conversation
|
This PR was opened as a Draft to discuss:
|
rcgen/src/sign_algo.rs
Outdated
| // Both openssl and webpki reject them. It *might* be possible that openssl | ||
| // accepts the certificate if the key is a proper RSA-PSS key, but ring doesn't | ||
| // support those: https://github.com/briansmith/ring/issues/1353 | ||
| // openssl accepts the certificate if the key is a proper RSA-PSS key, but |
There was a problem hiding this comment.
Happy to change this comment to whatever suits the project better :)
|
Why doesn't the parsec tool update to rcgen 0.13? I don't know that we'd want to backport to 0.9. |
|
Just leaving a note that I'll be travelling for the next week and probably won't have a chance to review this before then. |
|
I understand why you wouldn't want to backport, we'll have the parsec-tool update to rcgen 0.13.x. |
There was a problem hiding this comment.
Generally looks good to me. I think it would be OK to squash your two commits into one, and to move this out of draft status.
To be appropriate to merge it would also be nice to have test coverage. I would expect that it's possible to write a test using openssl in rcgen/tests/openssl.rs, and with some feature gating, with aws-lc-rs as the backing provider for rcgen's crypto feature. If you're especially keen it would be interesting to see if botan-rs could support this as well.
rcgen/src/sign_algo.rs
Outdated
| // accepts the certificate if the key is a proper RSA-PSS key, but ring doesn't | ||
| // support those: https://github.com/briansmith/ring/issues/1353 | ||
| // openssl accepts the certificate if the key is a proper RSA-PSS key, but | ||
| // ring doesn't support those: https://github.com/briansmith/ring/issues/1353 |
There was a problem hiding this comment.
This comment seems good as-is, but I think it would be prudent to rejig this so that the mention of there being no support in *ring* is part of the public rustdoc comment that makes it into documentation. Maybe something like:
/// RSA signing with PKCS#1 2.1 RSASSA-PSS padding and SHA-256 hashing as per [RFC 4055](https://tools.ietf.org/html/rfc4055)
///
/// Note: `*ring*` does not support this signature algorithm, and so it can not be used with the `crypto` feature
/// of `rcgen` when verifying signatures using the `ring` backend.
pub static PKCS_RSA_PSS_SHA256: SignatureAlgorithm = SignatureAlgorithm {It looks like aws-lc-rs does support these signature algorithms.
There was a problem hiding this comment.
The certificate being generated is failing here
There was a problem hiding this comment.
Have you considered making a minimal reproducer and opening an issue with aws-lc-rs? I've had good experience interacting with the maintainers.
| // | ||
| /// RSA signing with PKCS#1 2.1 RSASSA-PSS padding and SHA-256 hashing as per [RFC 4055](https://tools.ietf.org/html/rfc4055) | ||
| pub(crate) static PKCS_RSA_PSS_SHA256: SignatureAlgorithm = SignatureAlgorithm { | ||
| pub static PKCS_RSA_PSS_SHA256: SignatureAlgorithm = SignatureAlgorithm { |
There was a problem hiding this comment.
Should we add the _SHA384 and _SHA512 variants as well for parity with PKCS_RSA_XXX?
| @@ -160,7 +160,7 @@ pub(crate) mod algo { | |||
| params: SignatureAlgorithmParams::RsaPss { | |||
| // id-sha256 in https://datatracker.ietf.org/doc/html/rfc4055#section-2.1 | |||
| hash_algorithm: &[2, 16, 840, 1, 101, 3, 4, 2, 1], | |||
| salt_length: 20, | |||
| salt_length: 32, | |||
There was a problem hiding this comment.
I think it might merit a comment here that's its conventional to use a salt length equal to the size of the hash algorithm's digest (in this case, 32 bytes for the 256 bit digest produced by SHA256).
I don't think this is a requirement with any backing in specification text so it seems overly aggressive for OpenSSL to be rejecting other salt lengths, but I also don't see any reason to avoid doing what seems to be most conventional.
Make PKCS_RSA_PSS_SHA256 a publicly accessible algorithm so that CSRs can be created for RSA PSS. The default salt_len value for RSA PSS SHA256 is the current value, 20. However, the only application that we currently know can use the generated RSA PSS CSRs is Parsec https://github.com/parallaxsecond/parsec which requires a salt length of 32 to work with OPENSSL. * Change this value to 32 to be compatible with OpenSSL. On this topic, the spec states: "When signing, it is RECOMMENDED that the parameters, except for possibly saltLength, remain fixed for all usages of a given RSA key pair"; and this is the value we are changing. Signed-off-by: Tomás González <[email protected]>
A previous commit has added PKCS_RSA_PSS_SHA256 and made it publicly available. * Replicate the same behaviour for PKCS_RSA_PSS_SHA384 and PKCS_RSA_PSS_SHA512 Signed-off-by: Tomás González <[email protected]>
Only enable PKCS_RSA_PSS_SHA256 and not the rest of the variants as the tests for that are currently failing. Use aws_lc_rs for testing as supposedly this one does support PSS keys while ring does not. * Fix a logic error in the test in which verify_cert_basic was being run when verify_cert should have been and viceversa. Signed-off-by: Tomás González <[email protected]>
tgonzalezorlandoarm
force-pushed
the
rsa-pss-support
branch
from
b55c51f
to
f571744
Compare
|
Hi! I have:
I apologize for the delay, I have been busy with the recent release of our parsec openssl provider, with which we are able to perform a TLS handshake, using a CSR created with rcgen (with a ParsecRemoteKeyPair) for an RSA key with PSS signing. |
Make PKCS_RSA_PSS_SHA256 a publicly accessible algorithm so that CSRs can be created for RSA PSS.
This has been tested with https://github.com/parallaxsecond/parsec on this parsec-tool PR with the patched rcgen crate. Please check the results of openssl CSR verification in the CI run
For this to work with Parsec, this needs to be backported to v0.9.x
The tests are not enabled as parsec is not used during testing.