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

NoSSLError not caught #1401

Closed
bb-migration opened this Issue Jan 10, 2016 · 12 comments

Comments

3 participants
@bb-migration

bb-migration commented Jan 10, 2016

Originally reported by: Red_M (Bitbucket: Red_M, GitHub: Unknown)


I have found bug most likely caused by some bot on the internet as I get this every now and again.
I am unable to reproduce the error but I have found the root.

The trace back:

#!python
Traceback (most recent call last):
  File "/usr/lib/python2.7/threading.py", line 810, in __bootstrap_inner
    self.run()
  File "/usr/local/lib/python2.7/dist-packages/cherrypy/wsgiserver/wsgiserver2.py", line 1530, in run
    conn.communicate()
  File "/usr/local/lib/python2.7/dist-packages/cherrypy/wsgiserver/wsgiserver2.py", line 1378, in communicate
    req.simple_response("408 Request Timeout")
  File "/usr/local/lib/python2.7/dist-packages/cherrypy/wsgiserver/wsgiserver2.py", line 909, in simple_response
    self.conn.wfile.sendall("".join(buf))
  File "/usr/local/lib/python2.7/dist-packages/wsgiserver/wsgiserver2.py", line 1015, in sendall
    bytes_sent = self.send(data)
  File "/usr/local/lib/python2.7/dist-packages/cherrypy/wsgiserver/ssl_pyopenssl.py", line 109, in send
    *args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/cherrypy/wsgiserver/ssl_pyopenssl.py", line 91, in _safe_call
    raise wsgiserver.NoSSLError()
NoSSLError

This exception is not caught by the calls at line 909 or line 1378, it would be best to do so.


@bb-migration

This comment has been minimized.

Show comment
Hide comment
@bb-migration

bb-migration Jan 10, 2016

Original comment by Red_M (Bitbucket: Red_M, GitHub: Unknown):


I can write patches for this if needed.

bb-migration commented Jan 10, 2016

Original comment by Red_M (Bitbucket: Red_M, GitHub: Unknown):


I can write patches for this if needed.

@Red-M

This comment has been minimized.

Show comment
Hide comment
@Red-M

Red-M May 12, 2016

Just posting because the migration bot didn't pick up my GH account.

Red-M commented May 12, 2016

Just posting because the migration bot didn't pick up my GH account.

@jaraco

This comment has been minimized.

Show comment
Hide comment
@jaraco

jaraco May 14, 2016

Member

Hi Red. Thanks for linking your account.

Per the traceback, it seems that line 909 or 1378 are in wsgiserver2.

The exception is not caught..., it would be best to do so.

It's not obvious to my why it would be best to do so. What behavior are you expecting that you're not seeing? Why is suppressing the exception (at one of two places) a better solution? A patch may be useful to help illustrate what you have in mind.

Member

jaraco commented May 14, 2016

Hi Red. Thanks for linking your account.

Per the traceback, it seems that line 909 or 1378 are in wsgiserver2.

The exception is not caught..., it would be best to do so.

It's not obvious to my why it would be best to do so. What behavior are you expecting that you're not seeing? Why is suppressing the exception (at one of two places) a better solution? A patch may be useful to help illustrate what you have in mind.

@Red-M

This comment has been minimized.

Show comment
Hide comment
@Red-M

Red-M May 14, 2016

I believe that this due to some mismatch of SSL cipher or just not having SSL on the client but I cannot be sure.
I think it should be sending the HTTPS only message but this was a while ago so I'll need to read into it again.

Red-M commented May 14, 2016

I believe that this due to some mismatch of SSL cipher or just not having SSL on the client but I cannot be sure.
I think it should be sending the HTTPS only message but this was a while ago so I'll need to read into it again.

@Red-M

This comment has been minimized.

Show comment
Hide comment
@Red-M

Red-M Aug 3, 2016

Due to the recent changes with wsgiserver, I'd like to try and get another track back asap and also provide some patches when I am able to. Sorry about getting back to you a few months later.

Red-M commented Aug 3, 2016

Due to the recent changes with wsgiserver, I'd like to try and get another track back asap and also provide some patches when I am able to. Sorry about getting back to you a few months later.

@Red-M

This comment has been minimized.

Show comment
Hide comment
@Red-M

Red-M Aug 13, 2016

Exception in thread CP Server Thread-141:
Traceback (most recent call last):
  File "/usr/lib/python2.7/threading.py", line 810, in __bootstrap_inner
    self.run()
  File "/usr/local/lib/python2.7/dist-packages/cherrypy/wsgiserver/__init__.py", line 1583, in run
    conn.communicate()
  File "/usr/local/lib/python2.7/dist-packages/cherrypy/wsgiserver/__init__.py", line 1425, in communicate
   req.simple_response("408 Request Timeout")
  File "/usr/local/lib/python2.7/dist-packages/cherrypy/wsgiserver/__init__.py", line 899, in simple_response
    self.conn.wfile.write(EMPTY.join(buf))
  File "/usr/local/lib/python2.7/dist-packages/cherrypy/wsgiserver/__init__.py", line 1046, in write
    bytes_sent = self.send(data)
  File "/usr/local/lib/python2.7/dist-packages/cherrypy/wsgiserver/ssl_pyopenssl.py", line 109, in send
    *args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/cherrypy/wsgiserver/ssl_pyopenssl.py", line 91, in _safe_call
    raise wsgiserver.NoSSLError()
NoSSLError

Line 1425 in __init__.py for the wsgiserver, the except isn't wide enough and misses the HTTP request on a HTTPS socket.
I'm not sure what the end user sees as I've never been able to trigger it but it might be an idea to catch this and try to communicate over HTTP instead? or just return the stock standard "this is HTTPS" response.

Red-M commented Aug 13, 2016

Exception in thread CP Server Thread-141:
Traceback (most recent call last):
  File "/usr/lib/python2.7/threading.py", line 810, in __bootstrap_inner
    self.run()
  File "/usr/local/lib/python2.7/dist-packages/cherrypy/wsgiserver/__init__.py", line 1583, in run
    conn.communicate()
  File "/usr/local/lib/python2.7/dist-packages/cherrypy/wsgiserver/__init__.py", line 1425, in communicate
   req.simple_response("408 Request Timeout")
  File "/usr/local/lib/python2.7/dist-packages/cherrypy/wsgiserver/__init__.py", line 899, in simple_response
    self.conn.wfile.write(EMPTY.join(buf))
  File "/usr/local/lib/python2.7/dist-packages/cherrypy/wsgiserver/__init__.py", line 1046, in write
    bytes_sent = self.send(data)
  File "/usr/local/lib/python2.7/dist-packages/cherrypy/wsgiserver/ssl_pyopenssl.py", line 109, in send
    *args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/cherrypy/wsgiserver/ssl_pyopenssl.py", line 91, in _safe_call
    raise wsgiserver.NoSSLError()
NoSSLError

Line 1425 in __init__.py for the wsgiserver, the except isn't wide enough and misses the HTTP request on a HTTPS socket.
I'm not sure what the end user sees as I've never been able to trigger it but it might be an idea to catch this and try to communicate over HTTP instead? or just return the stock standard "this is HTTPS" response.

@jaraco

This comment has been minimized.

Show comment
Hide comment
@jaraco

jaraco Aug 13, 2016

Member

If the user is connecting to an SSL server but isn't doing SSL negotiation, I'm not sure there is a standard "this is HTTPS" response. I checked with Google's HTTPS server:

$ telnet www.google.com 443
Trying 2607:f8b0:4004:807::2004...
Connected to www.google.com.
Escape character is '^]'.
GET / HTTP/1.0
Connection closed by foreign host.

It didn't send anything, but instead just closed the connection.

I would expect CherryPy to do the same, and maybe that's what it does when the NoSSLError is raised. What's the unexpected behavior when this error is raised? It looks like that thread will crash. Is that a problem? Will CherryPy replace the thread?

Now that I look more closely at wsgiserver:1425, I see that FatalSSLAlert is caught, but not NoSSLError, even though both of these exceptions are defined alongside each other, and their both raised in the same context.

At this point, I do think that both errors should be caught.

Member

jaraco commented Aug 13, 2016

If the user is connecting to an SSL server but isn't doing SSL negotiation, I'm not sure there is a standard "this is HTTPS" response. I checked with Google's HTTPS server:

$ telnet www.google.com 443
Trying 2607:f8b0:4004:807::2004...
Connected to www.google.com.
Escape character is '^]'.
GET / HTTP/1.0
Connection closed by foreign host.

It didn't send anything, but instead just closed the connection.

I would expect CherryPy to do the same, and maybe that's what it does when the NoSSLError is raised. What's the unexpected behavior when this error is raised? It looks like that thread will crash. Is that a problem? Will CherryPy replace the thread?

Now that I look more closely at wsgiserver:1425, I see that FatalSSLAlert is caught, but not NoSSLError, even though both of these exceptions are defined alongside each other, and their both raised in the same context.

At this point, I do think that both errors should be caught.

@jaraco jaraco closed this in 4988a2f Aug 13, 2016

@Red-M

This comment has been minimized.

Show comment
Hide comment
@Red-M

Red-M Aug 13, 2016

I was meaning stock standard for cherrypy, more so meaning canned response.

Talking to an SSL port on cherrypy when the client isn't talking SSL, would at one time with cherrypy, returned a response to the client similar to "This server talks HTTPS on this port but you are using HTTP"

Thanks though!

Red-M commented Aug 13, 2016

I was meaning stock standard for cherrypy, more so meaning canned response.

Talking to an SSL port on cherrypy when the client isn't talking SSL, would at one time with cherrypy, returned a response to the client similar to "This server talks HTTPS on this port but you are using HTTP"

Thanks though!

@jaraco

This comment has been minimized.

Show comment
Hide comment
@jaraco

jaraco Aug 13, 2016

Member

Yeah, maybe you're right.

Member

jaraco commented Aug 13, 2016

Yeah, maybe you're right.

@jaraco jaraco reopened this Aug 13, 2016

@Red-M

This comment has been minimized.

Show comment
Hide comment
@Red-M

Red-M Aug 13, 2016

Found the line:

req.simple_response(
"400 Bad Request",
"The client sent a plain HTTP request, but "
"this server only speaks HTTPS on this port.")

Red-M commented Aug 13, 2016

Found the line:

req.simple_response(
"400 Bad Request",
"The client sent a plain HTTP request, but "
"this server only speaks HTTPS on this port.")

@jaraco jaraco closed this in 58511cd Aug 13, 2016

@jaraco

This comment has been minimized.

Show comment
Hide comment
@jaraco

jaraco Aug 13, 2016

Member

@Red_M: what do you think of that approach?

Member

jaraco commented Aug 13, 2016

@Red_M: what do you think of that approach?

@Red-M

This comment has been minimized.

Show comment
Hide comment
@Red-M

Red-M Aug 13, 2016

that seems like it should fix the problem.

Red-M commented Aug 13, 2016

that seems like it should fix the problem.

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