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

Finish migrating RSAOpenSsl from RSA* to EVP_PKEY* #54282

Merged
merged 18 commits into from
Jul 14, 2021

Conversation

bartonjs
Copy link
Member

@bartonjs bartonjs commented Jun 16, 2021

This change moves the RSAOpenSsl class (and the RSA.Create() internal class on Linux) to use EVP_PKEY
for the import and export operations, making the interaction with the underlying library based on
PKCS#8 PrivateKeyInfo and X.509 SubjectPublicKeyInfo key blobs.

A large portion of the code is just from splitting the key helpers files to not need all of the encrypted PKCS#8 support
in the System.Security.Cryptography.OpenSsl library, as the encrypted PKCS#8 <-> unencrypted PKCS#8 work provided
by the base classes is sufficient.

Once PKCS#8 and SPKI became the primary modes of interaction the spanified import and export routines for those got
overridden in RSAOpenSsl to avoid SPKI->RSAParameters->SPKI-style conversions.

This change completely eliminates SafeRsaHandle, and the only time that an RSA* is used at all is in the legacy RSAOpenSsl(IntPtr) constructor.

Contributes to #46526.

@ghost
Copy link

ghost commented Jun 16, 2021

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

This change moves the RSAOpenSsl class (and the RSA.Create() internal class on Linux) to use EVP_PKEY
for the import and export operations, making the interaction with the underlying library based on
PKCS#8 PrivateKeyInfo and X.509 SubjectPublicKeyInfo key blobs.

A large portion of the code is just from splitting the key helpers files to not need all of the encrypted PKCS#8 support
in the System.Security.Cryptography.OpenSsl library, as the encrypted PKCS#8 <-> unencrypted PKCS#8 work provided
by the base classes is sufficient.

Once PKCS#8 and SPKI became the primary modes of interaction the spanified import and export routines for those got
overridden in RSAOpenSsl to avoid SPKI->RSAParameters->SPKI-style conversions.

This change completely eliminates SafeRsaHandle, and the only time that an RSA* is used at all is in the legacy RSAOpenSsl(IntPtr) constructor.

Author: bartonjs
Assignees: -
Labels:

area-System.Security

Milestone: -

int algId);

internal static unsafe SafeEvpPKeyHandle DecodeSubjectPublicKeyInfo(
ReadOnlySpan<byte> source,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Since CryptoNative_DecodeSubjectPublicKeyInfo requires buf != nullptr and len > 0, may want a Debug.Assert(!source.IsEmpty); statement here.

(Same comment for related methods in this file.)

finally
{
CryptographicOperations.ZeroMemory(encryptedSpan);
CryptoPool.Return(encryptedRent!, clearSize: 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

encryptedRent may be null if an exception is thrown on line 203 - 204. I'd recommend moving lines 198 - 206 to be before the try block, which obviates this issue; or putting a null guard around this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is just a partial split-out with no changes, and there's a clock running for the preview snap, I'll tackle this across all the algorithms as a followup.

@bartonjs
Copy link
Member Author

Test failure is #55536

@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants