Add support for TLS/SSL mutual authentication #428

zuohaocheng · 2017-07-13T02:44:00Z

Add support in the TLS_FTPHandler to check a client certificate. This
type of support strengthens the security between the client and the
server, only allowing clients with a valid certificate to connect to the
server.

Updated the api.rst file with the two new configurable options to make
client authentication work.

Note: This is an improvement of #396 as the original committer has moved on to another team.

giampaolo

Thanks a lot for putting this together. I added a couple of comments inline but overall this looks good (and thanks for the unit tests).

Just one thing: this means the client can only authenticate if it provides a certfile, correct?

Last: please also update HISTORY and CREDITS files.

giampaolo · 2017-07-14T07:48:09Z

pyftpdlib/handlers.py

+ from OpenSSL.SSL import SESS_CACHE_OFF
+ from OpenSSL.SSL import VERIFY_CLIENT_ONCE
+ from OpenSSL.SSL import VERIFY_FAIL_IF_NO_PEER_CERT
+ from OpenSSL.SSL import VERIFY_PEER


I wouldn't do this at import time. If one of these constants is not available for some reason the whole FTPS support won't be available. Better to do SSL.* later, when if self.client_certfile is not None:.

giampaolo · 2017-07-14T07:54:44Z

docs/api.rst


+ The path of the certificate to check the client certificate against. (default ``None``).


How about adding "...only allowing clients with a valid certificate to connect to the server"?

Also, please add:

.. versionadded:: 1.5.3

Thanks for advices and done.

giampaolo · 2017-07-14T07:57:44Z

pyftpdlib/test/functional_ssl_client_certfile_tests.py

+# Does not follow naming convention of other tests because this
+# CANNOT be run in the same test suite with test_functional_ssl.
+# The test parallelism causes SSL errors when there should be none
+# Please run these tests separately


Mmmm this worries me. What does it mean exactly? That if one of these tests fail also the next ones related to SSL will fail as well?

Thanks for reminding. I found out the root cause is TLS_FTPHandler.get_ssl_context is a static method, thus it would get messed up in test cases. I changed it to non-static and the issue is gone, so these tests are merged back to test_functional_ssl.py.

giampaolo · 2017-07-14T08:01:44Z

pyftpdlib/handlers.py

+ cls.verify_certs_callback)
+ cls.ssl_context.load_verify_locations(cls.client_certfile)
+ cls.ssl_context.set_session_cache_mode(SESS_CACHE_OFF)
+ cls.ssl_options = cls.ssl_options | OP_NO_TICKET


It would be nice to add a comment explaining what SESS_CACHE_OFF and OP_NO_TICKET do.

giampaolo · 2017-07-14T08:02:50Z

pyftpdlib/handlers.py

+ self.log("Bad client certificate detected.")
+ else:
+ self.log("Client certificate is valid.")
+ return ok


What happens if the client submits an invalid cert from the server standpoint? I suppose the connection gets automatically closed due to a SSL.Error exception during the SSL handshake, correct? We already have a callback method for such an event, handle_failed_ssl_handshake. Does it get called?

It's true that handle_failed_ssl_handshake will be called when client certificate is not provided. Meanwhile, self.log("Bad client certificate detected.") will still be called if client does provide a certificate, but it could not pass the check with server one.

The actual result can be seen from test cases test_auth_client_nocert and test_auth_client_badcert.

Add support in the TLS_FTPHandler to check a client certificate. This type of support strengthens the security between the client and the server, only allowing clients with a valid certificate to connect to the server. Updated the api.rst file with the two new configurable options to make client authentication work

…vironment

…alidate_ssl_options

…n Travis

zuohaocheng · 2017-07-17T08:50:12Z

@giampaolo Thanks for your review, I've made changes accordingly and passed CI.

BTW, Issue #336 seems to be related to this PR.

zuohaocheng · 2017-07-21T04:20:58Z

@giampaolo May you kindly review the updated PR?

giampaolo · 2017-07-22T18:38:44Z

docs/api.rst

+ to the server (default ``None``).
+
+ .. versionadded:: 1.5.3 
+


This is not entirely clear. To be cristal clear, does it mean the client must have the exact same certificate? If so, I would recommend this wording:

The path to a file which contains a certificate to be used to identify the client. If specified, the client should is supposed to use the same certificate in order to connect.

Thanks for advice and reworded.

The certificates client side and server side need are the basically same, but for client side, private key is needed. To my understanding, it's a bit similar to a "reversed" SSL/TLS certificate validation.

giampaolo · 2017-07-22T18:39:35Z

pyftpdlib/handlers.py

@@ -3419,6 +3419,8 @@ class TLS_FTPHandler(SSLConnection, FTPHandler):
 certfile = None
 keyfile = None
 ssl_protocol = SSL.SSLv23_METHOD
+ # client certificate configurable attributes


this line is probably useless

giampaolo · 2017-07-22T18:40:45Z

pyftpdlib/handlers.py

- if cls.ssl_options:
- cls.ssl_context.set_options(cls.ssl_options)
- return cls.ssl_context
+ def validate_ssl_options(cls):


why was this renamed?

The static method here does only validation on startup, so I rename it to validate. get_ssl_context, which does the whole setup, is still there. Only thing different is, it is not a static method any more.

Why not put everything into get_ssl_context then or rename it to _validate_ssl_context? Later on you do:

ctx = TLS_FTPHandler.validate_ssl_options()

...which is a bit weird

giampaolo · 2017-07-22T18:41:50Z

pyftpdlib/test/test_functional_ssl.py

+ if use_client_cert:
+ self.handler.client_certfile = CLIENT_CERTFILE
+ else:
+ self.handler.client_certfile = None


the else block is probably unnecessary as client_certfile already defaults to None, no?

Hmm not really in this case. client_certfile is a static member, so it won't be reset if the class is instantiated multiple times. It's quite an unlikely use case in production code, but it does matter in unit tests.

zuohaocheng · 2017-07-25T12:56:40Z

@giampaolo Thanks very much for your review, updated per comments. Please kindly review it again and let me know if further update is necessary.

zuohaocheng · 2017-07-27T05:59:22Z

@giampaolo Updated, and fixed a test failure due to latest version of python/openssl 3.5 & 3.6, which is not related to the PR.

giampaolo · 2017-07-29T17:48:30Z

docs/api.rst

+ the client. If specified, only clients with a valid certificate are able
+ to connect to the server (default ``None``).
+
+ .. versionadded:: 1.5.3 


Shouldn't this be "clients with THE SAME certificate"?

giampaolo · 2017-07-29T17:53:17Z

pyftpdlib/handlers.py

+ cls.ssl_options = cls.ssl_options | OP_NO_TICKET
+ if cls.ssl_options:
+ ssl_context.set_options(cls.ssl_options)
+ return ssl_context


Sorry but I'm finding it difficult to review this code. Please just add the new functionality to the existent get_ssl_context method, without changing or refactoring anything else. If you/we find out it can be refactored in different methods and made it better let's just do it in another PR, but for now I suggest to keep all changes to the bare minimum.

giampaolo · 2017-07-29T17:54:19Z

pyftpdlib/test/test_functional_ssl.py

- finally:
- TLS_FTPHandler.ssl_context = None
+ # self.assertFalse(opts & SSL.OP_NO_SSLv3)
+ # self.assertFalse(opts & SSL.OP_NO_COMPRESSION)


Same here (keep changes to the bare minimum and refactor in another PR)

giampaolo · 2017-07-29T17:55:37Z

pyftpdlib/test/test_functional_ssl.py

+
+ def setUp(self):
+ self.server = FTPSServer(use_client_cert=True)
+ self.server.start()


Please do self.client = None and if self.client is not None: ... in the tearDown method.

zuohaocheng mentioned this pull request Jul 13, 2017

Add support for TLS/SSL mutual authentication #396

Closed

giampaolo requested changes Jul 14, 2017

View reviewed changes

bplost and others added 3 commits July 17, 2017 11:11

Use ssl.context for ftp client in ssl tests

c224708

Make handlers.get_ssl_context non-static to support multi-threaded en…

3ab8723

…vironment

zuohaocheng force-pushed the master branch 4 times, most recently from 5737721 to 6b4aae7 Compare July 17, 2017 08:20

zuohaocheng added 2 commits July 17, 2017 16:20

Bring back SSL option validation on startup by introducing handlers.v…

6b4aae7

…alidate_ssl_options

Change back to ftplib.FTP_TLS:certfile as :context is not supported o…

5c2e1f4

…n Travis

giampaolo mentioned this pull request Jul 17, 2017

Client certificate authentication #336

Closed

giampaolo requested changes Jul 22, 2017

View reviewed changes

Updates per code review

930a60a

zuohaocheng force-pushed the master branch from dd3dc46 to 930a60a Compare July 27, 2017 02:48

Remove check for SSL.OP_NO_SSLv2, as it is removed in OpenSSL

992a9b7

giampaolo reviewed Jul 29, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for TLS/SSL mutual authentication #428

Add support for TLS/SSL mutual authentication #428

zuohaocheng commented Jul 13, 2017

giampaolo left a comment

giampaolo Jul 14, 2017

zuohaocheng Jul 17, 2017

giampaolo Jul 14, 2017

giampaolo Jul 14, 2017

zuohaocheng Jul 17, 2017

giampaolo Jul 14, 2017

zuohaocheng Jul 17, 2017

giampaolo Jul 14, 2017

giampaolo Jul 14, 2017

zuohaocheng Jul 17, 2017 •

edited

zuohaocheng commented Jul 17, 2017

zuohaocheng commented Jul 21, 2017

giampaolo Jul 22, 2017

zuohaocheng Jul 25, 2017 •

edited

giampaolo Jul 22, 2017

giampaolo Jul 22, 2017

zuohaocheng Jul 25, 2017

giampaolo Jul 25, 2017

giampaolo Jul 22, 2017

zuohaocheng Jul 25, 2017

zuohaocheng commented Jul 25, 2017

zuohaocheng commented Jul 27, 2017 •

edited

giampaolo Jul 29, 2017

giampaolo Jul 29, 2017

giampaolo Jul 29, 2017

giampaolo Jul 29, 2017


		The path of the certificate to check the client certificate against. (default ``None``).

Add support for TLS/SSL mutual authentication #428

Are you sure you want to change the base?

Add support for TLS/SSL mutual authentication #428

Conversation

zuohaocheng commented Jul 13, 2017

giampaolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuohaocheng Jul 17, 2017 • edited

Choose a reason for hiding this comment

zuohaocheng commented Jul 17, 2017

zuohaocheng commented Jul 21, 2017

Choose a reason for hiding this comment

zuohaocheng Jul 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuohaocheng commented Jul 25, 2017

zuohaocheng commented Jul 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuohaocheng Jul 17, 2017 •

edited

zuohaocheng Jul 25, 2017 •

edited

zuohaocheng commented Jul 27, 2017 •

edited