Feature Request: ctor injection IXmlDecryptor not working as well as for IXmlEncryptor

Question

Feature Request: ctor injection IXmlDecryptor not working as well as for IXmlEncryptor

guardrex opened this issue 8 years ago · 14 comments

Consider the following replacements for IXmlEncryptor and IXmlDecryptor ...

public class CustomXmlEncryptor : IXmlEncryptor
{
    public CustomXmlEncryptor()
    {
    }

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

    private byte[] _key;

    public EncryptedXmlInfo Encrypt(XElement plaintextElement)
    {
        // Code here uses _key to encrypt
    }
}

public class CustomXmlDecryptor : IXmlDecryptor
{
    public CustomXmlDecryptor()
    {
    }

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

    private byte[] _key;

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

Both derived classes require a parameterless ctor. The CustomXmlEncryptor always gets the correct _key value via DI with my options-based ctor. However, the CustomXmlDecryptor always gets created using its parameterless ctor, thus it never gets a value in _key from DI.

I have a pathetic workaround using a global static, but I'd like to get that CustomXmlDecryptor on DI. Am I doing it wrong? Is this an unknown bug? Is this a known bug covered by Refactor DI support in DataProtection (DataProtection #134)?

Answer 1 · 2016-06-07T19:37:17.000Z

I also hit this when implementing a custom decryptor. I worked around it by taking advantage of the SimpleActivator fallback that calls a constructor with IServiceProvider, but naturally this requires DI in place.

Interested to know what the right approach is.

Answer 2 · 2016-06-07T19:40:45.000Z

If it's a bug, I wish it would get some ❤️. My workaround is crappier than your's: a global static to get the value into the ctor. (yuck!)

Answer 3 · 2016-06-09T15:20:42.000Z

I think I just ran into this, too, and also when implementing a custom decryptor. I'm implementing a custom IActivator since, in RC2 at least, it doesn't look like there's an IActivator registered in the service collection by default so everything IActivator is used for boils down to Activator.CreateInstance(). I tried implementing a custom activator but down in the internal key management stack it's not getting my registered activator and still uses the default one. This exception is thrown without calling my custom IActivator even though other service construction calls do go through it.

System.Security.Cryptography.CryptographicException: Assertion failed: CreateInstance returned null.
   at Microsoft.AspNetCore.Cryptography.CryptoUtil.Fail(String message)
   at Microsoft.AspNetCore.Cryptography.CryptoUtil.Fail[T](String message)
   at Microsoft.AspNetCore.DataProtection.ActivatorExtensions.CreateInstance[T](IActivator activator, String implementationTypeName)
   at Microsoft.AspNetCore.DataProtection.KeyManagement.XmlKeyManager.Microsoft.AspNetCore.DataProtection.KeyManagement.Internal.IInternalXmlKeyManager.DeserializeDescriptorFromKeyElement(XElement keyElement)
Microsoft.AspNetCore.DataProtection.KeyManagement.DefaultKeyResolver: Warning: Key {35528037-ba8f-4b9f-908e-1baa2935fb57} is ineligible to be the default key because its CreateEncryptorInstance method failed.

System.Security.Cryptography.CryptographicException: Assertion failed: CreateInstance returned null.
   at Microsoft.AspNetCore.Cryptography.CryptoUtil.Fail(String message)
   at Microsoft.AspNetCore.Cryptography.CryptoUtil.Fail[T](String message)
   at Microsoft.AspNetCore.DataProtection.ActivatorExtensions.CreateInstance[T](IActivator activator, String implementationTypeName)
   at Microsoft.AspNetCore.DataProtection.KeyManagement.XmlKeyManager.Microsoft.AspNetCore.DataProtection.KeyManagement.Internal.IInternalXmlKeyManager.DeserializeDescriptorFromKeyElement(XElement keyElement)
   at Microsoft.AspNetCore.DataProtection.KeyManagement.DeferredKey.<>c__DisplayClass1_0.<GetLazyEncryptorDelegate>b__0()
   at System.Lazy`1.CreateValue()
   at System.Lazy`1.LazyInitValue()
   at Microsoft.AspNetCore.DataProtection.KeyManagement.DefaultKeyResolver.CanCreateAuthenticatedEncryptor(IKey key)

Answer 4 · 2016-06-09T16:24:03.000Z

@blowdart thoughts?

Answer 5 · 2016-06-09T17:16:21.000Z

It was to work around some deficiencies with the DI stack at the time, including aspnet/DependencyInjection#90, aspnet/DependencyInjection#197 and the removal of ITypeActivator.

If the underlying deficiencies have been resolved since the original code was written then we could revisit this, however keep in mind that the DataProtection stack also needs to run in existing ASP.NET 4.x environments to support the compatibility layer, so your test matrix can't include just Core apps, and only DI scenarios.

Answer 6 · 2016-06-09T17:36:30.000Z

Can you tell me one thing guidance-wise: I don't like having a global static in my CustomXmlDecryptor, even if I am only contending with this DI problem temporarily. Yuck .....

public CustomXmlDecryptor()
{
    _key = Global.DataProtectionKey;
}

What is the correct workaround: Service locator pattern? What does that look like in this context to get something close to my goal of ...

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

Answer 7 · 2016-06-09T17:41:20.000Z

As I wasn't the dev I have no opinions on the right way to do this to be honest. All I can do is point you to the decryptors we provide.

Answer 8 · 2016-06-09T17:48:02.000Z

I can PR this thing if folks are interested, once it's a bit more polished. But, yeah, it's service location.

The DI problems mean you basically get either a parameterless constructor or a constructor that takes an IServiceProvider. So what I did was:

Create an "options" object that just has the certificate I want to pass around. I may add other options later, but for now the purpose it serves is being a strong type that isn't just X509Certificate2 in my service collection.

public class CertificateEncryptionOptions
{
  public X509Certificate2 Certificate { get; set; }
}

At app startup I register that with the service collection.

var opts = new CertificateEncryptionOptions { Certificate = cert };
services.AddSingleton(opts);

Then in my constructor for the encryptor/decryptor, I manually resolve the options.

public CertificateXmlDecryptor(IServiceProvider services)
{
  var options = services.GetRequiredService<CertificateEncryptionOptions>();
  // Get the certificate and do stuff.
}

Answer 9 · 2016-06-09T17:52:25.000Z

@tillig Ok, cool. I'll give that a shot this afternoon.

This issue is probably not getting a bug label because the issues that @blowdart listed and the one that I listed, Refactor DI support in DataProtection (DataProtection #134), are known.

I'll determine a suitable workaround with the service locator pattern that addresses my OP, post that here, then close this issue ... probably later today.

Answer 10 · 2016-06-09T20:48:02.000Z

Ok, so this seems to work ...

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

Are there any comments (gotchas?) on this workaround approach before I close?

Answer 11 · 2016-06-15T15:44:02.000Z

For future reference I posted my custom encryptors and Redis key storage repository.

Answer 12 · 2016-10-26T16:41:41.000Z

I was informed that this is a feature request (see #134 (comment)).

Answer 13 · 2016-11-01T16:31:33.000Z

Backlogging for now.

Answer 14 · 2018-01-01T23:08:54.000Z

This issue was moved to dotnet/aspnetcore#2523