Switch to use ssl.SSLContext in sync worker #2012
Conversation
| context.set_default_verify_paths() | ||
| ciphers = self.cfg.ssl_options['ciphers'] | ||
| # set_ciphers() will raise an exception if we pass None (default value) | ||
| if ciphers is not None and isinstance(ciphers, str): |
There was a problem hiding this comment.
if isinstance(ciphers, str): should work, right?
>>> isinstance(None, str)
False| client = ssl.wrap_socket(client, server_side=True, | ||
| **self.cfg.ssl_options) | ||
| # TODO: Make this reusable by gthread worker. Move to BaseWorker? | ||
| context = ssl.SSLContext(self.cfg.ssl_options['ssl_version']) |
There was a problem hiding this comment.
I wonder if we can build this context once when we parse the config and store it on self.cfg.
There was a problem hiding this comment.
Yes, building and storing it on self.cfg is definitely better than this :) We can move this snippet to util.create_ssl_context()and call it there.
I'm also planning to separate Settings and validate_* functions in gunicorn/config.py.
|
@berkerpeksag do you need any help or it? Let me know. |
|
bump |
|
I will work on this tonight. |
|
@berkerpeksag bump again :) what's missing there? Anything I can help? I think we need to make a release today. I willl take care of the 19. release right after. Let me know |
|
This is not ready yet and I probably won't have time to finish it until next weekend. |
|
ok I would then suggest to make a release of 20.0 today and release 20.1 when your code is out next week. What do you and others think about it?
|
|
@berkerpeksag �since i'm late in this release I wanted to finish this improvements before releasing it as it would be more than welcome. I can help on it. Others than @tilgovi comments, do you have others stuffs in mind you wanted to do on this. patch? |
|
bump @berkerpeksag , please let me know if you can do this , would be good to have this change so we can make a new stable release. |
|
Apologies for my late response. The good thing is that I've been working with X.509 certificates at work lately and I think I have a much better understanding of the ssl module now. I'll try finish this up before the end of the month. |
|
@berkerpeksag I propos to assign this to next release 20.1 to not wait more to release the 20.0. 20.1 can happen any time for me, even if it's mean 1 day after the 20.0 is out :) Is this OK for you? |
|
bump. |
|
I'm testing my branch with a self-signed certificate at the moment. I will try to push my updated branch today or tomorrow. |
| print(exc) | ||
| raise exc | ||
| # TODO: OP_CIPHER_SERVER_PREFERENCE is set by default since Python 3.6+. | ||
| context.options |= ssl.OP_CIPHER_SERVER_PREFERENCE |
There was a problem hiding this comment.
Any chance this can be exposed as a global configuration option? It would be very nice, particularly if you want to exclude specific protocol versions.
e.g.
ssl_options = ssl.OP_NO_TLSv1 | ssl.OP_NO_TLSv1_1 | ssl.OP_NO_TLSv1_2
There was a problem hiding this comment.
Yes, that's definitely possible!
BTW, I think SSLContext in Python 3.6+ sets all of those options by default.
|
@berkerpeksag i assume you're quite busy ... Can you let me know what still need to be done there to close the issue? I or maybe someone else can help :) |
|
Yes, unfortunately I may not be able to get to this in near future. There is a chance that I can finish it next month when I'm on vacation, but I've already broken too much promises :) What's missing:
|
|
@berkerpeksag I have submitted another PR #2649 which proposes similar |
|
superseded by #2649 which have been merged. Thanks for the PR anyway. |
Closes #1140