Skip to content
This repository has been archived by the owner on Mar 22, 2024. It is now read-only.

Add TLS/mTLS support for Tornjak #338

Merged
merged 29 commits into from
Jul 19, 2023
Merged

Add TLS/mTLS support for Tornjak #338

merged 29 commits into from
Jul 19, 2023

Conversation

mrsabath
Copy link
Contributor

@mrsabath mrsabath commented Jun 9, 2023

This PR adds TLS and mTLS connection to Tornjak backend. It is assumed users create secret tornjak-certs prior to deploying the helm charts when TLS and/or mTLS are enabled.

Sample secret (to be documented):

kubectl -n spire-server create secret generic tornjak-certs \
--from-file=key.pem="client.key"  \
--from-file=cert.pem="client.crt" \
--from-file=rootCA.pem="CA/rootCA.crt"

# testing TLS: 
curl --cacert CA/rootCA.crt https://localhost:20000/

# testing mTLS: 
curl  --cacert CA/rootCA.crt --key client.key --cert client.crt https://localhost:30000

charts/spire/charts/spire-server/values.yaml Outdated Show resolved Hide resolved
charts/spire/charts/spire-server/values.yaml Outdated Show resolved Hide resolved
charts/spire/charts/spire-server/values.yaml Outdated Show resolved Hide resolved
charts/spire/charts/spire-server/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@kfox1111 kfox1111 left a comment

Choose a reason for hiding this comment

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

I know its not done yet. But some ideas for value naming.

charts/spire/charts/spire-server/values.yaml Outdated Show resolved Hide resolved
charts/spire/charts/spire-server/values.yaml Outdated Show resolved Hide resolved
@mrsabath mrsabath marked this pull request as ready for review June 16, 2023 02:51
Copy link
Contributor

@marcofranssen marcofranssen left a comment

Choose a reason for hiding this comment

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

I think we should implement some plumbing into the chart.

E.g. allow to create the secret via the chart. Ensure there is just a single secret containing all the required certs.

That simplifies the volume mounts.

Furthermore I gave some inline feedback on the service and port. That can be much simpler by doing following

Service structure:

tornjak

  • http
    • 10080
  • https. only if tls/mtls enabled
    • 10443

secret for certs

tornjak-tls-secret:
ca.crt
server.crt
client.crt

Happy to pick this up in pairing session next week to further explain.

Also wondering if we should just leverage the certManager integration as contributed in the other PR.

We can sign these certs with the same Cert-manager Issuer and CA.

@kfox1111
Copy link
Contributor

@marcofranssen, I asked him to do it as two secrets before.

There are multiple reasons not exhaustive:

  1. tls certificates in k8s (secret type=tls) has a particular format that includes private key and cert. Other systems such as cert-manager can automatically create certs in this form. So its important that we support it.
  2. the user ca is needed for mtls. Its what the client certificate is verified against. if you use cert-manager to setup your user ca as well, its likely a completely separate ca then your service ca. This should come in through an alternate configmap/secret/the new ClusterTrustBundles type (ClusterTrustBundles (previously Trust Anchor Sets) kubernetes/enhancements#3257)

Having a create option though where the user can specify the secrets in values would be ok. That could be in this pr or in another followon feature pr?

@marcofranssen
Copy link
Contributor

marcofranssen commented Jun 17, 2023

@marcofranssen, I asked him to do it as two secrets before.

There are multiple reasons not exhaustive:

  1. tls certificates in k8s (secret type=tls) has a particular format that includes private key and cert. Other systems such as cert-manager can automatically create certs in this form. So its important that we support it.

Oh right! Very good point. Then others are just not using this type of secret. I like to use the k8s native types, so lets go with this type=tls indeed.

  1. the user ca is needed for mtls. Its what the client certificate is verified against. if you use cert-manager to setup your user ca as well, its likely a completely separate ca then your service ca. This should come in through an alternate configmap/secret/the new ClusterTrustBundles type (ClusterTrustBundles (previously Trust Anchor Sets) kubernetes/enhancements#3257)

Oh then it is probably the naming that confused me. However why would the CA be different?

I would do it like this

  • Cluster CA
    • server cert
    • intermediate CA
      • client cert
      • client cert
      • client cert

Isn't the root CA sufficient to build the trustchain? Now you made me doubt though :)

@kfox1111
Copy link
Contributor

User CA's sometimes need to be on the internet so that users can properly get certificates for themselves. Host CA's dont need to be accessible on the internet. So keeping them separate has security benefits.You don't necessarily want a User CA to be able to issue valid Host certs from the User CA as then there is more attack surface.

There are other reasons why you may want separate chains of trust there. Hosts often need to be trusted by browsers or other services not under the sysadmin's control, so having a more traditional chain of trust makes hosts validation easier. For example, with letsencrypt. While user ca validation only happens on your own hosts, so easier to distribute to the set of machines needing that CA for client validation.

So its best to allow treating host chain of trust completely separate from user chain of trust. Some users may combine the two if they like but that should be up to them and their situation.

@marcofranssen
Copy link
Contributor

User CA's sometimes need to be on the internet so that users can properly get certificates for themselves. Host CA's dont need to be accessible on the internet. So keeping them separate has security benefits.You don't necessarily want a User CA to be able to issue valid Host certs from the User CA as then there is more attack surface.

There are other reasons why you may want separate chains of trust there. Hosts often need to be trusted by browsers or other services not under the sysadmin's control, so having a more traditional chain of trust makes hosts validation easier. For example, with letsencrypt. While user ca validation only happens on your own hosts, so easier to distribute to the set of machines needing that CA for client validation.

So its best to allow treating host chain of trust completely separate from user chain of trust. Some users may combine the two if they like but that should be up to them and their situation.

So isn't that intermediate as I shown above exactly what you need to just make that available on the internet, but still have it trusted by the upstream cluster CA?

@kfox1111
Copy link
Contributor

User CA's sometimes need to be on the internet so that users can properly get certificates for themselves. Host CA's dont need to be accessible on the internet. So keeping them separate has security benefits.You don't necessarily want a User CA to be able to issue valid Host certs from the User CA as then there is more attack surface.
There are other reasons why you may want separate chains of trust there. Hosts often need to be trusted by browsers or other services not under the sysadmin's control, so having a more traditional chain of trust makes hosts validation easier. For example, with letsencrypt. While user ca validation only happens on your own hosts, so easier to distribute to the set of machines needing that CA for client validation.
So its best to allow treating host chain of trust completely separate from user chain of trust. Some users may combine the two if they like but that should be up to them and their situation.

So isn't that intermediate as I shown above exactly what you need to just make that available on the internet, but still have it trusted by the upstream cluster CA?

No. If you have your own private CA for the user ca, and a public chain of trust such as letsencrypt for the hosts, it is not possible for the intermediates to be rooted at just one CA. Nor is it desirable as I explained, for security reasons. You don't want machines to trust host certs signed by the user ca so yiou can't trust the a root where there is a shared root between a user and host ca.

Also, an intermediate CA is just a way to add an additional layer of security so you don't have to expose the root ca. The intermediate CA's can have expiry times much less then the root ca's expiration date. But can issue certs much the same as the root.

The user ca must be configurable separately then any ca used for the hosts.

@kfox1111
Copy link
Contributor

There are a few different options we may want to consider for the tls certs and for the user ca.....

For the host cert for each of the frontend and backend, we could support an enum with:
existing, cert-manager, values

where existing would just use the secret that exists, cert-manager would create a Certificate object to generate the tls secret, and values could create the secret from a key/cert thats specified in values

For now, this pr only supports an existing secret. Or the user can use cert-manager to create it manually. This is probably a good first step and the other options could be added in a follow up pr.

For the userCA, we have some similar options.... some combination of
existing-secret, existing-configmap, cert-manager, values

where existing-secret/configmap is preexisting objects, cert-manager would create a self signed user ca and then reference it, and values would have the ca.pem specified directly in the values and would get put in a secret.

Similar to the previous, I'd recommend the additional options be added in a follow up pr.

Copy link
Contributor

@kfox1111 kfox1111 left a comment

Choose a reason for hiding this comment

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

Some minor text things that could be changed, but could do it in a followup pr too if everyone else is happy with it the way it is.

charts/spire/charts/spire-server/README.md Show resolved Hide resolved
charts/spire/charts/spire-server/README.md Outdated Show resolved Hide resolved
@mrsabath
Copy link
Contributor Author

#338 (comment)
Thank you for the comment @kfox1111
I think the most recent version already has two mount-points: https://github.com/spiffe/helm-charts/blob/tornjak-tls/charts/spire/charts/spire-server/templates/statefulset.yaml#L213-L218

@mrsabath
Copy link
Contributor Author

@marcofranssen per your suggestion, Tornjak Helm charts currently determine automatically the Tornjak connection type, based on provided information (secrets or ConfigMaps). We will update the Tornjak code (spiffe/tornjak#268) to do this determination on the level of the Tornjak image. Once completed there, we will update these helm charts here with a new configuration format, as a follow-up PR.

mrsabath and others added 22 commits July 18, 2023 19:12
Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
Co-authored-by: Marco Franssen <marco.franssen@gmail.com>
Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
Co-authored-by: Marco Franssen <marco.franssen@gmail.com>
Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
Co-authored-by: Marco Franssen <marco.franssen@gmail.com>
Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
Co-authored-by: Marco Franssen <marco.franssen@gmail.com>
Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
@marcofranssen marcofranssen changed the title Add option to configure TLS/mTLS endpoint for Tornjak Add TLS/mTLS support for Tornjak Jul 19, 2023
Copy link
Contributor

@marcofranssen marcofranssen left a comment

Choose a reason for hiding this comment

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

LGTM

@marcofranssen marcofranssen merged commit f05cb4f into main Jul 19, 2023
43 checks passed
@marcofranssen marcofranssen deleted the tornjak-tls branch July 19, 2023 12:32
marcofranssen added a commit that referenced this pull request Jul 19, 2023
* 088b296 Improve tornjak service API to have object structure (#392)
* 522066e Align tornjak clientCA naming convention (#393)
* f05cb4f Add option to configure TLS/mTLS endpoint for Tornjak (#338)
* f461a01 Bump test chart dependencies (#386)
* ce39e82 Bump test chart dependencies (#382)
* 19d3208 Fix oidc provider config change not rolling out (#383)
* 0197621 Bump helm/kind-action from 1.7.0 to 1.8.0 (#384)
* 3ed1859 Add missing tolerations config to daemonsets (#381)
* 4ad68b1 Add namespace to spiffe-oidc-discovery-provider RBAC definitions (#379)
* c1b1dd3 Add additional domains to JWT issued items. (#230)
* 3405e13 Bump test chart dependencies
* 81452d5 Fix missing spiffe-csi-driver imagePullSecrets template (#376)

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
marcofranssen added a commit that referenced this pull request Jul 19, 2023
* 94a2b72 Merge pull request #324 from spiffe/enable-testing-multiple-charts
* e426bc0 Downgrade chart-testing tool to 3.8.0
* 09b4664 Utilize ct install --github-groups in ci workflow
* 42086bd Run example tests also on all k8s versions
* 4848c48 Improve Makefile help and implementation
* db06038 Increase some timeouts, trying to fix the tests
* 54ed71f Add back tests for examples
* 4b4cef1 Skip namespace-override test because of #330
* 380979c Prevent ci folder ending up in Helm package
* 06ddb7c Use chart-testing ci/*-values.yaml for testing
* a4c1de7 Add basic unit test framework (#390)
* 088b296 Improve tornjak service API to have object structure (#392)
* 522066e Align tornjak clientCA naming convention (#393)
* f05cb4f Add option to configure TLS/mTLS endpoint for Tornjak (#338)
* f461a01 Bump test chart dependencies (#386)
* ce39e82 Bump test chart dependencies (#382)
* 19d3208 Fix oidc provider config change not rolling out (#383)
* 0197621 Bump helm/kind-action from 1.7.0 to 1.8.0 (#384)
* 3ed1859 Add missing tolerations config to daemonsets (#381)
* 4ad68b1 Add namespace to spiffe-oidc-discovery-provider RBAC definitions (#379)
* c1b1dd3 Add additional domains to JWT issued items. (#230)
* 3405e13 Bump test chart dependencies
* 81452d5 Fix missing spiffe-csi-driver imagePullSecrets template (#376)

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants