Import PEM keys #34086

vcsjones · 2020-03-25T20:25:19Z

Dotnet-GitSync-Bot · 2020-03-25T20:25:23Z

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, to 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.

This is not strictly related to processing PEMs since the underlying import methods already test this.

vcsjones · 2020-03-26T22:21:07Z

@bartonjs this is ready for review when ever you have time to spare.

bartonjs · 2020-04-06T15:52:08Z

src/libraries/Common/src/Internal/Cryptography/PemKeyImportHelpers.cs

+                }
+
+                Debug.Assert(bytesWritten == base64size);
+                Span<byte> decodedBase64 = decodeBuffer.AsSpan(0, base64size);


In bizarro-world, where TryFromBase64Chars reports a different answer in a release build, let's use the answer it gave.

Suggested change

Span<byte> decodedBase64 = decodeBuffer.AsSpan(0, base64size);

Span<byte> decodedBase64 = decodeBuffer.AsSpan(0, bytesWritten);

bartonjs · 2020-04-06T15:53:49Z

src/libraries/Common/src/Internal/Cryptography/PemKeyImportHelpers.cs

+                    foundFields = fields;
+                    foundSlice = pem;
+                }
+                else if (label.SequenceEqual("ENCRYPTED PRIVATE KEY"))


I don't know if we need a PemHeaders constants class; but at least using a const string in this class would be better than repeating the literal across methods.

src/libraries/Common/src/Internal/Cryptography/PemKeyImportHelpers.cs

bartonjs · 2020-04-06T15:58:20Z

src/libraries/Common/src/Internal/Cryptography/PemKeyImportHelpers.cs

+
+                Debug.Assert(bytesWritten == base64size);
+                Span<byte> decodedBase64 = decodeBuffer.AsSpan(0, base64size);
+                importAction(decodedBase64, out _);


I assume that you've made a conscious choice to say it's OK to ignore trailing bytes here, since there's no check that the import action reports bytesRead == base64Size.

I can accept that... but a comment saying so would be nice. (Reasonable justification includes that the key has already been modified, so throwing and not have the same object modification effect)

bartonjs · 2020-04-06T16:00:59Z

...aries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/EC/ECKeyPemTests.cs

+        [Fact]
+        public void ImportFromPem_Pkcs8_Simple()
+        {
+            using TAlg key = CreateKey();


Please use the braced using statement anywhere a using statement is used.

bartonjs · 2020-04-06T16:04:30Z

...aries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/EC/ECKeyPemTests.cs

+Isuk92Ner/JmgKjYoSumHVmSNfZ9nLTVjxeD08pD548KWrqmJAeZNsDDqQ==
+-----END PUBLIC KEY-----";
+            using TAlg key = CreateKey();
+            AssertExtensions.Throws<ArgumentException>("input", () => key.ImportFromPem(pem));


I don't see a test that says what happens to this input when sent to the encrypted PEM import. (Either success, or still throwing). (Perhaps I'm just bad at searching/remembering)

...ies/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/RSAKeyPemTests.cs

bartonjs · 2020-04-06T16:15:13Z

src/libraries/System.Security.Cryptography.Algorithms/src/Resources/Strings.resx

+  <data name="Argument_PemImport_NoPemFound" xml:space="preserve">
+    <value>A supported key was not found.</value>
+  </data>
+  <data name="Argument_PemImport_AmbiguousPem" xml:space="preserve">
+    <value>Multiple supported keys were found.</value>
+  </data>
+  <data name="Argument_PemImport_EncryptedPem" xml:space="preserve">
+    <value>An encrypted key was found, but attempted to import it without a password.</value>
+  </data>


Some user friendliness enhancement suggestions:

Suggested change

<data name="Argument_PemImport_NoPemFound" xml:space="preserve">

<value>A supported key was not found.</value>

</data>

<data name="Argument_PemImport_AmbiguousPem" xml:space="preserve">

<value>Multiple supported keys were found.</value>

</data>

<data name="Argument_PemImport_EncryptedPem" xml:space="preserve">

<value>An encrypted key was found, but attempted to import it without a password.</value>

</data>

<data name="Argument_PemImport_NoPemFound" xml:space="preserve">

<value>No supported key formats were found. Check that the input represents the contents of a PEM-encoded key file, not the path to such a file.</value>

</data>

<data name="Argument_PemImport_AmbiguousPem" xml:space="preserve">

<value>The input contains multiple keys, but only one key can be imported.</value>

</data>

<data name="Argument_PemImport_EncryptedPem" xml:space="preserve">

<value>An encrypted key was found, but no password was provided. Use ImportFromEncryptedPem to import this key.</value>

</data>

bartonjs · 2020-04-06T16:16:54Z

src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/DSA.cs

+        /// </summary>
+        /// <param name="input">The PEM text of the key to import.</param>
+        /// <exception cref="ArgumentException">
+        /// <paramref name="input"/> does not contain a PEM-encoded key with a recognized label.


I believe once one para is being used everything needs to be para-ified.

<exception cref="ArgumentException"> <para><paramref name="input"/> does not contain a PEM-encoded key with a recognized label.</para> <para>-or</para> ... </exception>

bartonjs · 2020-04-06T16:17:35Z

src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/DSA.cs

+        /// <remarks>
+        ///   <para>
+        ///   Unsupported or malformed PEM-encoded objects will be ignored. If multiple supported PEM labels
+        ///   are found, an exception is raised to prevent importing a key when


Suggested change

/// are found, an exception is raised to prevent importing a key when

/// are found, an exception is thrown to prevent importing a key when

It seems it is going to take a while longer for Ruby part of my career path to wear off.

bartonjs · 2020-04-06T16:19:27Z

src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/DSA.cs

+        ///   When the base-64 decoded contents of <paramref name="input" /> indicate an algorithm that uses PBKDF1
+        ///   (Password-Based Key Derivation Function 1) or PBKDF2 (Password-Based Key Derivation Function 2),
+        ///   the password is converted to bytes via the UTF-8 encoding. This method
+        ///   only supports the binary (BER/CER/DER) encoding of EncryptedPrivateKeyInfo.


I think that this method only supports the PEM encoded version of EncryptedPrivateKeyInfo 😄.

Oh, I recall what I was trying to communicate - it just came out poorly. I was trying to say that the base-64 contents, when decoded, must be a BER, DER, CER {Encrypted}PrivateKeyInfo.

bartonjs · 2020-04-06T16:20:51Z

.../System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/ECDiffieHellman.cs

+                {
+                    return ImportPkcs8PrivateKey;
+                }
+                else if (label.SequenceEqual("PUBLIC KEY"))


Since these literals are spread out across multiple files, I think we should go ahead and create a PemConstants/PemHeaders/PemLabels constants file.

src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/DSA.cs

...tem.Security.Cryptography.Primitives/src/System/Security/Cryptography/AsymmetricAlgorithm.cs

Use para as required. raised -> thrown. Remove confusing text around BER,CER,DER.

Ensure ArgumentException is thrown with the right reason. Improve ambiguous PEM tests.

vcsjones · 2020-04-07T18:36:20Z

@bartonjs I think this addresses all of your feedback. The mono/linux/X509Certificates failure is unrelated.

Dotnet-GitSync-Bot added area-System.Security new-api-needs-documentation labels Mar 25, 2020

vcsjones force-pushed the 31201-impl branch from 18a50aa to 5262137 Compare March 25, 2020 21:37

vcsjones added 5 commits March 25, 2020 22:32

Import PEM encoded keys.

f2fb501

Documentation.

cc71923

Fix reference source

6c199de

Add missing doc comment.

8ae49ae

Skip overload tests for shim helpers.

7c09314

vcsjones force-pushed the 31201-impl branch from 35c50f8 to a3173c2 Compare March 26, 2020 02:33

This comment has been minimized.

Sign in to view

vcsjones force-pushed the 31201-impl branch from a3173c2 to df45473 Compare March 26, 2020 13:54

Skip explicit curve test for unsupported platforms

f7c2501

vcsjones force-pushed the 31201-impl branch from df45473 to f7c2501 Compare March 26, 2020 15:48

Remove explicit params test.

61d0ea5

This is not strictly related to processing PEMs since the underlying import methods already test this.

This comment has been minimized.

Sign in to view

Merge remote-tracking branch 'ms/master' into 31201-impl

e863fde

vcsjones mentioned this pull request Mar 30, 2020

RunContinueWithStressTestsNoState timing out in CI #2271

Closed

jaredpar mentioned this pull request Mar 30, 2020

BinaryFormatter Bitmap serialization test failed #28553

Closed

ericstj mentioned this pull request Mar 30, 2020

System.Runtime.Serialization.Formatters.Tests.BinaryFormatterTests failing on Bitmaps #34302

Closed

bartonjs reviewed Apr 6, 2020

View reviewed changes

src/libraries/Common/src/Internal/Cryptography/PemKeyImportHelpers.cs Show resolved Hide resolved

bartonjs reviewed Apr 6, 2020

View reviewed changes

...ies/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/RSAKeyPemTests.cs Show resolved Hide resolved

bartonjs reviewed Apr 6, 2020

View reviewed changes

src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/DSA.cs Show resolved Hide resolved

bartonjs reviewed Apr 6, 2020

View reviewed changes

...tem.Security.Cryptography.Primitives/src/System/Security/Cryptography/AsymmetricAlgorithm.cs Show resolved Hide resolved

vcsjones added 12 commits April 7, 2020 09:26

Merge remote-tracking branch 'ms/master' into 31201-impl

ccf1f7a

Use size based on actual decoding.

47880fb

Extract PEM labels out to constants.

62faaab

Braced usings.

50ad96b

Braced usings.

102ffe3

Braced usings.

06b2c93

Clearer exception message.

9e62845

Documentation feedback.

58dd3cb

Use para as required. raised -> thrown. Remove confusing text around BER,CER,DER.

Report consistent exception regardless of PEM order

2cadc5e

Improve tests.

0a54b92

Ensure ArgumentException is thrown with the right reason. Improve ambiguous PEM tests.

Explain bytesRead discard.

7a783ef

Missing EncryptedPrivateKeyInfo tests.

05990db

bartonjs approved these changes Apr 9, 2020

View reviewed changes

bartonjs merged commit 29df078 into dotnet:master Apr 9, 2020

vcsjones deleted the 31201-impl branch April 9, 2020 16:23

ghost locked as resolved and limited conversation to collaborators Dec 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import PEM keys #34086

Import PEM keys #34086

vcsjones commented Mar 25, 2020 •

edited

Loading

Dotnet-GitSync-Bot commented Mar 25, 2020

This comment has been minimized.

This comment has been minimized.

vcsjones commented Mar 26, 2020

bartonjs Apr 6, 2020

bartonjs Apr 6, 2020

bartonjs Apr 6, 2020

bartonjs Apr 6, 2020

bartonjs Apr 6, 2020

bartonjs Apr 6, 2020

bartonjs Apr 6, 2020

bartonjs Apr 6, 2020

vcsjones Apr 6, 2020

bartonjs Apr 6, 2020

vcsjones Apr 6, 2020

vcsjones Apr 6, 2020

bartonjs Apr 6, 2020

vcsjones commented Apr 7, 2020

	Span<byte> decodedBase64 = decodeBuffer.AsSpan(0, base64size);
	Span<byte> decodedBase64 = decodeBuffer.AsSpan(0, bytesWritten);

	/// are found, an exception is raised to prevent importing a key when
	/// are found, an exception is thrown to prevent importing a key when

Import PEM keys #34086

Import PEM keys #34086

Conversation

vcsjones commented Mar 25, 2020 • edited Loading

Dotnet-GitSync-Bot commented Mar 25, 2020

This comment has been minimized.

This comment has been minimized.

vcsjones commented Mar 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcsjones commented Apr 7, 2020

vcsjones commented Mar 25, 2020 •

edited

Loading