Summary

Support connecting to hosting services with self-signed certificates.

Review Request #9766 — Created March 12, 2018 and submitted March 19, 2018, 5:18 p.m.

Information

Owner

david

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Commit

9743316...

Reviewers

Groups

reviewboard

People

Description

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).

Testing Done

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 HostingServiceAccount.data field
contained the SSL certificate. Did some operations against the new
repository and saw that they completed successfully.

Issues

Description	From	Last Updated
F401 'reviewboard.hostingsvcs.service.HostingServiceClient' imported but unused	reviewbot	March 12, 2018, 3:36 p.m.
F841 local variable 'fingerprint' is assigned to but never used	reviewbot	March 12, 2018, 3:37 p.m.
F401 'urllib2.HTTPError' imported but unused	reviewbot	March 12, 2018, 3:37 p.m.
, optional	chipx86	March 15, 2018, 4:51 p.m.
Can we make these part of the function signature? Prevents crashes in exception handling if the keys are somehow missing.	chipx86	March 13, 2018, 4:03 p.m.
We should probably localize this.	chipx86	March 13, 2018, 4:07 p.m.
Can we introduce a named logger for this file, to provide some context?	chipx86	March 13, 2018, 4:07 p.m.
This can be a bit nicer as: if 'CERTIFICATE_VERIFY_FAILED' not in six.text_type(e): raise # Rest of the stuff here, sans …	chipx86	March 13, 2018, 4:05 p.m.
Are these in a specific format that we can document?	chipx86	March 13, 2018, 4:11 p.m.
Can you specify the format the fingerprint will be in?	chipx86	March 13, 2018, 4:14 p.m.
So I found out about this. This is an authentication realm, and is designed to provide just a bit of …	chipx86	March 13, 2018, 4:14 p.m.
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 …	chipx86	March 19, 2018, 4:53 p.m.
I believe we can end up in a situation where we hit this error on older Python 2.7.x releases (predating …	chipx86	March 19, 2018, 4:50 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/hostingsvcs/rbgateway.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F401 'reviewboard.hostingsvcs.service.HostingServiceClient' imported but unused
```
reviewboard/hostingsvcs/service.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F841 local variable 'fingerprint' is assigned to but never used
```
reviewboard/scmtools/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F401 'urllib2.HTTPError' imported but unused
```

Commit:

5f3880e501f21d1f7d9073bd834d5b71a629850a

766fcc50a0296d8ba6e02a0e09937a592850701f

Diff:

Revision 2 (+208 -61)

Show changes

	reviewboard/hostingsvcs/forms.py
	reviewboard/hostingsvcs/models.py
	reviewboard/hostingsvcs/rbgateway.py
	reviewboard/hostingsvcs/service.py
	reviewboard/scmtools/certs.py
	reviewboard/scmtools/forms.py
	reviewboard/templates/admin/repository_fields.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

This is excellent. Thanks for taking this on!

reviewboard/hostingsvcs/forms.py (Diff revision 2)

The issue has been resolved. Show all issues

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

reviewboard/hostingsvcs/models.py (Diff revision 2)
The issue has been dropped. Show all issues
```
We should probably localize this.
```
1. david March 13, 2018, 4:14 p.m.
  This is just bullet-proofing and isn't intended to be user-visible.
reviewboard/hostingsvcs/rbgateway.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can we introduce a named logger for this file, to provide some context?
```

reviewboard/hostingsvcs/service.py (Diff revision 2)

The issue has been resolved. Show all issues

This can be a bit nicer as:

if 'CERTIFICATE_VERIFY_FAILED' not in six.text_type(e):
    raise

# Rest of the stuff here, sans extra indentation.

reviewboard/scmtools/certs.py (Diff revision 2)

The issue has been dropped. Show all issues

Are these in a specific format that we can document?

david March 13, 2018, 4:14 p.m.

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.

reviewboard/scmtools/certs.py (Diff revision 2)

The issue has been resolved. Show all issues

Can you specify the format the fingerprint will be in?

david March 13, 2018, 4:14 p.m.

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.

chipx86 March 13, 2018, 4:39 p.m.

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?

reviewboard/scmtools/certs.py (Diff revision 2)

The issue has been dropped. Show all issues

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.

david March 13, 2018, 4:14 p.m.

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.

chipx86 March 13, 2018, 4:39 p.m.

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

Commit:

766fcc50a0296d8ba6e02a0e09937a592850701f

6b39cf9da347f5c86e9a69bc508c927499789e0b

Diff:

Revision 3 (+235 -75)

Show changes

	reviewboard/hostingsvcs/forms.py
	reviewboard/hostingsvcs/models.py
	reviewboard/hostingsvcs/rbgateway.py
	reviewboard/hostingsvcs/service.py
	reviewboard/scmtools/certs.py
	reviewboard/scmtools/forms.py
	reviewboard/templates/admin/repository_fields.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/hostingsvcs/forms.py (Diff revisions 2 - 3)

The issue has been dropped. Show all issues

, optional

david March 15, 2018, 4:23 p.m.

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.

reviewboard/hostingsvcs/service.py (Diff revision 3)

The issue has been resolved. Show all issues

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?

reviewboard/hostingsvcs/service.py (Diff revision 3)

The issue has been resolved. Show all issues

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')):
    raise

Commit:

6b39cf9da347f5c86e9a69bc508c927499789e0b

9743316f15f211971d756c4cf755e5047d6d652d

Diff:

Revision 4 (+241 -75)

Show changes

	reviewboard/hostingsvcs/forms.py
	reviewboard/hostingsvcs/models.py
	reviewboard/hostingsvcs/rbgateway.py
	reviewboard/hostingsvcs/service.py
	reviewboard/scmtools/certs.py
	reviewboard/scmtools/forms.py
	reviewboard/templates/admin/repository_fields.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-3.0.x (c3eed44)