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

Sensitive snake_case porter parameters fails to deploy when using azure.keyvault plugin #51

Closed
anatbal opened this issue Jan 22, 2023 · 4 comments · Fixed by #59
Closed
Assignees
Labels
1 - 🍫 Eat chocolate _after_ emergency donuts bug Oops, sorry!

Comments

@anatbal
Copy link
Contributor

anatbal commented Jan 22, 2023

Describe the bug

We are using porter version 1.0.0 with the Azure keyvault secrets plugin activated. We have all our parameters inside the porter.yaml config as snake cased, but once we mark it as sensitive like this:

 -name: some_password
  sensitive: true

we are having this error when trying to run porter install with the bundle:
failed to set secret for key XXX-some_password in azure-keyvault: keyvault.BaseClient#SetSecret: Invalid input: autorest/validation: validation failed: parameter=secretName constraint=Pattern value=\"0 XXX-some_password\" details: value doesn't match pattern ^[0-9a-zA-Z-]+$

This bug occurs since the name of the variable is "some_password" and the keyvault naming convention does not allow underscores (_) in the secret name.

To Reproduce

Steps to reproduce the behavior:

  1. Activate the Azure keyvault secrets plugin.
  2. Use a porter.yaml with a senstive snake cased param inside.
  3. Build the bundle
  4. Run porter install
  5. See error as specified above.

Expected behavior

There should be a way to overcome this and still support snake_case parameters without having to change them all to camelCased.

Version

Copy the output of porter version below

porter v1.0.4 (e3a2a5ef)

@carolynvs
Copy link
Member

What if the azure plugin to replace anything in the key that would fail the azure keyvault regex to use a hyphen instead when creating/retrieving secrets? 🤔

So a sensitive parameter named "some_password" would be stored as INSTALLATIONID-some-password, and then when porter attempts to retrieve it later, do the same replacement so that it can read it back.

However, this would mean that if someone mistyped their secret key in a parameter set file, that the azure plugin would automatically do the same replacement. For example, if they mistyped the secret source to use underscores (below), the plugin would attempt to read a secret from azure keyvault at my-db-password.

parameters:
- name: db_password
  source:
    secret: my_db_password

The plugin doesn't have a way to know that it's reading a secret specified by the user vs a secret managed by Porter (in the case of a sensitive parameter). So if we make this change in the plugin, it will impact both scenarios. We can mitigate the impact in the latter (typo) scenario by having the azure plugin print a more detailed error message when they key cannot be found, explaining the original key requested was replaced to something with hyphens so that it would be compatible with azure key vault.

@tamirkamara
Copy link

tamirkamara commented Jan 23, 2023

@carolynvs I'm less worried about the user specified secret as the user will find by themselves they can't create a secret named my_db_password in the Keyvault and will align both secret name on the cloud and the one in the porter config to match something that will work. Even on v0 we had all our secrets named my-db-password... but now with sensitive parameters it's a different story.

@carolynvs carolynvs transferred this issue from getporter/porter Jan 23, 2023
@carolynvs
Copy link
Member

@anatbal I am moving this issue to the azure-plugins repository where the change needs to be made. This may have broken your link to this issue from the Azure TRE repo.

@carolynvs carolynvs added the 1 - 🍫 Eat chocolate _after_ emergency donuts label Jan 24, 2023
@carolynvs carolynvs self-assigned this Feb 13, 2023
@carolynvs
Copy link
Member

Here is the proposed fix, in case you'd like to help review or test the change. #59

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 - 🍫 Eat chocolate _after_ emergency donuts bug Oops, sorry!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants