Support connecting to hosting services with self-signed certificates.

Review Request #9766 — Created March 12, 2018 and submitted

David Trowbridge
Review Board

Most hosting services are third-party systems that will be served with
valid, secure SSL certificates. A few (such as rb-gateway, gerrit, or
GitLab) are run locally, and therefore may be deployed using self-signed
certificates. Until a couple years ago, this wasn't a problem, because
Python's urllib2 implementation didn't actually check SSL certificates
for validity. As of 2.7.9, it does.

We had some infrastructure in place for accepting invalid or self-signed
certificates, but it was only implemented for SCMTools, and even then
only for SVN and Perforce. Accepting certificates for services that were
connected to via urllib2, and especially services that go through a
HostingService, was not implemented.

This change does that. We now check for SSL-related errors when
attempting to authorize the account. In that case, we'll fetch and
decode the certificate, and show the "I trust this host" prompt to the
user. If the user accepts, we'll store the certificate in the
HostingServiceAccount's data field. Future attempts to use that
account will then add that certificate to the validation change (and
disable hostname checking, since self-signed certificates often are not
pinned to the correct hostname).

Set up rb-gateway using a self-signed certificate (along with a pending
change to enable SSL in rb-gateway) and attempted to connect it. Saw
the SSL certificate prompt, and selected "I trust this host". The
repository entry was saved, and the field
contained the SSL certificate. Did some operations against the new
repository and saw that they completed successfully.

  • 0
  • 0
  • 9
  • 4
  • 13
Description From Last Updated
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.


David Trowbridge
Christian Hammond
  1. This is excellent. Thanks for taking this on!

  2. reviewboard/hostingsvcs/ (Diff revision 2)

    Can we make these part of the function signature? Prevents crashes in exception handling if the keys are somehow missing.

  3. reviewboard/hostingsvcs/ (Diff revision 2)

    We should probably localize this.

    1. This is just bullet-proofing and isn't intended to be user-visible.

  4. reviewboard/hostingsvcs/ (Diff revision 2)

    Can we introduce a named logger for this file, to provide some context?

  5. reviewboard/hostingsvcs/ (Diff revision 2)

    This can be a bit nicer as:

    if 'CERTIFICATE_VERIFY_FAILED' not in six.text_type(e):
    # Rest of the stuff here, sans extra indentation.
  6. reviewboard/scmtools/ (Diff revision 2)

    Are these in a specific format that we can document?

    1. I don't think so. SVN gives us a "cert_data" structure that has it in whatever format the SVN implementation gives us. I'm using ISO-8601 for the urllib-based one. It just is something to show in the "I trust this host" pop-up.

  7. reviewboard/scmtools/ (Diff revision 2)

    Can you specify the format the fingerprint will be in?

    1. Not in any super useful way. Different implementations use whatever methods they care to. SVN and perforce seem to use legacy MD5-sums, and my new code uses sha256.

    2. Okay. In that case, can you update the docs to mention that this can be in various forms and is not expected to be a particular type?

  8. reviewboard/scmtools/ (Diff revision 2)

    So I found out about this. This is an authentication realm, and is designed to provide just a bit of additional context as to what's being logged into, mainly for error messages. It's not really part of SSL itself, more a SVN thing, I believe.

    So perhaps what we want is to replace this with a dictionary of extra metadata to go along with the certificate, and have SVN populate that with "realm" and pull it out of there as needed.

    1. That seems like a fine solution but not for this change. I'd also be happy just getting rid of "realm" entirely since I don't think anyone actually cares about it.

    2. The SVN approval process requires it, so we still need to store it.

David Trowbridge
Christian Hammond
  2. reviewboard/hostingsvcs/ (Diff revisions 2 - 3)

    , optional

    1. This is not actually optional. I provide a default value because I need it to be a kwarg but it will always be provided by the caller and authorization won't succeed if it's not present.

Christian Hammond
  2. reviewboard/hostingsvcs/ (Diff revision 3)

    This doesn't exist in Python pre-2.7.9. Can you add a comment about the version requirement and that we shouldn't have been able to make it here on older versions due to the lack of ssl_cert in data?

  3. reviewboard/hostingsvcs/ (Diff revision 3)

    I believe we can end up in a situation where we hit this error on older Python 2.7.x releases (predating the better SSL support in 2.7.9) for bad SSL certificates, and then we'd end up calling into ssl.* functions that don't exist in that version. Let's wire things off so that we don't get any further if we're running 2.7.8 or lower:

    if ('CERTIFICATE_VERIFY_FAILED' not in six.text_type(e) or
        not hasattr(ssl, 'create_default_context')):
David Trowbridge
Christian Hammond
  1. Ship It!
David Trowbridge
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (c3eed44)