Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for TLS/SSL mutual authentication #428

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions CREDITS
Original file line number Diff line number Diff line change
Expand Up @@ -178,3 +178,12 @@ N: Dmitry Panov
C: UK
E: [email protected]
D: issue 262

N: Zuo Haocheng
C: CN
E: [email protected]
D: issue 336

N: Brandon Plost
E: [email protected]
D: issue 336
4 changes: 4 additions & 0 deletions HISTORY.rst
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@ Bug tracker at https://github.com/giampaolo/pyftpdlib/issues
Version: 1.5.3 - XXXX-XX-XX
===========================

**Enhancements**

- #336: Support client certificate authentication.

**Bug fixes**

- #414: Respond successfully to STOR only after closing file handle.
Expand Down
8 changes: 8 additions & 0 deletions docs/api.rst
Original file line number Diff line number Diff line change
Expand Up @@ -556,7 +556,15 @@ Extended handlers
When True requires SSL/TLS to be established on the data channel. This
means the user will have to issue PROT before PASV or PORT (default
``False``).

.. data:: client_certfile

The path to a file which contains a certificate to be used to identify
the client. If specified, only clients with a valid certificate are able
to connect to the server (default ``None``).

.. versionadded:: 1.5.3
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Extended authorizers
--------------------

Expand Down
62 changes: 45 additions & 17 deletions pyftpdlib/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -876,7 +876,7 @@ def close(self):
# Close file object before responding successfully to client
if self.file_obj is not None and not self.file_obj.closed:
self.file_obj.close()

if self._resp:
self.cmd_channel.respond(self._resp[0], logfun=self._resp[1])

Expand Down Expand Up @@ -3419,6 +3419,7 @@ class TLS_FTPHandler(SSLConnection, FTPHandler):
certfile = None
keyfile = None
ssl_protocol = SSL.SSLv23_METHOD
client_certfile = None
# - SSLv2 is easily broken and is considered harmful and dangerous
# - SSLv3 has several problems and is now dangerous
# - Disable compression to prevent CRIME attacks for OpenSSL 1.0+
Expand Down Expand Up @@ -3452,28 +3453,55 @@ def __init__(self, conn, server, ioloop=None):
self._extra_feats = ['AUTH TLS', 'AUTH SSL', 'PBSZ', 'PROT']
self._pbsz = False
self._prot = False
self.ssl_context = self.get_ssl_context()
self.init_ssl_context()

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

def verify_certs_callback(self, connection, x509,
errnum, errdepth, ok):
if not ok:
self.log("Bad client certificate detected.")
else:
self.log("Client certificate is valid.")
return ok
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@zuohaocheng zuohaocheng Jul 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


def init_ssl_context(self):
if self.ssl_context is None:
self.ssl_context = self.get_ssl_context()
if self.client_certfile is not None:
from OpenSSL.SSL import VERIFY_CLIENT_ONCE
from OpenSSL.SSL import VERIFY_FAIL_IF_NO_PEER_CERT
from OpenSSL.SSL import VERIFY_PEER
self.ssl_context.set_verify(VERIFY_PEER |
VERIFY_FAIL_IF_NO_PEER_CERT |
VERIFY_CLIENT_ONCE,
self.verify_certs_callback)
return self.ssl_context

@classmethod
def get_ssl_context(cls):
if cls.ssl_context is None:
if cls.certfile is None:
raise ValueError("at least certfile must be specified")
cls.ssl_context = SSL.Context(cls.ssl_protocol)
if cls.ssl_protocol != SSL.SSLv2_METHOD:
cls.ssl_context.set_options(SSL.OP_NO_SSLv2)
else:
warnings.warn("SSLv2 protocol is insecure", RuntimeWarning)
cls.ssl_context.use_certificate_chain_file(cls.certfile)
if not cls.keyfile:
cls.keyfile = cls.certfile
cls.ssl_context.use_privatekey_file(cls.keyfile)
if cls.ssl_options:
cls.ssl_context.set_options(cls.ssl_options)
return cls.ssl_context
if cls.certfile is None:
raise ValueError("at least certfile must be specified")
ssl_context = SSL.Context(cls.ssl_protocol)
if cls.ssl_protocol != SSL.SSLv2_METHOD:
ssl_context.set_options(SSL.OP_NO_SSLv2)
else:
warnings.warn("SSLv2 protocol is insecure", RuntimeWarning)
ssl_context.use_certificate_chain_file(cls.certfile)
if not cls.keyfile:
cls.keyfile = cls.certfile
ssl_context.use_privatekey_file(cls.keyfile)
if cls.client_certfile is not None:
from OpenSSL.SSL import OP_NO_TICKET
from OpenSSL.SSL import SESS_CACHE_OFF
ssl_context.load_verify_locations(
cls.client_certfile)
ssl_context.set_session_cache_mode(SESS_CACHE_OFF)
cls.ssl_options = cls.ssl_options | OP_NO_TICKET
if cls.ssl_options:
ssl_context.set_options(cls.ssl_options)
return ssl_context
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# --- overridden methods

Expand Down
50 changes: 50 additions & 0 deletions pyftpdlib/test/clientcert.pem
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
-----BEGIN CERTIFICATE-----
MIIDqzCCApOgAwIBAgIBADANBgkqhkiG9w0BAQsFADBwMQswCQYDVQQGEwJVUzEO
MAwGA1UECAwFVGV4YXMxEDAOBgNVBAcMB0hvdXN0b24xDDAKBgNVBAoMA1NMQjEM
MAoGA1UECwwDU1dUMQwwCgYDVQQDDANzd3QxFTATBgkqhkiG9w0BCQEWBmJwbG9z
dDAeFw0xNjA4MjMxMzQyMDZaFw0xNzA4MjMxMzQyMDZaMHAxCzAJBgNVBAYTAlVT
MQ4wDAYDVQQIDAVUZXhhczEQMA4GA1UEBwwHSG91c3RvbjEMMAoGA1UECgwDU0xC
MQwwCgYDVQQLDANTV1QxDDAKBgNVBAMMA3N3dDEVMBMGCSqGSIb3DQEJARYGYnBs
b3N0MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEArNnEnnUGpXLqnTTx
3td4OWQoKFBppifL6+r5y839CWT6GR3Xj5OlcJPXMKBO40H0StHOctshPL0/ZZeC
sNHT6O6c47nBc/pw3YX4GJjLJno0k1+xTSvmk+yhTF/i1ThDIq2YrlGWXHyo/jOe
gJc0T3L2u1Ivx+iOEAIP9uqBpAi3rhfZKBvVdDXb5J0TqouXt4jx5l8Fq577D9y4
W61nmj7FlquxClhGIgsNbMtlBIMkALNLq3kY+TqYatjRy6aS6mb55TfObjP+IOgz
8Jln937hb89eJopirwKMzD1EnJgBamMPNJjIhDQlHklMwkgynIKnSJghbjKBusaE
2xZEPQIDAQABo1AwTjAdBgNVHQ4EFgQU2hMnpneH1sZ79iWoBLue1+7f4NwwHwYD
VR0jBBgwFoAU2hMnpneH1sZ79iWoBLue1+7f4NwwDAYDVR0TBAUwAwEB/zANBgkq
hkiG9w0BAQsFAAOCAQEAdmPairr/lt63J3dfTSFNJTytvV1WW7Ak8NwH1hdheaYy
Tx9ffjRIZv9WyHEWb/1YCkKo08LqgnVh07HfW0JY1hqD0SwoHtPexTIgBnOsKvCr
q1gQjFuDg2wVV7cecYPQFYv9jweIe62OCapKl8PjmXii+qnxY/Qbbyx9bGYbR1k4
KJm073WwiqXXCS1JgOj9WH3I1Qa2Ptb6RO+Woy7ItA5ftQSp4EMTwhygOK0j0w9V
11MAPUtZ8/rTiD17HHVzKfbNmx4E6dtBV9E/gn464lrdNhxEaeN2wW42FU+CzeVa
/aYQbQTSXpI5tX7QTAcQrMAqp75EBMdYj5+9bRXIvw==
-----END CERTIFICATE-----
-----BEGIN PRIVATE KEY-----
MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQCs2cSedQalcuqd
NPHe13g5ZCgoUGmmJ8vr6vnLzf0JZPoZHdePk6Vwk9cwoE7jQfRK0c5y2yE8vT9l
l4Kw0dPo7pzjucFz+nDdhfgYmMsmejSTX7FNK+aT7KFMX+LVOEMirZiuUZZcfKj+
M56AlzRPcva7Ui/H6I4QAg/26oGkCLeuF9koG9V0NdvknROqi5e3iPHmXwWrnvsP
3LhbrWeaPsWWq7EKWEYiCw1sy2UEgyQAs0ureRj5Ophq2NHLppLqZvnlN85uM/4g
6DPwmWf3fuFvz14mimKvAozMPUScmAFqYw80mMiENCUeSUzCSDKcgqdImCFuMoG6
xoTbFkQ9AgMBAAECggEBAKRQ3G36N9g+VzQdKbU6xipgwSAZ2WU/vcZG+TI6Xsp4
eJw51zrBE+viTxYFvxihETezHXvoPj98dHECSBYJUlbDxtdhNbsoH/UmrwPK9Ixe
be6PcIA5NJf4whlVqdAiDQhBWLyWCMdhJlGJBqudkffZBR5r8corlCk5nK2Qnq8s
ncaaO1A7RLFXXCiwWdAi7bJEHZB2XFQYMctfJ+/m02s0IhIRmusH4XovdTucfykI
FstXtd5+AOfqsOkTn4kGOmG4ObKOXOp1XAjTTIV3xxU0CzYxE1CPhlimkAVLuRPs
bInojdtm80ZQY68xCOvUsq+WMIB+dKkeJKeTI6e/b+0CgYEA3nDQFto7pqtVZLUu
YVTlR6vxrXEYl8FIes8WlICBWXPnviFoiSLTB5SPF1Yvv/LKFkYQ96i2ep8FvZEy
Rbzt1xOOF9GWtO4lc6cBIzh31POJgY2B2PAo7qmhpyhutCaaotf5ukpx35eGlj5v
RT0wDN7Xt6S4arqgbngUdtHLhqsCgYEAxu2rv/GVU/DL3VuBHIky/OKFeL94EnIA
UUm5SRvd0SeHgcjd+EHG/u1nH1XC+Dwiw75Dsv/nqhekFqaQczYZlUrKa+mkdofV
i8DbXHies6qTwvYUIorhaiAoY+iRwJ/6d20oep+3fDSPPcVj7PakZpCK0sAesWX7
09muN0vLALcCgYAKr0iPkHQFEX3MlJdhvX417yBwwFn6ECK3I3NmNrX/4f1juJ8Y
1z9jwdMNv+oTQkpKv5rZCpWZVkIkVPEhQG38Qsg0hLDEiBvsbj0zv+ahqAEW5AE0
tnSA4k0Nhneq15/d6pnoROMrZk/kr6MQpFvGgn3CKHtjRQunwsTY4ELyeQKBgQCN
zlNGqvJmOhs5msc5Dly4hMncv7DahUXQrJtWkHTZajJgxE3ncQxoIdgHMF2iE0w8
+V7NNTtxtxSTyPzkBEbMc9pEfvNsQ3xo+XvmOV34ebqHml/UF+iEfJQOVHXCOMiV
Zc0bTMvB0L3jrNiEzXV4X8V2Ytn+X9LavCxC4ta9lQKBgHTQNDz+qdiLoeX68HKp
b5jd1H7nozWdcbcAO5tKNbSZvnY05QCjC+WkuIeUkKc2zcctIy306/iFRApguElm
z8sm8NnJIkJeRx2XmEE5Lcn64se3ml7qVlcFaYrVW8hDrrvlUAwzu+ZoPVD3DSiQ
EOnPOa2mEwR9YPyPaKdq+fkt
-----END PRIVATE KEY-----
2 changes: 1 addition & 1 deletion pyftpdlib/test/test_functional.py
Original file line number Diff line number Diff line change
Expand Up @@ -2132,7 +2132,7 @@ class _TestNetworkProtocols(object):
HOST = HOST

def setUp(self):
self.server = self.server_class((self.HOST, 0))
self.server = self.server_class(addr=(self.HOST, 0))
self.server.start()
self.client = self.client_class(timeout=TIMEOUT)
self.client.connect(self.server.host, self.server.port)
Expand Down
124 changes: 101 additions & 23 deletions pyftpdlib/test/test_functional_ssl.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import socket
import sys
import ssl
from ssl import SSLError

import OpenSSL # requires "pip install pyopenssl"

Expand Down Expand Up @@ -49,6 +50,8 @@

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'))

del OpenSSL

Expand Down Expand Up @@ -78,6 +81,13 @@ class FTPSServer(ThreadedTestFTPd):
handler = TLS_FTPHandler
handler.certfile = CERTFILE

def __init__(self, use_client_cert=False, *args, **kwargs):
if use_client_cert:
self.handler.client_certfile = CLIENT_CERTFILE
else:
self.handler.client_certfile = None
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

super(FTPSServer, self).__init__(*args, **kwargs)

class TLSTestMixin:
server_class = FTPSServer
client_class = FTPSClient
Expand Down Expand Up @@ -366,31 +376,27 @@ def test_ssl_options(self):
from OpenSSL import SSL
from OpenSSL._util import lib
from pyftpdlib.handlers import TLS_FTPHandler
try:
TLS_FTPHandler.ssl_context = None
ctx = TLS_FTPHandler.get_ssl_context()
# Verify default opts.
with contextlib.closing(socket.socket()) as s:
s = SSL.Connection(ctx, s)
opts = lib.SSL_CTX_get_options(ctx._context)
ctx = TLS_FTPHandler.get_ssl_context()
# Verify default opts.
with contextlib.closing(socket.socket()) as s:
s = SSL.Connection(ctx, s)
opts = lib.SSL_CTX_get_options(ctx._context)
if SSL.OP_NO_SSLv2 != 0:
self.assertTrue(opts & SSL.OP_NO_SSLv2)
self.assertTrue(opts & SSL.OP_NO_SSLv3)
self.assertTrue(opts & SSL.OP_NO_COMPRESSION)
TLS_FTPHandler.ssl_context = None # reset
# Make sure that if ssl_options is None no options are set
# (except OP_NO_SSLv2 whch is enabled by default unless
# ssl_proto is set to SSL.SSLv23_METHOD).
TLS_FTPHandler.ssl_context = None
TLS_FTPHandler.ssl_options = None
ctx = TLS_FTPHandler.get_ssl_context()
with contextlib.closing(socket.socket()) as s:
s = SSL.Connection(ctx, s)
opts = lib.SSL_CTX_get_options(ctx._context)
self.assertTrue(opts & SSL.OP_NO_SSLv3)
self.assertTrue(opts & SSL.OP_NO_COMPRESSION)
# Make sure that if ssl_options is None no options are set
# (except OP_NO_SSLv2 whch is enabled by default unless
# ssl_proto is set to SSL.SSLv23_METHOD).
TLS_FTPHandler.ssl_options = None
ctx = TLS_FTPHandler.get_ssl_context()
with contextlib.closing(socket.socket()) as s:
s = SSL.Connection(ctx, s)
opts = lib.SSL_CTX_get_options(ctx._context)
if SSL.OP_NO_SSLv2 != 0:
self.assertTrue(opts & SSL.OP_NO_SSLv2)
# self.assertFalse(opts & SSL.OP_NO_SSLv3)
self.assertFalse(opts & SSL.OP_NO_COMPRESSION)
finally:
TLS_FTPHandler.ssl_context = None
# self.assertFalse(opts & SSL.OP_NO_SSLv3)
# self.assertFalse(opts & SSL.OP_NO_COMPRESSION)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if hasattr(ssl, "PROTOCOL_SSLv2"):
def test_sslv2(self):
Expand All @@ -408,6 +414,78 @@ def test_sslv2(self):
self.client.ssl_version = ssl.PROTOCOL_SSLv2


@unittest.skipUnless(FTPS_SUPPORT, FTPS_UNSUPPORT_REASON)
class TestClientFTPS(unittest.TestCase):
"""Specific tests for TLS_FTPHandler class."""

def setUp(self):
self.server = FTPSServer(use_client_cert=True)
self.server.start()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


def tearDown(self):
self.client.ssl_version = ssl.PROTOCOL_SSLv23
with self.server.lock:
self.server.handler.ssl_version = ssl.PROTOCOL_SSLv23
self.server.handler.tls_control_required = False
self.server.handler.tls_data_required = False
self.server.handler.client_certfile = None
self.client.close()
self.server.stop()

def assertRaisesWithMsg(self, excClass, msg, callableObj, *args, **kwargs):
try:
callableObj(*args, **kwargs)
except excClass as err:
if str(err) == msg:
return
raise self.failureException("%s != %s" % (str(err), msg))
else:
if hasattr(excClass, '__name__'):
excName = excClass.__name__
else:
excName = str(excClass)
raise self.failureException("%s not raised" % excName)

def test_auth_client_cert(self):
self.client = ftplib.FTP_TLS(timeout=TIMEOUT,
certfile=CLIENT_CERTFILE)
self.client.connect(self.server.host, self.server.port)
# secured
try:
self.client.login()
except Exception:
self.fail("login with certificate should work")

def test_auth_client_nocert(self):
self.client = ftplib.FTP_TLS(timeout=TIMEOUT)
self.client.connect(self.server.host, self.server.port)
try:
self.client.login()
except SSLError as e:
# client should not be able to log in
if "SSLV3_ALERT_HANDSHAKE_FAILURE" in e.reason:
pass
else:
self.fail("Incorrect SSL error with" +
" missing client certificate")
else:
self.fail("Client able to log in with no certificate")

def test_auth_client_badcert(self):
self.client = ftplib.FTP_TLS(timeout=TIMEOUT, certfile=CERTFILE)
self.client.connect(self.server.host, self.server.port)
try:
self.client.login()
except Exception as e:
# client should not be able to log in
if "TLSV1_ALERT_UNKNOWN_CA" in e.reason:
pass
else:
self.fail("Incorrect SSL error with bad client certificate")
else:
self.fail("Client able to log in with bad certificate")


configure_logging()
remove_test_files()

Expand Down
1 change: 1 addition & 0 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ def main():
"pyftpdlib.test": [
"README",
'keycert.pem',
'clientcert.pem',
],
},
keywords=['ftp', 'ftps', 'server', 'ftpd', 'daemon', 'python', 'ssl',
Expand Down