From 0c24d32bf4477a2afef4f96b32dfae55a6046255 Mon Sep 17 00:00:00 2001 From: Christophe Haen Date: Tue, 5 May 2020 15:16:41 +0200 Subject: [PATCH 1/5] M2SSLTransport: catch exceptions --- .../private/Transports/M2SSLTransport.py | 32 ++++++++++++------- 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/Core/DISET/private/Transports/M2SSLTransport.py b/Core/DISET/private/Transports/M2SSLTransport.py index 7dd5acccd90..69d47997e63 100755 --- a/Core/DISET/private/Transports/M2SSLTransport.py +++ b/Core/DISET/private/Transports/M2SSLTransport.py @@ -8,6 +8,7 @@ import os import socket from M2Crypto import SSL, threading as M2Threading +from M2Crypto.SSL.Checker import SSLVerificationError from DIRAC.Core.Utilities.ReturnValues import S_OK, S_ERROR from DIRAC.Core.DISET.private.Transports.BaseTransport import BaseTransport @@ -93,10 +94,8 @@ def initAsClient(self): self.remoteAddress = self.oSocket.getpeername() return S_OK() - except socket.error as e: - # Other exception are probably SSL-related, in that case we - # abort and the exception is forwarded to the caller. - error = e + except (socket.error, SSLVerificationError, SSL.SSLError) as e: + error = "%s:%s" % (e, repr(e)) if self.oSocket is not None: self.oSocket.close() @@ -174,10 +173,13 @@ def acceptConnection(self): :returns: S_OK(SSLTransport object) """ - oClient, _ = self.oSocket.accept() - oClientTrans = SSLTransport(self.stServerAddress, ctx=self.__ctx) - oClientTrans.setClientSocket(oClient) - return S_OK(oClientTrans) + try: + oClient, _ = self.oSocket.accept() + oClientTrans = SSLTransport(self.stServerAddress, ctx=self.__ctx) + oClientTrans.setClientSocket(oClient) + return S_OK(oClientTrans) + except (SSL.SSLError, SSLVerificationError) as e: + return S_ERROR("Error in acceptConnection: %s %s" % (e, repr(e))) def _read(self, bufSize=4096, skipReadyCheck=False): """ Read bufSize bytes from the buffer. @@ -188,8 +190,11 @@ def _read(self, bufSize=4096, skipReadyCheck=False): :returns: S_OK(number of byte read) """ - read = self.oSocket.read(bufSize) - return S_OK(read) + try: + read = self.oSocket.read(bufSize) + return S_OK(read) + except (socket.error, SSL.SSLError, SSLVerificationError) as e: + return S_ERROR("Error in _read: %s %s" % (e, repr(e))) def isLocked(self): """ Returns if this instance is locked. @@ -207,5 +212,8 @@ def _write(self, buf): :returns: S_OK(number of bytes written) """ - wrote = self.oSocket.write(buf) - return S_OK(wrote) + try: + wrote = self.oSocket.write(buf) + return S_OK(wrote) + except (socket.error, SSL.SSLError, SSLVerificationError) as e: + return S_ERROR("Error in _write: %s %s" % (e, repr(e))) From 852d25aa7922485275444d675d7010bd280a1473 Mon Sep 17 00:00:00 2001 From: Christophe Haen Date: Thu, 7 May 2020 18:41:49 +0200 Subject: [PATCH 2/5] M2Crypto X509Chain: add a flag not to query the Registry when fetching the credentials --- Core/Security/m2crypto/X509Chain.py | 33 +++++++++++++++++------------ 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/Core/Security/m2crypto/X509Chain.py b/Core/Security/m2crypto/X509Chain.py index 8efc54a2019..4932e9df914 100644 --- a/Core/Security/m2crypto/X509Chain.py +++ b/Core/Security/m2crypto/X509Chain.py @@ -941,10 +941,13 @@ def isPUSP(self): return S_OK(False) @needCertList - def getCredentials(self, ignoreDefault=False): + def getCredentials(self, ignoreDefault=False, withRegistryInfo=True): """ Returns a summary of the credentials contained in the current chain :params ignoreDefault: (default False) If True and if no DIRAC group is found in the proxy, lookup the CS + :params withRegistryInfo: (default True) if set to True, will enhance the returned dict with info + from the registry + :returns: S_OK with the credential dict. Some parameters of the dict are always there, other depends on the nature of the Chain @@ -962,13 +965,13 @@ def getCredentials(self, ignoreDefault=False): Only for proxy: * identity: If it is a normal proxy, it is the DN of the certificate. If it is a PUSP, it contains the identity as in :py:meth:`.isPUSP` - * username: DIRAC username associated to the DN + * username: DIRAC username associated to the DN (needs withRegistryInfo) (see :py:func:`DIRAC.ConfigurationSystem.Client.Helpers.Registry.getUsernameForDN`) * group: DIRAC group, depending on ignoreDefault param(see :py:meth:`.getDIRACGroup`) * validGroup: True if the group found is in the list of groups the user belongs to * groupProperty: (only if validGroup) get the properties of the group - For Host certificate: + For Host certificate (needs withRegistryInfo): * group: always `hosts` * hostname: name of the host as registered in the CS (see :py:func:`DIRAC.ConfigurationSystem.Client.Helpers.Registry.getHostnameForDN`) @@ -976,7 +979,7 @@ def getCredentials(self, ignoreDefault=False): * groupProperties: host options (see :py:func:`DIRAC.ConfigurationSystem.Client.Helpers.Registry.getHostOption`) - If it is a user certificate: + If it is a user certificate (needs withRegistryInfo): * username: like for proxy * validDN: like proxy """ @@ -996,20 +999,22 @@ def getCredentials(self, ignoreDefault=False): credDict['identity'] = result['Identity'] credDict['subproxyUser'] = result['SubproxyUser'] - retVal = Registry.getUsernameForDN(credDict['identity']) - if not retVal['OK']: - return S_OK(credDict) - credDict['username'] = retVal['Value'] - credDict['validDN'] = True + if withRegistryInfo: + retVal = Registry.getUsernameForDN(credDict['identity']) + if not retVal['OK']: + return S_OK(credDict) + credDict['username'] = retVal['Value'] + credDict['validDN'] = True retVal = self.getDIRACGroup(ignoreDefault=ignoreDefault) if retVal['OK']: diracGroup = retVal['Value'] credDict['group'] = diracGroup - retVal = Registry.getGroupsForUser(credDict['username']) - if retVal['OK'] and diracGroup in retVal['Value']: - credDict['validGroup'] = True - credDict['groupProperties'] = Registry.getPropertiesForGroup(diracGroup) - else: + if withRegistryInfo: + retVal = Registry.getGroupsForUser(credDict['username']) + if retVal['OK'] and diracGroup in retVal['Value']: + credDict['validGroup'] = True + credDict['groupProperties'] = Registry.getPropertiesForGroup(diracGroup) + elif withRegistryInfo: retVal = Registry.getHostnameForDN(credDict['subject']) if retVal['OK']: credDict['group'] = 'hosts' From 810971ee6945ef77702ef71597eb8f387c48bd42 Mon Sep 17 00:00:00 2001 From: Christophe Haen Date: Thu, 7 May 2020 18:42:53 +0200 Subject: [PATCH 3/5] M2Utils: do not query Registry when retrieving remote certificate's info --- Core/DISET/private/Transports/SSL/M2Utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Core/DISET/private/Transports/SSL/M2Utils.py b/Core/DISET/private/Transports/SSL/M2Utils.py index 66edf2b8178..a45c8d15c44 100644 --- a/Core/DISET/private/Transports/SSL/M2Utils.py +++ b/Core/DISET/private/Transports/SSL/M2Utils.py @@ -136,7 +136,7 @@ def getM2SSLContext(ctx=None, **kwargs): def getM2PeerInfo(conn): """ Gets the details of the current peer as a standard dict. The peer details are obtained from the supplied M2 SSL Connection obj "conn". - The details returned are those from ~X509Chain.getCredentials: + The details returned are those from ~X509Chain.getCredentials, without Registry info: DN - Full peer DN as string x509Chain - Full chain of peer @@ -147,7 +147,7 @@ def getM2PeerInfo(conn): Returns a dict of details. """ chain = X509Chain.generateX509ChainFromSSLConnection(conn) - creds = chain.getCredentials() + creds = chain.getCredentials(withRegistryInfo=False) if not creds['OK']: raise RuntimeError("Failed to get SSL peer info (%s)." % creds['Message']) peer = creds['Value'] From cc31320885b84b0881cd2cb394304d658e199f94 Mon Sep 17 00:00:00 2001 From: Christophe Haen Date: Fri, 8 May 2020 15:42:17 +0200 Subject: [PATCH 4/5] M2Crypto: delegate the SSL handshake to task thread if DIRAC_M2CRYPTO_SPLIT_HANDSHAKE is True --- .github/workflows/basic.yml | 4 +- .../private/Transports/M2SSLTransport.py | 96 +++++++++++++++++-- .../environment_variable_configuration.rst | 3 + .../AdministratorGuide/technologyPreviews.rst | 7 +- tests/CI/run_docker_setup.sh | 2 + 5 files changed, 101 insertions(+), 11 deletions(-) diff --git a/.github/workflows/basic.yml b/.github/workflows/basic.yml index f87c3e6ad53..8c177a1e25a 100644 --- a/.github/workflows/basic.yml +++ b/.github/workflows/basic.yml @@ -13,9 +13,9 @@ jobs: matrix: command: - pytest - - DIRAC_USE_M2CRYPTO=Yes pytest + - DIRAC_USE_M2CRYPTO=Yes DIRAC_M2CRYPTO_SPLIT_HANDSHAKE=Yes pytest - pytest Core/Security/test - - DIRAC_USE_M2CRYPTO=Yes pytest Core/Security/test + - DIRAC_USE_M2CRYPTO=Yes DIRAC_M2CRYPTO_SPLIT_HANDSHAKE=Yes pytest Core/Security/test - tests/checkDocs.sh # TODO This should cover more than just tests/CI # Excluded codes related to sourcing files diff --git a/Core/DISET/private/Transports/M2SSLTransport.py b/Core/DISET/private/Transports/M2SSLTransport.py index 69d47997e63..acc66ce4803 100755 --- a/Core/DISET/private/Transports/M2SSLTransport.py +++ b/Core/DISET/private/Transports/M2SSLTransport.py @@ -23,9 +23,6 @@ # to M2Crypto: Quite a few functions will need mapping through from OpenSSL to # allow the CRL stack to be set on the X509 CTX used for verification. -# TODO: Catch exceptions (from M2 itself and my M2Utils module) and convert them -# into proper DIRAC style errors. - # TODO: Log useful messages to the logger @@ -147,30 +144,104 @@ def renewServerContext(self): self.__ctx = getM2SSLContext(self.__ctx, **self.__kwargs) return S_OK() - def handshake(self): + def handshake_singleStep(self): """ Used to perform SSL handshakes. These are now done automatically. """ # This isn't used any more, the handshake is done inside the M2Crypto library return S_OK() - def setClientSocket(self, oSocket): + def handshake_multipleSteps(self): + """ Perform SSL handshakes. + This has to be called after the connection was accepted (acceptConnection_multipleSteps) + + The remote credentials are gathered here + """ + try: + # M2Crypto does not provide public method to + # accept and handshake in two steps. + # So we have to do it manually + # The following lines are basically a copy/paste + # of the end of SSL.Connection.accept method + self.oSocket.setup_ssl() + self.oSocket.set_accept_state() + self.oSocket.accept_ssl() + check = getattr(self.oSocket, 'postConnectionCheck', + self.oSocket.serverPostConnectionCheck) + if check is not None: + if not check(self.oSocket.get_peer_cert(), self.oSocket.addr[0]): + raise SSL.Checker.SSLVerificationError( + 'post connection check failed') + + self.peerCredentials = getM2PeerInfo(self.oSocket) + + return S_OK() + except (socket.error, SSL.SSLError, SSLVerificationError) as e: + return S_ERROR("Error in handhsake: %s %s" % (e, repr(e))) + + def setClientSocket_singleStep(self, oSocket): """ Set the inner socket (i.e. SSL.Connection object) of this instance to the value of oSocket. + We also gather the remote peer credentials This method is intended to be used to create client connection objects from a server and should be considered to be an internal function. :param oSocket: client socket SSL.Connection object """ + + # TODO: The calling method (ServiceReactor.__acceptIncomingConnection) expects + # socket.error to be thrown in case of issue. Maybe we should catch the M2Crypto + # errors here and raise socket.error instead + self.oSocket = oSocket self.remoteAddress = self.oSocket.getpeername() self.peerCredentials = getM2PeerInfo(self.oSocket) - def acceptConnection(self): + def setClientSocket_multipleSteps(self, oSocket): + """ Set the inner socket (i.e. SSL.Connection object) of this instance + to the value of oSocket. + This method is intended to be used to create client connection objects + from a server and should be considered to be an internal function. + + :param oSocket: client socket SSL.Connection object + + """ + # warning: do NOT catch socket.error here, because for who knows what reason + # exceptions are actually properly used for once, and the calling method + # relies on it (ServiceReactor.__acceptIncomingConnection) + self.oSocket = oSocket + self.remoteAddress = self.oSocket.getpeername() + + def acceptConnection_multipleSteps(self): """ Accept a new client, returns a new SSLTransport object representing the client connection. + The connection is accepted, but no SSL handshake is performed + + :returns: S_OK(SSLTransport object) + """ + # M2Crypto does not provide public method to + # accept and handshake in two steps. + # So we have to do it manually + # The following lines are basically a copy/paste + # of the begining of SSL.Connection.accept method + try: + sock, addr = self.oSocket.socket.accept() + oClient = SSL.Connection(self.oSocket.ctx, sock) + oClient.addr = addr + oClientTrans = SSLTransport(self.stServerAddress, ctx=self.__ctx) + oClientTrans.setClientSocket(oClient) + return S_OK(oClientTrans) + except (socket.error, SSL.SSLError, SSLVerificationError) as e: + return S_ERROR("Error in acceptConnection: %s %s" % (e, repr(e))) + + def acceptConnection_singleStep(self): + """ Accept a new client, returns a new SSLTransport object representing + the client connection. + + The SSL handshake is performed here. + :returns: S_OK(SSLTransport object) """ try: @@ -178,9 +249,20 @@ def acceptConnection(self): oClientTrans = SSLTransport(self.stServerAddress, ctx=self.__ctx) oClientTrans.setClientSocket(oClient) return S_OK(oClientTrans) - except (SSL.SSLError, SSLVerificationError) as e: + except (socket.error, SSL.SSLError, SSLVerificationError) as e: return S_ERROR("Error in acceptConnection: %s %s" % (e, repr(e))) + # Depending on the DIRAC_M2CRYPTO_SPLIT_HANDSHAKE we either do the + # handshake separately or not + if os.getenv('DIRAC_M2CRYPTO_SPLIT_HANDSHAKE', 'NO').lower() in ('yes', 'true'): + acceptConnection = acceptConnection_multipleSteps + handshake = handshake_multipleSteps + setClientSocket = setClientSocket_multipleSteps + else: + acceptConnection = acceptConnection_singleStep + handshake = handshake_singleStep + setClientSocket = setClientSocket_singleStep + def _read(self, bufSize=4096, skipReadyCheck=False): """ Read bufSize bytes from the buffer. diff --git a/docs/source/AdministratorGuide/ServerInstallations/environment_variable_configuration.rst b/docs/source/AdministratorGuide/ServerInstallations/environment_variable_configuration.rst index 3e0645cbd4e..f279b565b8b 100644 --- a/docs/source/AdministratorGuide/ServerInstallations/environment_variable_configuration.rst +++ b/docs/source/AdministratorGuide/ServerInstallations/environment_variable_configuration.rst @@ -25,5 +25,8 @@ DIRAC_GFAL_GRIDFTP_SESSION_REUSE DIRAC_USE_M2CRYPTO If ``true`` or ``yes`` DIRAC uses m2crypto instead of pyGSI for handling certificates, proxies, etc. +DIRAC_M2CRYPTO_SPLIT_HANDSHAKE + If ``true`` or ``yes`` the SSL handshake is done in a new thread (default No) + DIRAC_VOMSES Can be set to point to a folder containing VOMSES information. See :ref:`multi_vo_dirac` diff --git a/docs/source/AdministratorGuide/technologyPreviews.rst b/docs/source/AdministratorGuide/technologyPreviews.rst index a0e2afdae4f..90cee4700e7 100644 --- a/docs/source/AdministratorGuide/technologyPreviews.rst +++ b/docs/source/AdministratorGuide/technologyPreviews.rst @@ -12,7 +12,8 @@ M2Crypto ======== We aim at replacing the home made wrapper of openssl pyGSI with the standard M2Crypto library. It is by default disabled. -You can enable it by setting the environment variable `DIRAC_USE_M2CRYPTO` to `Yes`. +You can enable it by setting the environment variable ``DIRAC_USE_M2CRYPTO`` to ``Yes``. +There is also a change of behavior in the way sockets connections are handled server side. The current (new) behavior is to perform the SSL handshake in the main thread and only delegate the execution of the method to another thread. The pyGSI behavior was to delegate the SSL handshake to the new thread, which is probably a better thing to do. You can revert back this behavior by setting `DIRAC_M2CRYPTO_SPLIT_HANDSHAKE` to `Yes`, but this has not been heavily tested yet. Possible issues --------------- @@ -20,5 +21,7 @@ Possible issues M2Crypto (or any standard tool that respects TLS..) will be stricter than PyGSI. So you may need to adapt your environment a bit. Here are a few hints: * SAN in your certificates: if you are contacting a machine using its aliases, make sure that all the aliases are in the SubjectAlternativeName (SAN) field of the certificates -* FQDN in the configuration: SAN normally contains only FQDN, so make sure you use the FQDN in the CS as well (e.g. `mymachine.cern.ch` and not `mymachine`) +* FQDN in the configuration: SAN normally contains only FQDN, so make sure you use the FQDN in the CS as well (e.g. ``mymachine.cern.ch`` and not ``mymachine``) * ComponentInstaller screwed: like any change you do on your hosts, the ComponentInstaller will duplicate the entry. So if you change the CS to put FQDN, the machine will appear twice. + +In case your services are not fast enough, and the socket backlog is full (``ss -pnl``), try setting ``DIRAC_M2CRYPTO_SPLIT_HANDSHAKE`` to ``Yes``. diff --git a/tests/CI/run_docker_setup.sh b/tests/CI/run_docker_setup.sh index 61f4875200f..7daafdf3624 100755 --- a/tests/CI/run_docker_setup.sh +++ b/tests/CI/run_docker_setup.sh @@ -156,10 +156,12 @@ function prepareEnvironment() { if [ -n "${SERVER_USE_M2CRYPTO+x}" ]; then echo "export DIRAC_USE_M2CRYPTO=${SERVER_USE_M2CRYPTO}" >> "${SERVERCONFIG}" + echo "export DIRAC_M2CRYPTO_SPLIT_HANDSHAKE=Yes" >> "${SERVERCONFIG}" fi if [ -n "${CLIENT_USE_M2CRYPTO+x}" ]; then echo "export DIRAC_USE_M2CRYPTO=${CLIENT_USE_M2CRYPTO}" >> "${CLIENTCONFIG}" + echo "export DIRAC_M2CRYPTO_SPLIT_HANDSHAKE=Yes" >> "${CLIENTCONFIG}" fi docker-compose -f ./docker-compose.yml up -d From 9ab61bff3c31e9d4ddd67042e6d35d9c24a17619 Mon Sep 17 00:00:00 2001 From: Christophe Haen Date: Fri, 8 May 2020 18:43:09 +0200 Subject: [PATCH 5/5] Docs: update information about the requirement for server certificate --- .../ServerInstallations/InstallingDiracServer.rst | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/docs/source/AdministratorGuide/ServerInstallations/InstallingDiracServer.rst b/docs/source/AdministratorGuide/ServerInstallations/InstallingDiracServer.rst index e08e1f9941c..b5571a95e5b 100644 --- a/docs/source/AdministratorGuide/ServerInstallations/InstallingDiracServer.rst +++ b/docs/source/AdministratorGuide/ServerInstallations/InstallingDiracServer.rst @@ -189,19 +189,8 @@ Reload the configuration and restart:: Server Certificates ------------------- -Server certificates are used for validating the identity of the host a given client is connecting to. By default -grid host certificate include host/ in the CN (common name) field. This is not a problem for DIRAC components -since DISET only keeps the host name after the **/** if present. - -However if the certificate is used for the Web Portal, the client validating the certificate is your browser. All browsers -will rise a security alarm if the host name in the url does not match the CN field in the certificate presented by the server. -In particular this means that *host/*, or other similar parts should nto be present, and that it is preferable to use -DNS aliases and request a certificate under this alias in order to be able to migrate the server to a new host without -having to change your URLs. DIRAC will accept both real host names and any valid aliases without complains. - -Finally, you will have to instruct you users on the procedure to upload the public key of the CA signing the certificate -of the host where the Web Portal is running. This depends from CA to CA, but typically only means clicking on a certain -link on the web portal of the CA. +Server certificates are used for validating the identity of the host a given client is connecting to. We follow the RFC 6125. +Basically, that means that the DNS name used to contact the host must be present in the ``SubjectAlternativeName``. ----------------- Using your own CA