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.

Sign up for GitHub

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

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6e6cb19
Support encryption_key and encryption_key_file values being set
Gabrielopesantos Jan 22, 2024
e5c43cb
Fix errors with names and file paths
Gabrielopesantos Feb 5, 2024
45de2e2
Remove providers from schema json
Gabrielopesantos Feb 18, 2024
18aaacb
Add missing checks to secret to be mount
Gabrielopesantos Feb 18, 2024
209a860
Fix typos in 'if' conditions and move set of encryption_key to its on…
Gabrielopesantos Feb 18, 2024
29efacc
Remove new line at end of values.yaml
Gabrielopesantos Feb 26, 2024
88edc9b
Add encryption key fields with an empty string to values.yaml
Gabrielopesantos Feb 26, 2024
84ccd5d
Enable using an existing secret with to enable encryption of sensitive
Gabrielopesantos Feb 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion charts/perses/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ description: Perses helm chart
icon: https://avatars.githubusercontent.com/u/77209215?s=200&v=4
type: application
version: 0.3.0
appVersion: "0.42.1"
appVersion: "0.43.0"
sources:
- https://github.com/perses/perses
annotations:
Expand Down
10 changes: 8 additions & 2 deletions charts/perses/templates/config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,12 @@ data:
config.yaml: |-
security:
readonly: {{ .Values.config.security.readOnly }}
{{- if and .Values.config.security.encryptionKeyFile .Values.config.security.encryptionKey }}
Copy link
Member

Choose a reason for hiding this comment

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

not possible as mutually exclusive

encryption_key_file: {{ printf "%s/key" (.Values.config.security.encryptionKeyFile | trimSuffix "/") }}
Copy link
Member

Choose a reason for hiding this comment

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

You're not taking into account your override* fields

{{- end }}
{{- if and (not .Values.config.security.encryptionKeyFile) .Values.config.security.encryptionKey }}
Copy link
Contributor

Choose a reason for hiding this comment

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

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 ?

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

encryption_key: {{ .Values.config.security.encryptionKey }}
{{- end }}
enable_auth: {{ .Values.config.security.enableAuth }}

database:
Expand All @@ -24,12 +30,12 @@ data:
sql:
{{- tpl (toYaml .) $ | nindent 8 }}
{{ end -}}

{{- with .Values.config.important_dashboards }}
important_dashboards:
{{- toYaml . | nindent 6 }}
{{- end }}

{{- with .Values.config.schemas }}
schemas:
{{- toYaml . | nindent 6 }}
Expand Down
17 changes: 17 additions & 0 deletions charts/perses/templates/secrets.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{{- if and .Values.config.security.encryptionKeyFile .Values.config.security.encryptionKey }}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

@Gabrielopesantos Gabrielopesantos Feb 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

apiVersion: v1
kind: Secret
metadata:
name: {{ include "perses.fullname" . }}-encryption-key
labels:
{{- include "perses.labels" . | nindent 4 }}
{{- with .Values.config.annotations }}
annotations:
{{- toYaml . | nindent 4 }}
{{- end }}
type: Opaque
data:
key: {{ .Values.config.security.encryptionKey | b64enc }}
{{- end }}

---
15 changes: 14 additions & 1 deletion charts/perses/templates/statefulset.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ spec:
- name: datasources
mountPath: /etc/perses/datasources
{{- end }}
{{- if and .Values.config.security.encryptionKeyFile .Values.config.security.encryptionKey }}
- name: encryptionkey
mountPath: {{ .Values.config.security.encryptionKeyFile }}
readOnly: true
{{- end }}
ports:
- name: http
containerPort: {{ .Values.service.targetPort }}
Expand Down Expand Up @@ -97,4 +102,12 @@ spec:
- name: datasources
configMap:
name: {{ include "perses.fullname" . }}-datasources
{{- end }}
{{- end }}
{{- if and .Values.config.security.encryptionKeyFile .Values.config.security.encryptionKey }}
- name: encryptionkey
secret:
secretName: {{ include "perses.fullname" . }}-encryption-key
items:
- key: key
path: "key"
{{- end }}
8 changes: 8 additions & 0 deletions charts/perses/values.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,14 @@
"type": "boolean",
"default": false
},
"encryptionKey": {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

"type": "string",
"default": ""
},
"encryptionKeyFile": {
"type": "string",
"default": ""
},
"enableAuth": {
"type": "boolean",
"default": false
Expand Down
2 changes: 1 addition & 1 deletion charts/perses/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,4 +191,4 @@ datasources:
# plugin:
# kind: PrometheusDatasource
# spec:
# directUrl: https://prometheus.demo.do.prometheus.io
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 ^^

# directUrl: https://prometheus.demo.do.prometheus.io
Loading