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

API Proposal: Import keys from RFC7468 PEM #31201

Closed
vcsjones opened this issue Oct 17, 2019 · 9 comments · Fixed by #34086
Closed

API Proposal: Import keys from RFC7468 PEM #31201

vcsjones opened this issue Oct 17, 2019 · 9 comments · Fixed by #34086
Labels
api-approved API was approved in API review, it can be implemented area-System.Security

Comments

@vcsjones
Copy link
Member

Proposal:

namespace System.Security.Cryptography {
  public partial class AsymmetricAlgorithm {
    public virtual void ImportFromEncryptedPem(ReadOnlySpan<char> input, ReadOnlySpan<char> password);
    public virtual void ImportFromEncryptedPem(ReadOnlySpan<char> input, ReadOnlySpan<byte> passwordBytes);
    public virtual void ImportFromPem(ReadOnlySpan<char> input);
  }

  public partial class RSA : AsymmetricAlgorithm {
    public override void ImportFromEncryptedPem(ReadOnlySpan<char> input, ReadOnlySpan<char> password);
    public override void ImportFromEncryptedPem(ReadOnlySpan<char> input, ReadOnlySpan<byte> passwordBytes);
    public override void ImportFromPem(ReadOnlySpan<char> input);
  }

  public partial class DSA : AsymmetricAlgorithm {
    public override void ImportFromEncryptedPem(ReadOnlySpan<char> input, ReadOnlySpan<char> password);
    public override void ImportFromEncryptedPem(ReadOnlySpan<char> input, ReadOnlySpan<byte> passwordBytes);
    public override void ImportFromPem(ReadOnlySpan<char> input);
  }

  public partial class ECDsa : AsymmetricAlgorithm {
    public override void ImportFromEncryptedPem(ReadOnlySpan<char> input, ReadOnlySpan<char> password);
    public override void ImportFromEncryptedPem(ReadOnlySpan<char> input, ReadOnlySpan<byte> passwordBytes);
    public override void ImportFromPem(ReadOnlySpan<char> input);
  }

  public partial class ECDiffieHellman : AsymmetricAlgorithm {
    public override void ImportFromEncryptedPem(ReadOnlySpan<char> input, ReadOnlySpan<char> password);
    public override void ImportFromEncryptedPem(ReadOnlySpan<char> input, ReadOnlySpan<byte> passwordBytes);
    public override void ImportFromPem(ReadOnlySpan<char> input);
  }
}

The expected usage of this is to be able to import RFC 7468 textually encoded public and private keys with an API that "does the right thing". The intent of this API would be to allow importing keys that are encoded with a label indicating what kind of key it is.

Usage example:

using var rsa = RSA.Create();
rsa.ImportFromPem(@"
-----BEGIN RSA PRIVATE KEY-----
MII....
-----END RSA PRIVATE KEY-----
-----BEGIN CERTIFICATE-----
MII....
-----END CERTIFICATE-----
");

Rationale:

There are a few different ways that a key can be encoded, when PEM encoded, such keys may look like "BEGIN PRIVATE KEY", "BEGIN RSA PRIVATE KEY", "BEGIN PUBLIC KEY", "BEGIN RSA PUBLIC KEY", etc. .NET Core 3 introduced APIs for importing these keys. However, this leaves developers the wrangle the key out of the file themselves, and know which API to use once they do. This is often not the experience with other software that when presented with a PEM textually encoded key, it often gets it right and just works.

Behaviors:

If the API can unambiguously determine the key that should be imported, it will import it, regardless of the rest of the contents such as a PEM aggregate. An example may be a PEM aggregate that contains a BEGIN CERTIFICATE and a BEGIN RSA PRIVATE KEY. In this circumstance, the BEGIN CERTIFICATE would be ignored because it is not a key.

If the contents contains multiple keys of the same algorithm, or a PKCS#8 key, an error is raised. This allows raising an error early without being forced to decode all PKCS#8 keys to determine what algorithm they are for.

For algorithms that support textually encoded parameters like DSA and ECDsa, if present, they will be imported as well and validated with the key. For example:

-----BEGIN EC PARAMETERS-----
BggqhkjOPQMBBw==
-----END EC PARAMETERS-----
-----BEGIN EC PRIVATE KEY-----
MHcCAQEEIIA1yny8h/fosik9s5aK19sIeGCxw1bjyBDD64G7LSkXoAoGCCqGSM49
AwEHoUQDQgAEwAWFpzlNs/YtqL5RxEeA66oqLDtrF5Et7mbyfhKoRYzjx/ItNUy/
UTyAoMKfzkEwM2PUsXZ23sVIjWD1KtOvMw==
-----END EC PRIVATE KEY-----

This private key is nistP256, as well as the EC parameters. Both explicit and named parameters should be supported. The parameters are not required, and appear at most once.

The PEM label and the key algorithm must match.

When ImportFromEncryptedPem is used, the PEM must be an encrypted private key. Even if contents contain exactly one key that could be imported because it is not encrypted, an error will still be raised.

The implementation of this is dependent on something like #29588 being implemented first.

Considerations:

  1. I considered ReadOnlySpan<byte> overloads, but that depends on encoding which complicates things, either by requiring one specific encoding (which?) or overloads to support all.
@vcsjones
Copy link
Member Author

/cc @bartonjs

@bartonjs
Copy link
Member

It might make more sense to be error if

  • Two(+) "BEGIN PRIVATE KEY" units are found
  • Two(+) "BEGIN [this kind of] KEY" units are found
  • One(+) each of "BEGIN PRIVATE KEY" and "BEGIN [this kind of] KEY" units are found.

That way it doesn't hit future algorithm errors on upgrade. (It won't crack the PKCS#8, just the mere presence raises contention)

Presumably there should be a sister method group for EncryptedPkcs8?

And, nitpicky, you forgot ECDH :)

@vcsjones
Copy link
Member Author

vcsjones commented Oct 17, 2019

@bartonjs

It might make more sense to be error if blah

I like your suggestion. For completeness, a BEGIN ENCRYPTED PRIVATE KEY will count as a BEGIN PRIVATE KEY.

Presumably there should be a sister method group for EncryptedPkcs8?

Added.

And, nitpicky, you forgot ECDH :)

Bah. Added.

@bartonjs
Copy link
Member

I was waffling on "BEGIN ENCRYPTED PRIVATE KEY" counting or not. I'm cool with being conservative and counting it (so an error for both "BEGIN ENCRYPTED PRIVATE KEY" and "BEGIN RSA PRIVATE KEY", but also an error for only "BEGIN ENCRYPTED PRIVATE KEY", unless that's supposed to try reading it with the null and empty passwords...)

@vcsjones
Copy link
Member Author

vcsjones commented Oct 17, 2019

unless that's supposed to try reading it with the null and empty passwords...

I hadn't planned on that. While that seems doable there is a lack of explicitness there that I personally don't care for.

@bartonjs
Copy link
Member

here is a lack of explicitness there that I personally don't care for.

I concur. But that's why I was on the fence of whether or not it would count as making the input "ambiguous" :)

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review labels Feb 11, 2020
@terrajobst
Copy link
Member

  • Looks good as proposed
namespace System.Security.Cryptography {
  public partial class AsymmetricAlgorithm {
    public virtual void ImportFromEncryptedPem(ReadOnlySpan<char> input, ReadOnlySpan<char> password);
    public virtual void ImportFromEncryptedPem(ReadOnlySpan<char> input, ReadOnlySpan<byte> passwordBytes);
    public virtual void ImportFromPem(ReadOnlySpan<char> input);
  }

  public partial class RSA : AsymmetricAlgorithm {
    public override void ImportFromEncryptedPem(ReadOnlySpan<char> input, ReadOnlySpan<char> password);
    public override void ImportFromEncryptedPem(ReadOnlySpan<char> input, ReadOnlySpan<byte> passwordBytes);
    public override void ImportFromPem(ReadOnlySpan<char> input);
  }

  public partial class DSA : AsymmetricAlgorithm {
    public override void ImportFromEncryptedPem(ReadOnlySpan<char> input, ReadOnlySpan<char> password);
    public override void ImportFromEncryptedPem(ReadOnlySpan<char> input, ReadOnlySpan<byte> passwordBytes);
    public override void ImportFromPem(ReadOnlySpan<char> input);
  }

  public partial class ECDsa : AsymmetricAlgorithm {
    public override void ImportFromEncryptedPem(ReadOnlySpan<char> input, ReadOnlySpan<char> password);
    public override void ImportFromEncryptedPem(ReadOnlySpan<char> input, ReadOnlySpan<byte> passwordBytes);
    public override void ImportFromPem(ReadOnlySpan<char> input);
  }

  public partial class ECDiffieHellman : AsymmetricAlgorithm {
    public override void ImportFromEncryptedPem(ReadOnlySpan<char> input, ReadOnlySpan<char> password);
    public override void ImportFromEncryptedPem(ReadOnlySpan<char> input, ReadOnlySpan<byte> passwordBytes);
    public override void ImportFromPem(ReadOnlySpan<char> input);
  }
}

@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Mar 3, 2020
@vcsjones
Copy link
Member Author

vcsjones commented Mar 25, 2020

@bartonjs

I'm almost done with this and am working through the XML docs. I do have a question about it: since these methods will ultimately use other public and documented import methods as the workhorse, how do exceptions get documented? Is it expected that every exception ImportSubjectPublicKeyInfo can throw also gets documented on ImportFromPem? Is there a way to say "and see these other methods for all the things it can throw?"

@bartonjs
Copy link
Member

Is it expected that every exception ImportSubjectPublicKeyInfo can throw also gets documented on ImportFromPem

I believe so, that's what I've been doing, at least.

Is there a way to say "and see these other methods for all the things it can throw?"

Adding a <seealso cref="whatever"/> has goodness, but I don't think there's an "import these exceptions"

Of course, you could decide to say <exception cref="CryptographicException">An error occurred during key import.</exception>. But consider the quality of docs you'd like to read while doing the "::sigh:: boilerplate documentation" shortcuts :smile:.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 12, 2020
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.

4 participants