Summary

Add TLS client authentication and server verification using custom CA

Review Request #11047 — Created June 18, 2020 and submitted Aug. 18, 2020, 9:37 a.m.

Information

Owner

iro

Repository

RBTools

Branch

master

Bugs

Depends On

Commit

680ad79...

Reviewers

Groups

rbtools

People

Description

This patch adds two new TLS-related features.

Allow the client to provide a certificate for TLS client authentication.
Previously, connecting to a server which requires clients to authenticate using
a TLS certificate was difficult and required using external tools such as stunnel.
Requiring client authentication adds security, because it prevents unauthorized users
without a certificate to even connect to the reviewboard instance. Furthermore,
it optionally allows for using the CN as the user's identity.
After this change, the user can specify a client certificate (and key) to present
to the server at the time of connection.
Allow the client to validate server certificates signed from custom CAs.
Previously, the user had to disable verification of the server certificate
when connecting to a server using a certificate signed by a custom CA.
After this change, the user can provide additional CAs that can be used to perform
server certificate validation, without disabling verification and thus providing a
more secure connection.

Testing Done

Minor testing using a private server signed with custom CA and requiring client certificate.

Issues

Description	From	Last Updated
This looks like a great addition! I think your description ended up being from a fixup commit. Can you flesh …	chipx86	Aug. 18, 2020, 9:26 a.m.
I get this stacktrace now. How can I fix that? Traceback (most recent call last): File "/home/andre/git/ExtendedApproval/contrib/mercurial_git_push.py", line 1389, in …	misery	Aug. 20, 2020, 9:48 a.m.
F401 'six.moves.http_client.UNAUTHORIZED' imported but unused	reviewbot	June 30, 2020, 2:48 a.m.
It was already there	iro	June 30, 2020, 4:58 a.m.
E501 line too long (97 > 79 characters)	reviewbot	June 30, 2020, 2:50 a.m.
E501 line too long (103 > 79 characters)	reviewbot	June 30, 2020, 4:57 a.m.

flake8 failed.

JSHint passed.

flake8

rbtools/api/request.py (Diff revision 1)

Show all issues

F401 'six.moves.http_client.UNAUTHORIZED' imported but unused

rbtools/api/request.py (Diff revision 1)
Show all issues
```
E501 line too long (97 > 79 characters)
```
rbtools/api/request.py (Diff revision 1)
Show all issues
```
E501 line too long (103 > 79 characters)
```

Show all issues

This looks like a great addition!

I think your description ended up being from a fixup commit. Can you flesh it out to go into some detail on this change?

There's some work that needs to be done on docstrings and code format. Do you want me to send some info your way to continue working on that, or do you want us to take this on?

iro June 30, 2020, 5 a.m.

I've updated the description, simplyfied the code, and did my best to respect the style. I'll be glad to fix all the remaining issues you will submit.

chipx86 July 6, 2020, 1:35 p.m.

I really appreciate it, Alessandro. This is looking great, thanks! We're trying to get a Review Board release put together, so give me a few more days on this. I really want to get your change in for RBTools, so it's on my radar for sure.

Change Summary:

After reading the standard library code I've greatly simplifyied the implementation

Summary:

-	Implement TLS client authentication and custom server CA
+	Add TLS client authentication and server verification using custom CA

Description:

~		Do not import from six.moves.http_client twice
	~	This patch adds two new TLS-related features.

~
~
	~	Allow the client to provide a certificate for TLS client authentication. Previously, connecting to a server which requires clients to authenticate using a TLS certificate was difficult and required using external tools such as stunnel. Requiring client authentication adds security, because it prevents unauthorized users without a certificate to even connect to the reviewboard instance. Furthermore, it optionally allows for using the CN as the user's identity. After this change, the user can specify a client certificate (and key) to present to the server at the time of connection.
	~	Allow the client to validate server certificates signed from custom CAs. Previously, the user had to disable verification of the server certificate when connecting to a server using a certificate signed by a custom CA. After this change, the user can provide additional CAs that can be used to perform server certificate validation, without disabling verification and thus providing a more secure connection.
-		Add rcfile config keys

Testing Done:

Minor testing using a private server signed with custom CA and requiring client certificate.

Commit:

-	596f1224166dd3469a83ee9712df92e164f93aee
+	680ad79312689c26ebb778b93d292e1384775bab

Diff:

Revision 2 (+38 -5)

Show changes

	rbtools/api/request.py
	rbtools/api/transport/sync.py
	rbtools/commands/__init__.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/api/request.py (Diff revision 1)
Show all issues
```
It was already there
```

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (2916427)

misery Aug. 18, 2020, 11:39 a.m.

Oh well, Firefox on Android posts empty comments if I click the "menu" button. Really strange. :-(

Show all issues

I get this stacktrace now. How can I fix that?

Traceback (most recent call last):
  File "/home/andre/git/ExtendedApproval/contrib/mercurial_git_push.py", line 1389, in hook
    return process_mercurial_hook(stdin, log)
  File "/home/andre/git/ExtendedApproval/contrib/mercurial_git_push.py", line 1350, in process_mercurial_hook
    return h.push_to_reviewboard(node)
  File "/home/andre/git/ExtendedApproval/contrib/mercurial_git_push.py", line 1254, in push_to_reviewboard
    self._set_root()
  File "/home/andre/git/ExtendedApproval/contrib/mercurial_git_push.py", line 1102, in _set_root
    raise e
AttributeError: 'MercurialGitHookCmd' object has no attribute 'transport_cls'

misery Aug. 20, 2020, 9:49 a.m.

Wrong review request... I mean /r/10894/

misery Aug. 20, 2020, 9:59 a.m.

Fixed here... sorry for the noise. A little bit stressful today.