postfinance / kubenurse

Kubernetes network monitoring
MIT License
416 stars 39 forks source link

ingress: use exact path #99

Closed AlexanderYastrebov closed 11 months ago

clementnuss commented 11 months ago

@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 commented 11 months ago

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

clementnuss commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

kubenurse endpoints are defined here: https://github.com/postfinance/kubenurse/blob/c4bf82fde7f72a6edbd859f16015f0f461da3f1b/internal/kubenurse/server.go#L142-L146

/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 commented 11 months ago

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