Support secure channels through SSL/TLS #33
Conversation
grpclib/https.py
Outdated
|
|
||
| # We want to negotiate using NPN and ALPN. ALPN is mandatory, but NPN may | ||
| # be absent, so allow that. This setup allows for negotiation of HTTP/1.1. | ||
| ctx.set_alpn_protocols(["h2", "http/1.1"]) |
There was a problem hiding this comment.
There is no need to use ALPN/NPN to negotiate about protocols, in gRPC only h2 is used with prior knowledge: https://python-hyper.org/projects/h2/en/stable/negotiating-http2.html#prior-knowledge
There was a problem hiding this comment.
Is there a significant performance hit or disadvantage to negotiating? Our server requires negotiating and it seems friendlier to set these by default than requiring passing a custom context in situations where negotiating is required.
grpclib/https.py
Outdated
| # RFC 7540 Section 9.2: Implementations of HTTP/2 MUST use TLS version 1.2 | ||
| # or higher. Disable TLS 1.1 and lower. | ||
| ctx.options |= ( | ||
| ssl.OP_NO_SSLv2 | ssl.OP_NO_SSLv3 | ssl.OP_NO_TLSv1 | ssl.OP_NO_TLSv1_1 |
There was a problem hiding this comment.
create_default_context already disables OP_NO_SSLv2, OP_NO_SSLv3 and OP_NO_COMPRESSION: https://github.com/python/cpython/blob/3.5/Lib/ssl.py#L438
grpclib/client.py
Outdated
| @@ -379,11 +380,13 @@ class Channel: | |||
| """ | |||
| _protocol = None | |||
|
|
|||
| def __init__(self, host='127.0.0.1', port=50051, *, loop, codec=None): | |||
| def __init__(self, host='127.0.0.1', port=50051, *, loop, codec=None, | |||
| secure=False): | |||
There was a problem hiding this comment.
It would be more flexible to have ssl: Union[bool, SSLContext]=False argument instead of secure: bool=False, so users will be able to provide any SSLContext they want for their case. If ssl==True, we create default context.
grpclib/https.py
Outdated
| @@ -0,0 +1,66 @@ | |||
| import ssl | |||
There was a problem hiding this comment.
Possibility of ImportError, if ssl module is unavailable. How to fix:
grpclib/https.py
Outdated
|
|
||
| # RFC 7540 Section 9.2.1: A deployment of HTTP/2 over TLS 1.2 MUST disable | ||
| # compression. | ||
| ctx.options |= ssl.OP_NO_COMPRESSION |
grpclib/https.py
Outdated
| """ | ||
|
|
||
|
|
||
| def get_http2_ssl_context(): |
There was a problem hiding this comment.
I think that it is too early to create additional module just for this function, which will be used only in one place. You can place it in utils.py or in client.py or directly in the Channel class as private method or in Channel.__init__ method in-place. And I think that there is no need to copy-paste whole function as is with full copyright notice, I think that this code is provided as documentation for reference and it is OK to use it's parts just with a link, pointing to the hyper-h2 project.
|
And please ensure that |
|
@mnito, will you have a time to finish this PR? Or maybe I can do this for you? |
|
Hey @vmagamedov, sorry I haven't gotten to this -- I have some time right now to make the requested changes. |
|
Hey @vmagamedov I made the requested changes and tested with both SSL-enabled and non-SSL-enabled servers. I left the protocol negotiation stuff in because we never had any problems using grpcio with our server and communication did not work without it. If the cost outweighs the benefits of leaving it in, feel free to take it out and we can always pass in a custom SSL context -- but I figured it would be a friendlier default. |
|
Made a mistake in handling the potential SSL import error -- updated. |
grpclib/client.py
Outdated
| ctx = ssl.create_default_context(purpose=ssl.Purpose.SERVER_AUTH) | ||
| ctx.options |= (ssl.OP_NO_TLSv1 | ssl.OP_NO_TLSv1_1) | ||
| ctx.set_ciphers('ECDHE+AESGCM:ECDHE+CHACHA20:DHE+AESGCM:DHE+CHACHA20') | ||
| ctx.set_alpn_protocols(['h2', 'http/1.1']) |
There was a problem hiding this comment.
I'm totally Ok with this, I don't worry about possible overhead, as this happens only once for a long-living connection. The only concern is that I don't think that we should advertise http/1.1 protocol support here. Can you remove it from the list and test with your server?
There was a problem hiding this comment.
Yeah you're probably right -- let me just do a quick test with our server
grpclib/client.py
Outdated
| @@ -389,16 +395,39 @@ def __init__(self, host='127.0.0.1', port=50051, *, loop, codec=None): | |||
| header_encoding='utf-8') | |||
| self._authority = '{}:{}'.format(self._host, self._port) | |||
|
|
|||
| if ssl and isinstance(ssl, bool): | |||
|
Should be good to go. |
|
Everything looks great, can you rebase your changes and resolve merge conflicts? |
|
Sure, do you want me to squash as well? |
|
Yes! It would be great. |
mnito
force-pushed
the
feature/secure_channel
branch
from
d0a23b0 to
cdc7e22
Compare
mnito
force-pushed
the
feature/secure_channel
branch
from
cdc7e22 to
079c70a
Compare
|
I rebased! Thanks for all your input. |
|
Thank you for your work! |
grpclibdoes not currently work with secure gRPC channels through HTTPS which we needed for our use case at Slyce -- this pull request uses hyper-h2's SSL context setup to create a secure channel if a secure keyword argument is provided when instantiating the channel.