Add X509EncryptedCredentials class #1000

GeoK · 2018-08-16T22:00:10Z

If only a certificate is provided, key wrap encryption algorithm
and data encryption algorithm will be set by default to RsaOaepKeyWrap
and A128CBC-HS256, respectively.

Add new ctor to EncryptingCredentials to allow users to pass
only a 'shared' symmetric key which will be used to encrypt data, but
it will not be serialized to a SAML token.
Add internal const strings DefaultAsymmetricKeyWrapAlgorithm and
DefaultSymmetricAlgorithm to indicate the default algorithms used for
key wrap and data encryption
Add protected ctor to EncryptingCredentials to check if a certificate
passed to X509EncryptedCredentials is null. Provides cleaner stack
trace in case of an exception caused by a null cert.
Refactor EncryptingCredentials. Move null/empty checks to setters
and provide clearer comments
Add tests for X509EncryiptingCredentials and EncryptingCredentials
classes

Resolves: #995
See also: #734

mafurman · 2018-08-17T19:27:17Z

src/Microsoft.IdentityModel.Tokens/EncryptingCredentials.cs

+        /// </remarks>
+        /// <param name="key"><see cref="SecurityKey"/></param>
+        /// <param name="enc">Data encryption algorithm to apply when encrypting plaintext.</param>
+        /// <exception cref="ArgumentException">If the <see cref="SecurityKey"/> is not <see cref="SymmetricSecurityKey"/>.</exception>


I would put an "a" right before SymmetricSecurityKey for readability.

mafurman · 2018-08-17T19:27:20Z

src/Microsoft.IdentityModel.Tokens/EncryptingCredentials.cs

        }

        /// <summary>
-        /// Gets the algorithm which used for token encryption.
+        /// Gets the key wrap encryption algorithm used for a session key encryption.


Get rid of the "a" before session key in this comment.

mafurman · 2018-08-17T19:27:23Z

src/Microsoft.IdentityModel.Tokens/EncryptingCredentials.cs

-            get;
-            private set;
+            get => _alg;
+            private set => _alg = string.IsNullOrEmpty(value) ? throw LogHelper.LogArgumentNullException("alg") : value;
        }


Maybe pass in nameof(value) instead of "alg" to LogArgumentNullException()?

Not sure if it makes sense as the resulting log message would look something like this:
IDX10000: The parameter value cannot be a 'null' or an empty object.

Using reflection, we can get names of parameters from a constructor but I don't believe that is a good solution.

mafurman · 2018-08-17T19:27:26Z

src/Microsoft.IdentityModel.Tokens/EncryptingCredentials.cs

        /// </summary>
        public CryptoProviderFactory CryptoProviderFactory { get; set; }

        /// <summary>
-        /// Gets the <see cref="SecurityKey"/> which used for signature valdiation.
+        /// Gets the <see cref="SecurityKey"/> which used for signature validation.
        /// </summary>


Get rid of the "which" in this comment.

mafurman · 2018-08-17T19:27:46Z

src/Microsoft.IdentityModel.Tokens/X509EncryptingCredentials.cs

+namespace Microsoft.IdentityModel.Tokens
+{
+    /// <summary>
+    /// An <see cref="X509EncryptingCredentials"/> designed for to construct an <see cref="EncryptingCredentials"/> based on a x509 certificate.


Either change this to "designed for constructing EncryptingCredentials" or "designed to construct EncryptingCredentials"

Maybe add in <see cref="X509Certificate2"/>?

mafurman · 2018-08-17T19:28:21Z

src/Microsoft.IdentityModel.Tokens/X509EncryptingCredentials.cs

+    public class X509EncryptingCredentials : EncryptingCredentials
+    {
+        /// <summary>
+        /// Constructs an <see cref="EncryptingCredentials"/> based on a x509 certificate.


Get rid of "an"

mafurman · 2018-08-17T19:28:43Z

src/Microsoft.IdentityModel.Tokens/X509EncryptingCredentials.cs

+        /// <param name="certificate">A <see cref="X509Certificate2"/></param>
+        /// <remarks>
+        /// <see cref="SecurityAlgorithms.DefaultAsymmetricKeyWrapAlgorithm"/> algorithm will be used by default as a key wrap encryption algorithm
+        /// <see cref="SecurityAlgorithms.DefaultSymmetricAlgorithm"/> algorithm will be used by default as a data encryption algorithm


Change to "The algorithm that will be used"

mafurman · 2018-08-17T19:29:28Z

src/Microsoft.IdentityModel.Tokens/X509EncryptingCredentials.cs

+        /// <exception cref="ArgumentNullException">if 'keyWrapEncryptionAlgorithm' is null or empty.</exception>
+        /// <exception cref="ArgumentNullException">if 'dataEncryptionAlgorithm' is null or empty.</exception>
+        public X509EncryptingCredentials(X509Certificate2 certificate, string keyWrapEncryptionAlgorithm, string dataEncryptionAlgorithm)
+            : base(new X509SecurityKey(certificate), keyWrapEncryptionAlgorithm, dataEncryptionAlgorithm)


Change this to use the constructor that takes in a certificate and not a key

Good catch!

mafurman · 2018-08-17T19:29:42Z

test/Microsoft.IdentityModel.Tokens.Tests/EncryptingCredentialsTests.cs

+
+namespace Microsoft.IdentityModel.Tokens.Tests
+{
+    public class EncryptingCredentialsTests


I would consider refactoring this test to use TheoryData if possible. I think the OpenIdConnectMessageTests.Constructors() test is similar to what you would need to do here.

Will do - it's much cleaner :)

CBaud · 2018-08-18T00:08:03Z

src/Microsoft.IdentityModel.Tokens/EncryptingCredentials.cs


-            if (string.IsNullOrWhiteSpace(enc))
-                throw LogHelper.LogArgumentNullException(nameof(enc));
+            if (key.GetType() != typeof(SymmetricSecurityKey))


Instead of checking for SymmetricSecurityKey explicitly, it would be good to enforce this contract by allowing any class that derives from SymmetricSecurityKey to be accepted. This could be accomplished using the 'is' keyword or by calling the Type.IsAssignableFrom() method.

Bonus points if we can enforce this contract using an interface instead. The current implementation is not extensible to classes that do not inherit from SymmetricSecurityKey. (e.g. a cloud-based symmetric key from a service like Key Vault) An interface could similarly be used to determine if the key derives from SymmetricSecurityKey (if that class implments the specified interface) and would also make this class extensible to any other security key that implements the specified contract.

Great input. I decided to use is as Type.IsAssignableFrom() is not supported in netstandard1.4
Good point w.r.t. an interface, will think about that.

brentschmaltz

George, have a look at these suggestions.

brentschmaltz · 2018-08-20T16:01:30Z

test/Microsoft.IdentityModel.Tokens.Tests/EncryptingCredentialsTests.cs

+            return new TheoryData<EncryptingCredentialsTheoryData>
+            {
+                new EncryptingCredentialsTheoryData
+                {


We don't need this case.

I'll remove it from the test case.

brentschmaltz · 2018-08-20T16:03:56Z

test/Microsoft.IdentityModel.Tokens.Tests/EncryptingCredentialsTests.cs

+                    Alg = null,
+                    Enc = null,
+                    ExpectedException = ExpectedException.ArgumentNullException("IDX10000:"),
+                    TestId = "null key"


Best not to have spaces ' a test name ', underscores '_', periods '.' in a TestId. This makes is harder to set a conditional break point when the test fails. Try double clicking on a TestId.

Got it - I'll replace TestId values.

brentschmaltz · 2018-08-20T16:05:09Z

test/Microsoft.IdentityModel.Tokens.Tests/EncryptingCredentialsTests.cs

+                    Key = null,
+                    Alg = SecurityAlgorithms.RsaOaepKeyWrap,
+                    Enc = SecurityAlgorithms.Aes128CbcHmacSha256,
+                    ExpectedException = ExpectedException.ArgumentNullException("IDX10000:"),


It is possible to check the exception contains "enc", this catches typo's in the error message.

I'm going to use values in the following format as expected substrings:
"IDX10000: The parameter enc"

brentschmaltz · 2018-08-20T16:09:27Z

test/Microsoft.IdentityModel.Tokens.Tests/EncryptingCredentialsTests.cs

+        [Theory, MemberData(nameof(ConstructorBTheoryData))]
+        public void ConstructorB(EncryptingCredentialsTheoryData theoryData)
+        {
+            TestUtilities.WriteHeader($"{this}.ConstructorB", theoryData);


TestUtilities.WriteHeader returns a CompareContext, so you can write:
var context = TestUtilities.WriteHeader($"{this}.ConstructorB", theoryData);

brentschmaltz · 2018-08-20T16:10:10Z

test/Microsoft.IdentityModel.Tokens.Tests/EncryptingCredentialsTests.cs

+                var encryptingCredentials = new EncryptingCredentials(theoryData.Key, theoryData.Enc);
+                //Algorithm value should be 'None'
+                IdentityComparer.AreEqual(encryptingCredentials.Alg, SecurityAlgorithms.None, context);
+                theoryData.ExpectedException.ProcessNoException();


Pass context to ProcessNoException.
theoryData.ExpectedException.ProcessNoException(context);

brentschmaltz · 2018-08-20T16:26:47Z

src/Microsoft.IdentityModel.Tokens/X509EncryptingCredentials.cs

+        /// <exception cref="ArgumentNullException">if 'certificate' is null.</exception>
+        /// <exception cref="ArgumentNullException">if 'keyWrapEncryptionAlgorithm' is null or empty.</exception>
+        /// <exception cref="ArgumentNullException">if 'dataEncryptionAlgorithm' is null or empty.</exception>
+        public X509EncryptingCredentials(X509Certificate2 certificate, string keyWrapEncryptionAlgorithm, string dataEncryptionAlgorithm)


suggest:
keyWrapEncryptionAlgorithm -> keyWrapAlgorithm

KeyWrap always implies some sort of encryption.

it makes sense - I'll rename.

brentschmaltz · 2018-08-20T16:28:43Z

src/Microsoft.IdentityModel.Tokens/SecurityAlgorithms.cs

@@ -113,6 +113,9 @@ public static class SecurityAlgorithms
        public const string Aes192CbcHmacSha384 = "A192CBC-HS384";
        public const string Aes256CbcHmacSha512 = "A256CBC-HS512";

+        internal const string DefaultAsymmetricKeyWrapAlgorithm = RsaOaepKeyWrap;
+        internal const string DefaultSymmetricAlgorithm = Aes128CbcHmacSha256;


suggest:
DefaultSymmetricAlgorithm -> DefaultSymmetricEncryptionAlgorithm

there are other SymmetricAlgorithms such as Signing.

it makes sense - I'll rename.

brentschmaltz · 2018-08-20T16:30:38Z

src/Microsoft.IdentityModel.Tokens/EncryptingCredentials.cs

-        /// <param name="enc">The encryption algorithm to apply.</param>
-        public EncryptingCredentials(SecurityKey key, string alg, string enc)
+        /// <param name="certificate"><see cref="X509Certificate2"/>.</param>
+        /// <param name="alg">A key wrap encryption algorithm to apply when encrypting a session key.</param>


suggest:
A key wrap encryption algorithm -> A key wrap algorithm

brentschmaltz · 2018-08-20T16:31:36Z

src/Microsoft.IdentityModel.Tokens/EncryptingCredentials.cs

-        public EncryptingCredentials(SecurityKey key, string alg, string enc)
+        /// <param name="certificate"><see cref="X509Certificate2"/>.</param>
+        /// <param name="alg">A key wrap encryption algorithm to apply when encrypting a session key.</param>
+        /// <param name="enc">Data encryption algorithm to apply when encrypting plaintext.</param>


suggest:
Data encryption algorithm to apply when encrypting plaintext ->
The Data encryption algorithm to apply

brentschmaltz · 2018-08-20T16:32:32Z

src/Microsoft.IdentityModel.Tokens/EncryptingCredentials.cs

+        /// <summary>
+        /// Initializes a new instance of the <see cref="EncryptingCredentials"/> class.
+        /// </summary>
+        /// <param name="key"><see cref="SecurityKey"/> to apply when encrypting a session key.</param>


suggest:
to apply when encrypting a session key ->
to use when encrypting a session key

GeoK · 2018-08-20T21:05:10Z

@brentschmaltz Thank you for the suggestions, I addressed them with: 7578fe6

brentschmaltz · 2018-08-21T17:26:01Z

If only a certificate is provided, the key wrap algorithm and data encryption algorithm will be set by default to RsaOaepKeyWrap and A128CBC-HS256, respectively. * Add new ctor to EncryptingCredentials to allow users to pass only a 'shared' symmetric key which will be used to encrypt data, but it will not be serialized to a SAML token. * Add internal const strings DefaultAsymmetricKeyWrapAlgorithm and DefaultSymmetricEncryptionAlgorithm to indicate default algorithms used for key wrap and data encryption * Add protected ctor to EncryptingCredentials to check if a certificate passed to X509EncryptedCredentials is null. Provides cleaner stack trace in case of an exception caused by a null cert. * Refactor EncryptingCredentials. Move null/empty checks to setters and provide clearer comments * Add tests for X509EncryiptingCredentials and EncryptingCredentials classes Resolves: #995 See also: #734

GeoK requested review from brentschmaltz and mafurman August 16, 2018 22:00

mafurman reviewed Aug 17, 2018

View reviewed changes

mafurman approved these changes Aug 17, 2018

View reviewed changes

CBaud reviewed Aug 18, 2018

View reviewed changes

GeoK force-pushed the geok/X509EncryptingCredentials branch 2 times, most recently from dbc3903 to 4728e83 Compare August 18, 2018 02:09

brentschmaltz requested changes Aug 20, 2018

View reviewed changes

GeoK force-pushed the geok/X509EncryptingCredentials branch from 8f3cc6b to e9fe530 Compare August 21, 2018 18:45

GeoK merged commit e9fe530 into dev Aug 21, 2018

GeoK deleted the geok/X509EncryptingCredentials branch August 21, 2018 18:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add X509EncryptedCredentials class #1000

Add X509EncryptedCredentials class #1000

GeoK commented Aug 16, 2018

mafurman Aug 17, 2018

mafurman Aug 17, 2018

mafurman Aug 17, 2018

GeoK Aug 17, 2018

mafurman Aug 17, 2018

mafurman Aug 17, 2018 •

edited

Loading

mafurman Aug 17, 2018

mafurman Aug 17, 2018

mafurman Aug 17, 2018

GeoK Aug 17, 2018

mafurman Aug 17, 2018

GeoK Aug 18, 2018

CBaud Aug 18, 2018

GeoK Aug 18, 2018

brentschmaltz left a comment

brentschmaltz Aug 20, 2018

GeoK Aug 20, 2018

brentschmaltz Aug 20, 2018

GeoK Aug 20, 2018

brentschmaltz Aug 20, 2018

GeoK Aug 20, 2018

brentschmaltz Aug 20, 2018

brentschmaltz Aug 20, 2018

brentschmaltz Aug 20, 2018

GeoK Aug 20, 2018

brentschmaltz Aug 20, 2018

GeoK Aug 20, 2018

brentschmaltz Aug 20, 2018

brentschmaltz Aug 20, 2018

brentschmaltz Aug 20, 2018

GeoK Aug 20, 2018

GeoK commented Aug 20, 2018

brentschmaltz commented Aug 21, 2018

Add X509EncryptedCredentials class #1000

Add X509EncryptedCredentials class #1000

Conversation

GeoK commented Aug 16, 2018

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

mafurman Aug 17, 2018 • edited Loading

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

brentschmaltz left a 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

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

GeoK commented Aug 20, 2018

brentschmaltz commented Aug 21, 2018

mafurman Aug 17, 2018 •

edited

Loading