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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enabled internal TLS between k8s pods by default #401

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

Conversation

jk464
Copy link
Contributor

@jk464 jk464 commented Feb 13, 2024

Continuing on from the theme in (#400) this PR is another attempt at improving the security hardening of StackStorm in k8s.

By default - all inter-pod communication between pods within a k8s deployment use non-encrypted protocols. (i.e. Between st2 and Mongo / RabbitMQ and Redis, and to the internal api, auth and stream endpoints within st2)

This is fine if your security model is hard (secure) on the outside (public facing), but crunchy (insecure) on the inside (private facing) - (like an Armadillo 馃槈).

However, ideally a modern approach would be zero-trust and secure communication everywhere.

This PR therefore, enables by default, Encryption/TLS everywhere*. It makes use of cert-manager to enabled automatic generation of certificates, either using a user provided CA (to allow use of a corporate CA) or if the user already has a cert issuer setup - one can provide that.

TLS is enabled/configured globally for st2 endpoints (api, auth, stream) under the st2 block in values.yaml

  tls:
    enabled: true
    secretName: "internal-tls"
    mountPath: "/etc/ssl/internal"
    certificate_issuer:
      existing: false
      name: stackstorm-issuer

And then the individual endpoints can be toggled on/off under their respective blocks in values.yaml.

Sadly the mongo and rabbitmq TLS can't be setup from the st2 config directly - so they're present (and have defaults configured) under their respective blocks as well.

The st2 templates have then been updated to automatically configure all the required settings to use these certificates to enabled TLS - and the cert/key/ca are also automatically mounted to all the containers.

Note since the api and stream endpoints don't support encryption, we have added a TLS proxy layer (using ghostunnel) infront of them to enable inter-pod encryption while allowing unencrypted communication on the intra-pod commuication between the st2 component and the proxy layer.

*Note that the OpenStack Tooz library, which st2 uses to talk to Redis, doesn't have support to configure the settings needed to enable TLS when using Sentinel - so Redis traffic is still unencrypted in this PR. However, we have an internal build of Tooz that adds this support, and we've also got the changes to the StackStorm helm template to support that working internally. Once we have our changes to Tooz merged upstream, we can provide a PR to stackstorm-k8s and st2 core library to enable this support as well. (You can get a sneak preview of those changes here https://github.com/jk464/stackstorm-k8s/tree/feature/redis-tls)

@pull-request-size pull-request-size bot added the size/L PR that changes 100-499 lines. Requires some effort to review. label Feb 13, 2024
Comment on lines 10 to 13
{{- if and .Values.st2.tls.enabled .Values.st2auth.tls.enabled }}
ST2_AUTH_URL: https://{{ .Release.Name }}-st2auth:9100/
{{- else }}
ST2_AUTH_URL: http://{{ .Release.Name }}-st2auth:9100/
Copy link
Member

Choose a reason for hiding this comment

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

We can reduce some of the repetition here by front loading the calculation:

Suggested change
{{- if and .Values.st2.tls.enabled .Values.st2auth.tls.enabled }}
ST2_AUTH_URL: https://{{ .Release.Name }}-st2auth:9100/
{{- else }}
ST2_AUTH_URL: http://{{ .Release.Name }}-st2auth:9100/
{{- $proto := "http" }}
{{- if and .Values.st2.tls.enabled .Values.st2auth.tls.enabled }}
{{- $proto := "https" }}
{{- end }}
ST2_AUTH_URL: {{ $proto }}://{{ .Release.Name }}-st2auth:9100/

and the same for the others.

Copy link
Member

Choose a reason for hiding this comment

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

oh. I see you're changing the ports for st2api and st2stream as well. Hmm.

st2auth already exposes the tls bits. Could we add tls to st2api and st2stream as well to avoid adding another process to the mix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I don't follow - do you mean enhance the st2apit and st2stream libraries themselves to support TLS out-of-the-box instead of having to stick stunnel infront of them on the k8s end?

Copy link
Member

@cognifloyd cognifloyd May 9, 2024

Choose a reason for hiding this comment

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

Sorry I don't follow - do you mean enhance the st2apit and st2stream libraries themselves to support TLS out-of-the-box instead of having to stick stunnel infront of them on the k8s end?

Yes. st2auth supports tls, so st2api and st2stream should do the same.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{{- if and .Values.st2.tls.enabled .Values.st2auth.tls.enabled }}
ST2_AUTH_URL: https://{{ .Release.Name }}-st2auth:9100/
{{- else }}
ST2_AUTH_URL: http://{{ .Release.Name }}-st2auth:9100/
{{- $proto := "http" }}
{{- $port := "9100" }}
{{- if and .Values.st2.tls.enabled .Values.st2auth.tls.enabled }}
{{- $proto := "https" }}
{{- $port := "9110" }}
{{- end }}
ST2_AUTH_URL: {{ $proto }}://{{ .Release.Name }}-st2auth:{{ $port }}/

Copy link
Member

Choose a reason for hiding this comment

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

So, it looks like we need to copy the st2auth ssl config opts to st2api and st2stream.

st2auth seems to define the ssl bits here:

The SSL bits would need to go in these st2api files:

The SSL bits would need to go in these st2stream files:

values.yaml Outdated Show resolved Hide resolved
values.yaml Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Fascinating. I should look into installing cert-manager again to see if that would be work in my cluster.

Right now, I'm using this chart to generate a CA+cert that I load into the st2web pods:
https://github.com/cognifloyd/helm-gen-self-signed-cert/

helm repo add self-signed-cert https://cognifloyd.github.io/helm-gen-self-signed-cert/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works well for us, only issue we encounter is using self-signed certs, doesn't work - cert-manager seems to forcibly sets isCA false in the certificate (regardless if you tell it not to) which breaks providing the self-signed cert as a CA, to self validate the cert.

Instead you have to do what we do here and create a internal CA and perform "proper" cert signing and provide the CA then. Although this does have the benefit for corporate users where if they have a corporate CA, they can use that and have "correctly" signed certs - so I guess its a better implementation in the end.

I did want to avoid adding a hard requirement for cert-manager - so I've left that as an optional feature, although I would recommend using it, since cert-manager is open source and has no associated cost and does massively simplify certificate management 馃槃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Scratch that I forgot what I did 馃槄 - this does require cert-manager nvm

@jk464 jk464 force-pushed the feature/internal_tls branch 2 times, most recently from ebd2243 to d6fd653 Compare May 8, 2024 14:32
@jk464 jk464 requested a review from cognifloyd May 8, 2024 20:56
@jk464 jk464 force-pushed the feature/internal_tls branch 4 times, most recently from 0a7c28b to 22bd9ba Compare May 8, 2024 21:48
values.yaml Show resolved Hide resolved
values.yaml Outdated
@@ -380,7 +389,19 @@ st2web:
securityContext: {} # NB: nginx requires some capabilities, drop ALL will cause issues.
# mount extra volumes on the st2web pod(s) (primarily useful for k8s-provisioned secrets)
## Note that Helm templating is supported in 'mount' and 'volume'
extra_volumes: []
extra_volumes:
Copy link
Member

Choose a reason for hiding this comment

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

extra_volumes is only for user-provided volumes so that if someone sets it to an empty list it doesn't break chart functionality.

@jk464
Copy link
Contributor Author

jk464 commented May 10, 2024

@cognifloyd thanks for the additional comments, I'll take a look at them on Monday, and I also have a go at updating st2api and stream to add TLS support then aswell 馃槉

@jk464 jk464 force-pushed the feature/internal_tls branch 9 times, most recently from 0310b41 to 86aaa90 Compare May 20, 2024 14:53
@jk464 jk464 force-pushed the feature/internal_tls branch 3 times, most recently from 66a9a9e to 664ef16 Compare May 20, 2024 15:53
@jk464 jk464 force-pushed the feature/internal_tls branch 2 times, most recently from 0e8bc77 to 6b5c2ec Compare May 21, 2024 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature security size/L PR that changes 100-499 lines. Requires some effort to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants