-
Notifications
You must be signed in to change notification settings - Fork 228
feature/enh290 Set OIDC clientSecret to be set via a secret #303
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Michael Dent <[email protected]>
@@ -65,7 +65,7 @@ | |||
<provider> | |||
<identifier>ldap-provider</identifier> | |||
<class>org.apache.nifi.ldap.LdapProvider</class> | |||
<property name="Authentication Strategy">SIMPLE</property> | |||
<property name="Authentication Strategy">{{.Values.auth.ldap.authStrategy}}</property> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the master
branch already has the code you want (not hardcoded with SIMPLE. Not sure why the PR still shows SIMPE on the left pane??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I don't recall changing this file.
For people who do GitOps we need a existingSecret to be referred to. Not using values. So the secrets aren't stored in git.
@@ -147,6 +147,9 @@ auth: | |||
discoveryUrl: #http://<oidc_provider_address>:<oidc_provider_port>/auth/realms/<client_realm>/.well-known/openid-configuration | |||
clientId: #<client_name_in_oidc_provider> | |||
clientSecret: #<client_secret_in_oidc_provider> | |||
# try to use an existing secret that has been sourced from a key vault so the clientSecret isn't stored in plaintext | |||
# if this is set then the clientSecret above is ignored. | |||
existingSecret: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wh not use the same property "clientSecret" with value comes from the vault or any other tools for secrets? Personally I think it adds confusion with two properties for the same purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I did it is what most public Helm charts I've seen do.
It lets the dev/test people just use a plaintext password. clientSecret is the actual password, not a reference to a k8s secret. Also in future the existingSecret could contain more than just one key, like it may need for ldap or the simple method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is also backwards compatible for existing user's config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen this pattern before too, it's pretty common
What this PR does / why we need it:
This PR sets an example of using a secret to set sensitive values - in this case the OIDC clientSecret.
It also adds the option of using an existing secret that has been set by another method such as Secret Store CSI driver.
A secret is created if no existing secret is specified. The secret is then mounted as a file within the pod. The container's 'command:' then reads the secret from the file using $(cat ). This is easier than rewriting the whole container start command.
Which issue this PR fixes
Fixes one secret, and gives an example for other secrets as mentioned in #290
Special notes for your reviewer:
Chart version not set, as it will likely be something else.
Checklist
[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]