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.

Sign up for GitHub

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

Jump to bottom

OpenSSL ENGINE support #88656

Merged
merged 8 commits into from
Jul 18, 2023
Merged

OpenSSL ENGINE support #88656

merged 8 commits into from
Jul 18, 2023

Conversation

krwq
Copy link
Member

@krwq krwq commented Jul 11, 2023 •
edited
Loading

Fixes: #55356

Allows using ENGINE with OpenSSL, for example to use TPM ECDSA key using tpm2tss engine:

// 0x81000007 - needs to be created first - see osslplugins/README.md how to get it
using SafeEvpPKeyHandle priKeyHandle = SafeEvpPKeyHandle.OpenPrivateKeyFromEngine("tpm2tss", "0x81000007");
using ECDsa ecdsaPri = new ECDsaOpenSsl(priKeyHandle);
// do stuff with ecdsaPri

Tests are manual and can be enabled with environmental variable. See osslplugins/README.md for more information on how to do it.

Note: Providers are cut from the proposal.

This code is based on @bartonjs's work https://github.com/bartonjs/runtime/blob/open_named_keys/ - it extends tests and make it react to environmental variable, ENGINE implementation is simplified and testing instructions document is added with how to run tests and what's required to set it up.

@dotnet-issue-labeler
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.

@ghost ghost assigned krwq Jul 11, 2023
@ghost
Copy link

ghost commented Jul 11, 2023

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes: #55356

Allows using ENGINE with OpenSSL, for example to use TPM ECDSA key using tpm2tss engine:

// 0x81000007 - needs to be created first - see osslplugins/README.md how to get it
using SafeEvpPKeyHandle priKeyHandle = SafeEvpPKeyHandle.OpenPrivateKeyFromEngine("tpm2tss", "0x81000007");
using ECDsa ecdsaPri = new ECDsaOpenSsl(priKeyHandle);
// do stuff with ecdsaPri

Tests are manual and can be enabled with environmental variable. See osslplugins/README.md for more information on how to do it.

Note: Providers are cut from the proposal.

This code is based on @bartonjs's work https://github.com/bartonjs/runtime/blob/open_named_keys/ - it extends tests, ENGINE implementation is simplified and testing instructions document is added with how to run tests and what's required to set it up.

Author: krwq
Assignees: -
Labels:

area-System.Security, new-api-needs-documentation
Milestone: -

@bartonjs
Copy link
Member

I sincerely believe that this should be deferred to vNext. Since ENGINEs are deprecated, ENGINE support shouldn't be added without Provider support, and it sounds like you're deferring Providers.

CIPop
CIPop reviewed Jul 11, 2023
View reviewed changes
Copy link
Member

@CIPop CIPop left a comment

Choose a reason for hiding this comment

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

I have a concern regarding lifetimes of the structural and functional references for the engine.
In my interpretation, these need to be kept alive for the duration of the key handle (e.g. in case SslStream decides to use the private key in a TLS handshake).

src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.EvpPkey.cs
}

[LibraryImport(Libraries.CryptoNative, StringMarshalling = StringMarshalling.Utf8)]
private static partial SafeEvpPKeyHandle CryptoNative_LoadPublicKeyFromEngine(
Copy link
Member

Choose a reason for hiding this comment

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

Note: I've never been able to store/load a public key in an engine before. Usually, with both TPM and PKCS#11 (via SoftHSM) the public keys were embedded in the X.509 certificate. That said, this could be allowed by some engines.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair point but as @bartonjs has already responded:#55356 (comment) "it's a low enough incremental cost that it doesn't hurt." which I fully agree

src/libraries/System.Security.Cryptography/tests/OpenSslNamedKeysTests.manual.cs
@@ -0,0 +1,194 @@
// Licensed to the .NET Foundation under one or more agreements.
Copy link
Member

Choose a reason for hiding this comment

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

This could help setting up an e2e test:

Running in this container: https://github.com/Azure/azure-iot-sdk-c/blob/main/jenkins/openssl-engine/Dockerfile
Script to prepare the HSM simulator: https://github.com/Azure/azure-iot-sdk-c/blob/main/jenkins/linux_openssl_engine.sh

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I will leave this out of scope of this PR and investigate this separately (or create issue depends on time).

src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey.c Show resolved Hide resolved
CIPop
CIPop approved these changes Jul 12, 2023
View reviewed changes
Copy link
Member

@CIPop CIPop left a comment

Choose a reason for hiding this comment

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

My concerns regarding object lifetime were addressed in openssl/openssl#21427

bartonjs
bartonjs reviewed Jul 12, 2023
View reviewed changes
src/libraries/System.Security.Cryptography/tests/OpenSslNamedKeysTests.manual.cs
Comment on lines +93 to +99
[ConditionalFact(nameof(ShouldRunEngineTests))]
public static void Engine_SanityTest()
{
Assert.False(ShouldFailTests, "This test is supposed to fail");
}

[ConditionalFact(nameof(ShouldRunTpmTssTests))]
public static void Tpm_SanityTest()
{
Assert.False(ShouldFailTests, "This test is supposed to fail");
}
Copy link
Member

Choose a reason for hiding this comment

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

While I generally do something like this during development, I don't know that we've checked in "I fail to prove the tests ran" tests before.

Copy link
Member

Choose a reason for hiding this comment

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

Since it's referenced in the instructions as how to verify manual test setup is working, I guess it makes sense.

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 found it useful when doing manual testing. Green on the first try always makes me a bit nervous for these kinds of tests. With this I can immediately exclude wrong env name or other external factors causing me to have green tests by accident

bartonjs
bartonjs reviewed Jul 12, 2023
View reviewed changes
src/libraries/System.Security.Cryptography/tests/osslplugins/e_dntest.c Outdated
Comment on lines 12 to 33
// PKCS#1 format
static char g_pubRsaKey[140] = {
0x30,0x81,0x89,0x02,0x81,0x81,0x00,0xBF,0x67,0x16,0x84,0x85,0x21,0x5A,0x6A,0xB8,
0x9B,0xCA,0xB9,0x33,0x1F,0x6F,0x5F,0x36,0x0F,0x43,0x00,0xBE,0x5C,0xF2,0x82,0xF7,
0x70,0x42,0x95,0x7E,0xA2,0x02,0x90,0x8B,0x22,0x79,0xF3,0x4A,0x42,0x6D,0x62,0xF5,
0x9D,0x6C,0x10,0x56,0xE3,0x6D,0xC9,0xF6,0xEE,0xA9,0xAE,0xB1,0xB3,0x1F,0x81,0x22,
0xF5,0x83,0xEE,0x9C,0xAE,0x2A,0x86,0xA4,0x71,0x44,0x90,0x5D,0xF0,0x54,0x41,0xB0,
0xA5,0xF2,0x9E,0x03,0xC5,0xAC,0x18,0x88,0xD9,0x37,0x44,0xD8,0x96,0x38,0xD8,0x3A,
0xC3,0x77,0x74,0xB3,0x39,0xE4,0xAF,0xB3,0x49,0xC7,0x14,0xB1,0x22,0x38,0xB0,0xF8,
0x1A,0x71,0x38,0x0F,0x05,0x1C,0x58,0x5C,0xB2,0x74,0x34,0xFA,0x54,0x4B,0xDA,0xC6,
0x79,0xE1,0xE1,0x65,0x81,0xD0,0xE9,0x02,0x03,0x01,0x00,0x01
};

// PKCS#1 format
char g_priRsaKey[608] = {
0x30,0x82,0x02,0x5C,0x02,0x01,0x00,0x02,0x81,0x81,0x00,0xBF,0x67,0x16,0x84,0x85,
Copy link
Member

Choose a reason for hiding this comment

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

When I was doing development on this, the public key named "first" and the private key named "first" didn't match (not that you necessarily could have seen that since I wrote it to load keys from files that I didn't check in). It looks like you've changed that here so that they're the same.

I thought it was valuable that they were mismatched, to show that the public/private namespace mattered.

Whether they're the same (as-is) or different (because you change it up), add a comment saying what their relationship is.

Copy link
Member Author

Choose a reason for hiding this comment

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

added a comment. I intentionally called the same to ensure we don't accidentally load the wrong one when same name is used

This was referenced Jul 13, 2023
Closed
Open
Closed
@krwq
Copy link
Member Author

krwq commented Jul 17, 2023

This is rebase only (trying to get CI to be green since I'm seeing lots of unrelated failures)

This was referenced Jul 17, 2023
Closed
Closed
@krwq

This comment was marked as off-topic.

Sign in to view
@azure-pipelines

This comment was marked as off-topic.

Sign in to view
@krwq

This comment was marked as off-topic.

Sign in to view
@azure-pipelines

This comment was marked as off-topic.

Sign in to view
@krwq

This comment was marked as off-topic.

Sign in to view
@azure-pipelines

This comment was marked as off-topic.

Sign in to view
@jeffhandley jeffhandley merged commit 420dd4e into dotnet:main Jul 18, 2023
This was referenced Jul 19, 2023
Closed
Open
@ghost ghost locked as resolved and limited conversation to collaborators Aug 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bartonjs bartonjs bartonjs left review comments

@vcsjones vcsjones vcsjones left review comments

@CIPop CIPop CIPop approved these changes

@jeffhandley jeffhandley jeffhandley approved these changes

@GrabYourPitchforks GrabYourPitchforks Awaiting requested review from GrabYourPitchforks

@jozkee jozkee Awaiting requested review from jozkee

Assignees

@krwq krwq

Labels
area-System.Security new-api-needs-documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add an easier way of opening named keys via OpenSSL
6 participants
@krwq @bartonjs @vcsjones @jeffhandley @GrabYourPitchforks @CIPop