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

OpenSSL providers support #104961

Merged
merged 17 commits into from
Jul 20, 2024
Merged

OpenSSL providers support #104961

merged 17 commits into from
Jul 20, 2024

Conversation

krwq
Copy link
Member

@krwq krwq commented Jul 16, 2024

Fixes: #89167

Add support for OpenSSL providers.

Note that some underlying changes had to be done to allow this support. That include

  • changing RSA/ECDSA/ECDH to use EVP_PKEY as first class citizen
  • SafeEvpPKeyHandle needs to keep provider alive (and therefore DuplicateHandle logic got a bit more complex)

DSA is not supported - it would need similar changes to ECDsa but it's unlikely to be needed by anyone these days.

Usage example:

byte[] data = ...;

// For TPM settings refer to provider documentation you're using, i.e. https://github.com/tpm2-software/tpm2-openssl/tree/master
// specific values are just example
using (SafeEvpPKeyHandle priKeyHandle = SafeEvpPKeyHandle.OpenKeyFromProvider("tpm2", "handle:0x81000007"))
using (ECDsa ecdsaPri = new ECDsaOpenSsl(priKeyHandle))
{
    byte[] signature = ecdsaPri.SignData(data, HashAlgorithmName.SHA256);
    // do stuff with signature
    // note: tpm2 does not allow Verify operations, public key needs to be exported and re-imported into new instance
}

Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

1 similar comment

This comment was marked as duplicate.

Comment on lines 354 to +309
public override ECParameters ExportParameters(bool includePrivateParameters)
{
ThrowIfDisposed();
return ECOpenSsl.ExportParameters(_key.Value, includePrivateParameters);

using (SafeEcKeyHandle ecKey = Interop.Crypto.EvpPkeyGetEcKey(_key.Value))
{
return ECOpenSsl.ExportParameters(ecKey, includePrivateParameters);
}
Copy link
Member

Choose a reason for hiding this comment

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

This is really chatty at the P/Invoke boundary. I'd expect us to never talk about EC_KEY* anymore except for the ECDsaOpenSsl(IntPtr) ctor (and the same for ECDH), and we just update the shim library to do what we used to do for EC_KEY but with EVP_PKEY.

The way this is written:

  • Fetch _key.Value to $key
  • $key.DangerousAddRef();
  • $eckey = new SafeEcKeyHandle();
  • P/invoke, $tmp = CryptoNative_EvpPkeyGetEcKey($key.handle);
  • $eckey.handle = $tmp;
  • $key.DangerousRelease();
  • $eckey.DangerousAddRef();
  • // export parameters
  • $eckey.DangerousRelease();
  • $eckey.Dispose(); => another P/Invoke

If the shim is adjusted, it is instead just

  • Fetch _key.Value to $key
  • $key.DangerousAddRef();
  • // export parameters
  • $key.DangerousRelease();

All work we already had to do, but with fewer intermediate finalizable objects, and fewer P/invoke crossings.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree this would be ideal but unless I'm missing something that doesn't look particularly straight forward and there don't seem to be much perf benefit from this since it's not in the hot path anywhere. I'd imagine we want to get rid of all old APIs usages because we want to get rid of OpenSSL 1.x support eventually? I'll open the work item for this unless it's easier than it looks. I assume you mean to use these new APIs: https://www.openssl.org/docs/man3.0/man3/EVP_PKEY_get_params.html ?

Copy link
Member

Choose a reason for hiding this comment

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

My meta-point is that if we're working in terms of EVP_PKEY, then we want our ExportParameters API to be based on EVP_PKEY rather than EC_KEY.

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened: #105173 to track this work

pkeyHandle.Dispose();
throw;
}
return Interop.Crypto.CreateEvpPkeyFromEcKey(currentKey);
Copy link
Member

Choose a reason for hiding this comment

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

Why is this type still based on EC_KEY instead of moving it to EVP_PKEY?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't realize it was possible because samples I found first get EC_KEY from them EVP_PKEY which we already had working. I'll refrain from this change in this PR but will create a workitem to follow up

Copy link
Member Author

Choose a reason for hiding this comment

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

Tacked by #105173

@krwq
Copy link
Member Author

krwq commented Jul 20, 2024

(I rebased because it didn't let me merge)

@krwq
Copy link
Member Author

krwq commented Jul 20, 2024

All failures seem unrelated and match issues pointed by build analysis.

@jeffhandley
Copy link
Member

jeffhandley commented Aug 2, 2024

The performance improvements detected in dotnet/perf-autofiling-issues#38713, dotnet/perf-autofiling-issues#38763, and dotnet/perf-autofiling-issues#38910 were unexpected outcomes of the changes made here. @bartonjs analyzed this correlation and arrived at the following conclusion.

This PR included a small change to SafeEvpPKeyHandle.DuplicateHandle. Before, calling it did EVP_PKEY_new() and copied the wrapped key into the new one. With evp_pkey1(someRSA) and evp_pkey2(someRSA), calling a set_key on the result of duplicate only modified the thing returned from duplicate. After the change, it up_refs the current EVP_PKEY, and returns a new SafeHandle instance wrapping it. If that is passed into EVP_PKEY_set1_RSA, it changes the original handle as well as the new output, which is a caller visible behavioral change.

This change seems acceptable and innocuous, but we will document it as a potential breaking change nonetheless.

The performance difference could have been affected by this change, but if so it's probably largely by removing a call to ERR_clear_error() from DuplicateHandle as a consequence of the restructuring above.

@jeffhandley jeffhandley added needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. labels Aug 2, 2024
@krwq krwq removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Aug 5, 2024
@bartonjs bartonjs added the cryptographic-docs-impact Issues impacting cryptographic docs. Cleared and reused after documentation is updated each release. label Aug 15, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Sep 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. cryptographic-docs-impact Issues impacting cryptographic docs. Cleared and reused after documentation is updated each release. new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OpenSSL provider support
5 participants