[ENHANCEMENT] Enable setting an encryption key to be used to encrypt/decrypt sensitive data #13

Gabrielopesantos · 2024-02-05T23:45:20Z

Description

Still ironing some things on this solution proposal, will add a description and mark the ticket as ready to be reviewed once I'm done.
If you see the PR in the meanwhile and think there's a better way to achieve this, feel free to comment.

Adds support for encryption_key and encryption_key_file values to be set, both "" by default. If encryption_key is set, the key is made available as an environment variable. If both values are set, the key is set on a secret, mount as a volume and made available to the perses process as a file. In cases encryption_key isn't set, the value of encryption_key_file, if set, is ignored.

At the time of development I thought of this approach or one where we would expect secrets with a given name to be present so they could be mount or loaded as env vars. I'm still not sure what would be better but feel free to leave feedback.

The PR doesn't add support auth providers as discussed but I intend to open another PR for it.

Checklist

Pull request has a descriptive title and context useful to a reviewer.
Pull request title follows the [<catalog_entry>] <commit message> naming convention using one of the following catalog_entry values: FEATURE, ENHANCEMENT, BUGFIX, BREAKINGCHANGE, IGNORE.
All commits have DCO signoffs.

encryption key is loaded as a env var or file depending if encryption_key_file is set or not Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>

Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>

… block Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>

nicolastakashi · 2024-02-20T13:53:33Z

charts/perses/values.yaml

@@ -191,4 +191,4 @@ datasources:
  #     plugin:
  #       kind: PrometheusDatasource
  #       spec:
-  #         directUrl: https://prometheus.demo.do.prometheus.io


(nit) can we remove this change, just to keep the PR clean as possible?

Sure, my editor added it I don't mind rolling back.

I have also an editor adding newlines in all files. I know that it can be a pain to manage so to me this not that a problem if you don't ^^

nicolastakashi · 2024-02-20T13:57:00Z

charts/perses/templates/secrets.yaml

@@ -0,0 +1,17 @@
+{{- if and .Values.config.security.encryptionKeyFile .Values.config.security.encryptionKey }}


We should provide the ability to users use an existing secret in the cluster.

Disable the secret creation

Provide the existing secret name and secret key to be mounted.

Do you have any suggestions for what would it look like? I mentioned this in the PR's description but I'm not sure about the implementation. This gets a bit complex as there are many ways to make the key available to the perses process.

If encryptionKey is the only value provided then we set it in the config.yaml file.
If encryptionKey and encryptionKeyFile are set, we create the secret and mount it on the pod.
If existingEncryptionKeySecret (or any other name) is given we just set the volume and volume mount in the statefulset with the information provided by the user.
But what about precedence? For me, the following makes sense but let me now what you think. Existing secret > Encryption key mount as file > Encryption key put in the config.yaml file.

P.S.
I was thinking of the following format for the existing secret information to be provided.

# -- Mount encryption key with an existing secret existingEncryptionKeySecret: secretName: "" secretKey: ""

EDIT: If you see this before I finish with the approach that I'm trying, please ignore "WIP" commit.

I agree with the precedence you shared.

Note that you can't set encryptionKey and encryptionKeyFile at the same time in the Perses config otherwise the server will crashed because the config is inaccurate.

See https://github.com/perses/perses/blob/main/pkg/model/api/config/security.go#L55-L56

nicolastakashi · 2024-02-20T13:59:16Z

charts/perses/templates/config.yaml

+      {{- if and .Values.config.security.encryptionKeyFile .Values.config.security.encryptionKey }}
+      encryption_key_file: {{ printf "%s/key" (.Values.config.security.encryptionKeyFile | trimSuffix "/") }}
+      {{- end }}
+      {{- if and (not .Values.config.security.encryptionKeyFile) .Values.config.security.encryptionKey }}


I might be wrong, but from my understanding if user provides a encryption key file he must to provide a encryption key.

Is that right @Nexucis ?

That's my understanding as well. But if an encryption key file is provided, and the file is mount on the pod, is there any reason to also add the key in the configuration file, config.yaml?

yeah encryptionKey and encryptionKeyFile are mutually exclusive but they are not mandatory.
If you don't provide it, there is one hardcoded that will be used.

nicolastakashi · 2024-02-20T13:59:59Z

charts/perses/values.schema.json

@@ -102,6 +102,14 @@
              "type": "boolean",
              "default": false
            },
+            "encryptionKey": {


I think makes sense add this values to the values.yaml as empty values, just to let users know it exists and is a possibility, as well as because the documentation is being created from the values file.

nicolastakashi · 2024-02-20T14:00:36Z

Thanks for handle this @Gabrielopesantos, just left some comments, nice work 💪🏽

Gabrielopesantos · 2024-02-22T00:46:26Z

Thanks for handle this @Gabrielopesantos, just left some comments, nice work 💪🏽

Thank you for the feedback, I will have a look as soon as I can. 👍

Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>

nicolastakashi · 2024-04-09T08:53:29Z

@Gabrielopesantos could you check CI?

Nexucis · 2024-04-22T10:26:43Z

@Gabrielopesantos the CI detect an format issue. Probably an issue related to the {if} {end}
Helm is always a pain when running into this issue :(

data Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>

Gabrielopesantos · 2024-04-25T12:22:11Z

My bad and thank you for the patience. I could have easily run helm lint locally and fixed these issues. Should be good to be reviewed now.

I'm not a fan of my implementation but couldn't think of another way to implement this logic of having precedence of values for the secret (even worse with go templates).

Nexucis · 2024-04-30T08:20:41Z

awesome thank you @Gabrielopesantos !

@celian-garcia and/or @nicolastakashi if you have time, could you please take a look ?
Thank you !

celian-garcia

Hello, thanks for this PR. I am trying to review but it sounds complex to me.

If I understand properly:

if user give an encryptionKey or encryptionKeyFile it uses it in the config. That part is simple.
Now if the user give an encryptionKeyFile it means that we need mount the volume.
Two solution : the user has a secret or the user want us to create a secret.
And if a user doesn't provide a file but provide a secret we want the file to be set.
Also I don't see a case where the encryptionKeyFile would exist without a k8s secret but maybe I'm wrong

Did I forget something? :D

If I try to translate it to a yaml self explanatory, I can draft a proposal that in my feeling would be less confusing for the user and easier to develop on your side. WDYT?

security: 
  encryptionKey: ""
  encryptionKeyFile: 
    activated: false # this is to ease the configuration if a user want to setup the secret but activate it or not easily
    secretName: '' # default to perses-full-name encryptionsecret
    secretKey: 'key' # (setting the default in the values file directly would prevent from forgetting a "| default" in the code)

Note:
For the mutually exclusive fields, you could do a check with helm on top of the templates using them. It would save you from some checks.
{{- if and .Values.security.encryptionKey .Values.security.encryptionKeyFile.activated }}
  {{- fail ( printf "encryptionKey and encryptionKeyFile are mutually exclusive" ) }}
{{- end }}

celian-garcia · 2024-04-30T08:53:59Z

charts/perses/values.yaml

@@ -191,4 +191,4 @@ datasources:
  #     plugin:
  #       kind: PrometheusDatasource
  #       spec:
-  #         directUrl: https://prometheus.demo.do.prometheus.io


I have also an editor adding newlines in all files. I know that it can be a pain to manage so to me this not that a problem if you don't ^^

celian-garcia · 2024-04-30T09:29:44Z

charts/perses/templates/config.yaml

@@ -12,6 +12,12 @@ data:
  config.yaml: |-
    security:
      readonly: {{ .Values.config.security.readOnly }}
+      {{- if and .Values.config.security.encryptionKeyFile .Values.config.security.encryptionKey }}


not possible as mutually exclusive

celian-garcia · 2024-04-30T09:30:21Z

charts/perses/templates/config.yaml

@@ -12,6 +12,12 @@ data:
  config.yaml: |-
    security:
      readonly: {{ .Values.config.security.readOnly }}
+      {{- if and .Values.config.security.encryptionKeyFile .Values.config.security.encryptionKey }}
+      encryption_key_file: {{ printf "%s/key" (.Values.config.security.encryptionKeyFile | trimSuffix "/") }}


You're not taking into account your override* fields

Gabrielopesantos · 2024-05-30T17:16:04Z

Hello, thanks for this PR. I am trying to review but it sounds complex to me.

If I understand properly:

if user give an encryptionKey or encryptionKeyFile it uses it in the config. That part is simple.

Now if the user give an encryptionKeyFile it means that we need mount the volume.
Two solution : the user has a secret or the user want us to create a secret.

And if a user doesn't provide a file but provide a secret we want the file to be set.

Also I don't see a case where the encryptionKeyFile would exist without a k8s secret but maybe I'm wrong

Did I forget something? :D

If I try to translate it to a yaml self explanatory, I can draft a proposal that in my feeling would be less confusing for the user and easier to develop on your side. WDYT?
security: 
  encryptionKey: ""
  encryptionKeyFile: 
    activated: false # this is to ease the configuration if a user want to setup the secret but activate it or not easily
    secretName: '' # default to perses-full-name encryptionsecret
    secretKey: 'key' # (setting the default in the values file directly would prevent from forgetting a "| default" in the code)
Note:
For the mutually exclusive fields, you could do a check with helm on top of the templates using them. It would save you from some checks.
{{- if and .Values.security.encryptionKey .Values.security.encryptionKeyFile.activated }}
  {{- fail ( printf "encryptionKey and encryptionKeyFile are mutually exclusive" ) }}
{{- end }}

Hey @celian-garcia ,
Thanks for the feedback. Your suggestion simplifies the UX and also the wiring on the templates. Still, unless I misunderstood your suggestion I don't think encryptionKey and encryptionKeyFile.activated are mutually exclusive as you commented.

To ensure we are on the same page, the following are the ways I believe were discussed to make the encryption key available to perses.

Set in encryption_key on the config file;
New secret, holding encryption key, mount as a file somewhere in the deployment resource and encryption_key_file set in the config file;
Existing secret, holding encryption key, mount as a file somewhere in the deployment resource and encryption_key_file set in the config file;

The following is how each method of setting the encryption key would be configured with your suggested YAML config.

security: 
  encryptionKey: "asdasdas"
  encryptionKeyFile: 
    activated: false
    secretName: ''
    secretKey: 'key'

security: 
  encryptionKey: "asdasdas"
  encryptionKeyFile: 
    activated: true
    secretName: ''
    secretKey: 'key'

  encryptionKey: ""
  encryptionKeyFile: 
    activated: true
    secretName: 'secret-asd'
    secretKey: 'key'

It works nicely for 1 and 3 and they don't have "mutually" exclusive fields used, however, for 2 encryptionKey and encryptionKeyFile.activated are needed at the same time for a new secret to be created.

Also, there's no way to set the path where the encryption key file would be mounted but that's not an issue as a default like /etc/perses/security/encryptionkey could be used.

celian-garcia · 2024-06-04T13:53:08Z

Hey @celian-garcia , Thanks for the feedback. Your suggestion simplifies the UX and also the wiring on the templates. Still, unless I misunderstood your suggestion I don't think encryptionKey and encryptionKeyFile.activated are mutually exclusive as you commented.

To ensure we are on the same page, the following are the ways I believe were discussed to make the encryption key available to perses.

Set in encryption_key on the config file;

New secret, holding encryption key, mount as a file somewhere in the deployment resource and encryption_key_file set in the config file;

Existing secret, holding encryption key, mount as a file somewhere in the deployment resource and encryption_key_file set in the config file;

The following is how each method of setting the encryption key would be configured with your suggested YAML config.
security: 
  encryptionKey: "asdasdas"
  encryptionKeyFile: 
    activated: false
    secretName: ''
    secretKey: 'key'
security: 
  encryptionKey: "asdasdas"
  encryptionKeyFile: 
    activated: true
    secretName: ''
    secretKey: 'key'
  encryptionKey: ""
  encryptionKeyFile: 
    activated: true
    secretName: 'secret-asd'
    secretKey: 'key'
It works nicely for 1 and 3 and they don't have "mutually" exclusive fields used, however, for 2 encryptionKey and encryptionKeyFile.activated are needed at the same time for a new secret to be created.

Also, there's no way to set the path where the encryption key file would be mounted but that's not an issue as a default like /etc/perses/security/encryptionkey could be used.

I'm reading myself again and I don't even know what I meant 😄 It's been a while.
Now reading myself I'm challenging the existence of encryptionKey field. It should always use a secret no?

I would even do now

encryptionKey:
  secret: ''   // Default value is {{perses full name}}
  useExistingSecret: false // by default it creates a secret
  key: 'key' // Default value that you've no reason to update if you don't have your own secret
  value: 'dfkskdfjhskdfjh' // this is the default value that you can use but not advised.

Then if you want to reuse a secret

encryptionKey:
  secret: 'myAlreadyExistingSecret' 
  useExistingSecret: true
  key: 'myEncryptionKey'

If you want to create a secret with default props

encryptionKey:
  value: 'mysecretvalue'

If you want to create a secret with custom props

encryptionKey:
  secret: 'myNotExistingCustomSecretName' // will be created
  key: 'mySecretKey' // will be created
  value: 'mysecretvalue'

Gabrielopesantos force-pushed the gabrielopesantos/helm-chart-update branch from c776b96 to c4a19be Compare February 5, 2024 23:46

Gabrielopesantos changed the title ~~WIP - [ENHANCEMENT] Enable setting an encryption key to be used to encrypt/decrypt sensitive data~~ [ENHANCEMENT] Enable setting an encryption key to be used to encrypt/decrypt sensitive data Feb 18, 2024

Gabrielopesantos added 3 commits February 18, 2024 22:15

Support encryption_key and encryption_key_file values being set

6e6cb19

encryption key is loaded as a env var or file depending if encryption_key_file is set or not Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>

Fix errors with names and file paths

e5c43cb

Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>

Remove providers from schema json

45de2e2

Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>

Gabrielopesantos force-pushed the gabrielopesantos/helm-chart-update branch from 6427d66 to 45de2e2 Compare February 18, 2024 22:15

Add missing checks to secret to be mount

18aaacb

Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>

Gabrielopesantos force-pushed the gabrielopesantos/helm-chart-update branch from 1ff4a56 to 18aaacb Compare February 18, 2024 22:39

Fix typos in 'if' conditions and move set of encryption_key to its on…

209a860

… block Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>

Gabrielopesantos force-pushed the gabrielopesantos/helm-chart-update branch from 01aec23 to 209a860 Compare February 18, 2024 22:55

Gabrielopesantos marked this pull request as ready for review February 18, 2024 22:56

Gabrielopesantos requested a review from Nexucis as a code owner February 18, 2024 22:56

Nexucis requested review from Gladorme and nicolastakashi February 19, 2024 09:43

nicolastakashi reviewed Feb 20, 2024

View reviewed changes

Remove new line at end of values.yaml

29efacc

Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>

Gabrielopesantos force-pushed the gabrielopesantos/helm-chart-update branch from 5d55c4c to 51ebb75 Compare February 26, 2024 21:26

Add encryption key fields with an empty string to values.yaml

88edc9b

Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>

Gabrielopesantos force-pushed the gabrielopesantos/helm-chart-update branch from 51ebb75 to 88edc9b Compare February 26, 2024 22:02

Gabrielopesantos force-pushed the gabrielopesantos/helm-chart-update branch from bc655ee to 110590f Compare April 7, 2024 22:45

Gabrielopesantos force-pushed the gabrielopesantos/helm-chart-update branch from 110590f to 0f0d417 Compare April 19, 2024 12:12

Enable using an existing secret with to enable encryption of sensitive

84ccd5d

data Signed-off-by: Gabriel Santos <gabrielopesantos97@gmail.com>

Gabrielopesantos force-pushed the gabrielopesantos/helm-chart-update branch from 0f0d417 to 84ccd5d Compare April 25, 2024 12:15

celian-garcia reviewed Apr 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENHANCEMENT] Enable setting an encryption key to be used to encrypt/decrypt sensitive data #13

[ENHANCEMENT] Enable setting an encryption key to be used to encrypt/decrypt sensitive data #13

Gabrielopesantos commented Feb 5, 2024 •

edited

Loading

nicolastakashi Feb 20, 2024

Gabrielopesantos Feb 26, 2024

celian-garcia Apr 30, 2024

nicolastakashi Feb 20, 2024

Gabrielopesantos Feb 26, 2024 •

edited

Loading

Nexucis Feb 28, 2024

nicolastakashi Feb 20, 2024

Gabrielopesantos Feb 26, 2024

Nexucis Feb 28, 2024

nicolastakashi Feb 20, 2024

nicolastakashi commented Feb 20, 2024

Gabrielopesantos commented Feb 22, 2024

nicolastakashi commented Apr 9, 2024

Nexucis commented Apr 22, 2024

Gabrielopesantos commented Apr 25, 2024 •

edited

Loading

Nexucis commented Apr 30, 2024

celian-garcia left a comment

celian-garcia Apr 30, 2024

celian-garcia Apr 30, 2024

celian-garcia Apr 30, 2024

Gabrielopesantos commented May 30, 2024 •

edited

Loading

celian-garcia commented Jun 4, 2024

		@@ -0,0 +1,17 @@
		{{- if and .Values.config.security.encryptionKeyFile .Values.config.security.encryptionKey }}

[ENHANCEMENT] Enable setting an encryption key to be used to encrypt/decrypt sensitive data #13

Are you sure you want to change the base?

[ENHANCEMENT] Enable setting an encryption key to be used to encrypt/decrypt sensitive data #13

Conversation

Gabrielopesantos commented Feb 5, 2024 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gabrielopesantos Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolastakashi commented Feb 20, 2024

Gabrielopesantos commented Feb 22, 2024

nicolastakashi commented Apr 9, 2024

Nexucis commented Apr 22, 2024

Gabrielopesantos commented Apr 25, 2024 • edited Loading

Nexucis commented Apr 30, 2024

celian-garcia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gabrielopesantos commented May 30, 2024 • edited Loading

celian-garcia commented Jun 4, 2024

Gabrielopesantos commented Feb 5, 2024 •

edited

Loading

Gabrielopesantos Feb 26, 2024 •

edited

Loading

Gabrielopesantos commented Apr 25, 2024 •

edited

Loading

Gabrielopesantos commented May 30, 2024 •

edited

Loading