Add TLS client authentication and server verification using custom CA

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

iro
RBTools
master
680ad79...
rbtools

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

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. 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
Review request changed

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.

   
~  

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

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

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
iro
  1. 
      
  2. rbtools/api/request.py (Diff revision 1)
     
     

    It was already there

  3. 
      
Loading...