-
-
Notifications
You must be signed in to change notification settings - Fork 263
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
base: master
Are you sure you want to change the base?
Changes from 5 commits
de14407
c224708
3ab8723
6b4aae7
5c2e1f4
930a60a
992a9b7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 of the certificate to check the client certificate against. | ||
When provided, only allowing clients with a valid certificate to connect | ||
to the server (default ``None``). | ||
|
||
.. versionadded:: 1.5.3 | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
Extended authorizers | ||
-------------------- | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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]) | ||
|
||
|
@@ -3419,6 +3419,8 @@ class TLS_FTPHandler(SSLConnection, FTPHandler): | |
certfile = None | ||
keyfile = None | ||
ssl_protocol = SSL.SSLv23_METHOD | ||
# client certificate configurable attributes | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this line is probably useless |
||
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+ | ||
|
@@ -3457,23 +3459,50 @@ def __init__(self, conn, server, ioloop=None): | |
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's true that The actual result can be seen from test cases |
||
|
||
def get_ssl_context(self): | ||
if self.ssl_context is None: | ||
self.ssl_context = self.validate_ssl_options() | ||
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 | ||
def validate_ssl_options(cls): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why was this renamed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The static method here does only validation on startup, so I rename it to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not put everything into
...which is a bit weird |
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
# --- overridden methods | ||
|
||
|
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----- |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ | |
import socket | ||
import sys | ||
import ssl | ||
from ssl import SSLError | ||
|
||
import OpenSSL # requires "pip install pyopenssl" | ||
|
||
|
@@ -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 | ||
|
||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm not really in this case. |
||
super(FTPSServer, self).__init__(*args, **kwargs) | ||
|
||
class TLSTestMixin: | ||
server_class = FTPSServer | ||
client_class = FTPSClient | ||
|
@@ -360,15 +370,15 @@ def try_protocol_combo(self, server_protocol, client_protocol): | |
# for proto in protos: | ||
# self.try_protocol_combo(ssl.PROTOCOL_TLSv1, proto) | ||
|
||
# On OSX TLS_FTPHandler.get_ssl_context()._context does not exist. | ||
# On OSX TLS_FTPHandler.validate_ssl_options()._context does not exist. | ||
@unittest.skipIf(OSX, "can't get options on OSX") | ||
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() | ||
ctx = TLS_FTPHandler.validate_ssl_options() | ||
# Verify default opts. | ||
with contextlib.closing(socket.socket()) as s: | ||
s = SSL.Connection(ctx, s) | ||
|
@@ -382,7 +392,7 @@ def test_ssl_options(self): | |
# ssl_proto is set to SSL.SSLv23_METHOD). | ||
TLS_FTPHandler.ssl_context = None | ||
TLS_FTPHandler.ssl_options = None | ||
ctx = TLS_FTPHandler.get_ssl_context() | ||
ctx = TLS_FTPHandler.validate_ssl_options() | ||
with contextlib.closing(socket.socket()) as s: | ||
s = SSL.Connection(ctx, s) | ||
opts = lib.SSL_CTX_get_options(ctx._context) | ||
|
@@ -408,6 +418,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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||
|
||
|
There was a problem hiding this comment.
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"?