Refactored DI support #203

ajaybhargavb · 2017-02-22T19:30:24Z

Issue - #134

Refactored builder extensions and service collection extensions
Refactored Settings/Configuration/Descriptor
Removed ConfigurationCommon/AuthenticatedEncryptorConfigurationExtensions
Added IAuthenticatedEncryptorFactory and implementations
Refactored IKey to have Descriptor instead of CreateEncryptorInstance()
Handled Repository/Encryptor special logic
Added samples
Updated tests

guardrex · 2017-02-23T06:42:11Z

@ajaybhargavb Will these changes fix #154?

TL;DR ... The prob described in that issue was that DI wasn't working for IXmlDecryptor ...

public class CustomXmlDecryptor : IXmlDecryptor
{
    private byte[] _key;

    public CustomXmlDecryptor()
    {
    }

    public CustomXmlDecryptor(IOptions<CustomDataProtectionOptions> dataProtectionOptions)
    {
        _key = dataProtectionOptions.Value.Key;
    }

    public XElement Decrypt(XElement encryptedElement)
    {
        // Code here uses _key to decrypt
    }
}

_key wasn't getting a value, so I've had to use ...

public CustomXmlDecryptor(IServiceProvider services)
{
    var dataProtectionOptions = 
        services.GetRequiredService<IOptions<CustomDataProtectionOptions>>();
    _key = dataProtectionOptions.Value.Key;
}

... to make it work.

ajaybhargavb · 2017-02-23T18:47:43Z

@guardrex, this change won't fix the issue that you referenced. It was never supported. IXmlDecryptor was not refactored as part of this change because it gets serialized and changing it would break existing applications.

Eilon · 2017-02-28T19:56:09Z

samples/CustomEncryptorSample/Program.cs

+            var serviceCollection = new ServiceCollection();
+            serviceCollection.AddLogging();
+            serviceCollection.AddDataProtection()
+                .PersistKeysToFileSystem(new DirectoryInfo(@"c:\temp-keys"))


Should this use some other path for the sake of the sample? Does this work on non-Windows?

Eilon · 2017-02-28T19:56:42Z

samples/KeyManagementSample/Program.cs

+            var serviceCollection = new ServiceCollection();
+            serviceCollection.AddDataProtection()
+                // point at a specific folder and use DPAPI to encrypt keys
+                .PersistKeysToFileSystem(new DirectoryInfo(@"c:\temp-keys"))


Eilon · 2017-02-28T19:57:09Z

samples/KeyManagementSample/Program.cs

+            // one key in the key ring
+            services.GetDataProtector("Sample.KeyManager.v1").Protect("payload");
+            Console.WriteLine("Performed a protect operation.");
+            Thread.Sleep(2000);


What's this for?

Eilon · 2017-02-28T19:57:40Z

samples/NonDISample/Program.cs

+        {
+            // get the path to %LOCALAPPDATA%\myapp-keys
+            var destFolder = Path.Combine(
+                Environment.GetEnvironmentVariable("LOCALAPPDATA"),


What happens on non-Windows?

Eilon · 2017-02-28T21:25:40Z

...ft.AspNetCore.DataProtection/AuthenticatedEncryption/ManagedAuthenticatedEncryptorFactory.cs

+            {
+                throw Error.Common_PropertyCannotBeNullOrEmpty(nameof(configuration.EncryptionAlgorithmType));
+            }
+            typeof(SymmetricAlgorithm).AssertIsAssignableFrom(configuration.EncryptionAlgorithmType);


What's this?

Talked offline

Eilon · 2017-02-28T22:40:36Z

src/Microsoft.AspNetCore.DataProtection/XmlEncryption/DpapiNGXmlEncryptor.cs

@@ -118,7 +107,7 @@ internal static string GetDefaultProtectionDescriptorString()
            using (var currentIdentity = WindowsIdentity.GetCurrent())
            {
                // use the SID to create an SDDL string
-                return Invariant($"SID={currentIdentity.User.Value}");
+                return String.Format(CultureInfo.InvariantCulture, "SID={0}", currentIdentity.User.Value);


Eilon · 2017-02-28T22:40:54Z

src/Microsoft.AspNetCore.DataProtection/Repositories/RegistryXmlRepository.cs

@@ -107,7 +87,11 @@ private static RegistryKey GetDefaultHklmStorageKey()
                    // Even though this is in HKLM, WAS ensures that applications hosted in IIS are properly isolated.
                    // See APP_POOL::EnsureSharedMachineKeyStorage in WAS source for more info.
                    // The version number will need to change if IIS hosts Core CLR directly.
-                    var aspnetAutoGenKeysBaseKeyName = Invariant($@"SOFTWARE\Microsoft\ASP.NET\4.0.30319.0\AutoGenKeys\{WindowsIdentity.GetCurrent().User.Value}");
+                    var aspnetAutoGenKeysBaseKeyName = String.Format(


string! And elsewhere, too.

Eilon · 2017-02-28T22:41:39Z

...AspNetCore.Cryptography.Internal.Test/Microsoft.AspNetCore.Cryptography.Internal.Test.csproj

@@ -21,4 +21,8 @@
    <PackageReference Include="xunit" Version="2.2.0-*" />
  </ItemGroup>

+  <ItemGroup>
+    <Service Include="{82a7f48d-3b50-4b1e-b82e-3ada8210c358}" />


Does VS just put this here? If we remove it, will VS put it back? (If so, leave it.)

Yes, VS will just put this back.

Ah ok that's fine then.

Eilon · 2017-02-28T22:43:42Z

...atedEncryption/ConfigurationModel/CngGcmAuthenticatedEncryptorDescriptorDeserializerTests.cs

+
+        private static IAuthenticatedEncryptor CreateEncryptorInstanceFromDescriptor(CngGcmAuthenticatedEncryptorDescriptor descriptor)
+        {
+            var key = new Key(


Ick, can this use named params?

Eilon · 2017-02-28T23:39:10Z

test/Microsoft.AspNetCore.DataProtection.Test/RegistryPolicyResolverTests.cs

-            IServiceCollection serviceCollection = new ServiceCollection();
-            RunTestWithRegValues(serviceCollection, new Dictionary<string, object>()
+            // Arrange
+            var registryEntries = new Dictionary<string, object>()
            {
                ["unused"] = 42


If it's unused, do we need to set any values in the registry at all?

ajaybhargavb · 2017-03-10T03:41:53Z

🆙 📅

Eilon

😄

rynowak · 2017-03-14T19:59:11Z

...Microsoft.AspNetCore.DataProtection/AuthenticatedEncryption/AuthenticatedEncryptorFactory.cs

-    /// Settings for configuring authenticated encryption algorithms.
-    /// </summary>
-    public sealed class AuthenticatedEncryptionSettings : IInternalAuthenticatedEncryptionSettings
+    public class AuthenticatedEncryptorFactory : IAuthenticatedEncryptorFactory


Should this be sealed?

Yes it should. Looks like I missed this one. Good catch 👍

- Refactored builder extensions and service collection extensions - Refactored Settings/Configuration/Descriptor - Removed ConfigurationCommon/AuthenticatedEncryptorConfigurationExtensions - Added IAuthenticatedEncryptorFactory and implementations - Refactored IKey to have Descriptor instead of CreateEncryptorInstance() - Handled Repository/Encryptor special logic - Added samples - Updated tests

ajaybhargavb · 2017-03-15T03:33:25Z

Thanks @Eilon @rynowak @GrabYourPitchforks

rynowak · 2017-03-15T04:29:27Z

🎉

ajaybhargavb assigned rynowak Feb 22, 2017

dnfclas added the cla-already-signed label Feb 22, 2017

ajaybhargavb force-pushed the ajbaaska/refactor-dp-final branch from 4df6218 to 8ee57f7 Compare February 23, 2017 19:54

Eilon suggested changes Feb 28, 2017

View reviewed changes

ajaybhargavb mentioned this pull request Mar 8, 2017

Move NullLoggerFactory and NullLoggerOfT to Microsoft.Extensions.Logging.Abstractions aspnet/Logging#566

Closed

Eilon approved these changes Mar 10, 2017

View reviewed changes

rynowak approved these changes Mar 14, 2017

View reviewed changes

ajaybhargavb force-pushed the ajbaaska/refactor-dp-final branch from 6db8037 to cde3b96 Compare March 15, 2017 03:29

ajaybhargavb merged commit cde3b96 into dev Mar 15, 2017

ajaybhargavb deleted the ajbaaska/refactor-dp-final branch March 15, 2017 03:31

ajaybhargavb added a commit to aspnet/Security that referenced this pull request Mar 15, 2017

React to aspnet/DataProtection#203

fc57e0a

ajaybhargavb mentioned this pull request Mar 15, 2017

React to Data Protection refactor aspnet/Security#1150

Merged

natemcmaster mentioned this pull request Jun 13, 2017

Remove DefaultKeyServices #248

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored DI support #203

Refactored DI support #203

ajaybhargavb commented Feb 22, 2017

guardrex commented Feb 23, 2017

ajaybhargavb commented Feb 23, 2017

Eilon Feb 28, 2017

Eilon Feb 28, 2017

Eilon Feb 28, 2017

Eilon Feb 28, 2017

Eilon Feb 28, 2017

ajaybhargavb Mar 2, 2017

Eilon Feb 28, 2017

Eilon Feb 28, 2017

Eilon Feb 28, 2017

ajaybhargavb Mar 2, 2017

Eilon Mar 3, 2017

Eilon Feb 28, 2017

ajaybhargavb Mar 10, 2017

Eilon Feb 28, 2017

ajaybhargavb commented Mar 10, 2017

Eilon left a comment

rynowak Mar 14, 2017

ajaybhargavb Mar 15, 2017

ajaybhargavb commented Mar 15, 2017

rynowak commented Mar 15, 2017

Refactored DI support #203

Refactored DI support #203

Conversation

ajaybhargavb commented Feb 22, 2017

guardrex commented Feb 23, 2017

ajaybhargavb commented Feb 23, 2017

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

ajaybhargavb commented Mar 10, 2017

Eilon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajaybhargavb commented Mar 15, 2017

rynowak commented Mar 15, 2017