ingress: use exact path

[AlexanderYastrebov]

No description provided.

[clementnuss]

@AlexanderYastrebov can you elaborate on why this change is needed ?

it would prevent e.g. fetching /alive and thereby retrieving debug information for example.
I'm not sure this would help

[AlexanderYastrebov]

kubenurse only uses /alwayshappy endpoint for ingress check. With pathType: Prefix all metrics can be potentially exposed to the internet

[clementnuss]

in our environment, we also use other endpoints, and I don't think it would be good changing the default prefixPath for everyone.

however, if you want to make that configurable in the Helm Chart, I would merge it 🙃

please rebase your branch on master, as I pushed a few CI fixes, and make your pathType prefix configurable from within values.yaml, and we will look at it again.

[AlexanderYastrebov]

in our environment, we also use other endpoints

Do you use other endpoints from outside of kubenurse? I thought this ingress is dedicated to kubenurse and kubenurse ingress checker does not use any other endpoint. Per-pod metrics should not be collected via ingress anyways.

My point is that this might be a security issue if someone deploys kubenurse with public hostname and thus exposes /metrics to the internet.

[clementnuss]

kubenurse endpoints are defined here:

kubenurse/internal/kubenurse/server.go

Lines 142 to 146 in c4bf82f

	mux.HandleFunc("/ready", server.readyHandler())
	mux.HandleFunc("/alive", server.aliveHandler())
	mux.HandleFunc("/alwayshappy", func(http.ResponseWriter, *http.Request) {})
	mux.Handle("/metrics", promhttp.HandlerFor(promRegistry, promhttp.HandlerOpts{}))
	mux.Handle("/", http.RedirectHandler("/alive", http.StatusMovedPermanently))

/alive gives info that leaks neighbouring pod names
/metrics gives info that leaks other nodes nodes

while I agree this is not best practice to have that exposed through an ingress, it is sometimes useful in our environments to take a look at those values.

also, at this point, I don't think changing the default behaviour of the helm chart makes sense, as other people might be using /alive to check the discovery state of kubenurse.

my point is that while your proposal is very valid (and it should have been done like that from the beginning), I'm not going to change the default behaviour of the Helm Chart. I'd be really happy if you updated your PR to make the ingress prefixPath configurable 🙃

[AlexanderYastrebov]

Sorry, I do not know much about Helm.
Let's see whether this becomes a CVE.