Support HTTPS for /healthz, /readyz, /metrics and /debug endpoints

[timuthy]

How to categorize this issue?

/area security
/kind enhancement

What would you like to be added:
Similar to Gardenlet, controller-manager, admission-controller, resource-manager, scheduler and seed-admission-controller should serve /healthz, /readyz and /metrics endpoints via HTTPS instead of HTTP.
Additionally, /debug should also be served via HTTPS if enabled (ref #4567).

[rfranzke]

Should we close this in favor of #4251? cc @timebertt

[timuthy]

Hm, I don't think that one comes with the other. The scheduler for instance was changed to use the Controller-Runtime but the endpoints are still served via http only. Even on the contrary, with the Controller-Runtime serving these endpoints with https is not supported yet (kubernetes-sigs/controller-runtime#979). Hence, we should only close it if we decide that this is not too relevant.

[timebertt]

Once we refactor gardenlet to a native controller-runtime component (#4251), i.e. use a c-r manager to expose metrics and health endpoints, we will also be back to plain http for /healthz, /readyz, /metrics and /debug there as well.

Do we consider this to be important?
If so, we could

• a) tackle Manager should expose http.Handlers to allow extension by users kubernetes-sigs/controller-runtime#1007 and contribute the required mechanisms to c-r
  • in that case, we probably would want to allow reusing the same listener (port, certs, etc.) for webhook server, health and metrics endpoints as our components currently use one port for everything
• b) use kube-rbac-proxy as a sidecar container for our components like described in ✨ Add TLS serving option for metrics kubernetes-sigs/controller-runtime#993 (comment)
  • in addition to TLS, this would allow configuring RBAC for the endpoints (e.g. like it is the case for API servers)

If not and we want to go with HTTP for the endpoints, we need to

• c) adapt seed-prometheus scrape config to allow scraping with http scheme: Seed monitoring enhancement #4706 (comment)

If we should decide for a), it might make sense to start with this in parallel to the other steps in #4251, so that we don't need to add configuration knobs for different ports and so on that will be replaced shortly afterwards again.

Looking for more opinions here!

[timuthy]

Thanks a lot @timebertt for outlining the options. I personally prefer b) because it additionally delivers RBAC capabilities. The disadvantage I see is yet another vertically scaled side-car and I hope that if we decide for option b), we don't see this as the #1 reason for Pod evictions.

[timebertt]

Thanks @timuthy for adding your opinion here. I like b) as well.

I recently learned, that kubebuilder actually uses kube-rbac-proxy for securing the metrics endpoints etc. by default: https://book.kubebuilder.io/reference/metrics.html#protecting-the-metrics
So I think, b) should be a safe bet :)

[gardener-ci-robot]

The Gardener project currently lacks enough active contributors to adequately respond to all issues and PRs.
This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed
  You can:
• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Close this issue or PR with /close

/lifecycle rotten

[timebertt]

/remove-lifecycle rotten

[gardener-ci-robot]

The Gardener project currently lacks enough contributors to adequately respond to all issues and PRs.
This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close

/lifecycle stale

[timebertt]

/remove-lifecycle stale

[rfranzke]

I vote for closing this one and simply relying on what the controller-runtime library provides. If it's HTTP for the metrics and health endpoints, then it's HTTP and good enough (I don't see any security risk whatsoever with this).

[timuthy]

I kind of agree here, even though I'm a bit more concerned that we don't have authorization for /debug which is however disabled by default. Any strong concerns from your side @timebertt?
IIRC, the main reason for opening this issue was to mimic what K8s did with their components and to make it consistent with Gardenlet when we didn't use CR.

[rfranzke]

Any feedback @timebertt?

[timebertt]

I still vote for implementing option b) (adding kube-rbac-proxy) which is very little effort.
This plays nicely with what the community does (kubebuilder, etc.) and follows best practices. I would consider this to be "what controller-runtime/kubebuilder provides" even though it is a sidecar component rather than a go library feature.

I even have some running examples in one of my projects, which can be used as a template.

[rfranzke]

In an internal chat we figured that we won't be able to use the kube-rbac-proxy approach for the /healthz and /readyz endpoints since we otherwise would need to write JWT tokens into the {live,ready}nessProbes of the various Deployments (which isn't a great idea).

Since /debug is disabled by default and typically only used for dev purposes, /metrics is the only endpoint left that would "benefit" from using kube-rbac-proxy.

However, /metrics doesn't expose sensitive data and is in addition only reachable cluster-internally (this is different for the comparable endpoints in kube-apiserver).

In this light, we agreed to not invest here since the additional complexity/effort is not worth it.

/close

[gardener-prow]

@rfranzke: Closing this issue.

In response to this:

In an internal chat we figured that we won't be able to use the kube-rbac-proxy approach for the /healthz and /readyz endpoints since we otherwise would need to write JWT tokens into the {live,ready}nessProbes of the various Deployments (which isn't a great idea).

Since /debug is disabled by default and typically only used for dev purposes, /metrics is the only endpoint left that would "benefit" from using kube-rbac-proxy.

However, /metrics doesn't expose sensitive data and is in addition only reachable cluster-internally (this is different for the comparable endpoints in kube-apiserver).

In this light, we agreed to not invest here since the additional complexity/effort is not worth it.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.