[feat] Azure.Extensions.AspNetCore.Configuration.Secrets load expired secrets in IConfiguration.

[sameernafdey]

Library name and version

Azure.Extensions.AspNetCore.Configuration.Secrets [1.2.2]

Describe the bug

ASP Net Core document for Key Vault Configuration is misleading when it says Disabled and Expired Secrets are excluded from configuration provider. However, in reality they are included.

So, if documentation is correct and I would argument that it is correct, then then the code shown below must be fixed.

KeyVaultSecretManager.cs#L74

  /// <summary>
  /// Checks if <see cref="KeyVaultSecret"/> value should be retrieved.
  /// </summary>
  /// <param name="secret">The <see cref="SecretProperties"/> instance.</param>
  /// <returns><code>true</code> if secrets value should be loaded, otherwise <code>false</code>.</returns>
  public virtual bool Load(SecretProperties secret)
  {
      return true;
  }

Details

Expected behavior

Let's say I have 3 secrets in my key vault:

1. SecretA - Expired
2. SecretB - No Expiration Set
3. SecretC - Expiring in Future.

I expected SecretB and SecretC should be loaded into IConfiguration but NOT SecretA because it has expiration date that is in past.

Actual behavior

All Secrets are loaded including expired secrets.

Reproduction Steps

Here is how I'm loading secrets in IConfiguration

   configurationBuilder.AddAzureKeyVault(
          new Uri("https://my-app-secrets-kv.vault.azure.net/"),
          new DefaultAzureCredential(),
          new AzureKeyVaultConfigurationOptions{
              ReloadInterval = TimeSpan.FromSeconds(30),
          }
      );

However I found a work around which help me exclude expired secret.

Created KeyVaultSecertManagerSkipsExpiredSecrets.cs to override the default Load method on KeyVaultSecretManager.

    public class KeyVaultSecertManagerSkipsExpiredSecrets : KeyVaultSecretManager
    {
        public override bool Load(SecretProperties secret)
        {
            return secret.ExpiresOn == null || secret.ExpiresOn >= DateTimeOffset.Now;
        }
    }

Then used it as follows:

   configurationBuilder.AddAzureKeyVault(
          new Uri("https://my-app-secrets-kv.vault.azure.net/"),
          new DefaultAzureCredential(),
          new AzureKeyVaultConfigurationOptions{
              ReloadInterval = TimeSpan.FromSeconds(30),
              Manager = new KeyVaultSecertManagerSkipsExpiredSecrets()
          }
      );

Please suggest if I am doing something wrong. What is expected behavior? Should the expired secrets not be excluded?

Environment

• ASP Net Core 3.1
• Service Fabric 7.2.477.9590 [This is irrelevant though]

[jsquire]

Hi @sameernafdey. Thank you for reaching out and we regret that you're experiencing difficulties. I'm not sure why the decision was made to return all secrets, but I agree that it does not seem like a helpful default behavior. I'm going to reach out to some folks who may have better insight into the history to plan next steps. Once I do, I'll provide an update here.

[heaths]

It's by design that SDK returns disabled and/or expired secrets, keys, and certificates. It's up to the caller to decide what to do with them. But, in this case, the configuration extension is the caller and, yes, I agree it probably makes sense to not return at least disabled secrets. Not sure we should elide expired secrets, though. Disabling is an explicit action, while expiration is not and could break untold numbers of services out there. Perhaps, instead, filtering out expired secrets - perhaps even disabled, just for safety - could be opt-in somehow.

[jelmer1]

Hi, I am not seeing this behavior for disabled secrets. When examining the secret, I can see that secret.Enabled is false. However, though the KeyVaultSecretManager() method returns true, when I look at my config collection for AzureKeyVaultConfigurationProvider, disabled secret keys are not in the collection. I did not investigate expired secrets and cannot speak to that.

[heaths]

I've opened https://github.com/dotnet/AspNetCore.Docs/issues/26714 to discuss changing the docs, but I do think we should return expired secrets. Just because they are expired doesn't mean they aren't needed.

At this point, though, I'm hesitant to change the implementation because it will be a breaking behavioral change.

[jsquire]

At this point, though, I'm hesitant to change the implementation because it will be a breaking behavioral change.

I agree that a breaking change should be avoided. My proposal would be to consider introducing an option for IncludeExpiredSecrets that defaults to true so the current behavior is preserved but can be changed without needing to write a custom secret manager.

[heaths]

Separate properties for disabled and expired should be added. Rarely would I expect disabled secrets to be needed, but there are plenty of scenarios that may still need expired secrets. As secrets (keys, etc.) rotate, those older ones are still need in many scenarios.

[heaths]

@KrzysztofCwalina @tg-msft what do you think about @jsquire's proposal, that I add IncludeExpiredSecrets and IncludeDisabledSecrets to AzureKeyVaultConfigurationOptions, which would look sometihng like this:

builder.Configuration.AddAzureKeyVault(
    new Uri($"https://{builder.Configuration["KeyVaultName"]}.vault.azure.net/"),
    new DefaultAzureCredential(),
    new AzureKeyVaultConfigurationOptions
    {
        IncludeDisabledSecrets = false,
        IncludeExpiredSecrets = false,
    });

These would be true by default. My concern here - though we can solve it with doc comments - is that it may not work if someone provides their own KeyVaultSecretManager, for which they can also accomplish this fairly easily now:

builder.Configuration.AddAzureKeyVault(
    new Uri($"https://{builder.Configuration["KeyVaultName"]}.vault.azure.net/"),
    new DefaultAzureCredential(),
    new AzureKeyVaultConfigurationOptions
    {
        Manager = new MyKeyVaultSecretManager(),
    });

// ...
class MyKeyVaultSecretManager : KeyVaultSecretManager
{
  public override bool Load(SecretProperties properties) =>
    properties.Enabled.HasValue &&
    properties.Enabled.Value &&
    properties.ExpiresOn.Hasvalue &&
    properties.ExpiresOn.Value > DateTimeOffset.Now;
}

Doc comments could simply indicate that if you set Manager and either/both of the other two, it's up to you to use them or not. But now we have two ways of accomplishing the same thing. Is this okay?

[tg-msft]

I'd be okay with either introducing the properties or a States enum like https://docs.microsoft.com/en-us/dotnet/api/azure.storage.blobs.models.blobstates?view=azure-dotnet and defaults that mirror today's semantics. Do we think there are a lot of customers deriving from the manager? If so, maybe we could solve that with an overload that called the default method and then did the filtering.

[heaths]

I'd be surprised if more than a handful of devs are extending the manager. The BlobStates isn't bad, but I was thinking more along the lines of DefaultAzureCredentialOptions. On that note, to have a false default, maybe I should use Exclude as a prefix like DACOptions does i.e., ExcludeDisabledSecrets = false and ExcludeExpiredSecrets = false by default.

[heaths]

If we make these changes, should update the docs referenced in dotnet/AspNetCore.Docs#26714

[heaths]

To note: expired and disabled secrets are listed from Key Vault but disabled secrets cannot be individually retrieved from Key Vault. The service does not allow it.

See my comments for details.

[pallavit]

@heaths, @jsquire I have not seen many usages of this, do you think this is something we should consider exposing? Also I am assuming this is owned by our team as I see this being updated mostly by our team members :).

[jsquire]

@pallavit: I think it's worth keeping as a backlog issue. We've got an approach that looks to have architect support and it's low effort. I'm hoping that we can pick this up in the current extensions sweep that we're working through.

[github-actions[bot]]

Hi @sameernafdey, we deeply appreciate your input into this project. Regrettably, this issue has remained unresolved for over 2 years and inactive for 30 days, leading us to the decision to close it. We've implemented this policy to maintain the relevance of our issue queue and facilitate easier navigation for new contributors. If you still believe this topic requires attention, please feel free to create a new issue, referencing this one. Thank you for your understanding and ongoing support.