Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

[cetic/nifi] Allow use of existing secrets for ALL sensitive Helm chart values #290

Open
jstewart612 opened this issue Mar 16, 2023 · 6 comments · May be fixed by #303
Open

[cetic/nifi] Allow use of existing secrets for ALL sensitive Helm chart values #290

jstewart612 opened this issue Mar 16, 2023 · 6 comments · May be fixed by #303
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@jstewart612
Copy link

Is your feature request related to a problem? Please describe.
This Helm chart has numerous places where you can enter sensitive credentials as a Helm value override. This breaks the basic security model of GitOps, making this chart only suitable for use with a literal "helm install", and even then, object last modified annotations will contain the plain text secrets in them. This is operationally unsafe and a compliance nightmare for any organization.

Describe the solution you'd like
Every single Helm value with the word "secret" in it gets an equivalent "existingSecret" key where you can specify subkey "name" and subkey "key", mapping to the name of the secret and the name of the key inside that secret, respectively.

Describe alternatives you've considered
I reviewed your helm template for OIDC configuration and it does not permit any other value to be specified other than the client secret.

Additional context
This is a very good and useful Helm chart but this is a critical step for operational security in GitOps environments.

@banzo banzo added the enhancement New feature or request label Mar 16, 2023
@banzo
Copy link
Contributor

banzo commented Mar 17, 2023

@jstewart612 thank you for raising this issue. Could you please point to an example/documentation/tutorial for the solution you are describing?

Note: We have also very briefly looked at helm-secrets+sops but it is adding a new dependency to the chart.

@jstewart612
Copy link
Author

@banzo most of your problem is your approach to blocks like these:

https://github.com/cetic/helm-nifi/blob/master/templates/statefulset.yaml#L116-L369

You tell the Helm chart, inline in a StatefulSet, to render a giant template string invoked by bash -ce. As a result, when you render the Helm chart, the password renders in plaintext in the object definition in Kubernetes / as a result of helm template commands, etc. You could go with numerous approaches to address this. My suggestion would be to never render a sensitive piece of data outside of a Secret. I'd move the entirety of these script generation blocks into a Secret, then simply tell your StatefulSet to load the script or file located at x, and have a volume and volumeMount from that SecretRef.

https://kubernetes.io/docs/concepts/configuration/secret/#using-secrets-as-files-from-a-pod

Once you can do that, it's a fairly easy hop, skip, and a jump, to wrap the rendering of the secret behind the truthiness of another key.

if existingSecret key defined -> do not render this secret

Granted, you're doing a lot of interpretation for people, so if they chose to use this existingSecret model, they'd lose a lot of the convenience functions... but that's a choice I'd argue the user should have.

At a minimum, exfiltrating these giant blocks of text with passwords in them out of the StatefulSet and into Secrets, and then just having all these commands simply reference a file loaded from a secretRef, with an option to specify an existingSecret and then not render that Secret at all, would be a huge win. Then, you don't have to be opinionated on which secrets protection mechanism the end user employs. They can make those decisions for themselves.

Let me know if you have any other questions. I may personally try my hand at a PR demonstrating what I mean, as well.

@banzo
Copy link
Contributor

banzo commented Apr 18, 2023

@jstewart612 thank you for the explanation.

I may personally try my hand at a PR demonstrating what I mean, as well.

That would be most excellent.

@banzo banzo added the help wanted Extra attention is needed label Jun 3, 2023
@emrge-michaeld emrge-michaeld linked a pull request Jun 8, 2023 that will close this issue
3 tasks
@emrge-michaeld
Copy link
Contributor

@banzo I've created a pull request to set an example of how this could be done.
Helm either creates a secret using the helm value or allows for an existingSecret.
I chose the easier option of not rewriting the whole container 'command:' section, but just having that script read the secret as a file using $(cat ).

Ideally the file would be created using the Secret Store CSI driver directly and no k8s Secret is created at all, but not all users would want to use that method, and it would be messy to have that option.

@emrge-michaeld
Copy link
Contributor

Using this method, I'd have one secret/mount per optional block. For blocks with multiple secrets, use one Secret and many keys. Each get using the simple mount method (ie no subPath), it automatically creates a file per key within the folder.
Thus singleUser secret has key1: username, key2: password.

apiVersion: v1
kind: Secret
metadata:
  name: {{ template "apache-nifi.fullname" . }}-singleuser
...
data:
  username: {{ .Values.auth.singleUser.username | b64enc }}
  password: {{ .Values.auth.singleUser.passowrd | b64enc }}
kind: Statefulset
...
    volumeMount:
     - name: singleuser-secret
        mountPath: /mnt/secrets/singleUser
       readOnly: true
  volume:
        - name: singleuser-secret
          secret:
            {{- if not .Values.auth.singleUser.existingSecret }}
            secretName: {{ template "apache-nifi.fullname" . }}-singleuser
            {{- else }}
            secretName: {{ .Values.auth.singleUser.existingSecret  }}
            {{- end }}

creates:

/mnt/secrets/singleUser/username
/mnt/secrets/singleUser/password

which can then be individually used:
prop_replace ... $(cat /mnt/secrets/singleUser/username)
Putting them in their own /mnt/secrets/ folder allows for multiple sets of secrets.

@banzo
Copy link
Contributor

banzo commented Jun 9, 2023

@jstewart612 is #303 in line with what you proposed?

@banzo banzo linked a pull request Jun 9, 2023 that will close this issue
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants