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

Jump to bottom

configmaps to secrets, missing db config in inject block and ingress example structure. #56

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

configmaps to secrets, missing db config in inject block and ingress example structure. #56

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 step-certificates/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
apiVersion: v1
name: step-certificates
version: 1.15.17
version: 1.15.18
Copy link
Contributor

Choose a reason for hiding this comment

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

One note on the version bump.

Since this PR changed the expected format of several of the values parameters, we may want to bump a minor version instead of a patch version if anyone has taken a dependency on 1.15.17.

appVersion: 0.15.15
description: An online certificate authority and related tools for secure automated certificate management, so you can use TLS everywhere.
keywords:
Expand Down
7 changes: 6 additions & 1 deletion step-certificates/templates/ca.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,15 @@ spec:
mountPath: /home/step/db
readOnly: false
{{- end }}
volumes:
volumes:
- name: certs
{{- if .Values.inject.enabled }}
secret:
secretName: {{ include "step-certificates.fullname" . }}-certs
{{- else }}
configMap:
name: {{ include "step-certificates.fullname" . }}-certs
{{- end }}
- name: config
configMap:
name: {{ include "step-certificates.fullname" . }}-config
Expand Down
11 changes: 1 addition & 10 deletions step-certificates/templates/configmaps.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,23 +23,14 @@ data:
{{- end }}
{{- end }}
---
{{- if or .Values.bootstrap.configmaps .Values.inject.enabled }}
{{- if and .Values.bootstrap.configmaps (not .Values.inject.enabled) }}
apiVersion: v1
kind: ConfigMap
metadata:
name: {{ include "step-certificates.fullname" . }}-certs
namespace: {{ .Release.Namespace }}
labels:
{{- include "step-certificates.labels" . | nindent 4 }}
{{- if .Values.inject.enabled }}
data:
intermediate_ca.crt: |-
{{- .Values.inject.certificates.intermediate_ca | nindent 4 }}
root_ca.crt: |-
{{- .Values.inject.certificates.root_ca | nindent 4 }}
ssh_host_ca_key.pub: {{ .Values.inject.certificates.ssh_host_ca }}
ssh_user_ca_key.pub: {{ .Values.inject.certificates.ssh_user_ca }}
{{- end }}
{{- end }}
---
{{- if and .Values.bootstrap.configmaps (not .Values.inject.enabled) }}
Expand Down
28 changes: 19 additions & 9 deletions step-certificates/templates/secrets.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,23 @@ type: smallstep.com/private-keys
metadata:
name: {{ include "step-certificates.fullname" . }}-secrets
namespace: {{ .Release.Namespace }}
stringData:
intermediate_ca_key: |-
{{- .Values.inject.secrets.x509.intermediate_ca_key | nindent 4 }}
root_ca_key: |-
{{- .Values.inject.secrets.x509.root_ca_key | nindent 4 }}
ssh_host_ca_key: |-
{{- .Values.inject.secrets.ssh.host_ca_key | nindent 4 }}
ssh_user_ca_key: |-
{{- .Values.inject.secrets.ssh.user_ca_key | nindent 4 }}
data:
intermediate_ca_key: {{ .Values.inject.secrets.x509.intermediate_ca_key }}
root_ca_key: {{ .Values.inject.secrets.x509.root_ca_key }}
ssh_host_ca_key: {{ .Values.inject.secrets.ssh.host_ca_key }}
ssh_user_ca_key: {{ .Values.inject.secrets.ssh.user_ca_key }}
{{- end }}
---
{{- if .Values.inject.enabled }}
apiVersion: v1
kind: Secret
type: smallstep.com/certs
metadata:
name: {{ include "step-certificates.fullname" . }}-certs
Copy link
Contributor

Choose a reason for hiding this comment

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

@maraino I don't have an issue with this moving from ConfigMaps to Secrets if @ricosega has a compelling reason for doing so. However, part of the reason I didn't when I introduced the inject capability was that I didn't have a security reason for doing so. If Kubernetes supported TLS ingress configurations with key passwords, I would have merged these two configuration objects into a single secret to take advantage of that capability. But without that support, I kept them separate to discourage people from creating root certificates without passwords to leverage the TLS Ingress capability.

At the end of the day, my thought was that I was going to end up sprinkling the CA certificates absolutely everywhere in my environment so why does it need to be secret.

I don't have any objection to this change, but would be very interested to hear why @ricosega thinks it should move.

Copy link
Author

Choose a reason for hiding this comment

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

I agree when we say that a cert is public, the point is that probably many people can use a helm chart in a CI/CD environment, so they will have to substitute variables in the values.yaml and here it comes the problem.

A certificate or a key, because of their structure and length are difficult to store in an environment variable to be replaced later, if you try it you will get an error. The only way to do it is to save the certificate in oneline and then do tricks and things to have it the same way. Instead if you save your certificate or key in base64 in oneline it will always preserves the structure and it is easier then to replace it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ricosega Moving the ConfigMaps to secrets might be a breaking change for some users. Can you investigate if storing the data as binaryData in the ConfigMap will solve your use case?
See https://kubernetes.io/docs/concepts/configuration/configmap/#configmap-object

PS: I've been on vacation.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry for the delay I've been on vacation too.
If we do not want to create a breaking change to any current user the only thing we can do is to make it retrocompatible, keeping the previous config and adding this new one for example with a value like, isBase64Encoded = bool.
What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ricosega I think its best not to introduce a breaking change for this. I would be open to seeing a isBase64Encoded option that allows the user to pass their value base64 encoded with the value of said flag defaulting to false. The helm chart could then decode the value and place it in the configmap.

Also, you may want to update your branch to the latest master. Support for registration authorities was added while you were away and the latest version of the chart is now 1.16.1

namespace: {{ .Release.Namespace }}
data:
intermediate_ca.crt: {{ .Values.inject.certificates.intermediate_ca }}
root_ca.crt: {{ .Values.inject.certificates.root_ca }}
ssh_host_ca.pub: {{ .Values.inject.certificates.ssh_host_ca }}
ssh_user_ca.pub: {{ .Values.inject.certificates.ssh_user_ca }}
{{- end }}
41 changes: 15 additions & 26 deletions step-certificates/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@ inject:
minVersion: 1.2
maxVersion: 1.3
renegotiation: false
db:
type: "badger"
dataSource: "/home/step/db"
badgerFileLoadingMode: "MemoryMap"
defaults.json:
ca-url: https://mysteprelease-step-certificates.default.svc.cluster.local
ca-config: /home/step/config/ca.json
Expand Down Expand Up @@ -106,26 +110,20 @@ inject:

certificates:
# intermediate_ca contains the text of the intermediate CA Certificate
# This value must be base64 encoded.
intermediate_ca: ""
# intermediate_ca: |
# -----BEGIN CERTIFICATE-----
# ...
# -----END CERTIFICATE-----

# root_ca contains the text of the root CA Certificate
# This value must be base64 encoded.
root_ca: ""
# root_ca: |
# -----BEGIN CERTIFICATE-----
# ...
# -----END CERTIFICATE-----

# ssh_host_ca contains the text of the public ssh key for the SSH root CA
# This value must be base64 encoded.
ssh_host_ca: ""
# ssh_host_ca: "ecdsa-sha2-nistp384 public-key-here=="

# ssh_user_ca contains the text of the public ssh key for the SSH root CA
# This value must be base64 encoded.
ssh_user_ca: ""
# ssh_user_ca: "ecdsa-sha2-nistp384 public-key-here=="

secrets:
# ca_password contains the password used to encrypt x509.intermediate_ca_key, ssh.host_ca_key and ssh.user_ca_key
Expand All @@ -135,36 +133,24 @@ inject:

x509:
# intermediate_ca_key contains the contents of your encrypted intermediate CA key
# This value must be base64 encoded.
intermediate_ca_key: ""
# intermediate_ca_key: |
# -----BEGIN EC PRIVATE KEY-----
# ...
# -----END EC PRIVATE KEY-----

# root_ca_key contains the contents of your encrypted root CA key
# Note that this value can be omitted without impacting the functionality of step-certificates
# If supplied, this should be encrypted using a unique password that is not used for encrypting
# the intermediate_ca_key, ssh.host_ca_key or ssh.user_ca_key.
# This value must be base64 encoded.
root_ca_key: ""
# root_ca_key: |
# -----BEGIN EC PRIVATE KEY-----
# ...
# -----END EC PRIVATE KEY-----

ssh:
# ssh_host_ca_key contains the contents of your encrypted SSH Host CA key
# This value must be base64 encoded.
host_ca_key: ""
# host_ca_key: |
# -----BEGIN OPENSSH PRIVATE KEY-----
# ...
# -----END OPENSSH PRIVATE KEY-----

# ssh_user_ca_key contains the contents of your encrypted SSH User CA key
# This value must be base64 encoded.
user_ca_key: ""
# user_ca_key: |
# -----BEGIN OPENSSH PRIVATE KEY-----
# ...
# -----END OPENSSH PRIVATE KEY-----


# service contains configuration for the kubernetes service.
Expand Down Expand Up @@ -231,6 +217,9 @@ ingress:
enabled: false
annotations: {}
hosts: []
# - host: ca.example.com
# paths:
# - /
tls: []

# resources contains the CPU/memory resource requests/limits.
Expand Down