Add support for TLS/SSL mutual authentication #396

bplost · 2016-08-23T15:48:20Z

Add support for client certificat authentication in TLS_FTPHandler

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

lurch · 2016-08-23T15:53:53Z

pyftpdlib/handlers.py

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


Looks like this line is still using tabs, rather than spaces.

lurch · 2016-08-23T15:57:19Z

Just out of curiosity, what happens when the certificate is invalid? Does it just print "Bad client certificate detected" or does the connection get refused? Should some kind of warning or exception be raised, so that it can be detected programmatically?

bplost · 2016-08-23T15:59:35Z

Connection gets rejected. The error sent to the client through pyOpenSSL is a standard ssl error with the correct information in it - the client I'm using (WinSCP) shows that the certificate is invalid (or if no certificate is sent, then it shows that the server is expecting a certificate). I'll add screenshots when i create a new pull request.

lurch · 2016-08-23T16:02:24Z

No need to keep creating new PRs - you can simply update the existing PR with git push -f

bplost · 2016-08-23T16:03:53Z

Thanks @lurch, will do

bplost · 2016-08-23T16:09:24Z

Client error screenshots (WinSCP) when attempting connection with a bad certificate and with no certificate.

bplost · 2016-08-23T18:11:47Z

Anyone have ideas why Python 3.x is getting socket timeout errors in the AppVeyor build/test?

lurch · 2016-08-23T23:55:44Z

pyftpdlib/handlers.py

@@ -3449,6 +3453,15 @@ def __init__(self, conn, server, ioloop=None):

        def __repr__(self):
            return FTPHandler.__repr__(self)
+


flake8 (which is run by the Travis builds) is complaining about this line being blank, but still containing space characters

giampaolo · 2016-08-24T00:27:43Z

Some preliminary questions:

how does this relate to certfile and keyfile class attributes? Is tls_mutual_certfile supposed to be different than certfile or they can be the same?
what happens exactly in case the client does not provide a certificate? I see a callback function is called but what will the server do? Raise an exception? At the very least the callback function should log the event.

Some considerations:

the tls_mutual_authentication bool option looks unnecessary; I guess you can simply ad/define a tls_mutual_certfile option (which I would rename to mutual_certfile)
the PR is lacking tests

giampaolo · 2016-08-24T00:32:19Z

pyftpdlib/handlers.py

+                #print "Bad client certificate detected."
+            #else:
+                #print "Client certificate ok."
+            return ok


If this is called when a client connects, as I suppose, then this should be a classmethod.

bplost · 2016-08-24T12:17:37Z

Answers to your questions @giampaolo

The mutual certfile is what the server checks the client's certificate against, to verify that the client is allowed to connect to that particular server. It should be different than certfile and keyfile
When the client provides no certificate, the OpenSSL library takes care of sending the correct response to the client (see screenshot above). I will add logging here.
Agreed that the bool option is unnecessary and will remove
Will add tests

giampaolo · 2016-08-24T12:21:35Z

When the client provides no certificate, the OpenSSL library takes care of sending the correct
response to the client (see screenshot above). I will add logging here.

But what happens on the server side? The client gets disconnected? When exactly? Is an exception raised? Is the callback function called before or after that happens?
Ideally, the server should gracefully handle that event, meaning catch the exception and log a message. If OpenSSL takes care of sending "something" which tells the client it should provide a certificate, then fine.

bplost · 2016-08-24T12:28:29Z

The client gets disconnected during the SSL handshake. Sample log:

After the handshake fails, the checks you already have in place take care of handling it gracefully and the server just keeps going.
The callback function is not called when no certificate is passed (as the failure happens earlier). The callback function is called when a client certificate is passed (valid or invalid). If the certificate is valid, the connection is allowed. If the certificate is invalid, again the OpenSSL library sends the correct message to the client that the cert is invalid, and I will add the internal logging into the callback method.

Cleaned up the support for client certificate checking Added test file for specific client authentication tests - Good cert (allows connection) - Bad cert (does not allow connection) - No cert (does not allow connection)

bplost · 2016-08-25T18:26:21Z

Cleaned up code and added tests

lurch · 2016-08-25T22:00:22Z

pyftpdlib/test/test_functional_ssl_client_certfile.py

+CERTFILE = os.path.abspath(os.path.join(os.path.dirname(__file__),
+                                        'keycert.pem'))
+CLIENT_CERTFILE = os.path.abspath(os.path.join(os.path.dirname(__file__),
+                                        'clientcert.pem'))


Looks like you forgot to add this clientcert.pem file to the PR?

Fix description in api.rst Add clientcert.pem Rename test file to avoid tests attempting to run two servers on the same port, causing all sorts of SSL mayhem and timeout issues

Fix syntax per Flake8 comments https://travis-ci.org/giampaolo/pyftpdlib/jobs/155320579

remove trailing whitespace

lurch · 2016-08-26T14:08:34Z

@giampaolo Apparently flake8 no longer supports Python 2.6 (https://gitlab.com/pycqa/flake8/issues/187 )
Could the Travis config be modified to not run flake8 on python2.6 ? (which would then stop Travis errors like https://travis-ci.org/giampaolo/pyftpdlib/jobs/155328753 )

bplost · 2016-08-26T14:09:45Z

I think at this point none of the failing tests are due to my changes

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

This PR is refined by #428 and can be closed.

bplost added 2 commits August 23, 2016 10:32

Fix indentation errors in handlers.py

bb52b92

lurch reviewed Aug 23, 2016
View reviewed changes

Update 'print' statements to use logger

933257d

Remove print/log statements

128b61f

lurch reviewed Aug 23, 2016
View reviewed changes

giampaolo reviewed Aug 24, 2016
View reviewed changes

Clean up client-certfile support

c3675b9

Cleaned up the support for client certificate checking Added test file for specific client authentication tests - Good cert (allows connection) - Bad cert (does not allow connection) - No cert (does not allow connection)

lurch reviewed Aug 25, 2016
View reviewed changes

bplost added 3 commits August 26, 2016 07:05

Updates per comments

aa41ce3

Fix description in api.rst Add clientcert.pem Rename test file to avoid tests attempting to run two servers on the same port, causing all sorts of SSL mayhem and timeout issues

Flake8 fixes

0928806

Fix syntax per Flake8 comments https://travis-ci.org/giampaolo/pyftpdlib/jobs/155320579

Small flake8 fix

099630f

remove trailing whitespace

zuohaocheng mentioned this pull request Jul 13, 2017

Add support for TLS/SSL mutual authentication #428

Open

giampaolo closed this Jul 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for TLS/SSL mutual authentication #396

Add support for TLS/SSL mutual authentication #396

bplost commented Aug 23, 2016

lurch Aug 23, 2016

lurch commented Aug 23, 2016

bplost commented Aug 23, 2016 •

edited

Loading

lurch commented Aug 23, 2016

bplost commented Aug 23, 2016

bplost commented Aug 23, 2016

bplost commented Aug 23, 2016

lurch Aug 23, 2016

giampaolo commented Aug 24, 2016

giampaolo Aug 24, 2016 •

edited

Loading

bplost commented Aug 24, 2016

giampaolo commented Aug 24, 2016

bplost commented Aug 24, 2016

bplost commented Aug 25, 2016

lurch Aug 25, 2016

lurch commented Aug 26, 2016

bplost commented Aug 26, 2016 •

edited

Loading

zuohaocheng commented Jul 13, 2017

		@@ -3449,6 +3453,15 @@ def __init__(self, conn, server, ioloop=None):

		def __repr__(self):
		return FTPHandler.__repr__(self)

Add support for TLS/SSL mutual authentication #396

Add support for TLS/SSL mutual authentication #396

Conversation

bplost commented Aug 23, 2016

lurch Aug 23, 2016

Choose a reason for hiding this comment

lurch commented Aug 23, 2016

bplost commented Aug 23, 2016 • edited Loading

lurch commented Aug 23, 2016

bplost commented Aug 23, 2016

bplost commented Aug 23, 2016

bplost commented Aug 23, 2016

lurch Aug 23, 2016

Choose a reason for hiding this comment

giampaolo commented Aug 24, 2016

giampaolo Aug 24, 2016 • edited Loading

Choose a reason for hiding this comment

bplost commented Aug 24, 2016

giampaolo commented Aug 24, 2016

bplost commented Aug 24, 2016

bplost commented Aug 25, 2016

lurch Aug 25, 2016

Choose a reason for hiding this comment

lurch commented Aug 26, 2016

bplost commented Aug 26, 2016 • edited Loading

zuohaocheng commented Jul 13, 2017

bplost commented Aug 23, 2016 •

edited

Loading

giampaolo Aug 24, 2016 •

edited

Loading

bplost commented Aug 26, 2016 •

edited

Loading