Runtime: Introduce ISecretStore

[tomkerkhove]

Is your feature request related to a problem? Please describe.

As of today; IConfiguration provides integration with Azure Key Vault which is by nature a secret store, not configuration information.

Describe the solution you'd like

I'd love to have an ISecretStore or ISecretProvider to provide a clear distinction between configuration & secret information so that it can be treated how they should be.

Too often I've seen when people get access to secrets while they are not aware of them or they do not correctly handle them by logging them, not providing transient handling of expired secrets and more.

By providing a seperation in interfaces this can be improved.

[davidfowl]

I'm not convinced we should make a parallel hierarchy of interfaces for this. Seems like lots of effort for very little gain. The secret provider would need all of the same features as the configuration provider.

[iamalexmang]

Respectfully, I agree with @tomkerkhove on this one: if I, the developer, am told that I treat fetching my data (irrelevant of its nature, secret of not) the same, one would argue that I pretty much shouldn’t care where I store it too.
After all, why don’t we all store secrets in a source-control-backed appsettings.json and make our lifes easy? ##ENDOFSARCASM
Here’s what I’m saying: secrets are to be audited. They require transient error handling. They require event-based fetching (when, for example, a certificate gets close to hitting its validity period). Additionally, outside the KeyVault word, fetching data from a secret store requires a extra-secret (consider a scenario where Managed Identities don’t exist), which in turn is an operation which has to be carefully audited.
Configuration fetching on the other hand, doesn’t.
Unless we have a clear separation between secrets vs. plain config, this argument might later turn into an argument of storing secrets in App Config, rather than KeyVault.

[shahiddev]

Agree with @tomkerkhove we should make a very clear distinction between secrets and configuration. At the moment it's too easy to blur the distinction and although there may be a lot of similarities in the way we interact with config vs secrets they shouldn't be treated as the same type of data.

[davidfowl]

I'm still not convinced, but to move the discussion forward I'd suggest a couple of things:

• Describing the problem being solved with concrete examples (with code) showing the bad usage today and how this new API solves the problem.
• Describe the process for how customers are supposed to decide when you use this new API vs IConfiguration (essentially write the usage documentation for the feature).
• Writing up a full API proposal for the new API including the abstraction and a couple of implementations.
  
  
  
  • Describing how this API interacts with things like IOptions<T> and change notification.
  
  

Too often I've seen when people get access to secrets while they are not aware of them or they do not correctly handle them by logging them, not providing transient handling of expired secrets and more.

That's a great example, I would elaborate how this solves that problem and how consuming APIs would handle secrets (does ILogger have to understand the concept of a secret?).

[tomkerkhove]

What I'm doing on projects is introduce ISecretProvider (but ISecretStore might be better) next to IConfiguration so that it allows us to inject both where need be.

If we need to access configuration, they use IConfiguration; for secrets they have to use ISecretProvider which is async for all operations because most of the times they go fetch them somewhere else across the network and require memory caching.

With IConfiguration it's simple simple to add different providers and the order of them are important. If you screw them up, you'll end up poking Azure Key Vault, before checking local configuration first, for a basic configuration change. The issue? Azure Key Vault has low rate limiting, adds latency, etc.

Other than that, it provides explicitness around where it comes from; when using IConfiguration it can come from any provider but with ISecretProvider it's clear it's coming from a secret store that's been added which makes it more explicit and narrows the scope.

What would be the best way to write the proposal @davidfowl? Do you have an example available somewhere? Happy to fill this in.

Note - There is a place for a ConfigurationSecretStore, ie local dev, which uses IConfiguration but that does not mean that they have to be the same.

[davidfowl]

What would be the best way to write the proposal @davidfowl? Do you have an example available somewhere? Happy to fill this in.

There's no template but I would like you to address each of the concerns I called out concretely as I think it'll help me and others better understand the benefits you see (like making the secret API async, and what providers make sense for it and how change notification works if any). The explicitness of the API isn't enough to introduce a new abstraction, the cost is high and we have to reconcile with out existing tech and guidance. That needs to be part of this proposal.

[tomkerkhove]

What it looks like today

public static IHostBuilder CreateHostBuilder(string[] args) =>
    Host.CreateDefaultBuilder(args)
        .ConfigureAppConfiguration((context, config) =>
        {
            if (context.HostingEnvironment.IsProduction())
            {
                var builtConfig = config.Build();

                var azureServiceTokenProvider = new AzureServiceTokenProvider();
                var keyVaultClient = new KeyVaultClient(
                    new KeyVaultClient.AuthenticationCallback(
                        azureServiceTokenProvider.KeyVaultTokenCallback));

                config.AddAzureKeyVault(
                    $"https://{builtConfig["KeyVaultName"]}.vault.azure.net/",
                    keyVaultClient,
                    new DefaultKeyVaultSecretManager());
            }

            config.AddEnvironmentVariables();
        })
        .ConfigureWebHostDefaults(webBuilder =>
        {
            webBuilder.UseStartup<Startup>();
        });

Proposal

Introduce ISecretStore which leverages functionality similar to IConfiguration but purely focussed on secrets:

• It is async for all operations since most secret stores are externally
• It does not need hierarchical support
• Early stage it feels like support for IOptions is not needed as you'd like to have them on a per-secret basis, but can see potential scenarios for it
• Change notifications are a bit more tricky given they mainly depend on external systems, but services such as Key Vault support event grid events.
  
  
  
  • This allows you to asynchronously update cached secrets, see Arcus Background Jobs but this would obviously not be something that's part of .NET but rather community / Azure NuGet package
  
  

I've started a small example (which doesn't work but it compiles) with a proposal of "API design" - https://github.com/tomkerkhove/dotnet-secretstore

In terms of secret providers I'm thinking:

• Azure Key Vault
• HashiCorp Vault
• Google Key Management
• AWS Secrets Manager
• Configuration - Used for local development / when no other option
• Raw file - Secrets tend to be persisted in Kubernetes

Configuration

Secret stores can be configured via ConfigureSecretStore which feels similar to ConfigureAppConfiguration allowing you to add providers and gives access to configuration:

public static IHostBuilder CreateHostBuilder(string[] args) =>
    Host.CreateDefaultBuilder(args)
        .ConfigureAppConfiguration((context, config) =>
        {
            config.AddEnvironmentVariables();
        })
        .ConfigureSecretStore((context, config, secretStores) =>
        {
            if (context.HostingEnvironment.IsProduction())
            {
                var keyVaultName = configuration["KeyVaultName"];
                secretStores.AddAzureKeyVault(keyVaultName);
            }
            else
            {
                secretStores.AddConfiguration();
            }
        })
        .ConfigureWebHostDefaults(webBuilder =>
        {
            webBuilder.UseStartup<Startup>();
        });

Usage

You can use DI to get ISecretStore and simply request to get a secret value.

[ApiController]
[Route("[controller]")]
public class WeatherForecastController : ControllerBase
{
    private readonly IConfiguration _configuration;
    private readonly ISecretStore _secretStore;

    public WeatherForecastController(IConfiguration configuration, ISecretStore secretStore)
    {
        _configuration = configuration;
        _secretStore = secretStore;
    }

    [HttpGet]
    public async Task<IActionResult> Post(WeatherForecast weatherForecast)
    {
        var apiUrl = _configuration["api:url"];

        // You should not be doing this but it's just to prove the point
        HttpClient httpClient = new HttpClient
        {
            BaseAddress = new Uri(apiUrl)
        };

        var apiKey = await _secretStore.Get("APIs-Weather-Key");
        httpClient.DefaultRequestHeaders.Add("x-api-key", apiKey);

        var rawForecast = JsonConvert.SerializeObject(weatherForecast);
        await httpClient.PostAsync("/weather/report", new StringContent(rawForecast));

        return Ok();
    }
}

Hope this helps illustrate (some of) your questions.

[davidfowl]

Thanks @tomkerkhove! I have to say, I'm still not convinced on the value here besides having a new API with an async and pull based:

It is async for all operations since most secret stores are externally

It does not need hierarchical support

Why not?

Early stage it feels like support for IOptions is not needed as you'd like to have them on a per-secret basis, but can see potential scenarios for it

How would that complicate the interface? Options is all synchronous today. It seems like this would require options to be async also making an outbound call per key to do options binding. This also seems to require hierarchy in secret retrieval to bind complex objects.

Change notifications are a bit more tricky given they mainly depend on external systems, but services such as Key Vault support event grid events.

The IConfiguration interface supports this today and it's how changes are plumbed into the system. It seems like this is trying to change the paradigm where the backing store is queried everytime a key is accessed (pull vs push model).

Azure Key Vault
HashiCorp Vault
Google Key Management
AWS Secrets Manager
Configuration - Used for local development / when no other option
Raw file - Secrets tend to be persisted in Kubernetes

All of these already have configuration providers.

Making it async also has lots of follow on effects:

• ConfigureServices is synchronous today, this would force it to be synchronous.

In your sample it seems like you're also proposing that we change the hosting APIs to integrate secrets as well as configuration.

I don't see why you'd use this instead of the configuration API.

cc @rynowak @DamianEdwards @glennc for other opinions.

[rynowak]

It is async for all operations since most secret stores are externally

I can think of a bunch of problems this introduces.

Imagine something like configuring an HttpClient with an access token. Or configuring some other kind of client to use a secret. Now DI has to support async factory methods, or you just have a bunch of async client factories.

I don't see what's so problematic about sync APIs. Sure, they don't reflect how the underlying system functions, but they do reflect an easy way to consume data.

Even if you made the secrets APIs async, would you pull the latest value each time its accessed? It's likely you'd end up with a background hosted service that refreshes secrets (and all of the complexity of doing this synchronously).

This topic comes up often in the context on config, because there are async config providers too. We've never had a compelling reason to do it. When you make a decision like this (config is synchronous), its easy to point out that there some limitations, but that doesn't make it the wrong decision.

---

I think I'm with David in not seeing how this solves the problems that are being put forth.

Too often I've seen when people get access to secrets while they are not aware of them or they do not correctly handle them by logging them

This could potentially be addressed by just not dumping all of your configuration to logging 😆. That's a glib reply I know, and people make mistakes. I think most of the teams that are internal to microsoft that we talk to take the opposite approach to this - they have auditing and validation around what gets logged.

I think fundamentally what you're asking here is that there's a type (that isn't string) that doesn't tostring to the underlying secret value right? If think we can all agree that there if there were a data type that represents a configuration/secret value that requires some work to convert to a string, it would be useful in preventing accidental disclosure -- example from Blazor. Therefore an API proposal that returns strings probably doesn't go far enough if the problem is worth solving. As soon as you have a "helper method" the context that you got the data from the secret store is lost - types are the solution that .NET provides to this.

I think there's some flexibility to try and address this from within the configuration API surface as well. We can't break all of the stuff that's there but you could image a new set of APIs on IConfiguration that allow you to read secrets. A recipe might look like:

• New methods on IConfiguration for interacting with secrets (it doesn't return strings)
• A way for a configuration provider to say "I have secrets" - possibly a new optional interface
• A way for the person doing configuration to choose what mode they want a configuration provider to operate in - "regular mode" or "secrets mode" (this is the most contraversial and hardest thing)

If a configuration provider is operating in "secrets mode" it doesn't map keys directly to values - it maps to something structured - maybe 3-4 keys total, and the the Secret instead can be constructed from those.

You could get really tricky with this.

{
    "secretvalue": "actual secret value"
}

Behaves as-if you had

{
    "secretvalue": {
        "expiry": "..."
        "key": "38484a-...rest of the guid",
        "38484a-...rest of the guid": "actual secret value"
    }
}

The use of a guid for indirection here makes it so that this data can be part of the configuration system, while making it impossible to write code that bypasses the GetSecretValue code path to read the value (without duplicating its logic).

not providing transient handling of expired secrets and more.

Your proposal does not address that.

[davidfowl]

The idea of a secret as a first class concept has legs. Lots of teams have been talking to us about this internally at Microsoft as well (to avoid logging things like PII) and it’s worth considering but it seems separate from the proposal here.

[tomkerkhove]

Frankly, I think you're comparing it too much with IConfiguration and you should think about it without thinking what IConfiguration provides today.

ISecretStore will be different and have different needs. Having support for the suggested providers in IConfiguration doesn't mean that we should stick with that imo.

It does not need hierarchical support

Why not?

Because, AFAIK, secret stores don't support this.

Even if you made the secrets APIs async, would you pull the latest value each time its accessed? It's likely you'd end up with a background hosted service that refreshes secrets (and all of the complexity of doing this synchronously).

That can be done but should have the flexibility to make that decision yourself; and add caching if you want to.

This could potentially be addressed by just not dumping all of your configuration to logging 😆. That's a glib reply I know, and people make mistakes. I think most of the teams that are internal to microsoft that we talk to take the opposite approach to this - they have auditing and validation around what gets logged.

I think fundamentally what you're asking here is that there's a type (that isn't string) that doesn't tostring to the underlying secret value right?

Frankly, I think the secrets are mainly raw strings coming and you should not rely on developers to do the right thing with IConfiguration. The provider approach is very good because it's flexible and you don't care where it comes from, but from the consumer side this is also a challenge.

Fixing the logging issue will be reduced if you introduce ISecretStore.

Frankly, I'm not sure why it would be much of an issue to introduce ISecretStore which is similar to IConfiguration but not identical. The concept of the providers would be the same and how you wire it up but for the rest they are not the same.

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. Please help me learn by adding exactly one area label.

[tomkerkhove]

For those interested, you can now use Arcus Secret Store (read more in our blog post).

Get started with:

PM > Install-Package Arcus.Security.Core

_This is obviously a first version and we are open for feedback_

[maryamariyan]

Based on the discussions that @rynowak and @davidfowl raised closing this issue. Please feel free to reopen if you think we should reconsider this or have a further discussion