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

Support azure blob storage auth with managed identity. #15753

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mariojsnunes
Copy link
Contributor

@mariojsnunes mariojsnunes commented Apr 14, 2024

Fixes: #12639

TODO:

  • Add docs

To test:

  1. Configure a Storage Account on Azure:
    a. Create a Storage Account
    b. Go to Configuration and disable "Allow storage account key access"
    c. Go to Access Control (IAM) and add the role Storage Blob Data Contributor to the account that you use on Visual Studio (Tools > Options > Azure Service Authentication > Account Selection).
  2. Update the following files on OrchardCore.Cms.Web:

Program.cs

builder.Services.AddAzureClients(clientBuilder =>
{
    clientBuilder.AddBlobServiceClient(new Uri("https://<account-name>.blob.core.windows.net")).WithName("ShellsBlobStorage");
    clientBuilder.UseCredential(new DefaultAzureCredential());
});

builder.Services
    .AddOrchardCms()
    .ConfigureServices(x => x.AddAzureClients(clientBuilder =>
    {
        clientBuilder.AddBlobServiceClient(new Uri("https://<account-name>.blob.core.windows.net")).WithName("MediaBlobStorage");
        clientBuilder.AddBlobServiceClient(new Uri("https://<account-name>.blob.core.windows.net")).WithName("DataProtectionBlobStorage");
        clientBuilder.UseCredential(new DefaultAzureCredential());
    }), -1)
    .AddAzureShellsConfiguration()
    .AddSetupFeatures("OrchardCore.AutoSetup");

appsettings.json

 "OrchardCore_DataProtection_Azure": {
   "ConnectionString": "", // Set to your Azure Storage account connection string.
   "AzureClientName": "DataProtectionBlobStorage", // Set to your Azure Client name.
   "ContainerName": "dataprotection", // Default to dataprotection. Templatable, refer docs.
   "BlobName": "", // Optional, defaults to Sites/tenant_name/DataProtectionKeys.xml. Templatable, refer docs.
   "CreateContainer": true // Creates the container during app startup if it does not already exist.
 },
  "OrchardCore_Media_Azure": {
   "ConnectionString": "", // Set to your Azure Storage account connection string.
   "AzureClientName": "MediaBlobStorage", // Set to your Azure Client name.
   "ContainerName": "testoc", // Set to the Azure Blob container name. Templatable, refer docs.
   "BasePath": "media", // Optionally, set to a path to store media in a subdirectory inside your container. Templatable, refer docs.
   "CreateContainer": true, // Activates an event to create the container if it does not already exist.
   "RemoveContainer": true // Whether the 'Container' is deleted if the tenant is removed, false by default.
 },
  "OrchardCore_Shells_Azure": {
   "ConnectionString": "", // Set to your Azure Storage account connection string.
   "AzureClientName": "ShellsBlobStorage", // Set to your Azure Client name.
   "ContainerName": "hostcontainer", // Set to the Azure Blob container name.
   //"BasePath": "some/base/path", // Optionally, set to a subdirectory inside your container.
   "MigrateFromFiles": true // Optionally, enable to migrate existing App_Data files to Blob automatically.
 },

@mariojsnunes
Copy link
Contributor Author

I'd add a new ManagedIdentityClientId property to MediaBlobStorageOptions and possibly BlobShellStorageOptions. Not BlobStorageOptions because the Azure Image Cache of ImageSharp doesn't support it

@Piedone I know you said this, but how would you instantiate the client at BlobFileStore without adding those properties to BlobStorageOptions? Maybe with a different approach?

@mariojsnunes
Copy link
Contributor Author

Some of the code is based on AzureAI module as per @MikeAlhayek suggestion.

@mariojsnunes
Copy link
Contributor Author

A few topics to be worked on:

  1. I realize now that would be better to do this with .AddAzureClients and IAzureClientFactory like the original PR. But where would .AddAzureClients be added? To projects that reference OrchardCore (and we mention in on the docs)?
  2. Should this be done for DataProtection too?

@Piedone Piedone marked this pull request as draft April 14, 2024 23:41
@Piedone
Copy link
Member

Piedone commented Apr 15, 2024

Please mark this PR ready for review, i.e. not draft, once it's not a WIP.

Yeah, indeed, you'll need to extend BlobStorageOptions for BlobFileStore. Under #15028 this will be an issue, since currently, it can be nicely used by all Blob storage-using code. For the two features in Media.Azure, an intermediate base class makes both of the have the exact same configuration options.

Since as mentioned, the Azure ImageSharp Image Cache only supports key-based configuration, this inheritance chain will need to be broken and the current classes copied, or some runtime exception added to make it fail if you try to configure managed identity for ImageSharp. I'm not sure which one is better (both are bad), but if that PR gets merged first, please implement something for this.

@mariojsnunes mariojsnunes marked this pull request as ready for review April 16, 2024 10:14
@mariojsnunes
Copy link
Contributor Author

I've added testing instructions to the first comment.

I'm not sure which one is better (both are bad)

I think we can simply not use the "AzureClientName" config on whichever module doesn't support it.
The main difference from the original PR is that I didn't create a Factory for BlobFileStore, so each module has full control over it.

@Piedone
Copy link
Member

Piedone commented Apr 16, 2024

OK then. So, this is ready then, minus documentation?

@mariojsnunes
Copy link
Contributor Author

OK then. So, this is ready then, minus documentation?

Yes, but I'd appreciate if someone else could test in case I overlooked something.

  • Media
  • DataProtection
  • Shells

Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to support multi-tenancy and configuration providers with every feature. So, this should also work with configuration coming solely from IShellConfiguration, thus supporting hierarchical configuration; we shouldn't have to configure anything in Program (having that option is good, but that being required doesn't match how Orchard operates).

I'm talking about adding URLs there. It's fine to have something like AddAzureManagedIdentity() to opt-in Iike AddAzureShellsConfiguration(), but the rest should happen internally and ultimately from IShellConfiguration.

Copy link

This pull request has merge conflicts. Please resolve those before requesting a review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to connect to Azure Blob storage with AAD authentication/managed identity
2 participants