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

Catch known built-in SSL provider errors #4

Merged
merged 3 commits into from Mar 2, 2017

Conversation

Projects
None yet
3 participants
@Safihre
Contributor

Safihre commented Feb 14, 2017

We (well, our users) have been using CherryPy with the built-in SSL module quite extensively and a number of errors have come up that are thrown by the built-in provider that can just be ignored.
Using the pyopenssl module, these are caught by pyopenssl, but not by ssl.

An easy example is running the http://testssl.sh/ script against a CherryPy server that uses the built-in provider with any type of certificates.

Maybe we can add some logging about dropped connections to the access log? I wasn't sure, because nowhere in these modules the logging is used.

@Safihre

This comment has been minimized.

Contributor

Safihre commented Feb 17, 2017

No feedback? 😔

@jaraco

This comment has been minimized.

Member

jaraco commented Feb 18, 2017

I don't really have a comment on whether this is right or not. It seems suitable.

You're right that you can't log to the cherrypy access log, because this package is meant to be agnostic to cherrypy and used by other servers.

Can you add a changelog entry to CHANGES.rst explaining the change?

@@ -1,3 +1,9 @@
v5.2.0

This comment has been minimized.

@webknjaz

webknjaz Feb 18, 2017

Member

@jaraco shouldn't this be patch version bump?

@Safihre

This comment has been minimized.

Contributor

Safihre commented Mar 2, 2017

😪

@webknjaz

This comment has been minimized.

Member

webknjaz commented Mar 2, 2017

@Safihre could you please resolve conflicts?

@webknjaz

This comment has been minimized.

Member

webknjaz commented Mar 2, 2017

I wish there were automated tests for this.
Thanks for contribution!

@webknjaz webknjaz merged commit 8f531ca into cherrypy:master Mar 2, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment