Skip to content
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

Only use SSL socket if kwargs are not all defaults #1639

Closed
wants to merge 1 commit into from

Conversation

JMANN240
Copy link
Contributor

This is just a small change to accommodate for use cases in which SSL will be used in one environment but not in the other. It allows for a configuration file to dictate SSL kwargs such as keyfile and certfile are while refraining from using SSL if the kwargs given are the default values. In my particular case that led to me changing the code I was doing exactly this (see below code snippet). I am using a configuration file to specify the location of the keyfile and certfile while running my app on my production server, but on my local machine there are no SSL files needed. Previously, the code would only check to see if SSL kwargs were provided and use an SSL socket regardless of value. Now if the SSL kwargs are provided with their default values SSL is not used.

socketio.run(app, host=settings.get("flask_host", "0.0.0.0"), 
                 port=settings.get("port", 8000), 
                 debug=settings.get("debug", False), 
                 keyfile=settings.get("keyfile"), 
                 certfile=settings.get("certfile"))

@miguelgrinberg
Copy link
Owner

Thanks. I recoded it in a simpler way, but I think it should work for your purposes. Let me know if you have any problems.

@JMANN240
Copy link
Contributor Author

Hey Miguel, thanks for getting back to me so quickly. The simpler way doesn't quite work for a few reasons. First, on line 616 I think you might have meant to type kwargs.get(k) rather than ssl_args.get(k) because ssl_args is a list and does not have a get method. Second, the SSL kwargs must all be popped from kwargs, otherwise they get passed to the eventlet server's constructor and cause some issues. This is why in my code I kept a dict ssl_params (any SSL kwarg that was passed, for popping purposes) and a dict non_default_ssl_params (to check if any were set to a non-default value, for actually creating the socket). You can see in my code that I split up the popping (lines 621-622 of my fork) and the socket creation (lines 623-626 of my fork) because the popping needs done whether or not the SSL kwargs are default.

I would also like to note than None is not the default for all of the ssl_args, but it should work for our purposes. Thanks again.

@miguelgrinberg
Copy link
Owner

I corrected that mistake shortly after I committed the first attempt, are you looking at the repository as it is now?

@JMANN240
Copy link
Contributor Author

JMANN240 commented Jul 17, 2021

Oh, my bad. I was looking at commit d6bd555. The popping issue still stands, as when SSL-related kwargs are given that are None, they don't get popped from kwargs, and are passed along to the eventlet server, resulting in this traceback:

Exception in thread Thread-1:
Traceback (most recent call last):
  File "C:\Users\JT\AppData\Local\Programs\Python\Python38\lib\threading.py", line 932, in _bootstrap_inner
    self.run()
  File "C:\Users\JT\AppData\Local\Programs\Python\Python38\lib\threading.py", line 870, in run
    self._target(*self._args, **self._kwargs)
  File "C:\Users\JT\Code\whiteboard\env\lib\site-packages\flask_socketio\__init__.py", line 624, in run_server
    eventlet.wsgi.server(eventlet_socket, app,
TypeError: server() got an unexpected keyword argument 'keyfile'

I can change my fork to feature a for loop rather than a dict comprehension so it pops the kwargs off as it goes if that would be better. That way we wouldn't need 2 dicts of SSL params.

@miguelgrinberg
Copy link
Owner

@JMANN240 I missed the removal of the unused arguments, sorry about that. Let me know if it is better now.

I would also like to note than None is not the default for all of the ssl_args, but it should work for our purposes.

Yeah, I actually do not want to hardcode defaults from another package here. All this package cares is if you include the argument or not. The fact that a value is a default or not should not matter. Using None seems to be sufficient, since there are no arguments that use None as a meaningful non-default setting.

@JMANN240
Copy link
Contributor Author

Looks great, very robust now.

Yeah, I actually do not want to hardcode defaults from another package here.

That makes sense. I agree that None is sufficient, just wanted to bring it to your attention. Thanks again for doing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants