Enable TLS on gRPCIngress if RAY_USE_TLS is on #34403
Conversation
3883d4c to
af1e460
Compare
| ) | ||
| address = "[::]:{}".format(self.port) | ||
| try: | ||
| self.grpc_port = add_port_to_grpc_server(self.server, address) |
There was a problem hiding this comment.
In high level, we should make secured/insecure configurable.
There was a problem hiding this comment.
I used the add_port_to_grpc_server(self.server, address) function, which internally depends on RAY_USE_TLS environment variable. Does that make it configurable? I can add a comment in my code to indicate this
There was a problem hiding this comment.
Hi @ashahab , RAY_USE_TLS will trigger all communication internally with TLS, is this the case you are expecting? Or you only want to ingress port to be TLS secured?
There was a problem hiding this comment.
I suggest having separate variable to control it
There was a problem hiding this comment.
@sihanwang41 Thank you for your review.
If RAY_USE_TLS is separate from another variable (e.g. RAY_USE_TLS_INGRESS), that may confuse the meaning of "RAY_USE_TLS" and allow insecure communication when it's on.
IMHO it is better to error on the side of caution and ensure all endpoints(ingress, headnode, and worker) encrypt on the wire when RAY_USE_TLS is on. This also allows reusing the add_port_to_grpc_server function which already has built in support. Happy to hear more on your thoughts.
There was a problem hiding this comment.
hi @ashahab , if you set RAY_USE_TLS, all internal communication will be under TLS (potentially hurting performance), I think you only want to have ingress port as TLS right?
If yes above,
RAY_SERVE_GRPC_TLS_INGRESS = True or RAY_USE_TLS = True, we both set the secure port. Otherwise we use insecure port, what do you think?
There was a problem hiding this comment.
@sihanwang41 Thank you for the prompt response.
Yes, I agree with you on the potential performance hit with RAY_USE_TLS.
My intent and proposal is to encrypt all the endpoints, and not just the ingress port. From a security perspective, this is the safer approach, given that if the data that is consumed by the endpoint needs to be encrypted between client and RayServe, it is unlikely that it does not need to be encrypted when it's routed from head to worker.
As for performance, I plan to follow this up with some benchmarks and potential improvements on the TLS communication:
- Ensuring TLS 1.3 is used at each endpoint, reducing handshake time by merging server hello, client key verify, cert, and verify.
- Session resumption
- Dynamic record sizes.
There was a problem hiding this comment.
@sihanwang41 I'd like. your feedback on this. Thank you!
47a6368 to
81af41b
Compare
|
lint is still failing |
Signed-off-by: Abin Shahab <ashahab@linkedin.com>
|
@sihanwang41 Thank you for the approval. Please let me know when/how it can be merged? We plan to use this capability soon. Thank you! |
|
Hi @edoakes ^^ can you take a look? |
Signed-off-by: Abin Shahab <ashahab@linkedin.com>
Enabling TLS for the serve GRPC endpoints.
Why are these changes needed?
Related issue number
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.