Add new module for OrchardCore.KeyVault.Azure

[JoshLefebvre]

Based on conversations had in #6359

[JoshLefebvre]

If this looks good, I would be happy to add some further documentation on how to use

[deanmarcussen]

Looks great, will take a closer look tomorrow if @jtkech doesn't tonight

• 1 for some docs :)

[jtkech]

Otherwise LGTM

[JoshLefebvre]

Thanks @jtkech. Made all above requested changes and added the documentation.

[jtkech]

@JoshLefebvre perfect i approved it

Just one last thing, maybe not the right place for the readme as OrchardCore.KeyVault.Azure is not a module but a core library. It does not bother me but i don't often work on documentation, i'm too lazy ;) so we need a confirmation, @agriffard can you confirm?

[deanmarcussen]

Looks good @JoshLefebvre

I fixed a couple of typos in the docs.

Can you move it to the core section as @jtkech suggested, and also it needs an entry in the mkdocs.yml or it won't display on readthedocs

[xperiandri]

Is it ready to merge?

[sebastienros]

Can we have a small example that uses directly AzureKeyVaultSecretManager such that standard ASP.NET tutorials can be followrd?

[JoshLefebvre]

Do you mean an example of IConfiguration? I have been using it to retrieve my media and database connection strings. For example in DatabaseShellConfigurationSources.cs:

Using the following Keys:
OrchardCore--OrchardCore---Shells---Database--ConnectionString
OrchardCore--OrchardCore---Shells---Database--DatabaseProvider (Not actually a secret, but required to get the section)

[deanmarcussen]

What Seb was asking for is something that matches the getting started guides from the ASP.NET docs here https://docs.microsoft.com/en-us/aspnet/core/security/key-vault-configuration?view=aspnetcore-3.1

Where they are using the AzureServiceTokenProvider callbacks and such.

Which makes me also wonder if we should not be using the callback technique, rather than having our own configuration secrets for it?

[jtkech]

@deanmarcussen

Here @JoshLefebvre just override the DefaultKeyVaultSecretManager with a new AzureKeyVaultSecretManager and provide an AddOrchardCoreAzureKeyVault() extension that uses it and other config values.

But people can just follow the aspnet documentation and then use new AzureKeyVaultSecretManager() in place of new DefaultKeyVaultSecretManager()

So maybe just remove the AddOrchardCoreAzureKeyVault() that is just an example on how to use the new AzureKeyVaultSecretManager and let people follow the doc and do their own extension if they want.

[xperiandri]

Do I understand your implementation right that you have added replacement of --- to _ along with default -- to : behavior?

[jtkech]

@xperiandri Exactly, this is what @JoshLefebvre did ;)

[sebastienros]

Is that something that is common? Either in OC or ASP.NET with Keyvault?

[xperiandri]

Cool, so is only getting started guide left?

[agriffard]

Please resolve conflict.

[MarcBruins]

The preferred way of communicating with the Azure Keyvault for a web app is through Managed identity rather then a service principal with a object id and password. Can we make sure that that scenario is also supported? I can work on this if you like

[Skrypt]

@MarcBruins I think that ultimately we should have an external OC Management tool that could be deployed as an Azure Function that would allow to configure all things related with the Azure Portal services. Here, what I don't like is the fact that OC configures it's own Azure Services. This can easily lead to security issues.

Also, I think that it could be a good way to start experiment things with the new Azure SDK.

[agriffard]

Can you please fix the conflicts @JoshLefebvre?

[JoshLefebvre]

@agriffard yes sorry for the hiatus. I have a lot more free time now and hope to actively start contributing to this project again. I will try to get this done today

[JoshLefebvre]

@deanmarcussen @agriffard so I've updated the Key Vault package to make use of the new Azure SDK package Azure.Extensions.AspNetCore.Configuration.Secrets to connect to keyvault. I have also added the Azure.Identity package which is now the recommended way to authenticate with Azure resources via Active Directory token authentication and should resolve the issues people were mentioning above .

Azure Identity supports:

• Service principal authentication
• Managed identity authentication
• User principal authentication

[agriffard]

Can you please review it @deanmarcussen ?

[deanmarcussen]

Great, thanks for updating this @JoshLefebvre - they've definitely improved the libraries a lot since moving to the new Azure packages.

Couple of thoughts, and I will bring @jtkech in here as well, as he may agree / disagree

Because this is a program level configuration, I don't think it should be using the OrchardCore appsettings section. That would suggest it is configurable at a tenant level, but as it is wholly for the host, it isn't tenant configurable. (please correct me if I'm wrong)

I'm also thinking it's not a module. Because we can't load it modularly, for the same reasons.

Which makes it more like the Shells.Azure configuration, which we did as a seperate nuget package, as part of the OrchardCore folder, rather than as part of modules.

Lastly we need to pick a name. Because it's KeyVault Configuration - rather than KeyVault itself, which would have a read/write option, and we might be using it for read / write (i.e. it'd be good to do a Shells.KeyVault version at some point with read/write, and I'm hoping to do a Secrets KeyVault provider for the Secrets module #7891, again for read/write). Both of those might connection to a different KeyVault - in the case of secrets, it might be a different vault for each tenant (as an option).

Azure have called it Azure.Extensions.AspNetCore.Configuration.Secrets - which is not helpful ;)

Would OrchardCore.Configuration.KeyVault be a good name?
or
OrchardCore.Configuration.Azure (I prefer KeyVault as it seems explicit and more obvious, but naming is hard)

[jtkech]

@deanmarcussen okay i will take a look on it this night

[jtkech]

@deanmarcussen

Yes i agree, didn't see it was under the OC.Modules folder, need to be under the OrchardCore folder.

Then yes, good to have custom config / settings sources for all tenants and tenant specific, as we did from blob and database, but yes in a separate PR. So we already have OrchardCore_Shells_Azure, maybe rename it to OrchardCore_Shells_Azure_Blob so that we will be able to use OrchardCore_Shells_Azure_KeyVault. But that's okay to keep OrchardCore_Shells_Azure and then use OrchardCore_Shells_KeyVault ;)

Meanwhile for this PR, idem here i would suggest OrchardCore.Configuration.Azure.KeyVault, but maybe too long, in our solution there is no project with more than 3 terms, so i also suggest OrchardCore.Configuration.KeyVault.

[agriffard]

Please merge dev and solve appsettings.json conflict.

[deanmarcussen]

@JoshLefebvre sounds like @jtkech and I agree on the name, and to move it out of modules.

I don't think it should be using the OrchardCore appsettings section

I withdraw my comment here, we already hosted the shells settings under OrchardCore so for consistency lets keep that the same. Even if it is only host only, not tenant based.

So just to move the folder, and rename thank you

[jtkech]

I agree

[JoshLefebvre]

Thanks @jtkech and @deanmarcussen. Yes agreed, makes much more sense in the OrchardCore module now that you explain it. I've also updated the name to OrchardCore.Configuration.KeyVault as suggested :)

[agriffard]

Build retry

[jtkech]

LGTM

[agriffard]

Thank you. 163rd project in the solution.