OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.39k stars 2.38k forks source link

Update Azure Blob Storage to follow MS security best practices #16336

Open ShaneCourtrille opened 3 months ago

ShaneCourtrille commented 3 months ago

Is your feature request related to a problem? Please describe.

We are currently implementing Microsoft best practices for managing access to Azure Blob Storage and we've ran into a problem with the existing OC BlobFileStore.cs due to the fact we cannot easily override how the BlobContainerClient is created.

Describe the solution you'd like

OC BlobFileStore should follow MS best practices which require the usage of either a Microsoft Entra ID or User delegation SAS as a fallback for those who aren't using Entra.

Describe alternatives you've considered

Our current solution involves adding a new interface which allows us to control how the client is being created like this. This works well enough for those who are heavily customizing OC but doesn't seem like an appropriate solution for the general usage.

public BlobFileStore(IBlobStorageOptions options, IClock clock, IContentTypeProvider contentTypeProvider, IBlobContainerClientFactory blobContainerClientFactory)
        {
            _options = options;
            _clock = clock;
            _contentTypeProvider = contentTypeProvider;
            _blobContainer = blobContainerClientFactory.Create(_options);

            if (!String.IsNullOrEmpty(_options.BasePath))
            {
                _basePrefix = NormalizePrefix(_options.BasePath);
            }
        }
Piedone commented 3 months ago

Could you please follow the issue template? Otherwise, we'd need to have a conversation about the same questions :).

Piedone commented 3 months ago

Thanks! What exactly would be a solution for you? I.e. what needs to be coded? New configuration options for BlobFileStore? BTW you can override IMediaFileStore and use a custom one (if this is not what you already do in the code snippet).

github-actions[bot] commented 3 months ago

It seems that this issue didn't really move for quite a while despite us asking the author for further feedback. Is this something you'd like to revisit any time soon or should we close? Please reply.

ShaneCourtrille commented 2 months ago

@Piedone We are going to customize the code so that we can provide our own IBlobContainerClientFactory but it would be better if OC followed MS best practices by supporting Microsoft Entra ID or User delegation SAS token usage out of the box.

Piedone commented 2 months ago

OK, thanks, so we're talking about adding new configuration options.

sebastienros commented 2 months ago

This should be done for all Azure Services, ideally support all the auth schemes that are supported by each service. (Microsoft Entra ID, managed user id, connection string, ...)

github-actions[bot] commented 2 months ago

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

sebastienros commented 2 months ago

@MikeAlhayek really wants this to be consistent across features. Maybe we could have a common Authentication section (Azure SDK has a similar thing) that is bound for each feature, and configure the clients with these.