azure-dev icon indicating copy to clipboard operation
azure-dev copied to clipboard

Add secure() for secrets

Open pamelafox opened this issue 1 year ago • 15 comments

This PR makes it so that you can securely pass secrets into the container-app modules, by wrapping it in a secure() decorator. Unfortunately, we can't mark arrays as @secure, so you have to instead pass in an object of the secret key/values, and we turn that back into the expected array format.

Note that I haven't tested these exact modules since I'm working off a fork of the modules (my use case isn't compatible with the azd modules.. will file an issue about that). You can see my change here, which works: https://github.com/Azure-Samples/langfuse-on-azure/pull/9

I don't think any of the TODO templates utilize secrets for container apps, so not sure there's a test to do on the azd template side.

pamelafox avatar Mar 25 '24 20:03 pamelafox

@v-xuto - Can you please search the templates published at awesome-azd for container apps that use secrets to see how big of a breaking change this would be?

jongio avatar Mar 26 '24 18:03 jongio

@jongio @wbreza , templates are not automatically updated to the latest bits from /core templates. We don't even have a quick way of doing the update. When folks want to pull and sync to latest /core templates, they need to clone azure-dev repo, pull latest main and copy/paste the core modules folder to their template.

For what I've seen, folks would only try to update individual modules when there's an error of there is a required version update for the smart-defaults.

Is that enough data to lower the relevance of the breaking changes effects on /core modules?

vhvb1989 avatar Mar 26 '24 19:03 vhvb1989

It might be good to check for secrets usage just to make sure there aren't any there that are using current secrets, since they result in security alerts. But agree that I don't particularly think it should block if it doesnt affect TODO (which I didnt see evidence of, but worth checking again!)

pamelafox avatar Mar 26 '24 19:03 pamelafox

Yes, just a scan to see if they are using and warn them as a courtesy.

@v-xuto please...

  1. Check all awesome azd templates for container/secrets usage and report back.
  2. Regression test related todo templates that use this with the changes from this PR.

jongio avatar Mar 26 '24 19:03 jongio

@jongio

  • For all awesome templates,running:azd template list, there are a total of 97 templates, of which 35 use container/secrets.
  • For related todo templates(todo-nodejs-mongo-aca,todo-python-mongo-aca,todo-java-mongo-aca), We did not continue to use the current PR change for testing because we encountered the following issues before any changes were made.

image

v-jiaodi avatar Mar 27 '24 10:03 v-jiaodi

@jongio

  • For all awesome templates,running:azd template list, there are a total of 97 templates, of which 35 use container/secrets.
  • For related todo templates(todo-nodejs-mongo-aca,todo-python-mongo-aca,todo-java-mongo-aca), We did not continue to use the current PR change for testing because we encountered the following issues before any changes were made.

image

@wbreza thoughts on that error?

jongio avatar Mar 28 '24 18:03 jongio

@jongio , the error was due to: https://teams.microsoft.com/l/message/19:[email protected]/1711500134341?tenantId=72f988bf-86f1-41af-91ab-2d7cd011db47&groupId=3e17dcb0-4257-4a30-b843-77f47f1d4121&parentMessageId=1711500134341&teamName=Azure%20SDK&channelName=Engineering%20System%20%F0%9F%9B%A0%EF%B8%8F&createdTime=1711500134341

It should work now, as the roles are restored

vhvb1989 avatar Mar 28 '24 18:03 vhvb1989

btw, @jongio @v-jiaodi , there are big changes on all todo-templates seating on the staging branch. You might want to look into that branch instead of main until we release the new version (probably next week)

vhvb1989 avatar Mar 28 '24 18:03 vhvb1989

It should work now, as the roles are restored

@vhvb1989 Tested again and still encountered the same issue.

Environment:

  • Template: todo-nodejs-mongo-aca.
  • branch: staging
  • OS: Windows
  • Azd version : 1.8.0-beta.1 (commit https://github.com/Azure/azure-dev/commit/733bfdc26c871be4e9b3e077069a6e0d209c837d)

Here is my sub permission list: image

v-jiaodi avatar Mar 29 '24 03:03 v-jiaodi

@vhvb1989 Tested again and still encountered the same issue.

Should really be fixed now

mikeharder avatar Mar 29 '24 05:03 mikeharder

@v-jiaodi , can you try again? (You might need to azd auth logout and azd auth login again. The roles should be restored by now

vhvb1989 avatar Mar 29 '24 05:03 vhvb1989

@vhvb1989 Now it can work normally.

v-jiaodi avatar Mar 29 '24 05:03 v-jiaodi

  1. Regression test related todo templates that use this with the changes from this PR.

@jongio For related todo templates(todo-nodejs-mongo-aca,todo-python-mongo-aca,todo-java-mongo-aca), it works with the changes from this PR.

v-jiaodi avatar Mar 29 '24 09:03 v-jiaodi

@rajeshkamal5050 - It would be good to include this in breaking changes list with instructions on how to implement with the new method. Pamela is that something you can write up?

jongio avatar Apr 01 '24 14:04 jongio

Suggested for releasse notes-

Breaking change, made to improve security: The container-app.bicep and container-app-upsert.bicep modules now mark the secrets parameter as secure(), and expect secrets to be an object (versus an array). You will need to reformat your array of [{name: 'secret-name', value: secretValue}, ...] as an object like {'secret-name': secretValue, ...} instead.

pamelafox avatar Apr 01 '24 21:04 pamelafox

@pamelafox FYI {SECRET_NAME: SECRET_VALUE, ...} doesn't work. You need {secret-name: SECRET_VALUE, ...} since you're creating a secret name and not the environment variable name of the secret.

image

weikanglim avatar Apr 05 '24 00:04 weikanglim

@weikanglim Ah yeah, my placeholders were misleading. Updated suggested text to look more like a real diff (such as https://github.com/Azure-Samples/langfuse-on-azure/commit/105148fffde072dd03e56c8d686963e665b8013e)

pamelafox avatar Apr 05 '24 13:04 pamelafox