-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
agent: only enable TLS on gRPC if the HTTPS API port is enabled #5287
Conversation
Currently the gRPC server assumes that if you have configured TLS certs on the agent (for RPC) that you want gRPC to be encrypted. If gRPC is bound to localhost this can be overkill. For the API we let the user choose to offer HTTP or HTTPS API endpoints independently of the TLS cert configuration for a similar reason. This setting will let someone encrypt RPC traffic with TLS but avoid encrypting local gRPC traffic if that is what they want to do.
706b234
to
8a14299
Compare
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.
Hmm thanks for proactive fix but I'm not sure it's what we want. Not having separate config was intentional - in the RFC I went back and forth on adding extra flags to set this up and decided against it.
The actual behaviour intended and currently documented (hint needs change if we do accept this and don't think I saw it in the diff) is that gRPC uses TLS IFF https for client API is configured. So it should only be enabled if client is serving the regular API over https too not just TLS certs present.
If that's not the behaviour then it's a bug we should fix but unless we have have a really good reason to want to expose one API securely and the other not (I can't think of one given that Envoy can easily talk to secure gRPC just as easily even if it's not super necessary) then I'd still vote for not adding yet more config options for this.
My operational intent when enabling TLS locally was to secure agent-to-agent communications (lan/wan RPC traffic). I did not enable the HTTPS API port at all. My reading of the docs would say that because I did not enable the HTTPS API my gRPC connections should not use TLS:
Not configured:
The docs confusion likely stems from RPC and HTTPS both using the same TLS config file settings for keys, rather than two separate sets of keys. The snippet of code that configures the gRPC server only checks for the existence of the TLS key config settings but does not check for the existence of the HTTPS API port being set. Perhaps rather than adding a new config option I could change the patch to do what the docs imply, that is: have it check for certfile + keyfile + https_port>0? |
In that case the PR is reduced to just:
|
@rboyer Are there any scenarios where gRPC is useful even if HTTP(s) are both disabled? I think there probably is but I have nothing concrete. |
You mean if the agent has no API exposed? gRPC is only for serving |
Yeah, so does envoy require the HTTP API at all? |
Envoy does not talk to the HTTP api, but the |
@rboyer In that case it would make sense to tie TLS for gRPC to the HTTPSPort. |
Yeah from a security coupling perspective it does seem a little silly for the bootstrap command to send an ACL token over localhost-API insecurely only to then have envoy use the same ACL token to talk over localhost-gRPC securely. Someone eavesdropping on loopback could still egress the token secret by snooping on the API instead of gRPC. |
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.
Perfect, thanks!
Currently the gRPC server assumes that if you have configured TLS certs on the agent (for RPC) that you want gRPC to be encrypted. If gRPC is bound to localhost this can be overkill. For the API we let the user choose to offer HTTP or HTTPS API endpoints independently of the TLS cert configuration for a similar reason.
This setting will let someone encrypt RPC traffic with TLS but avoid encrypting local gRPC traffic if that is what they want to do by only enabling TLS on gRPC if the HTTPS API port is enabled.