Add TLS client authentication and server verification using custom CA

Review Request #11047 — Created June 18, 2020 and submitted

Information

iro
RBTools
master
680ad79...

Reviewers

This patch adds two new TLS-related features.

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

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

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

Description From Last Updated

This looks like a great addition! I think your description ended up being from a fixup commit. Can you flesh …

chipx86chipx86

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 …

miserymisery

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

reviewbotreviewbot

It was already there

iroiro

E501 line too long (97 > 79 characters)

reviewbotreviewbot

E501 line too long (103 > 79 characters)

reviewbotreviewbot
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

chipx86
  1. 
      
  2. 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?

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

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

  3. 
      
iro
iro
  1. 
      
  2. rbtools/api/request.py (Diff revision 1)
     
     
    Show all issues

    It was already there

  3. 
      
david
  1. Ship It!
  2. 
      
iro
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.0.x (2916427)
misery
  1. 
      
  2. 
      
misery
  1. 
      
    1. Oh well, Firefox on Android posts empty comments if I click the "menu" button. Really strange. :-(

  2. 
      
misery
  1. 
      
  2. 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'
    
    1. Wrong review request... I mean /r/10894/

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

  3.