Support custom CA certificates in Helm #2984
Conversation
|
@corneliusludmann @geropl @stefanstoeckigt Please review this PR for custom CA certs. |
|
@jgallucci32 Thanks a lot for your contribution. I really appreciate this! I'm currently organizing a review. Thanks for your patience and sorry for the delay. |
|
hi @jgallucci32, thank you for your contribution! IIRC We haven't documented it yet, but we'll need a CLA before we can merge this, I'll reach out to you about it via email. |
|
@meysholdt Yes you are correct. It is expected to be a full bundle inclusive of your custom certs. This is typically done by adding your custom certs to a local Linux OS (i.e. /usr/local/share/ca-certificates) and running |
|
@jgallucci32, the ca volume and NODE_EXTRA_CA_CERTS should also be added to the workspace-template.yaml. This will allow the workspace pod to have access to the ca. Without the NODE_EXTRA_CA_CERTS in the workspace pod you wont be able to download any vs-code extensions when the workspace is running. |
|
@stefanstoeckigt Interesting I hadn't thought about adding it there. We just repackaged the public Do you have a recommendation as to where to add it in |
|
@jgallucci32, i think adding it under the coreWorkspaceConfig section, like below maybe. {{- define "coreWorkspaceConfig" }}
{{- $comp := .comp -}}
spec:
{{- if $comp.pullSecret }}
imagePullSecrets:
- name: {{ toYaml $comp.pullSecret.secretName }}
{{- end }}
{{- if .Values.caBundleSecretName }}
containers:
- name: workspace
volumeMounts:
{{ include "gitpod.caBundleVolumeMount" . | indent 8 }}
env:
- name: NODE_EXTRA_CA_CERTS
value: /etc/ssl/certs/ca-certificates.crt
volumes:
{{ include "gitpod.caBundleVolumeMount" . | indent 8 }}
{{- end }} |
|
@meysholdt @corneliusludmann what are your thoughts about also including the proposed changes for |
Signed-off-by: jgallucci32 <[email protected]>
jgallucci32
force-pushed
the
issue-2615
branch
from
3cae036 to
5930420
Compare
jgallucci32
force-pushed
the
issue-2615
branch
from
0f7d3b8 to
41ab5c0
Compare
|
In image-builder-deployment.yaml, {{- if .Values.caBundleSecretName }} should be replaced with {{- if $.Values.caBundleSecretName }} - to reference the global scope. |
There was a problem hiding this comment.
Hi @jgallucci32,
First, an apology that we've been so slow with your great PR. Internally, we are not yet so well attuned to the processing of community PRs. But we will get better. I promise! 😃
I left a couple of comments. Please have a look.
For the workspace-template.yaml file, I think it makes sense to add the custom CA as well. This diff works for me:
diff --git a/chart/templates/workspace-template.yaml b/chart/templates/workspace-template.yaml
index 80beebb3..a012a84b 100644
--- a/chart/templates/workspace-template.yaml
+++ b/chart/templates/workspace-template.yaml
@@ -10,6 +10,19 @@ spec:
imagePullSecrets:
- name: {{ toYaml $comp.pullSecret.secretName }}
{{- end }}
+ {{- if .gp.caBundleSecretName }}
+ containers:
+ - name: workspace
+ volumeMounts:
+{{ include "gitpod.caBundleVolumeMount" . | indent 4 }}
+ env:
+ - name: NODE_EXTRA_CA_CERTS
+ value: /etc/ssl/certs/ca-cert-gitpod.crt
+ - name: REQUESTS_CA_BUNDLE
+ value: /etc/ssl/certs/ca-cert-gitpod.crt
+ volumes:
+{{ include "gitpod.caBundleVolume" .root | indent 2 }}
+ {{- end }}
{{- end }}
{{- define "coreWorkspaceAffinity" -}}
{{- $comp := .comp -}}I also suggest, that we add an empty caBundleSecretName: property to the chart/values.yaml file (probably below certificatesSecret: …). Thus, we have some kind of documentation that the configuration value exist.
If you don't have time for this PR right now, please let me know. Then I will gladly take over.
|
|
||
| {{- define "gitpod.caBundleVolumeMount" -}} | ||
| - name: ca-bundle-certs | ||
| mountPath: /etc/ssl/certs/ca-certificates.crt |
There was a problem hiding this comment.
What do you think about mounting just the root CA with a unique name (e.g. ca-cert-gitpod.crt) in the folder /etc/ssl/certs/ instead of the full ca-certificates.crt (addresses the comments #2984 (comment) and #2984 (comment))?
| mountPath: /etc/ssl/certs/ca-certificates.crt | |
| mountPath: /etc/ssl/certs/ca-cert-gitpod.crt |
Then, you don't need to run update-ca-certificates on a local Linux and can add your root CA directly. In my tests, that works pretty fine.
| @@ -64,6 +64,9 @@ spec: | |||
| secret: | |||
| secretName: {{ $sec.secret }} | |||
| {{- end }} | |||
| {{- if .Values.caBundleSecretName }} | |||
There was a problem hiding this comment.
As mentioned in #2984 (comment) by another user, this does not work for me too but need to be this:
| {{- if .Values.caBundleSecretName }} | |
| {{- if $.Values.caBundleSecretName }} |
| @@ -78,6 +81,9 @@ spec: | |||
| {{- range $idx, $sec := $comp.registryCerts }} | |||
| - mountPath: /etc/docker/certs.d/{{- if eq $sec.name "builtin" -}}{{ template "gitpod.builtinRegistry.name" $this.root }}{{ else }}{{ $sec.name }}{{ end }} | |||
| name: docker-tls-certs-{{ $idx }} | |||
| {{- if .Values.caBundleSecretName }} | |||
There was a problem hiding this comment.
Same here (#2984 (comment))
| {{- if .Values.caBundleSecretName }} | |
| {{- if $.Values.caBundleSecretName }} |
| {{- end }} | ||
| {{- if .Values.caBundleSecretName }} | ||
| - name: NODE_EXTRA_CA_CERTS | ||
| value: /etc/ssl/certs/ca-certificates.crt |
There was a problem hiding this comment.
If we change the cert name in the _helper.tpl file (see above) we need to change it here as well, right?
| value: /etc/ssl/certs/ca-certificates.crt | |
| value: /etc/ssl/certs/ca-cert-gitpod.crt |
|
@corneliusludmann Thanks for getting back, I'll try to make some time to put in the suggested changes. I'll run through our setup to verify the alternate location for the CA Bundle works without changes (other than the node.js env var). I've always thought you had to use the specific |
|
@corneliusludmann I have good news and bad news. I did some preliminary testing on using a different file for the custom CA certs and validated all but the The Long term, this is something the folks at Gitpod need to take into consideration. Option A (current PR) overwrites the bundled CA Certs making it more straightforward to fix certs, but requires all certs to be in the bundle. Option B (suggested change) uses a different CA bundle, but requires use of special environment variables for each client library needing to connect to an endpoint with a custom SSL cert. This method is more explicit, but does not guarantee the bundle can only contain additive certs as some client libraries when using an env var will completely ignore the local ca bundle, defeating the purpose of keeping the bundle to only what you need (node.js does it right with the "EXTRA" approach, but not all clients do this). Here is the exact error I see in |
|
@jgallucci32 @corneliusludmann can we align with Kubernetes and use a TLS secret? If the secret contains a custom CA, it must be concatenated in the |
@aledbf Kubernetes TLS secrets are primarily used with TLS termination of Ingress resources and requires both a public certificate and private key during creation. Since a custom CA bundle only includes public certificates, it would not make sense to create a TLS secret where you would have to fake the private key in order to create the secret (which may cause issues) Here is what happens when attempting to create a TLS secret for a CA bundle $ kubectl create secret tls my-tls-secret --cert=/etc/ssl/certs/ca-certificates.crt
error: flag key is required
See 'kubectl create secret tls -h' for help and examples |
Git also supports // We need to tell Git where to find CA certs in case someone added custom CA certs
// to ws-daemon. We're re-using ws-daemon's /etc above.
spec.Process.Env = append(spec.Process.Env, "GIT_SSL_CAPATH=/etc/ssl/certs")should do the trick. |
@csweichel: Thanks for the hint. I tried this, but this doesn't work. At first, I think we need to add this here instead: like: env = append(env, "GIT...=....")However, I don't get if stat, err := os.Stat("/etc/ssl/certs/ca-cert-gitpod.crt"); err == nil && stat.Size() > 0 {
// We need to tell Git where to find CA certs in case someone added custom CA certs.
env = append(env, "GIT_SSL_CAINFO=/etc/ssl/certs/ca-cert-gitpod.crt")
}but that breaks cloning from github.com (because it uses the EDIT: Ah, that's the reason why |
|
Hi guys, Will this feature eventually integrated in the main branch one day ? Is there a plan for that ? Thanks Regards |
|
@zazizou This is still on my to-do list. This PR was written against version 0.6.0 so in addition to a rebase I am going to need to deploy the latest version in a test cluster to validate it still works as intended. I'm trying to get some time to do this as soon as I can. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This is still needed. |
|
what about accepting the working solution with the complete bundle first, and only afterwards go for the sophiticated solution? This is a major blocker to use gitpod in internal networks. |
|
That is my plan with this MR. My preference is to expect a full bundle and replace the OS certs as that has less maintenance overhead. Anything more sophisticated should come later.
In terms of being blocked, there are workarounds which we’ve been running for 6 months without issue. If you need me to repost them here let me know.
Right now I’m working a complete redeploy as 0.10.0 no longer works on RHEL 8 due to Kernel 5.2 requirement. Looking to get this validated on Ubuntu.
…Sent from my iPhone
On Sep 17, 2021, at 02:25, SirLemyDanger ***@***.***> wrote:
what about accepting the working solution with the complete bundle first, and only afterwards go for the sophiticated solution? This is a major blocker to use gitpod in internal networks.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
|
@jgallucci32 First, thanks a lot for your effort on this MR. And yes, I would be interested in your workarounds. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@corneliusludmann Is #4322 addressing/solving this issue? |
Looking at the PR it is solving a different problem. It looks like cert-manager is being implemented using its default setup of generating certificates from known sources (i.e. Letsencrypt). If it is being used to sign a self-signed or custom CA, then the certificate stores need to be updated using the methods outlined in this PR. FYI, I am ready to work this PR but am stuck. 0.10.0 is the latest release but the master contains the changes for image-builder-mk3 which is something I cannot test. How do I deploy the latest helm chart for master? I only know how to deploy what is helm which is 0.10.0 currently. |
|
Hi @jgallucci32. I need to update the documentation for self-hosted to work with Image Builder Mk3. I'll schedule this work for tomorrow |
|
@SirLemyDanger See #2615 (comment) for |
|
Hi @jgallucci32. I've had a look at releasing from The |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
/make not stale or something
From: stale[bot] ***@***.***>
Sent: Saturday, October 16, 2021 7:13 AM
To: gitpod-io/gitpod ***@***.***>
Cc: Josiah Ritchie ***@***.***>; Manual ***@***.***>
Subject: Re: [gitpod-io/gitpod] Support custom CA certificates in Helm (#2984)
[External: This message originated outside SOLUTE. Click links or open attachments only if you recognize the sender and know the contents are safe.]
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <#2984 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA5JFL65H4KWQ7IXVXB47DUHFM3VANCNFSM4WL6PFFQ> .
|
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@jgallucci32 Do you mind if I pick up you commit and finish this job again? |
|
@iQQBot I don't mind, I have no way to test this right now since the I can certainly review your updated PR. |
|
Great,thank you so much. |
|
Thanks for coming back to this @jgallucci32! @iQQBot feel free to cherry pick any of the commits from @jgallucci32 if changes here are still relevant and open a follow up PR for this. 🏀 |
|
/assign |
Adds support to Helm charts for custom CA certificates. Approach derived from from https://github.com/goharbor/harbor-helm project for variable names and formatting.
Fixes #2615
Signed-off-by: jgallucci32 [email protected]