Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"application/json" content type is not handled in KeyVault references #78

Open
vputsenko opened this issue Jul 3, 2024 · 6 comments
Open
Assignees
Labels
enhancement New feature or request

Comments

@vputsenko
Copy link

Internally AzureKeyVaultKeyValueAdapter doesn't handle ContentType property of secret value and returns naked value.

So we have dual behavior when same JSON setting stored directly or using keyvault reference with application/json content type returned as object or string.

I think problem seats at this peace of AzureKeyVaultKeyValueAdapter code
if (client) { // TODO: what if error occurs when reading a key vault value? Now it breaks the whole load. const secret = await client.getSecret(secretName, { version }); return [setting.key, secret.value]; // here it returns raw value without considering secret.properties.contentType value }

Thank you,

@Eskibear Eskibear added the bug Something isn't working label Jul 4, 2024
@Eskibear Eskibear self-assigned this Jul 4, 2024
@Eskibear
Copy link
Member

Eskibear commented Jul 4, 2024

Good catch, it's a bug. Secret value is directly returned without being processed by JSON adapter. Will fix it soon.

@Eskibear
Copy link
Member

Eskibear commented Jul 5, 2024

@avanigupta
AFAIK dotnet provider doesn't honor content type of a key vault secret either, and it flattens JSON into configuration entries. This use case is valid for JavaScript, not sure if it also applies to dotnet and if we want to support this in dotnet provider.

@Eskibear Eskibear added enhancement New feature or request and removed bug Something isn't working labels Jul 9, 2024
@Eskibear
Copy link
Member

Eskibear commented Jul 9, 2024

Besides content type, there are also some other properties we might honor, activation date and expiration date. probably we should do some verification, not to return the value (or throw an exception) if current date is not within the range [activation, expiration].

@Eskibear
Copy link
Member

@vputsenko
It's still under discussion with some concerns. So I closed the quickfix PR in favor of a general solution for similar use cases.

Currently you can use KeyVaultOptions.secretResolver to resolve the secret reference with customized logic as a workaround.

    /**
     * Specifies the callback used to resolve key vault references that have no applied SecretClient.
     * @param keyVaultReference The Key Vault reference to resolve.
     * @returns The secret value.
     */
    secretResolver?: (keyVaultReference: URL) => string | Promise<string>;

Note: Here I think secretResolver should return known type instead of string, as we allow values of the config map to be any type.

e.g.

const settings = await load(connectionString, {
    keyVaultOptions: {
        secretResolver: (kvrUrl) => {
            // create a secret client to fetch secret value from Azure Key Vault
            const secretClient = new SecretClient(kvrUrl, new DefaultAzureCredential());
            const secret = secretClient.getSecret(secretName);
            // an example to parse JSON for specific content type, can be more robust to deal with complicated MIME types
            if (secret.properties.contentType === "application/json") {
                return JSON.parse(secret.value) ;
            } else {
                return secret.value;
            }
        }
    }
});

@vputsenko
Copy link
Author

Hi, i understand that .NET impl has different handling for JSON secrets, but also we need to answer question "How contentType of keyvault secret should be used then ?)" I thought that its direct role to tell how data should be interpreted ... I understand that fix should be common for all platforms, may be we can add some flag for .NET client to have this behavior there too ? I would say more that flattening is not always desired behavior too.

@Eskibear
Copy link
Member

@zhiyuanliang-ms it's on your radar now ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants