WIP: feat: add support for TLS and extra Certificate Authorities #657
1 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: lunny/helm-chart#657
Reference in New Issue
Block a user
No description provided.
Delete Branch "joshsizer/add-tls-and-extra-ca-support"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Description of the change
This change has two features associated:
Benefits
The benefits of this change is that if a user is using PKI, with a private Certificate Authority, they can utilize the PKI with Gitea and its dependencies
Possible drawbacks
This will not generate the PKI for you - you must provide it (using cert-manager makes this easy)
Checklist
values.yamland added to theREADME.mdusing readme-generator-for-helmREADME.md(not available here - no breaking changes)Hey @joshsizer, thanks for contributing!
I think both of your suggested additions are already possible right now:
b13063ad7a/values.yaml (L174)extraVolumeMountsandextraVolumesDid you see/try these upfront and if so, why weren't they sufficient for your use case?
Hey @pat-s, thanks for taking a look at this MR!
I'll address both of points you brought up:
The secret for TLS you referenced is for an Ingress, which is a different situation than what this MR is attempting to solve for. Currently, this chart doesn't support in a first-class way enabling TLS on the Gitea server itself. If a user of the chart wants to implement Pod to Pod TLS, without using something like Istio, they have to do so through
extraVolumesandextraVolumeMounts. While this is possible, it's not a user-friendly interface - I took a lot of inspiration for the TLS changes from bitnami charts like Redis and MongoDB, which support setting TLS on the servers themselves, which is different than at the ingress level. Happy to expand on this if needed.Unfortunately, simply adding additional CA bundles via
extraVolumesandextraVolumeMountsdoes not work. You need to place the CA bundles on the correct place in the file system, and call theupdate-ca-certificatescommand. This MR helps the user mount the CAs in the correct place, as well as add some initContainers that call theupdate-ca-certificatescommand. Additionally, this was tested with the rootless image, which is why there is an extra initContainer to copy the default system certs & additional certs to an emptyDir volume with permissions that respect the securityContexts.So, I think this MR does add a lot of value to the chart, since it gives users an easy interface to enabling TLS on the Gitea server (for inter-pod TLS), as well as adds the ability to trust additional CAs.
There is a way to properly mount custom CAs into Gitea containers. https://github.com/go-gitea/gitea/issues/14102#issuecomment-1305973019
The referenced values are now located at https://github.com/cmu-sei/foundry-appliance/blob/main/foundry%2Fgitea.values.yaml
But I am open to adding a value setting to simplify mounting custom CAs into the correct place. @pat-s, you too?
I just tried this locally, without calling update-ca-certificates and it worked! (mounting the certs to /etc/ssl/certs) Thanks for the pointer.
I can go ahead and update the MR to simplify this.
@justusbunsi What do you think about simplifying setting Gitea up for serving TLS (the other set of changes in this MR)?
Hrm. On the one hand I see the benefit and understand your use case. But I also agree with @pat-s that this is already possible using the extraVolumes/extraVolumeMounts option.
Like with custom CAs, I personally think a dedicated option helps setting up Gitea in a consistent way. However, the current approach looks like it would somehow conflict with
ingress.tls. Havingtls.enabled: truebut not settingingress.tlswhileingress.enabled: trueresults in an ingress routing from a non-https port to a https port. So there has to be some sort of failsafe between both options. And right now I am not sure how to handle that conflict.But before continuing this part, let's wait for @pat-s thoughts on it. I am open to both, as long as it simplifies the overall configuration by adding new options.
And a heads up: In case we agree on adding both options, please split this PR into two. One for CAs and one for tls.
I wanted to point out this general approach as well, but I wouldn't have had a link that even references a blog post.
I also know the pain of calling
update-ca-certificatesandupdate-ca-trust(RHEL) but I've also found out in the last year that every distro has their specific locations which "auto-add" the certs in them to the trust-store. Not sure if this is something that existed forever or has been added recently but maybe it is also just that the websearch is still dominated by the call "update-ca-xy" approach.Yes and no.
Yes: Sure, simplifying the workflow to add/mount custom resources is always great from a user PoV.
No: But we also need to make sure to follow "best practices" WRT to helm chart templating/structuring to make it easy to use the chart.
I.e. I am not aware of other charts right now that have a dedicated config option to mount custom CA's. The "canonical" way to add such is by using
extraVolumeMountsand friends. These exist in every chart. And if one has to do this once, chances are high one needs to do this multiple times. And if this holds true, one can just c/p the YAML which is .I am managing a lot of chart-based deployments myself and every time I come across a "custom" chart which does everything different in terms of naming and overall structure, I am a bit 🥴️
A good overall reference are the bitnami charts as they represent a quasi standard in the field due to their popularity.
In the end I like the idea of generic
extraVolumeMountsandextraVolumesas they allow doing pretty much anything that is needed in private environments WRT to custom assets. It is important that they allow both referencing k8s-secrets and other input types.So for your use case, you should be able to do both the internal TLS communication handling and CA cert parsing using the existing
extraContainerVolumeMounts. If not, we can certainly help/find a way to get this going for you.@justusbunsi Let me know what your thoughts are on the topic of "custom chart additions" and "best practices" I shared above. This is of course off-topic here somewhat but also an important point overall.
I'm very sympathetic to following best practices - helm charts can become unruly very quickly.
I would like to mention that some bitnami charts, like Redis, MongoDB, and PostgreSQL support enabling TLS on the server (they will mount the secrets to the pods for you).
On the CA side, I actually feel like this is a feature that is missing from these charts. While I understand you can accomplish adding extra CAs through
extraVolumesandextraVolumesMounts, I've always perceived these as escape hatches for the user - for when the chart doesn't handle something they need in a first class way. It would always be preferable if the chart handled it in a first class way.Of course, a lot of the ways people use these escape hatches in charts are pretty unique to them, but I don't think the ability to enable TLS or to add CAs is all that esoteric.
It seems we all aim for a rock-solid solution and are fine with following best-practices. So here's my suggestion:
extraVolumesandextraVolumeMountswould be a great example for the Chart documentation. Either atdocs/or within the README. That should probably eliminate the implementation needs.ingress.tlshandling when configuring Gitea. As mentioned in https://gitea.com/gitea/helm-chart/pulls/657#issuecomment-824334, that is missing and might be the initial intention to add the new settings. @joshsizer please correct me if I'm wrong here.I understand where you're coming from. I also don't know why things are as they are, but what I can think of is:
extraVolumeMounts/extraVolumesIngress TLS: Agree with @justusbunsi
Hey folks, actually just ran into an issue with the way this MR sets up CAs - first off it only handles secrets - what if a CA is passed in as a config map? What if it has a different key (ca.pem vs ca.crt)? So I am totally onboard with ya'll, let's just let users configure the CAs through extraVolumes and extraVolumeMounts.
I will look into how to better check for ingress TLS vs Gitea TLS conflicts. With NGINX, you can add an annotation:
nginx.ingress.kubernetes.io/backend-protocol: HTTPS, but I'm not sure if there is an implementation agnostic way to configure this. Not sure if you guys have any input here?Due to the variety of backends, ingress often works through annotations, like the one shown for NGINX. I don't have much experience with this specific one, though.
I didn't check in detail all comments above again, but in my case, I usually use
ingress.tls[0].secretNameand reference to a secret named<domain>-tls(which in my case is generated bycertmanager). This is combined with theingress-nginxchart. AFAIU though you want to run Gitea itself with HTTPS directly rather than HTTP so the connection between the ingress controller and the service endpoint is also encrypted.I've done this a few times in some environments (but not for Gitea) and always found it quite a hassle. But it might/should be possible.
Sorry for the hiatus, I am still interested in working on this. Hopefully will get a chance to work on this in the next few weeks
Checkout
From your project repository, check out a new branch and test the changes.