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

Add secure() for secrets #3589

Merged
merged 1 commit into from Apr 8, 2024
Merged

Add secure() for secrets #3589

merged 1 commit into from Apr 8, 2024

Conversation

pamelafox
Copy link
Member

@pamelafox pamelafox commented Mar 25, 2024

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:
Azure-Samples/langfuse-on-azure#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.

Copy link
Contributor

@wbreza wbreza left a comment

Choose a reason for hiding this comment

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

This looks good but also would be considered a breaking change for any existing templates leveraging the secrets parameter.

We should at least do a pass on any of our known Todo templates to validate nothing is breaking.

@jongio
Copy link
Member

jongio commented Mar 26, 2024

@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?

@vhvb1989
Copy link
Member

@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?

@pamelafox
Copy link
Member Author

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!)

@jongio
Copy link
Member

jongio commented Mar 26, 2024

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.

@v-jiaodi
Copy link
Member

@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

@jongio
Copy link
Member

jongio commented Mar 28, 2024

@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?

@vhvb1989
Copy link
Member

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)

@v-jiaodi
Copy link
Member

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 733bfdc)

Here is my sub permission list:
image

@mikeharder
Copy link
Member

@vhvb1989 Tested again and still encountered the same issue.

Should really be fixed now

@vhvb1989
Copy link
Member

@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

@v-jiaodi
Copy link
Member

@vhvb1989 Now it can work normally.

@v-jiaodi
Copy link
Member

  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.

@jongio
Copy link
Member

jongio commented Apr 1, 2024

@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?

@pamelafox
Copy link
Member Author

pamelafox commented Apr 1, 2024

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.

@weikanglim
Copy link
Contributor

@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

Copy link
Contributor

@weikanglim weikanglim left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for submitting this change!

@pamelafox
Copy link
Member Author

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

@weikanglim weikanglim merged commit 6abd371 into Azure:main Apr 8, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants