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

does not work HTTPS on gunicorn 19.3.0 and tornado 4.3 #1135

Closed
shrkw opened this Issue Oct 27, 2015 · 13 comments

Comments

Projects
None yet
4 participants
@shrkw

shrkw commented Oct 27, 2015

I'm not sure this is a problem on gunicorn, but please ask a question here.
I'm working on an implementation simple tornado app and it works on gunicorn, also it listen HTTPS. It worked great with gunicorn 18.0 + tornado 3.0.
However, after doing I upgrade gunicorn to 19.3.0 for using ca-certs option (also upgrade tornado to 4..2.1), it does not work fine.

If someone point out the cause of this is which one gunicorn or tornado, it would be great help to me.

here is a starting command line.

$ gunicorn --certfile=/home/hiro/201510/server.crt --keyfile=/home/hiro/201510/server.key -b 0.0.0.0:16189 -w 1 -k "tornado" 'httpproxy:get_service("tcp://0.0.0.0:5555")'

and stacktrace is following:

[2015-10-27 20:29:04 +0000] [4360] [INFO] Booting worker with pid: 4360
ERROR:tornado.application:Exception in callback (<gunicorn.sock.TCPSocket object at 0x2a6bc50>, <function null_wrapper at 0x2c91488>)
Traceback (most recent call last):
  File "/home/hiro/.virtualenvs/CP275_asyncproxy/lib/python2.7/site-packages/tornado/ioloop.py", line 866, in start
    handler_func(fd_obj, events)
  File "/home/hiro/.virtualenvs/CP275_asyncproxy/lib/python2.7/site-packages/tornado/stack_context.py", line 275, in null_wrapper
    return fn(*args, **kwargs)
  File "/home/hiro/.virtualenvs/CP275_asyncproxy/lib/python2.7/site-packages/tornado/netutil.py", line 265, in accept_handler
    callback(connection, address)
  File "/home/hiro/.virtualenvs/CP275_asyncproxy/lib/python2.7/site-packages/tornado/tcpserver.py", line 239, in _handle_connection
    do_handshake_on_connect=False)
  File "/home/hiro/.virtualenvs/CP275_asyncproxy/lib/python2.7/site-packages/tornado/netutil.py", line 501, in ssl_wrap_socket
    context = ssl_options_to_context(ssl_options)
  File "/home/hiro/.virtualenvs/CP275_asyncproxy/lib/python2.7/site-packages/tornado/netutil.py", line 471, in ssl_options_to_context
    assert all(k in _SSL_CONTEXT_KEYWORDS for k in ssl_options), ssl_options
AssertionError: {'do_handshake_on_connect': False, 'certfile': '/home/hiro/201510/server.crt', 'suppress_ragged_eofs': True, 'ciphers': 'TLSv1', 'ssl_version': 3, 'cert_reqs': 0, 'ca_certs': None, 'keyfile': '/home/hiro/201510/server.key'}

_SSL_CONTEXT_KEYWORDS declared in netutil.py is following:

_SSL_CONTEXT_KEYWORDS = frozenset(['ssl_version', 'certfile', 'keyfile',
                                   'cert_reqs', 'ca_certs', 'ciphers'])
@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Oct 27, 2015

Owner

seems like tornado is now limiting the number of settings. suppress_ragged_eofs is added by gunicorn here but not accepted. A patch is coming :)

Owner

benoitc commented Oct 27, 2015

seems like tornado is now limiting the number of settings. suppress_ragged_eofs is added by gunicorn here but not accepted. A patch is coming :)

@shrkw

This comment has been minimized.

Show comment
Hide comment
@shrkw

shrkw Oct 28, 2015

sorry, I edited this comment because my previous comment was wrong. Your suggestion made perfect! Thank you so much!

*** /home/hiro/.virtualenvs/CP275_asyncproxy/lib/python2.7/site-packages/gunicorn/workers/gtornado_.py  2015-10-28 10:54:14.405533346 +0900
--- /home/hiro/.virtualenvs/CP275_asyncproxy/lib/python2.7/site-packages/gunicorn/workers/gtornado.py   2015-10-28 10:34:09.014425348 +0900
***************
*** 3,8 ****
--- 3,9 ----
  # This file is part of gunicorn released under the MIT license.
  # See the NOTICE for more information.

+ import copy
  import os
  import sys

***************
*** 89,96 ****
              server_class = _HTTPServer

          if self.cfg.is_ssl:
              server = server_class(app, io_loop=self.ioloop,
!                     ssl_options=self.cfg.ssl_options)
          else:
              server = server_class(app, io_loop=self.ioloop)

--- 90,100 ----
              server_class = _HTTPServer

          if self.cfg.is_ssl:
+             _ssl_opt = copy.deepcopy(self.cfg.ssl_options)
+             del _ssl_opt["do_handshake_on_connect"]
+             del _ssl_opt["suppress_ragged_eofs"]
              server = server_class(app, io_loop=self.ioloop,
!                     ssl_options=_ssl_opt)
          else:
              server = server_class(app, io_loop=self.ioloop)

shrkw commented Oct 28, 2015

sorry, I edited this comment because my previous comment was wrong. Your suggestion made perfect! Thank you so much!

*** /home/hiro/.virtualenvs/CP275_asyncproxy/lib/python2.7/site-packages/gunicorn/workers/gtornado_.py  2015-10-28 10:54:14.405533346 +0900
--- /home/hiro/.virtualenvs/CP275_asyncproxy/lib/python2.7/site-packages/gunicorn/workers/gtornado.py   2015-10-28 10:34:09.014425348 +0900
***************
*** 3,8 ****
--- 3,9 ----
  # This file is part of gunicorn released under the MIT license.
  # See the NOTICE for more information.

+ import copy
  import os
  import sys

***************
*** 89,96 ****
              server_class = _HTTPServer

          if self.cfg.is_ssl:
              server = server_class(app, io_loop=self.ioloop,
!                     ssl_options=self.cfg.ssl_options)
          else:
              server = server_class(app, io_loop=self.ioloop)

--- 90,100 ----
              server_class = _HTTPServer

          if self.cfg.is_ssl:
+             _ssl_opt = copy.deepcopy(self.cfg.ssl_options)
+             del _ssl_opt["do_handshake_on_connect"]
+             del _ssl_opt["suppress_ragged_eofs"]
              server = server_class(app, io_loop=self.ioloop,
!                     ssl_options=_ssl_opt)
          else:
              server = server_class(app, io_loop=self.ioloop)

shrkw pushed a commit to shrkw/gunicorn that referenced this issue Oct 28, 2015

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Oct 28, 2015

Collaborator

Might Tornado want to add these options?

Collaborator

tilgovi commented Oct 28, 2015

Might Tornado want to add these options?

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Oct 28, 2015

Collaborator

Not saying that changes the solution here, now, but maybe we can notify them?

Collaborator

tilgovi commented Oct 28, 2015

Not saying that changes the solution here, now, but maybe we can notify them?

@shrkw

This comment has been minimized.

Show comment
Hide comment
@shrkw

shrkw Oct 28, 2015

Thank you for your comment. Exactly, tornado should possibly accept these ssl options and looks the change on tornado side would not be complex. I'll notify them.
After confirmation on tornado side, I'll remove PR and close this, or may ask here again.

shrkw commented Oct 28, 2015

Thank you for your comment. Exactly, tornado should possibly accept these ssl options and looks the change on tornado side would not be complex. I'll notify them.
After confirmation on tornado side, I'll remove PR and close this, or may ask here again.

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Oct 28, 2015

Collaborator

We can merge it, and then update it with conditionals when we know what tornado version to test for.

Collaborator

tilgovi commented Oct 28, 2015

We can merge it, and then update it with conditionals when we know what tornado version to test for.

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Oct 28, 2015

Owner

+1
On Wed, 28 Oct 2015 at 06:20, Randall Leeds notifications@github.com
wrote:

We can merge it, and then update it with conditionals when we know what
tornado version to test for.


Reply to this email directly or view it on GitHub
#1135 (comment).

Owner

benoitc commented Oct 28, 2015

+1
On Wed, 28 Oct 2015 at 06:20, Randall Leeds notifications@github.com
wrote:

We can merge it, and then update it with conditionals when we know what
tornado version to test for.


Reply to this email directly or view it on GitHub
#1135 (comment).

@shrkw

This comment has been minimized.

Show comment
Hide comment
@shrkw

shrkw Oct 30, 2015

It seems tornado side does not want to allow do_handshake_on_connect option and allow_ragged_eofs.

do_handshake_on_connect=True is for blocking mode, so we definitely don't want to allow that. And allow_ragged_eofs seems redundant with SSLIOStream's error handling.

tornadoweb/tornado#1570

I'm not sure which side should accept the change.

shrkw commented Oct 30, 2015

It seems tornado side does not want to allow do_handshake_on_connect option and allow_ragged_eofs.

do_handshake_on_connect=True is for blocking mode, so we definitely don't want to allow that. And allow_ragged_eofs seems redundant with SSLIOStream's error handling.

tornadoweb/tornado#1570

I'm not sure which side should accept the change.

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Oct 30, 2015

Collaborator

We should merge the PR here.

Collaborator

tilgovi commented Oct 30, 2015

We should merge the PR here.

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Oct 30, 2015

Collaborator

By the way, I agree with Ben's comment about SSLContext.

Collaborator

berkerpeksag commented Oct 30, 2015

By the way, I agree with Ben's comment about SSLContext.

@benoitc benoitc added the bug :( label Oct 30, 2015

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Nov 2, 2015

Owner

@tilgovi isn't the PR merged?

Owner

benoitc commented Nov 2, 2015

@tilgovi isn't the PR merged?

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Nov 2, 2015

Owner

@berkerpeksag can we use an SSLContext easily with python < 2.7.9 ? In the other case I would suggest for the 19.x version to stick to the current usage but removing useless options for tornado and introduce it in 20.x with python 3 support only. Thoughts?

Owner

benoitc commented Nov 2, 2015

@berkerpeksag can we use an SSLContext easily with python < 2.7.9 ? In the other case I would suggest for the 19.x version to stick to the current usage but removing useless options for tornado and introduce it in 20.x with python 3 support only. Thoughts?

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Nov 2, 2015

Collaborator

@berkerpeksag can we use an SSLContext easily with python < 2.7.9 ?

Unfortunately, no, we can't use it.

In the other case I would suggest for the 19.x version to stick to the current usage but removing useless options for tornado and introduce it in 20.x with python 3 support only.

That looks like a good idea to me. I've opened #1140.

Collaborator

berkerpeksag commented Nov 2, 2015

@berkerpeksag can we use an SSLContext easily with python < 2.7.9 ?

Unfortunately, no, we can't use it.

In the other case I would suggest for the 19.x version to stick to the current usage but removing useless options for tornado and introduce it in 20.x with python 3 support only.

That looks like a good idea to me. I've opened #1140.

@benoitc benoitc closed this in #1136 Nov 16, 2015

fofanov pushed a commit to fofanov/gunicorn that referenced this issue Mar 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment