search

Kong / charts

Helm chart for Kong
Apache License 2.0
242 stars 474 forks source link

Use mTLS by default for 3.x #959

Closed rainest closed 8 months ago

rainest commented 8 months ago

What this PR does / why we need it:

Enables mTLS on the controller and admin API by default.

Changes the default gateway discovery strategy to pod for compatibility with generated certificates. This soft breaks compatibility with controllers older than 3.0 (you can disable mTLS but need to come up with your own approach to admin API security).

Uses the CA generated for the controller client certificate by default if no other CA is configured for the admin API.

Which issue this PR fixes

Part of the split chart work. Since we need to expose the admin API outside the Pod network, we need to protect it, and mTLS is a readily and universally available option for this.

I don't know of a great way to

Special notes for your reviewer:

Going to 3.x for staging instead of main.

The original implementation and UX has some issues that make this change a bit kludgy, but I'm on the fence about completely refactoring it and breaking existing config.

mTLS does have a one significant drawback: client certificates are generally annoying for human-use clients, and using them complicates using Manager and performing diagnostic admin API requests with curl. The generated certificate is particularly bad for this, since there's no mechanism to issue and distribute multiple certificates for the controller and other clients. You'd likely want to operate your own CA if you use Manager or the admin API regularly. I have not tested to see if mTLS introduces additional complications in Manager (beyond browser client certificates being generally annoying).

I don't have any great alternatives here. RBAC is a viable alternative that is simpler to use for non-controller requests, but is not universally available. An additional localhost-only HTTP listen would work for direct admin API requests with a port-forward, but would be a poor option for Manager use.

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

czeslavo commented 8 months ago

I posted some context that could help understand some design decisions made in the past: https://github.com/Kong/charts/issues/921#issuecomment-1847053879

Given that we now are going to have two separate Deployments in one Helm release, I think we could consider merging the ingressController.adminApi.tls.client and admin.tls.client settings because now it's technically possible. I'll leave it up to you to decide if we want to do that in 3.x.