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(helm): added jwt secret auto gen (#1062) #1092

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

danielhass
Copy link
Contributor

Introduced a new auto generated secret key that will only be
generated on first install called "jwtSignatureGeneratorSecret".
The value of this secret key is added via an environment variable
setting to the depoyment only if no user defined variable with
the same name is found. This should avoid duplicate env var names
that can cause trouble with subsequent changes to the deployment
object.

I would like to get some feedback on the implementation design. I tried to introduce the new feature without breaking existing behavior.

Summary of changes:

  • the release-name-akhq-secrets is now created always and the previous conditional is moved closer to the application-secrets.yml key
  • a Helm lookup is used to only generate the secret once on first install and reuse it's value on subsequent upgrades. This should make it possible to slowly shift traffic between pods on e.g. upgrades as all instances use the same signature generator secret
  • the deployment env section now incorporates a bunch of logic to check if a user as explicitly set the MICRONAUT_SECURITY_TOKEN_JWT_SIGNATURES_SECRET_GENERATOR_SECRET env var. In this case the auto generated value will not be used (however it will still be generated and written to the secret).

One point of uncertainty at this point is the question if we should include a general switch to completely disable this new feature. However with the default setting being active as the goal of #1062 is to make the chart secure per default.

Introduced a new auto generated secret key that will only be
generated on first install called "jwtSignatureGeneratorSecret".
The value of this secret key is added via an environment variable
setting to the depoyment only if no user defined variable with
the same name is found. This should avoid duplicate env var names
that can cause trouble with subsequent changes to the deployment
object.
@danielhass
Copy link
Contributor Author

@tchiotludo would love to hear your feedback on this before moving on.

@tchiotludo
Copy link
Owner

hi @danielhass,

thanks for that. I look at quickly but I found it's some kind of breaking change.
You will add extra env vars if the users to have defined it, but since most of people is using configuration (or secrets files), it will be override by your values.

I think I would prefer:

jwtSecret:
  create: true
  value: "xx"

to allow people to define one or to let autocreate one.

What do you think about that?

@danielhass
Copy link
Contributor Author

@tchiotludo I get your concern. Actually I don't know enough about Micronaut to comment on the precedence of the different ways of setting the jwt generator secret. I don't know if a env variable would overwrite a configuration file supplied setting or vice versa.

Based on your feedback I will try to adapt my PR to a more explizit variant were users have to opt-in in order to use the autogen feature.

However my ultimate goal in #1062 was to provide akhq users with a secure installation out-of-the-box. Switching to a explicit setting somehow contradicts this. What would you think of an addition to the charts NOTES.txt that points users to the autogen flag if its not already set? Otherwise users would need to check the akhq container logs to find the Micronaut warning about the usage of the default JWT generator secret.

@tchiotludo
Copy link
Owner

@danielhass : any env vars will override any application files in micronaut.
The better will be to add the generated jwt to secrets in order to create automatically the application-secrets.yml if not provide or to merge with provided one.

I think we could kept the conf for convenience :

jwtSecret:
  create: true
  value: "xx"

what do you think ?

@danielhass
Copy link
Contributor Author

@tchiotludo thanks for the feedback and clarifying the precedence of configs in Micronaut.

I'm not sure if I get your proposal right but I would not be a huge fan of merging a generated secret into the user supplied values saved in the application-secrets.yml. I honestly think that could cause even more confusion. If we want to support the explicit way of enabling the JWT generator secret auto generation and additionally provide the user with the aforementioned convenience option I would suggest that the auto generated or user supplied value is saved as a separate key inside the akhq secret. Afterwards I would provide it via an secret based environment variable to the deployment.

This would have the additional charm that even if users use other ways of providing a secret the convenience option or auto generated value will always have a higher precedence. And as this values.yaml option is a clear user opt-in for me this sounds quite right.

What to you think about this proposal? If it sounds good to you I will go ahead and try to implement it this way.

@vutkin
Copy link

vutkin commented Jul 22, 2022

Best solution: https://github.com/tchiotludo/akhq/pull/1152/files#diff-9cc2bd3ea2b84a15de094176387d72447ead7aa612f26fd272f05e4996559752R16

Please see 16th line.

than just use

    micronaut:
      security:
        enabled: true
        token:
          jwt:
            signatures:
              secret:
                generator:
                  secret: ${JWT_TOKEN}

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.

3 participants