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

Dynamically get certificates from ingresses' TLS secrets #54

Closed
Aluxima opened this issue Sep 27, 2021 · 2 comments · Fixed by #68
Closed

Dynamically get certificates from ingresses' TLS secrets #54

Aluxima opened this issue Sep 27, 2021 · 2 comments · Fixed by #68

Comments

@Aluxima
Copy link
Contributor

Aluxima commented Sep 27, 2021

Declaring all TLS certificates and managing them alongside Yggdrasil can be a challenge when working with many ingresses all using different certificates.

We would like to make Yggdrasil fetch (and watch) TLS secrets declared in ingresses' spec.tls and use them.

To make this functionality transparent to those who don't need it, we can imagine simply adding a syncSecrets Yggdrasil configuration option (false by default) that would ignore certificates if true:

{
  "nodeName": "foo",
  "ingressClasses": ["multi-cluster", "multi-cluster-staging"],
  "certificates": [
    {
      "hosts": ["*.api.com"],
      "cert": "path/to/cert",
      "key": "path/to/key"
    }
  ],
  "clusters": [
    {
      "token": "xxxxxxxxxxxxxxxx",
      "apiServer": "https://cluster1.api.com",
      "ca": "pathto/cluster1/ca"
    }
  ]
}

=>

{
  "nodeName": "foo",
  "ingressClasses": ["multi-cluster", "multi-cluster-staging"],
  "syncSecrets": true,
  "clusters": [
    {
      "token": "xxxxxxxxxxxxxxxx",
      "apiServer": "https://cluster1.api.com",
      "ca": "pathto/cluster1/ca"
    }
  ]
}

Any other approach in mind? Maybe one to be able to use both static certificates and TLS secrets at the same time?

@Joseph-Irving
Copy link
Contributor

I don't think supporting both would be needed for a first pass, neither of us need that option so I would be keen not to over-engineer the solution yet leave it open enough to do that later if need be.

How do you propose managing the secrets? You could go and fetch the secret on every sync but that could be quite inefficient at scale, you could end up making a lot of api calls, alternatively a store could be used but by default that would have all secrets in your cluster being store in yggdrasil, which doesn't seem ideal as you're only interested in the tls related ones.

Also worth noting that by adding this it does make Yggdrasil more of a target for intruders as it will have access to all the secrets in all your clusters.

@Aluxima
Copy link
Contributor Author

Aluxima commented May 13, 2022

Hello, sorry for the late reply to this issue.
We have been testing some implementation for this (see master...Aluxima:secrets-sync) and were able to selectively fetch the tls secrets using a store and dynamically use them as downstream certificates.

The synchronization flow was copied from the ingresses sync and definitely needs cleaning and improvement.
I saw this comment about adding events handlers, maybe what we could do is switch to using a shared informer that will be used to watch both ingresses and secrets using proper events, a bit like haproxy ingress-controller does (see https://github.com/haproxytech/kubernetes-ingress/blob/master/pkg/k8s/main.go#L148 and https://github.com/haproxytech/kubernetes-ingress/blob/master/pkg/controller/monitor.go#L27)

Also I noticed some complexity issues in the way that the snapshots are done. Right now yggdrasil does a snapshot (here) at each and every resource addition/change/deletion so the startup phase with thousands of ingresses and secrets never ends :D
But I think this deserves its own issue.

About the security concerns of yggdrasil holding access to k8s secrets, it is an accepted risk on our side and the rights will not be needed for those who will not use this option.

Do you think it is a good way to go for this new feature?

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 a pull request may close this issue.

2 participants