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.

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

Configurable custom CA #17

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

Conversation

ondrejtomcik
Copy link

Dear smallstep team,
I was missing the functionality to set my custom CA certificate. Here is the proposal how it could be done.
Looking forward to your comments.

Copy link
Collaborator

@maraino maraino left a comment

Choose a reason for hiding this comment

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

Hi @ondrejtomcik, thanks for the PR, I've added some comments so we can have more flexibility to add the sources of the root certificate and key.

@@ -60,6 +60,7 @@ chart and their default values.
| `ca.db.size` | Persistent volume size | `10Gi` |
| `ca.runAsRoot` | Run the CA as root. | `false` |
| `ca.bootstrap.postInitHook` | Extra script snippet to run after `step ca init` has completed. | `""` |
| `ca.bootstrap.rootCA.secret`| Name of the custom root CA secret (k8s tls secret) to be used. | `""` |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you split the root certificate and key in two configurations, and add name and key for both?

Although ideally, the both values should be YAML objects that you can set as you want, i.e

- name: step-certificate-ca-volume
   secret: 
     name: root

but also

- name: step-certificate-ca-volume
  configMap:
    name: certificates

or even more complex configurations like

- name: step-certificate-ca-volume
  configMap:
    name: certificates
    items: 
      - key: ROOT_CERT
        path: root_ca.crt

So basically, be able to define a full reference to anything you want. The volume names should be something fixed like the one you're proposing, {{ include "step-certificates.fullname" . }}-ca-volume, but the reference should be a YAML object instead of just a string.

Copy link
Author

@ondrejtomcik ondrejtomcik May 18, 2020

Choose a reason for hiding this comment

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

Hello @maraino . Thanks for your comments. Agree with parametrizing cert and key file names, but disagree with using configmap. Correct is to use kubernetes secret, which can be still used and meet your requirements as well.
I would propose to use generic k8s secret as follows:
kubectl create secret generic my-root-ca --from-file=certs/root_ca.crt --from-file=certs/root.key

and passing to smallstep helmchart rootCA.secret.name, rootCA.cert rootCA.key.
where values for my example would be: rootCA.secret.name: my-root-ca, rootCA.cert: root_ca.crt and rootCA.key: root.key

Do you agree?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The certificate itself is not really secret, but what I wanted address here is to be able to mount from CSI interfaces, and get the key from for example Vault.

To simplify the integration makes sense to just add one volume instead of two. Both a secret and CSI can have multiple objects.

- name: {{ include "step-certificates.fullname" . }}-ca-volume
secret:
secretName: "{{ .Values.ca.bootstrap.rootCA.secret }}"
{{- end }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, this will be two volumes, one for the certificate, and one for the key, and the code for one of them would look like:

volumes: 
{{ - if .Values.ca.bootstrap.rootCertRef }}
- name: {{ include "step-certificates.fullname" . }}-root-volume
 {{- toYaml .Values.ca.bootstrap.rootCertRef | nindent xx }}
{{- end}}

And one similar for the key, but you'll have the flexibility to define both cert and key in just one volume.

Copy link
Author

Choose a reason for hiding this comment

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

Based on my reply above it should be just one volume, following standard k8s secret.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just one volume, but being able to mount from different sources. I think this would make it:

{{- toYaml .Values.ca.bootstrap.rootRef | nindent xx }}

Can you call it rootRef? or something similar to make clear that is a reference.

@@ -107,7 +107,7 @@ data:
--provisioner "{{.Values.ca.provisioner.name}}" \
--with-ca-url "{{include "step-certificates.url" .}}" \
--password-file "$TMP_CA_PASSWORD" \
--provisioner-password-file "$TMP_CA_PROVISIONER_PASSWORD" {{ if not .Values.ca.db.enabled }}--no-db{{ end }}
--provisioner-password-file "$TMP_CA_PROVISIONER_PASSWORD" {{ if not .Values.ca.db.enabled }}--no-db{{ end }} {{ if .Values.ca.bootstrap.rootCA.secret }}--root /tmp/certs/tls.crt --key /tmp/certs/tls.key{{ end }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add it in a new line, and now that you're on it, can you also add the --no-db to a new line :)

Copy link
Author

Choose a reason for hiding this comment

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

Actually, not sure how as \ has to be on previous line, but only if values are available. Not sure how to parametrize new line properly. Any idea ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

As we only adding one volume, we can not just use bash for this without making it complex, so I think you should name it in the way that step names those files when you do step ca init, so root_ca.crt and root_ca_key.

You can add variables for those, and you can leave those names as default because you can do the if with the rootRef.

@maraino maraino self-assigned this May 18, 2020
@maraino
Copy link
Collaborator

maraino commented May 18, 2020

@ondrejtomcik I think I've addressed all your questions.

@maraino
Copy link
Collaborator

maraino commented May 26, 2020

@ondrejtomcik are you going to add to this PR the support for a custom volume definition as I suggested?

@ondrejtomcik
Copy link
Author

Yes, but not this week. Sorry.

@maraino maraino removed their assignment Aug 14, 2020
@ondrejtomcik
Copy link
Author

Sorry, it was de-prioritized a bit on our end @maraino . I will come back to it during the next week and finalize it.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants