Skip to content
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

add NULL parameter to AlgorithmIdentifier of encoded keys #92

Merged
merged 1 commit into from Jul 25, 2021

Conversation

clenimar
Copy link
Contributor

as per the RFC 8017 [1], the NULL parameter MUST be present if the OID in
AlgorithmIdentifier is rsaEncryption. this commit adds an explicit
NULL to the AlgorithmIdentifier so that RSA keys encoded by this crate
are compliant to the RFC:

The object identifier rsaEncryption identifies RSA public and private
keys as defined in Appendices A.1.1 and A.1.2. The parameters field
has associated with this OID in a value of type AlgorithmIdentifier
SHALL have a value of type NULL.

[1] https://tools.ietf.org/html/rfc8017#appendix-A

fixes #91

@randombit
Copy link

The public key should also have an explicit NULL included in the algorithm identifier (line ~335 of encode.rs)

Can be confirmed by openssl genrsa | openssl rsa -pubout | openssl asn1parse

@tarcieri
Copy link
Member

Yes, it should. Note that this is correctly handled (or at east, easily handled correctly) by the pkcs8 crate.

I've also partially written (but have not yet published) a pkcs1 crate as well. Once I finish that up and release it I can look at migrating the rsa crate over to using it.

@clenimar clenimar force-pushed the rsaEncryption-NULL-parameter branch from c1977df to 0494a8a Compare June 22, 2021 19:35
@clenimar
Copy link
Contributor Author

Thanks, @randombit, I completely missed it. Now fixed.

@tarcieri: I didn't integrate to pkcs8 right away because it's currently not a dependency of this crate. If you feel strongly against using the simple fix proposed here, I could look into it. Thanks, great work!

@tarcieri
Copy link
Member

The simple fix is fine and I approved this PR. @dignifiedquire generally maintains this crate though, so I was waiting for his sign-off.

@dignifiedquire
Copy link
Member

can you rebase on master to fix ci please?

@dignifiedquire
Copy link
Member

@clenimar did you verify these with openssl?

@tarcieri
Copy link
Member

@dignifiedquire I haven't verified the encoded bytes, but this looks structurally the same as a PKCS#8-encoded RSA key generated by OpenSSL

https://github.com/RustCrypto/utils/blob/master/pkcs8/tests/private_key.rs#L81-L91

@est31
Copy link
Contributor

est31 commented Jul 20, 2021

@dignifiedquire I tried out this PR locally with the example from #95 and it works. So with this PR, ring can successfully import the key.

as per the RFC 8017 [1], the NULL parameter MUST be present if the OID in
AlgorithmIdentifier is `rsaEncryption`. this commit adds an explicit
NULL to the AlgorithmIdentifier so that RSA key encoded by this crate
are compliant to the RFC:

> The object identifier rsaEncryption identifies RSA public and private
> keys as defined in Appendices A.1.1 and A.1.2.  The parameters field
> has associated with this OID in a value of type AlgorithmIdentifier
> SHALL have a value of type NULL.

[1] https://tools.ietf.org/html/rfc8017#appendix-A

fixes RustCrypto#91
@clenimar clenimar force-pushed the rsaEncryption-NULL-parameter branch from 0494a8a to 1a2e62d Compare July 20, 2021 13:51
@clenimar
Copy link
Contributor Author

@clenimar did you verify these with openssl?

@dignifiedquire yes, I did. Also been using this PR with ring, as @est31 mentioned. Rebased now.

Thanks!

@tarcieri tarcieri merged commit 5d608e0 into RustCrypto:master Jul 25, 2021
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

Successfully merging this pull request may close these issues.

Encoded keys missing required NULL parameter in AlgorithmIdentifier
5 participants