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 an easier way of opening named keys via OpenSSL #55356

Closed
Tracked by #64488
bartonjs opened this issue Jul 8, 2021 · 9 comments · Fixed by #88656
Closed
Tracked by #64488

Add an easier way of opening named keys via OpenSSL #55356

bartonjs opened this issue Jul 8, 2021 · 9 comments · Fixed by #88656
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Security
Milestone

Comments

@bartonjs
Copy link
Member

bartonjs commented Jul 8, 2021

When a key is available via an ENGINE (or the new OSSL3 provider model) by name, it currently requires a couple of fussy P/Invokes.

Proposal

namespace System.Security.Cryptography
{
    public partial class SafeEvpPKeyHandle
    {
        // OpenSSL ENGINEs (only plugin model for OSSL 1.1, deprectated (but present) in 3.0+)
        public static SafeEvpPKeyHandle OpenPrivateKeyFromEngine(string engineName, string keyId) => throw null;
        public static SafeEvpPKeyHandle OpenPublicKeyFromEngine(string engineName, string keyId) => throw null;

        // EDIT (krwq): this part got approved during API review but got cut due to issues with tpm provider - see comments
        // OpenSSL Providers (new plugin model for OSSL 3.0+)
        // public static SafeEvpPKeyHandle OpenKeyFromProvider(string providerName, string keyUri) => throw null;
    }
}

Points worth mentioning

  • OpenSSL Engines (deprecated in OpenSSL 3.0) make a distinction between loading private keys and loading public keys.
    • Since "load public" and "load private" are different registration points, supporting one does not imply the other, and there's nothing saying that load_public("some key") and `load_private("some key") are related.
  • OpenSSL Engines use the verb "load" here.
  • Windows CNG uses the verb "Open", and .NET CngKey uses "Open"
  • OpenSSL 3 has a very different model of loading keys
    • They're by "URI" (with provider-dependent schemes). For example, the tpm2 provider supports key URIs like "handle:0x81000000" and "object:".
    • There's no "from this provider" key load, but you can build a context filter to limit the interrogations to specific providers (and thus to specifically one).
    • The API doesn't make a distinction from loading private keys or public keys (or non-certificates, )
    • A single URI can return multiple objects. We have to make Decisions. (That decision is just like with PFX: first private ?? first public ?? throw).
  • OpenSSL 3's verb is "open" (or/also "attach" when opening from stdin makes sense)
  • We don't have a good way of expressing what kind of key the key is (RSA, DSA, etc)... that's an exercise left to the caller.

Usage examples

RSA Public key from ENGINE

byte[] data = ...;
byte[] signature = ...;

using (SafeEvpPKeyHandle pubKeyHandle = SafeEvpPKeyHandle.OpenPublicKeyFromEngine("yourEngineName", "someKeyId"))
using (RSA rsaPub = new RSAOpenSsl(pubKeyHandle))
{
    if (!rsaPub.VerifyData(data, signature, HashAlgorithmName.SHA256, RSASignaturePadding.Pss))
    {
        // handle bad signature
    }
}

RSA Private key from ENGINE

byte[] data = ...;

using (SafeEvpPKeyHandle priKeyHandle = SafeEvpPKeyHandle.OpenPrivateKeyFromEngine("yourEngineName", "someKeyId"))
using (RSA rsaPri = new RSAOpenSsl(priKeyHandle))
{
    byte[] signature = rsaPri.SignData(data, HashAlgorithmName.SHA256, RSASignaturePadding.Pss);
    // do something with signature
}

RSA Private key from Provider

byte[] data = ...;

// For TPM settings refer to provider documentation you're using, i.e. https://github.com/tpm2-software/tpm2-openssl/tree/master
// opening key might look like: SafeEvpPKeyHandle.OpenKeyFromProvider("tpm2", "handle:0x81000000")
using (SafeEvpPKeyHandle priKeyHandle = SafeEvpPKeyHandle.OpenKeyFromProvider("yourProviderName", "someKeyUri"))
using (RSA rsaPri = new RSAOpenSsl(priKeyHandle))
{
    byte[] signature = rsaPri.SignData(data, HashAlgorithmName.SHA256, RSASignaturePadding.Pss);
    // do something with signature
}
Original speculation

The easiest option, is to load just the key handle, a la

namespace System.Security.Cryptography
{
    partial class SafeEvpPKeyHandle
    {
        public static SafeEvpPKeyHandle LoadKey(string providerName, string keyId) => throw null;
    }
}

which turns into something like

ENGINE* e = ENGINE_by_id(providerName);
EVP_PKEY* key = NULL;

if (e != NULL)
{
    if (ENGINE_init(e))
    {
        key = ENGINE_load_private_key(e, keyId, NULL, NULL);
        ENGINE_finish(e);
    }
}

return key;

That needs to be reconciled with OSSL3 providers to make sure that two strings is sufficient.

Loading just the EVP_PKEY better enables the scenario of loading a key from an HSM (or other ENGINE-based system), but does still require a bit of a dance with a certificate (when applicable).

using (X509Certificate2 pubCert = ...)
using (SafeEvpPKeyHandle keyHandle = SafeEvpPKeyHandle.LoadKey(...))
{
    switch (pubCert.GetKeyAlgorithm())
    {
        case Oids.Rsa:
            using (RSAOpenSsl rsa = new RSAOpenSsl(keyHandle))
            {
                return pubCert.CopyWithPrivateKey(rsa);
            }
        case Oids.Dsa:
            ...
        case Oids.Ecc:
        case Oids.Ecdh:
        {
            using (ECDiffieHellmanOpenSsl ecdh = ...)
            ...
        }
        default:
            throw something;
    }
}

So perhaps we can find a better way of doing that. Especially since the CopyWithPrivateKey implementation eventually throws away the wrapper type and just saves a copy of the key handle.

That helper operation should fail on macOS (assuming it's tied to the OpenSSL library), since macOS can't bind OpenSSL keys without exporting them.

@bartonjs bartonjs added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jul 8, 2021
@bartonjs bartonjs added this to the 7.0.0 milestone Jul 8, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Security untriaged New issue has not been triaged by the area owner labels Jul 8, 2021
@ghost
Copy link

ghost commented Jul 8, 2021

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

Issue Details

When a key is available via an ENGINE (or the new OSSL3 provider model) by name, it currently requires a couple of fussy P/Invokes.

The easiest option, is to load just the key handle, a la

namespace System.Security.Cryptography
{
    partial class SafeEvpPKeyHandle
    {
        public static SafeEvpPKeyHandle LoadKey(string providerName, string keyId) => throw null;
    }
}

That needs to be reconciled with OSSL3 providers to make sure that two strings is sufficient.

Loading just the EVP_PKEY better enables the scenario of loading a key from an HSM (or other ENGINE-based system), but does still require a bit of a dance with a certificate (when applicable).

using (X509Certificate2 pubCert = ...)
using (SafeEvpPKeyHandle keyHandle = SafeEvpPKeyHandle.LoadKey(...))
{
    switch (pubCert.GetKeyAlgorithm())
    {
        case Oids.Rsa:
            using (RSAOpenSsl rsa = new RSAOpenSsl(keyHandle))
            {
                return pubCert.CopyWithPrivateKey(rsa);
            }
        case Oids.Dsa:
            ...
        case Oids.Ecc:
        case Oids.Ecdh:
        {
            using (ECDiffieHellmanOpenSsl ecdh = ...)
            ...
        }
        default:
            throw something;
    }
}

So perhaps we can find a better way of doing that. Especially since the CopyWithPrivateKey implementation eventually throws away the wrapper type and just saves a copy of the key handle.

That helper operation should fail on macOS (assuming it's tied to the OpenSSL library), since macOS can't bind OpenSSL keys without exporting them.

Author: bartonjs
Assignees: -
Labels:

api-suggestion, area-System.Security, untriaged

Milestone: 7.0.0

@bartonjs bartonjs removed the untriaged New issue has not been triaged by the area owner label Jul 8, 2021
@bartonjs
Copy link
Member Author

bartonjs commented Jul 8, 2021

Explictly out of scope: Putting the key into the ENGINE/provider in the first place.

@jkotas
Copy link
Member

jkotas commented Jul 8, 2021

public static SafeEvpPKeyHandle LoadKey(string providerName, string keyId) => throw null

Openssl has two APIs ENGINE_load_public_key and ENGINE_load_private_key.

I assume this will wrap ENGINE_load_private_key. Should the API name explicitly say LoadPrivateKey? Or maybe we should have both LoadPublicKey and LoadPrivateKey APIs?

@bartonjs
Copy link
Member Author

bartonjs commented Jul 9, 2021

Or maybe we should have both LoadPublicKey and LoadPrivateKey APIs?

Yeah, that's probably reasonable. I'm way less clear on the use case for LoadPublicKey (since a software key is probably fine and faster (or fast enough) for public ops); but it's a low enough incremental cost that it doesn't hurt.

@benlongo
Copy link

When a key is available via an ENGINE (or the new OSSL3 provider model) by name, it currently requires a couple of fussy P/Invokes.

Is doing this currently documented anywhere? I'd like to pull an RSA private key out of a TPM2

@bartonjs bartonjs added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Feb 6, 2023
@bartonjs
Copy link
Member Author

bartonjs commented Feb 6, 2023

Having proven to myself that (engineName, keyId) and (providerName, keyUri) is sufficient for loading keys (in all contexts that I could identify), I've marked this as ready for review.

@bartonjs
Copy link
Member Author

bartonjs commented Feb 6, 2023

Is doing this currently documented anywhere? I'd like to pull an RSA private key out of a TPM2

As far as I know, the C code to open a named key from a specific OSSL_PROVIDER looks like https://github.com/bartonjs/runtime/blob/294131ec9e02bc8604c0a8cba8423b82032539c0/src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey.c#L321-L421. I only used it to load keys named like file:/path/to/some.key from the core provider, though (due to not being able to write a working provider of my own, and not having easy access to a computer with both OpenSSL 3 and a TPM to try the tpm2 provider on).

For the tpm2 provider (https://github.com/tpm2-software/tpm2-openssl) that'd probably be "tpm2" as the provider name and something like handle:0x81000000 or object:<path_to_serialized_parameters>.

If you have an ENGINE instead of provider, then that'd be the code from the top post:

ENGINE* e = ENGINE_by_id(providerName);
EVP_PKEY* key = NULL;

if (e != NULL)
{
    if (ENGINE_init(e))
    {
        key = ENGINE_load_private_key(e, keyId, NULL, NULL);
        ENGINE_finish(e);
    }
}

return key;

Either way, you'd currently need to either provide your own C wrapper-target for those, or convert them all to P/Invokes.

@terrajobst
Copy link
Member

terrajobst commented Mar 9, 2023

Video

  • Looks good as proposed
namespace System.Security.Cryptography;

public partial class SafeEvpPKeyHandle
{
    [UnsupportedOSPlatform("android")]
    [UnsupportedOSPlatform("browser")]
    [UnsupportedOSPlatform("ios")]
    [UnsupportedOSPlatform("tvos")]
    [UnsupportedOSPlatform("windows")]
    public static SafeEvpPKeyHandle OpenPrivateKeyFromEngine(string engineName, string keyId);

    [UnsupportedOSPlatform("android")]
    [UnsupportedOSPlatform("browser")]
    [UnsupportedOSPlatform("ios")]
    [UnsupportedOSPlatform("tvos")]
    [UnsupportedOSPlatform("windows")]
    public static SafeEvpPKeyHandle OpenPublicKeyFromEngine(string engineName, string keyId);

    [UnsupportedOSPlatform("android")]
    [UnsupportedOSPlatform("browser")]
    [UnsupportedOSPlatform("ios")]
    [UnsupportedOSPlatform("tvos")]
    [UnsupportedOSPlatform("windows")]
    public static SafeEvpPKeyHandle OpenKeyFromProvider(string providerName, string keyUri);
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Mar 9, 2023
@jeffhandley jeffhandley assigned krwq and unassigned bartonjs Mar 23, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 11, 2023
@krwq
Copy link
Member

krwq commented Jul 11, 2023

Due to testing I've made I'll cut Provider part from this proposal. Some providers can work with API designed as is with just simple Interop to OpenSSL but they're mostly not that useful. Some more useful providers like tpm require (well, they load but keys don't work) that you always create them in separate library context which would require quite a bit of refactoring to support (or lookup which would make everyone suffer lookup penalty). It might be simpler to implement RSA/ECDsa directly using providers than trying to get SafeEvpPKeyHandle to work with them since that implementation could pass in library context. My attempts to get that to work can be found here: https://github.com/krwq/runtime/blob/open_named_keys/src/libraries/System.Security.Cryptography/tests/osslplugins/testprovider.c where I experimented with similar setup to what we do in .NET.

but my finding is that this currently doesn't work in the global context (not sure if this is openssl bug, provider bug or by design) but can be made to work when loaded in separate library context.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 18, 2023
@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.
Labels
api-approved API was approved in API review, it can be implemented area-System.Security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants