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

[AVM Question/Feedback]: Accessing Storage Account Access Key #1780

Closed
1 task done
bgawale opened this issue Apr 30, 2024 · 15 comments
Closed
1 task done

[AVM Question/Feedback]: Accessing Storage Account Access Key #1780

bgawale opened this issue Apr 30, 2024 · 15 comments
Assignees
Labels
Needs: Attention 👋 Reply has been added to issue, maintainer to review Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue Type: Question/Feedback 🙋 Further information is requested or just some feedback

Comments

@bgawale
Copy link

bgawale commented Apr 30, 2024

Check for previous/existing GitHub issues

  • I have checked for previous/existing GitHub issues

Description

We've utilized the AVM approach for deploying several storage accounts, as detailed in the repository: https://github.com/Azure/bicep-registry-modules/tree/main/avm/res/storage/storage-account

What's the recommended method for accessing the access key of these storage accounts? We've observed that the outputs don't include any parameters for this purpose.

This concern arises particularly when deploying multiple resources, such as storage accounts and Azure Functions, where the Azure Functions' configuration settings needs the connection string containing the access key of the storage account. Currently, we're blocked from achieving this.

Any guidance on this matter would be greatly appreciated.

@bgawale bgawale added Needs: Triage 🔍 Maintainers need to triage still Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue Type: Question/Feedback 🙋 Further information is requested or just some feedback labels Apr 30, 2024
@jreinhardtproarch
Copy link

I concur with this issue. This is an acute problem I am facing as I try to deploy the AVM avm/res/web/site for a FunctionApp. That module does not deploy the mandatory WEBSITE_CONTENTAZUREFILECONNECTIONSTRING appSetting (see https://learn.microsoft.com/en-us/azure/azure-functions/functions-app-settings#website_contentazurefileconnectionstring) and that requires the full connection string too. I will create a separate issue for that template though as I think that template could more be fixed to include deployment of this appSetting without necessarily surfacing the keys in this storage template.

@fblix
Copy link
Contributor

fblix commented May 3, 2024

Hi,

thanks for opening this issue.

Thats actually quite a tricky one, the only way around this I am aware of, would be to store the access key in a keyvault, if a certain set of parameters (e.g. kv-name, secret-name, etc) are provided.

But maybe there are other ways, too.

@eriqua @AlexanderSehr @ChrisSidebotham do you have any ideas or references how other modules did it?

@fblix fblix added Status: Help Wanted 🆘 Extra attention is needed and removed Needs: Triage 🔍 Maintainers need to triage still labels May 3, 2024
@bgawale
Copy link
Author

bgawale commented May 3, 2024

Thank you for your reply @fblix. Ideally, as part of infrastructure provisioning, the access key should be stored in the key vault as a secret, with its value seeded. However, the question remains: how to access the access key for the storage account when using the storage account module?

@fblix
Copy link
Contributor

fblix commented May 3, 2024

That was what my comment was supposed to be about. Currently I do not know of a way to securely achieve this, but an idea could be to extend the storage account module to accept a target keyvault as a parameter in which the module would store the access key.

@Agazoth
Copy link
Contributor

Agazoth commented May 3, 2024

This is not only a challenge for storage accounts but also the cognitive services (OpenAI, Speech, Search etc.) produce secrets that need to be stored safely in a key vault.

the AVM document db module has a solution that i actually like very much: https://github.com/Azure/bicep-registry-modules/blob/main/avm/res/document-db/database-account/main.bicep

It takes a key vault object . If only the name of the key vault is submitted, all other variables are calculated and all secrets are stored safely in the key vault. If additional details are added, the names in the key vault can be controlled.

This is done by using a local key vault module within the document db module.

I think it would be a good idea to have a uniform way to get secrets into key vault from all modules that produce secrets.

Currently, we are working on a deployment that has both storage accounts, several cognitive bits and document db, and document db is the only one that plays nicely with key vault.

We would be happy to helt with implementing a uniform solution in the modules where we currently need to use our own modules because of this secret-to-key-vault issue, so our deployment can be AVM only :-)

What is the teams view on the solution done in document db and are there any discussions on how to manage the secrets with a common approach?

@jreinhardtproarch
Copy link

TL;DR: I would take a keyVault as an option, and provide switches to write either the key itself or the connection string to that key value and whether you do one or both key.

I do see your idea for using a keyvault but that may violate the AVM principles of creating an unnecessary dependancy (even if optional) for a very common thing (i.e. a storage account). It seems to me that the resource provider already provides the analogous means called "listKey()" even if it isn't included in the output specifically. But perhaps the whole point is that the output is stored permanently in the deployment object and thus that would be a very insecure manner. A key vault would be better than nothing, for sure, but it may not always work (as in my case of a function app, there are scenario's where a key vault reference seem to not do well with the basic connection strings).

@Agazoth
Copy link
Contributor

Agazoth commented May 3, 2024

TL;DR: I would take a keyVault as an option, and provide switches to write either the key itself or the connection string to that key value and whether you do one or both key.

I do see your idea for using a keyvault but that may violate the AVM principles of creating an unnecessary dependancy (even if optional) for a very common thing (i.e. a storage account). It seems to me that the resource provider already provides the analogous means called "listKey()" even if it isn't included in the output specifically. But perhaps the whole point is that the output is stored permanently in the deployment object and thus that would be a very insecure manner. A key vault would be better than nothing, for sure, but it may not always work (as in my case of a function app, there are scenario's where a key vault reference seem to not do well with the basic connection strings).

listKey() does not work on the module objects:
image

@jreinhardtproarch
Copy link

Exactly, you're affirming what I said. But this means that AVM, practically speaking, is inferior because we do not have such a means to use. It isn't AVM's fault, but rather a Bicep's language limitation. My solution for my scenario was to not use the AVM module for storage accounts, instead using a resource declaration. That is not what I think the intended vision is for AVM, so if a solution could be figured out here that would be great.

@Agazoth
Copy link
Contributor

Agazoth commented May 3, 2024

But using the resource declaration makes it hard to do looping in subscription deployments. That is kind of the great thing about the AVM modules. You are not allowed to add a scope to resource deployments.

@AlexanderSehr
Copy link
Contributor

AlexanderSehr commented May 3, 2024

Hey together,
just want to also provide my 5 cents. I actually think this was discussed also in a different issue just recently. But anyways, here it goes:
From where I'm standing, the recommended way would be to deploy the resources as you do right now, and subsequently reference them as lightweight existing resources whereever you are using them as the existing reference allows you to use all the functions you'd want to use for the given resources.
That's the good news - but some may already see the catch: You can only reference an existing resource if it actually exists in that deployment (module/file) by the time you initiate set deployment. That means what you cannot do is to deploy for example the storage account module and in your workload.bicep file, and in the same file also have an existing reference to it. However, there is a way to achieve this afterall - but requires to build you solution with Bicep's language constraints in mind. Here is what you can do:

  • Let's assume you have a workload.bicep that is the file you start you deployment with and deploys all your storage accounts, congnitive services accounts etc.
  • Now instead of also deploying, for example, the function app, in the same file, create a 2nd file application.bicep (just making up names here, sorry) in which you put all your subsequent code that builds on top of these dependencies.
  • Next, you tie the 2 together by having the workload.bicep file invoke the application.bicep file as a local file reference via the module key word, and pass the resource IDs of your storage accounts in.
  • Finally, in the application.bicep file, you can use these resource Ids to create all hollow existing references and use any property or function you want.
    In case somebody is wondering why this works: The invocation of the appllication.bicep file creates a nested Microsoft.Resources/deployments which is only expanded/evaluated when it starts. By the time this happens, all dependencies in the main workload.bicep file are already deployed, so everything deploys just fine. If it still doesn't make sense, let me know and I'll draft an example.

Sadly, while working, this is of course a workaround which is annoying to say the least. There are 2 things that would help with this once implemented in Bicep

  • There is an issue open to add @secure() also to outputs of modules. If we'd have that, we could return the keys as a normal output without exposing it (what it would do today).
  • Alternatively, and even better, would be if the modules could return the entire ARM object. If that were the case, you could actually do what was requested initially and invoke, for example, listKeys() on the module's output. This is on the PGs backlog for some time now, and if I'm not mistaken it should come latest with Bicep 1.0. But only Clippy knows when that's going to be the case, so it's not really an option to wait for it.

@Agazoth
Copy link
Contributor

Agazoth commented May 3, 2024

In my opinion the modules should make life easier. Having to do part of the deployment in one file and then handle the rest in another file and having to add extra "existing" resource code in the second file is far from ideal for the deployment builder. The module should take care of that.

The implementation made in Document DB does make a larger module, but it seems fitting for resources that emit secrets which
should not be passed between modules until we get @secure() export.

We have been waiting for a solution for this scenario for a long time. Is anything planned in the upcoming sprints to fix it?

Unfortunately I have not been able to follow the discussions about this very hot topic. Does anyone have a link to the discussions or does anyone know, why the beautiful implementation in document db was allowed in the first place? Or is it planned to get trimmed away again?

@AlexanderSehr
Copy link
Contributor

AlexanderSehr commented May 4, 2024

Hey @Agazoth,
my comment above was intended to describe how you could operate in the existing setup - given the constraints of the Bicep language today.
'The module should take care of that' is indeed ideally the case, but if Bicep cannot return secrets nor the ARM object, there is only so much the module owner can do (that is, implement workarounds, to work around the language's constraints). I understand your frustration, and believe it or not, I share it, but we have to be careful not to mix language constraints with module constraints.

That being said, the KeyVault-Secerets solution of Document DB is a also valid workaround. It's still a workaround (and comes with it's own challenges) but the module owner can certainly add it. Once the desired Bicep features are released, it may render it redundant, but like I wrote in the original comment, give a missing timeline it's not really worth waiting for.

Regarding your question 'why it was allowed' - it's simple, it was allowed because it does not violate any specs and the module owner @bryansan-msft contributed it. 🚀 That's it. I would be very suprised if there'd be any plan to remove it again.
If that happened with any other module features (because it sounds like there's history), please let me know.

Long story short, if @fblix is up for it, he can certainly add the implementation to the storage account module, as would be the owner of the Cognitive Services module you mentioned. It's an optional feature afterall, and does not open any new branching paths regarding how the module works (which would add to the module's complexity).

PS: Personally, when building solutions I prefer going with 'existing' resources simply for the fact that they provide all the features I want (i.e., aside from functions, also all kinds of the latest properties), and simply build the solutions with the structural constraints in mind. If working with Bicep taught me anything, then it's that I'll end up with multiple files sooner or later anyways 😄 (given existing references, scope changes, grouping, etc. all requiring just that). But everyone has their own preferences.

@ktremain
Copy link
Contributor

Until there is a way to manage a secure output in bicep, there isn't a way to achieve this request securely entirely withing the scope of the storage account resource module in a way that would meet the WAF framework security model. This is probably a candidate to be closed for now and look to (re-)open when future functionality is available.

@ktremain ktremain added Needs: Author Feedback 👂 Awaiting feedback from the issue/PR author and removed Status: Help Wanted 🆘 Extra attention is needed labels Jul 15, 2024
@Agazoth
Copy link
Contributor

Agazoth commented Jul 16, 2024

Or tie it to #1934

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 Reply has been added to issue, maintainer to review and removed Needs: Author Feedback 👂 Awaiting feedback from the issue/PR author labels Jul 16, 2024
@AlexanderSehr
Copy link
Contributor

Agreed @Agazoth,
@ktremain for context: @eriqua asked @Agazoth a while back to open the isssue #1934 to discuss and agree on an approach if needed for certain modules (as opposed to implementing custom solutions for each). For have a few good candidates on the table and are closed to defining a spec for it.

@github-project-automation github-project-automation bot moved this from Needs: Triage to Done in AVM - Module Issues Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Attention 👋 Reply has been added to issue, maintainer to review Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue Type: Question/Feedback 🙋 Further information is requested or just some feedback
Projects
Development

No branches or pull requests

6 participants