Skip to content

Conversation

@anarcat
Copy link

@anarcat anarcat commented Oct 31, 2018

SSLCertificateError was never part of the official tornado API and was
removed in v5. Instead of relying on that undocumented API, we use the
exception from the core ssl module which tornado now uses anyways.

Closes: #384

I remembered to:

  • Update or add unit tests if needed (tests should already be failing)
  • Update or add documentation/comments if needed (not needed)
  • Made sure stray files or whitespace didn't get committed
  • If significant changes, branch from develop and set to merge into develop
  • Read the guidelines for contributing

SSLCertificateError was never part of the official tornado API and was
removed in v5. Instead of relying on that undocumented API, we use the
exception from the core `ssl` module which tornado now uses anyways.

Closes: ArchiveTeam#384
@JustAnotherArchivist
Copy link
Contributor

Sounds good to me.

It appears that even older versions of Tornado use ssl.CertificateError (or rather, ssl.match_hostname) anyway if available (Python 3.2+). This was introduced in Tornado 3.0.1 according to the changelog.
According to the comments in their code, ssl is unavailable on Google App Engine, but I'd imagine that this would mean that SSL is broken entirely there? Since we already rely on ssl.match_hostname anyway in wpull.network.connection.SSLConnection._verify_cert, that's not a concern for us.
On old Python versions (before 3.2), where ssl.match_hostname doesn't exist, Tornado 3.2.0 to 4.5.3 falls back to a backport of that method. In that case, tornado.netutil.SSLCertificateError is not a subclass of ssl.CertificateError (because the latter doesn't exist, duh). But wpull 2 only supports Python 3.4 and up anyway, so this is also not a problem.

However, many tests are currently failing for various reasons, so this will have to wait until #393 and the other test failures introduced by the changes between 2.0.1 and 2.0.3 are resolved. I'll hopefully get to that soon.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants