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

feat: improve parsing of secret envs #296

Merged
merged 4 commits into from
Jul 23, 2021

Conversation

Demonsthere
Copy link
Collaborator

@Demonsthere Demonsthere commented Jul 13, 2021

Related issue(s)

Checklist

  • I have read the contributing guidelines
    and signed the CLA.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got green light (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added necessary documentation within the code base (if
    appropriate).

Further comments

{{- if .Values.hydra.config.secrets.system -}}
{{- $tmp := "" -}}
{{- range .Values.hydra.config.secrets.system -}}
{{- $tmp = printf "%s,'%s'" $tmp . -}}
Copy link
Member

Choose a reason for hiding this comment

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

This will print single quotes around values? That is currently not properly parsed by hydra, see upstream PR ory/x#373

Copy link
Collaborator Author

@Demonsthere Demonsthere Jul 13, 2021

Choose a reason for hiding this comment

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

This will print the supplied secrets as SECRETS_SYSTEM='foo bar 123 456 lorem','foo bar 123 456 lorem 1','foo bar 123 456 lorem 2','foo bar 123 456 lorem 3'. It seems to be working aka, hydra is running and not complaining about them

Copy link
Member

Choose a reason for hiding this comment

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

yes it is not complaining but the secrets are now actually including the quote, therefore they are different

Copy link
Member

Choose a reason for hiding this comment

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

and then hydra fails when trying to use those secrets later on for older records

Copy link
Member

Choose a reason for hiding this comment

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

TL;DR
We have to use some csv encoding function here instead, that also properly escapes commas and double quotes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes i think it makes the most sense, currently we force a string, which can be either a simple value or stringied json array

Copy link
Member

Choose a reason for hiding this comment

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

OK not the best user experience though.
Why not

{{- define "hydra.secrets.system" -}}
  {{- if .Values.hydra.config.secrets.system -}}
    {{- if kindIs "[]string" .Values.hydra.config.secrets.system -}}
      {{- mustToRawJson .Values.hydra.config.secrets.system -}}
    {{- else -}}
      {{- fail "Expected .Values.hydra.config.secrets.system to be a list of strings" -}}
    {{- end -}}
  {{- else if .Values.demo -}}
  ["a-very-insecure-secret-for-checking-out-the-demo"]
  {{- end -}}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This could also work, we just need to stringify at the end, as the result would be interpreted in bash [ string1 string2] and hydra doesn't like that

Copy link
Member

Choose a reason for hiding this comment

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

It is up to you, I just think that it would be a bad user experience to put raw JSON into the yaml, instead of automatically encoding the yaml values when rendering the template.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had to modify it slightly, but was able to make it work ;)

@Demonsthere Demonsthere marked this pull request as ready for review July 15, 2021 09:15
@Demonsthere Demonsthere requested a review from zepatrik July 15, 2021 09:15
Copy link
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

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

Looks good to me 😉

@Demonsthere
Copy link
Collaborator Author

@aeneasr What do you think?

@aeneasr aeneasr merged commit 27fae16 into ory:master Jul 23, 2021
@Demonsthere Demonsthere deleted the fix/hydra-secrets-handling branch July 23, 2021 09:08
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.

Space character in secret.system value
3 participants