Error Suppression and Vault Name Mapping

[BCoskun]

Describe the proposal

We are working on Azure Key Vault in our project and notice that there is no error suppression functionality (or not granular enough for certain use case) when there the Key Vault Secret name doesn't have match with application configuration name.

For example;
We couldn't define .NET configuration hierarchy in Azure KeyVault due to naming convention limitation for secret names. Key Vault only accepts dash and Alphanumeric values for the names so we can't use "ConnectionStrings:DatabaseConnStr" or "ConnectionStrings__DatabaseConnStr" as name.

Purposed solution
Allocate special metadata prefix like "Dapr." in metadata keys and use them to control library behavior.

Example Solution in Library

LoadAsync()  method In  DaprSecretStoreConfigurationProvider.cs file
...

    if (secretDescriptors != null)
            {
                foreach (var secretDescriptor in secretDescriptors)
                {
                    var vaultSecretName = String.Empty; 
                        secretDescriptor.Metadata.TryGetValue("Dapr.VaultMapping.Name", out vaultSecretName);

                        if (!String.IsNullOrWhiteSpace(vaultSecretName))
                            secretName = vaultSecretName;

                        Dictionary<string, string> result =  new Dictionary<string, string>();
                        
                        try
                        {
                            result = await client
                                .GetSecretAsync(store, secretName, secretDescriptor.Metadata)
                                .ConfigureAwait(false);
                        }
                        catch (DaprException e)
                        {
                            string? ensure_exist;
                            secretDescriptor.Metadata.TryGetValue("Dapr.Ensure", out ensure_exist);
                            if (String.IsNullOrWhiteSpace(ensure_exist))
                                ensure_exist = "false";
                            
                            if (Convert.ToBoolean(ensure_exist))
                            {
                                throw e;
                            }
                            
                            Console.WriteLine($"'{secretName}' doesn't exists in Key Vault but because it doesn't require existence so all errors suppressed!");
                           result = new Dictionary<string, string>();

                        }

                        foreach (var key in result.Keys)
                        {
                            if (data.ContainsKey(key))
                            {
                                throw new InvalidOperationException(
                                    $"A duplicate key '{key}' was found in the secret store '{store}'. Please remove any duplicates from your secret store.");
                            }
                            
                            data.Add(normalizeKey ? NormalizeKey(secretDescriptor.SecretName) : secretDescriptor.SecretName, result[key]);
                        }

...

Example usage in Program.cs

var daprClientBuilder = new DaprClientBuilder();
var daprClient = daprClientBuilder.Build();

var secretDescriptors = new List<DaprSecretDescriptor>
{

// Mapped with Vault name with old exception behavior. 
// The Secret name should exist on Keyvault otherwise it will throw the exception. 

    new DaprSecretDescriptor("ConnectionStrings:DatabaseConnStr", new Dictionary<string, string>()
    {
        {"Dapr.Ensure","true"},
        {"Dapr.VaultMapping.Name", "Microsservice-DatabaseConnStr"}
    }),

// The exception will be suppressed
    new DaprSecretDescriptor("ConnectionStrings:StorageLocalFolder",new Dictionary<string, string>()
    {
        {"Dapr.Ensure","false"},
        {"Dapr.VaultMapping.Name", "Object-Storage-ContainerName-CfgNotExists"}
    }),

// Old Behavior     
    new DaprSecretDescriptor("Old-Beahvior-StorageAzureConnectionString")
    
};

IConfiguration config = new ConfigurationBuilder()
    .AddJsonFile("appsettings.json")
    .AddDaprSecretStore("azurekeyvault",  secretDescriptors, daprClient, TimeSpan.FromSeconds(10) )
    .AddEnvironmentVariables()
    .Build();

var connectionString = config.GetConnectionString("DatabaseConnStr");
var fileStorageConnectionString = config.GetConnectionString("StorageLocalFolder");
var azureStorageConnectionString = config.GetConfig("Old-Beahvior-StorageAzureConnectionString");

[halspang]

@BCoskun - Thanks for opening this issue!

Just to clarify for myself, what we're trying to do here is:

1. Allow secrets which can't be stored in the .NET hierarchy pattern to still be referenced in the code that way. We're doing this by providing a secondary key that will actually be used to query the secretstore.
2. Allow certain keys to not exist, also based on a specific key.

I can see the value of this, but I don't know if I agree with using the reserved metadata values. The metadata here is actually sent to the secretstore so instead I'd rather just add the direct fields to the DaprSecretDescriptor. Perhaps something like this?

/// <summary>
/// Gets or sets the secret name.
/// </summary>
public string SecretName { get; }

/// <summary>
/// A collection of metadata key-value pairs that will be provided to the secret store. The valid metadata keys and values are determined by the type of secret store used.
/// </summary>
public IReadOnlyDictionary<string, string> Metadata { get; }

public bool IsRequired { get; }

public string SecretKey { get; }

/// <summary>
/// Secret Descriptor Construcutor
/// </summary>
public DaprSecretDescriptor(string secretName) : this(secretName, new Dictionary<string, string>(), true, secretName)
{

}

public DaprSecretDescriptor(string secretName, IReadOnlyDictionary<string, string> metadata, bool isRequired, string secretKey)
{
...
}

Thoughts?

[BCoskun]

This seems more elegant solution for the required feature.

This simplifies to LoadAsync Method as well;

            if (secretDescriptors != null)
            {
                foreach (var secretDescriptor in secretDescriptors)
                {
                        
                        Dictionary<string, string> result =  new Dictionary<string, string>();
                        
                        try
                        {
                            result = await client
                                .GetSecretAsync(store, secretDescriptor.SecretKey, secretDescriptor.Metadata)
                                .ConfigureAwait(false);
                        }
                        catch (DaprException e)
                        {
                            
                            if (secretDescriptor.IsRequired)
                            {
                                throw e;
                            }
                            
                            Console.WriteLine($"'{secretDescriptor.SecretName}' doesn't exists in Key Vault but because it doesn't require existence so all errors suppressed!");

                            result = new Dictionary<string, string>();
                        }

                        foreach (var key in result.Keys)
                        {
                            if (data.ContainsKey(key))
                            {
                                throw new InvalidOperationException(
                                    $"A duplicate key '{key}' was found in the secret store '{store}'. Please remove any duplicates from your secret store.");
                            }
                            
                            data.Add(normalizeKey ? NormalizeKey(secretDescriptor.SecretName) : secretDescriptor.SecretName, result[key]);
                        }
                }

                Data = data;
            }

The Program.cs will change as

var secretDescriptors = new List<DaprSecretDescriptor>
{

// Mapped with Vault name with old exception behavior. 
// The Secret name should exist on Keyvault otherwise it will throw the exception. 

    new DaprSecretDescriptor("ConnectionStrings:DatabaseConnStr", new Dictionary<string, string>(), 
true, "Microsservice-DatabaseConnStr"),

// The exception will be suppressed
    new DaprSecretDescriptor("ConnectionStrings:StorageLocalFolder",new Dictionary<string, string>(), 
false, "Object-Storage-ContainerName-CfgNotExists"),

// Old Behavior     
    new DaprSecretDescriptor("Old-Beahvior-StorageAzureConnectionString")
    
};

IConfiguration config = new ConfigurationBuilder()
    .AddJsonFile("appsettings.json")
    .AddDaprSecretStore("azurekeyvault",  secretDescriptors, daprClient, TimeSpan.FromSeconds(10) )
    .AddEnvironmentVariables()
    .Build();

var connectionString = config.GetConnectionString("DatabaseConnStr");
var fileStorageConnectionString = config.GetConnectionString("StorageLocalFolder");
var azureStorageConnectionString = config.GetConfig("Old-Beahvior-StorageAzureConnectionString");

[yash-nisar]

/assign

[yash-nisar]

@BCoskun I'm not sure if I understand why we need this change.

In https://learn.microsoft.com/en-us/aspnet/core/security/key-vault-configuration?view=aspnetcore-7.0#secret-storage-in-the-production-environment-with-azure-key-vault, I see that:

Hierarchical values (configuration sections) use -- (two dashes) as a delimiter

Will this help in any way ? What exactly are you trying to accomplish ?

What do you mean by "Key Vault Secret name doesn't have match with application configuration name" ? Are you talking about Azure App Configuration ?

In the draft PR, we are potentially just querying the secret store with a different key. Also, in this case:

new DaprSecretDescriptor("ConnectionStrings:DatabaseConnStr", new Dictionary<string, string>(), 
true, "Microsservice-DatabaseConnStr")

Are you fetching var connectionString = config.GetConnectionString("DatabaseConnStr"); from the DaprSecretDescriptor ? If yes, I'm not sure where we are storing it.

[yash-nisar]

Also, I think Azure Key Vault doesn't allow in key vault secret names by definition so I strongly feel it would be up to the client to bridge the gap of parity between the key vault secret name and the application configuration name. The SDK/Dapr is just a means of querying the Azure Key Vault and fetching the secret.

[BCoskun]

Hi @yash-nisar ,

One of the main issue was SDK was not allowing Dapr Secret Block to retrieve the secret(s) without throwing exception when the some secret(s) not exists on the vault.

This proposal offering way get list of secrets that some of them might not be exists in vault without throwing exception.

Also with this change we are giving way to retrieve the secret and map the name as developer configured DaprSecretDescriptor so that he/she don't need to re-map the values again. Well, I understand you have some strong feeling about mapping is responsibility for client but from my point of view the developer explicitly defining this mapping on configuration and additional functions could help developer for these type of extra works.

In our case we were storing secrets appliction.json files and when we start using Dapr we had to add additional mapping section because Azure Key Vault was not supporting ":" character.

[yash-nisar]

@BCoskun Thanks for the prompt response.

Can you explain what do you mean by "we are giving way to retrieve the secret and map the name as developer configured DaprSecretDescriptor so that he/she don't need to re-map the values again" ?

[BCoskun]

@yash-nisar ,

Sorry I was keep updating my original answer above. Let me know if that is not converging your question.

[yash-nisar]

@BCoskun Also, with regards to throwing an exception if the secret is not found is expected. See the responses here: https://docs.dapr.io/reference/api/secrets_api/#response-codes

I think it should be up to the client to make sure they're querying with the correct secret name. I'm trying to understand the hurdles that you are facing while implementing this.

[BCoskun]

@yash-nisar,

I understand that is expected from DAPR API but my interpration of "204 Secret not found" might not be exception. especially for multiple secret(s) retrieval. In my mind we don't need to throw exception for Secret not found or allow the developer to control the behavior, Another point is DAPR API doesn't have multiple secret retrival functionality but SDK has it so we are providing more functionality in SDK which means they are not functionally map 1 to 1 and SDK doesn't allow us gracefully handle errors for retrieving multiple secrets from the vault.

Having said that I think this is matter of taste and style.

If this explanation is not good enough we can close the proposal.

Thanks.

[yash-nisar]

@BCoskun

We do have an option for multiple secret retrievals in dapr (https://docs.dapr.io/reference/api/secrets_api/#get-bulk-secret) and the dotnet-sdk (https://github.com/dapr/dotnet-sdk/blob/master/src/Dapr.Client/DaprClientGrpc.cs#L1253) if that's what you are looking for.

There can never be a feature that exists in the SDK but not in the Dapr runtime because the SDK makes calls to the runtime APIs itself via HTTP/GPRC.

As for gracefully handling errors, how about including this block in a try catch block itself ?

var result = await client.GetSecretAsync(store, SecretName, Metadata).ConfigureAwait(false);

[BCoskun]

Thanks for your feedback.

[jamesmcroft]

@yash-nisar is this enhancement still expected to be worked on?

I feel there are valid scenarios where you want to have a nullable configuration value where by if it does not exist in vault, then ignore it instead of throwing an exception.

[yash-nisar]

Hey @jamesmcroft, sorry I have been involved in other high priority stuff. I am happy to hand it over to whoever wants to work on it.

[jamesmcroft]

@yash-nisar I can look at finalizing this from your previous PR

[jamesmcroft]

@yash-nisar thanks for supporting on the original work for this issue. This has now been merged in with additional changes to finish up the previous PR you opened. #1051 can now be closed also.

Looking forward to taking advantage of this change in a future release of the SDK, and I hope that @BCoskun and others find this useful also.