-
Notifications
You must be signed in to change notification settings - Fork 593
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
feat(adminapi): support specification of http/https via appProtocol on the admin port #5251
base: main
Are you sure you want to change the base?
Conversation
…n the admin port.
…n the admin port.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me like a good alternative. As I understand the benefit is that users using any mesh will be able to use all mesh features with that in place in comms between KIC and Kong, which is not possible if we exclude Admin API from meshes with the use of annotations. BTW if we'd like to merge this, we'd have to make it possible to not add the excluding annotations in Helm Deployments (I believe they're hardcoded now). |
Yeah, it's along the lines of what @czeslavo is saying. As an operator, I would like to leverage my mesh for the various benefits - mutual TLS, observability etc - that it provides. #4942 allows Gateway Discovery to work in a mesh environment, but it does that by just excluding the traffic from the mesh. This PR would allow me as an operator to have gateway discovery work AND allow me to keep the traffic inside of the mesh. |
Ok, I think it makes sense. Will defer to @rainest as he is the original author of #4942. |
tl;dr I don't think we should handle this by allowing HTTP; we should figure out what we can disable to make HTTPS viable instead. AFAIK the only reason for exclusion is because we do our own mTLS and the client certificate can't traverse the hops. Although meshes provide their own methods we can use for validation, they're not guaranteed consistent across all meshes. Unfortunately our research in #4698 didn't dig too deep into the connection to confirm that was indeed the issue, and there's some oddness in the report, namely that it doesn't actually enable mTLS AFAICT (the HTTP effectively removes that concern, but is still unfortunately HTTP and removes everything else that HTTPS provides, namely the ability to verify upstream. I think we'd want to instead figure out a way to get HTTPS to work, even if that means disabling parts of it without full verification--although the meshes (at least Kuma, and presumably other Envoy-based ones) can effectively guarantee that only they can populate their client cert info header, we'd unfortunately need to verify it gateway-side, and AFAIK have no way to do this. |
Wouldn't it make sense to offload mTLS to the Mesh if a user already uses that? As I understand, both Kuma and Istio allow configuring mTLS which would be a preferable way to authenticate between services in such an environment, allowing not to worry about another custom layer of authentication (which our mTLS kinda is).
Is that an issue? If an operator decides to implement mTLS on their own (by any means, Mesh in our case), why not allow them to do so by consciously committing to plain-text communication? We would not distribute this config by default in charts, but we may document that's possible, with a note for users to make sure they know what they're doing. |
Generally a +1 to @czeslavo's comment - as an operator our preferred way to authenticate workloads is via the mesh that we are using. I think the issue in #4698 was indeed around double TLS (notably, not double mTLS). We had the same problem with Istio and GD - a lot of meshes expect that traffic coming into the sidecar is in plaintext and that the mesh will be the one responsible for the TLS component (https://istio.io/latest/docs/ops/common-problems/network-issues/#double-tls).
This exactly. As an operator, I understand that enabling plain text generically is a bad practice, but I would be turning plaintext on knowing that my infrastructure is already handling the necessary security (which may even be superior security-wise to the built in (m)TLS support as issuing/rotation/revocation is much more automated in most service meshes than it would be for the controller specifically) |
#4698 as originally presented did not actually result in any issues. We wanted to handle controller-initiated mTLS in mesh environments (because we want that to eventually be the default), but that's separate from the admin API and discovery client using HTTPS. The original repro steps unfortunately appear incorrect, and I'm not sure what was missed there. @backjo did you have a set of configurations that did indeed not work if you removed the exemptions? The example in https://istio.io/latest/docs/ops/common-problems/network-issues/#double-tls looks like it would cause issues because the endpoint in question ( HTTP-level metrics would probably be unavailable--it's not clear if Istio has a client-side HTTPS proxy within the mesh that can terminate, encrypt between sidecars, and then encrypt again to the destination (searches re termination are mostly bringing up docs related to its external gateway functionality). |
@rainest checking here, will circle back |
@backjo hey again, checking back on this, had you heard anything from your end? |
@rainest checked quite a few ways with our mesh configuration - ultimately, we have not been able to get the mesh authorization working with double TLS. I think the best path forward here would still be to allow users the flexibility of using HTTP if they opt-in, given that telemetry benefits from a service mesh would come along with that for free as well. |
What this PR does / why we need it:
This PR allows for users to indicate that HTTP may be used to communicate with the Admin API instead of HTTPS. While it is generally a best practice to use HTTPS, some clients may be leveraging Service Meshes like Kuma or Istio to manage transparent mutual TLS connections between workloads (as seen in #4698).
This PR allows the operator to indicate via the admin port
appProtocol
that they wish to use HTTP.Which issue this PR fixes:
Related to #4698
Special notes for your reviewer:
PR Readiness Checklist:
Complete these before marking the PR as
ready to review
:CHANGELOG.md
release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR