dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.3k stars 9.96k forks source link

Add the ability to delay the receipt of a certificate in the extension method DataProtectionBuilderExtensions.ProtectKeysWithCertificate #23033

Open shazhko-artem2 opened 4 years ago

shazhko-artem2 commented 4 years ago

Problem

In my current project, we have several certificate providers, so we use the certificate factory. Also, we use data protection and protect keys with a certificate. The problem is that DataProtectionBuilderExtensions.ProtectKeysWithCertificate requires a certificate directly, for which we need a certificate factory, but for getting a factory, we need an IServiceProvider.

Solution

We need to add a method overload with the following parameters.

public static IDataProtectionBuilder ProtectKeysWithCertificate(this IDataProtectionBuilder builder, Func<IServiceProvider, X509Certificate2 > factory){...}

Similar existing solutions

A similar solution already exists for AddKeyEscrowSink, but not for ProtectKeysWithCertificate.

public static IDataProtectionBuilder AddKeyEscrowSink(this IDataProtectionBuilder builder, Func<IServiceProvider, IKeyEscrowSink> factory){...}
ghost commented 3 years ago

Thanks for contacting us. We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

Shazhko-Artem commented 3 years ago

Hi guys. I investigated the codebase and understood that this requires slightly more changes than expected to implement this feature. The main problem that we need to pass this certificate for two options (KeyManagementOptions and XmlKeyDecryptionOptions) but we should invoke the certificate factory only once to be sure that these two options will have the same certificate.

I have an idea of how to implement this with as few changes as possible. We can create a class that will be the container for the value. This class will have a method to get the value, which takes a factory as a parameter. If the value is not yet in the container, then the factory will be called and the resulting value will be stored in the container. The next time this method is called, the value will be taken from the container.

Examples of such a solution.

  1. Class Lazy<> - allows you to pass a delegate (value factory) to the constructor which will be called once and store the value inside the class. In our situation, we cannot use this class, since the delegate needs to be passed to the constructor, but we cannot do this, since, at the time of creating an instance of the Lazy class, we still do not have a service provider.
  2. ConcurrentDictionary<> - it has a GetOrAdd method, the first parameter is a key, and the second is a value factory. This is not suitable for our situation because we do not have a collection, we have a single certificate.

Example of implementation

ValueBox is a container for a value.

internal class ValueBox<TValue>
{
    private readonly object _lock = new object();
    private volatile bool _hasValue = false;
    private TValue _value;

    public TValue GetOrSet(Func<TValue> factory)
    {
        lock (_lock)
        {
            if (_hasValue)
            {
                return _value;
            }

            _value = factory();
            _hasValue = true;
        }

        return _value;
    }
}

A new extension method with the ability to delay the receipt of a certificate

public static IDataProtectionBuilder ProtectKeysWithCertificate(
    this IDataProtectionBuilder builder,
    Func<IServiceProvider, X509Certificate2> provider)
{
    if (builder == null)
    {
        throw new ArgumentNullException(nameof(builder));
    }

    if (provider == null)
    {
        throw new ArgumentNullException(nameof(provider));
    }

    var certificateBox = new ValueBox<X509Certificate2>();
    builder.Services.AddSingleton<IConfigureOptions<KeyManagementOptions>>(services =>
    {
        var loggerFactory = services.GetService<ILoggerFactory>() ?? NullLoggerFactory.Instance;
        return new ConfigureOptions<KeyManagementOptions>(options =>
        {
            var certificate = certificateBox.GetOrSet(() => provider(services));
            options.XmlEncryptor = new CertificateXmlEncryptor(certificate, loggerFactory);
        });
    });
    builder.Services.AddSingleton<IConfigureOptions<XmlKeyDecryptionOptions>>(services =>
    {
        return new ConfigureOptions<XmlKeyDecryptionOptions>(options =>
        {
            var certificate = certificateBox.GetOrSet(() => provider(services));
            options.AddKeyDecryptionCertificate(certificate);
        });
    });
    return builder;
}
Shazhko-Artem commented 3 years ago

Hi @HaoK, could you check my suggestion, please?

Shazhko-Artem commented 3 years ago

@blowdart, @javiercn, @mkArtakMSFT, sorry to bother you, could you please check my suggestion above please? Or mention someone who can do it.

javiercn commented 3 years ago

@Shazhko-Artem I'm not familiar with this area of the code, so I'll let others chime in. However, the way you are using config is less than ideal.

It's likely much easier to do this by doing something like:

services.TryAddEnumerable(ServiceDescriptor.Singleton<IConfigureOptions<KeyManagementOptions>>(sp => sp.GetRequiredService<CertificateConfig>()));
services.TryAddEnumerable(ServiceDescriptor.Singleton<IConfigureOptions<XmlKeyDecriptionOptions>>(sp => sp.GetRequiredService<CertificateConfig>()));
services.AddSingleton(sp => new CertificateConfig(...));

I believe there is also a Configure overload that does this for you. @HaoK will know better.

Shazhko-Artem commented 3 years ago

@javiercn Thanks for the answer. I really appreciate your answer. In your example, the CertificateConfig should implement two interfaces (IConfigureOptions<KeyManagementOptions> and IConfigureOptions<XmlKeyDecriptionOptions>) and invoke factory func only in one time and reuse the result (certificate) for configure the second options type.

But I got the main idea of your example. We can create an IDataProtectionCertificateProvider interface that allows us to get the single certificate when we configure these options. The implementation will accept a factory function in the constructor.

Example

The IDataProtectionCertificateProvider interface provides the same certificate into different option types.

public interface IDataProtectionCertificateProvider
{
    X509Certificate2 Get();
}

The implementation of the IDataProtectionCertificateProvider.

public class DataProtectionCertificateProvider<TDep> : IDataProtectionCertificateProvider
{
    private readonly TDep _dependency;
    private readonly Func<TDep, X509Certificate2> _factory;
    private readonly object _lock = new object();
    private volatile bool _hasCert = false;
    private X509Certificate2 _certificate;
    public DataProtectionCertificateProvider(TDep dependency, Func<TDep, X509Certificate2> factory)
    {
        this._dependency = dependency;
        this._factory = factory;
    }
    public X509Certificate2 Get()
    {
        lock (_lock)
        {
            if (_hasCert)
            {
                return _certificate;
            }
            _certificate = _factory(_dependency);
            _hasCert = true;
        }
        return _certificate;
    }
}

A new extension method with the ability to delay the receipt of a certificate by using data protection certificate provider

public static IDataProtectionBuilder ProtectKeysWithCertificate(
            this IDataProtectionBuilder builder,
            Func<IServiceProvider, X509Certificate2> factory)
{
     // ...
    builder.Services.AddSingleton<IDataProtectionCertificateProvider>(services=>
        new DataProtectionCertificateProvider<IServiceProvider>(services, factory));
    builder.Services.AddSingleton<IConfigureOptions<KeyManagementOptions>>(services =>
    {
        var loggerFactory = services.GetService<ILoggerFactory>() ?? NullLoggerFactory.Instance;
        return new ConfigureOptions<KeyManagementOptions>(options =>
        {
            var certificateProvider = services.GetRequiredService<IDataProtectionCertificateProvider>();
            var certificate = certificateProvider.Get();
            options.XmlEncryptor = new CertificateXmlEncryptor(certificate, loggerFactory);
        });
    });
    builder.Services.AddSingleton<IConfigureOptions<XmlKeyDecryptionOptions>>(services =>
    {
        return new ConfigureOptions<XmlKeyDecryptionOptions>(options =>
        {
            var certificateProvider = services.GetRequiredService<IDataProtectionCertificateProvider>();
            var certificate = certificateProvider.Get();
            options.AddKeyDecryptionCertificate(certificate);
        });
    });
    return builder;
}

But as you can see we have the same logic in the DataProtectionCertificateProvider as previously suggested ValueBox<>.

Another solution

We can create a separate intermediate options class that will have a certificate property.

Example

Intermediate options class.

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

A new extension method with the ability to delay the receipt of a certificate by using intermediate options class

public static IDataProtectionBuilder ProtectKeysWithCertificate(
            this IDataProtectionBuilder builder,
            Func<IServiceProvider, X509Certificate2> factory)
{
    // ...
    builder.Services.AddSingleton<IConfigureOptions<DataProtectionCertificateOptions>>(services =>
        new ConfigureOptions<DataProtectionCertificateOptions>(options =>
        {
            options.Certificate = factory(services);
        }));
    builder.Services.AddSingleton<IConfigureOptions<KeyManagementOptions>>(services =>
    {
        var loggerFactory = services.GetService<ILoggerFactory>() ?? NullLoggerFactory.Instance;
        return new ConfigureOptions<KeyManagementOptions>(options =>
        {
            var certificateOptions = services.GetService<IOptions<DataProtectionCertificateOptions>>().Value;
            var certificate = certificateOptions.Certificate;
            options.XmlEncryptor = new CertificateXmlEncryptor(certificate, loggerFactory);
        });
    });
    builder.Services.AddSingleton<IConfigureOptions<XmlKeyDecryptionOptions>>(services =>
    {
        return new ConfigureOptions<XmlKeyDecryptionOptions>(options =>
        {
            var certificateOptions = services.GetService<IOptions<DataProtectionCertificateOptions>>().Value;
            var certificate = certificateOptions.Certificate;
            options.AddKeyDecryptionCertificate(certificate);
        });
    });
    return builder;
}

But I don't like the name DataProtectionCertificateOptions and that we are using this class to pass the value to other options.

Shazhko-Artem commented 3 years ago

There is another alternative way to implement this. This is like what @javiercn suggested. We can use the Dependency Injection functionality to call the factory once and provide the resulting certificate to the places where we configure the model.

The ValueBox<TValue> allows injecting value into the configure options.

internal class ValueBox<TValue>
{
    public ValueBox(TValue value)
    {
        Value = value;
    }

    public TValue Value { get; }
}

Configure KeyManagementOptionsand XmlKeyDecryptionOptions

public static IDataProtectionBuilder ProtectKeysWithCertificate(
    this IDataProtectionBuilder builder,
    Func<IServiceProvider, X509Certificate2> factory)
{
    // ...

    builder.Services.AddSingleton<ValueBox<X509Certificate2>>(services =>
        new ValueBox<X509Certificate2>(factory(services)));
        builder.Services.AddSingleton<IConfigureOptions<KeyManagementOptions>>(services =>
    {
        var loggerFactory = services.GetService<ILoggerFactory>() ?? NullLoggerFactory.Instance;
        return new ConfigureOptions<KeyManagementOptions>(options =>
        {
            var certificateBox = services.GetRequiredService<ValueBox<X509Certificate2>>();
            var certificate = certificateBox.Value;
            options.XmlEncryptor = new CertificateXmlEncryptor(certificate, loggerFactory);
        });
    });
    builder.Services.AddSingleton<IConfigureOptions<XmlKeyDecryptionOptions>>(services =>
    {
        return new ConfigureOptions<XmlKeyDecryptionOptions>(options =>
        {
            var certificateBox = services.GetRequiredService<ValueBox<X509Certificate2>>();
            var certificate = certificateBox.Value;
            options.AddKeyDecryptionCertificate(certificate);
        });
    });
    return builder;
}
mkarpuk commented 2 years ago

Simply making XmlKeyDecryptionOptions public will give us capabilities to setup services ourselves (instead of relying on helper methods). Currently it seems impossible...

ghost commented 1 year ago

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

ghost commented 8 months ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.