Explicitly remove support of SSLv2 #396

stlaz · 2017-01-13T11:37:09Z

It was possible to set tls_version_min/max to 'ssl2', even though
newer versions of NSS will fail to set this as a valid TLS version.
This patch explicitly checks for deprecated TLS versions prior to
creating a TLS connection.

https://fedorahosted.org/freeipa/ticket/6607

tiran · 2017-01-17T14:50:31Z

What is the point of supporting SSL 3.0, TLS 1.0 and TLS 1.1 on the client side these days? How about we remove ancient and potentially dangerous TLS versions completely?
Would it be possible to validate the values during API initialization?

stlaz · 2017-01-19T09:01:58Z

I think we may need to discuss the support on Monday meeting, generally I think SSL 3.0 and TLS 1.0 should not be supported but there might be troubles with connectivity to legacy IPA servers
Yes, although in that case we would have to fail instead of falling back to "reasonable defaults" as Env object attribute values cannot be changed once set

HonzaCholasta · 2017-02-07T12:17:33Z

ipalib/config.py

+        if self.tls_version_min not in TLS_VERSIONS:
+            raise errors.EnvironmentError(
+                "Unknown TLS version '{ver}' set in tls_version_min."
+                .format(self.tls_version_min))


I think you meant tls_version_max.

HonzaCholasta · 2017-02-07T12:19:38Z

ipapython/nsslib.py

+
+    if min_version_idx < min_allowed_idx:
+        min_version_idx = min_allowed_idx
+        root_logger.warning("tls_version_min set too low ('{old}'),"


Please use a module-specific logger rather than the root logger:

from ipapython.ipa_log_manager import log_mgr logger = log_mgr.get_logger(__name__)

Shall I replace all appearances of root_logger in the module, then?

HonzaCholasta · 2017-02-07T12:20:17Z

ipapython/nsslib.py

+        min_version_idx = TLS_VERSIONS.index(tls_version_min)
+    except ValueError:
+        raise InvocationError("tls_version_min ('{val}') is not a known "
+                              "TLS version.".format(val=tls_version_min))


InvocationError is not right. I think RuntimeError should be OK. Or NetworkError if you want to use an IPA error.

stlaz · 2017-02-07T13:15:05Z

Did not realize merging to Env from default constants was happening in the end of _finalize_core(), moved the checks in config.py accordingly.
Also, for some reason, github shows root_logger issue as solved but it's not - should all root_logger appearances be replaces by a module-own logger?

HonzaCholasta · 2017-02-07T15:10:27Z

@stlaz, you don't have to replace root_logger in old code, but don't use it in new code.

stlaz · 2017-02-08T07:38:33Z

Done. Also added a docstring to the get_proper_tls_version_span() function.

HonzaCholasta · 2017-02-15T07:18:08Z

LGTM.

tkrizek · 2017-02-16T09:26:39Z

ipapython/nsslib.py

+        max_version_idx = TLS_VERSIONS.index(tls_version_max)
+    except ValueError:
+        raise ValueError("tls_version_max ('{val}') is not a known "
+                         "TLS version.".format(val=tls_version_max))


Would it be possible to move try-except block to a separate function to avoid the copy-pasta for min and max version checks?

max_version_idx = get_proper_tls_version( max_version_idx, "max_version_idx", min_version_idx=min_version_idx)

I would rather avoid such overkill.

tkrizek · 2017-02-16T09:32:50Z

ipapython/nsslib.py

+        logger.warning("tls_version_max set too low ('{old}'),"
+                       "using '{new}' instead"
+                       .format(old=tls_version_max,
+                               new=TLS_VERSIONS[max_version_idx]))


Same as above - would it be possible to use a function to avoid copy-pasta?

tkrizek · 2017-02-16T09:49:05Z

ipapython/nsslib.py

+    :param tls_version_max:
+        the higher value in the TLS min-max span, raised to tls_version_min
+        if lower than TLS_VERSION_MINIMAL
+    """


Please add :raises: to docstring.

tkrizek · 2017-02-16T11:30:48Z

Please update the commit title and description to make it clear that it also removes support of SSLv3.

It was possible to set tls_version_min/max to 'ssl2' or 'ssl3', even though newer versions of NSS will fail to set this as a valid TLS version. This patch explicitly checks for deprecated TLS versions prior to creating a TLS connection. Also, we don't allow tls_version_min/max to be set to a random string anymore. https://fedorahosted.org/freeipa/ticket/6607

tkrizek · 2017-02-16T16:25:58Z

ipapython/nsslib.py

+        max_version_idx = TLS_VERSIONS.index(tls_version_max)
+    except ValueError:
+        raise ValueError("tls_version_max ('{val}') is not a known "
+                         "TLS version.".format(val=tls_version_max))


MartinBasti · 2017-02-17T09:06:45Z

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/ac6f573a3014aa09811ca1559d470afe75eadbec

stlaz mentioned this pull request Feb 2, 2017

Remove nsslib from IPA #367

Closed

5 tasks

stlaz force-pushed the sslv2_explicitly_remove branch from 4980a7b to be99b83 Compare February 7, 2017 11:41

HonzaCholasta reviewed Feb 7, 2017

View reviewed changes

stlaz force-pushed the sslv2_explicitly_remove branch from be99b83 to db2b762 Compare February 7, 2017 13:11

stlaz force-pushed the sslv2_explicitly_remove branch from db2b762 to c59b2ae Compare February 7, 2017 14:42

stlaz force-pushed the sslv2_explicitly_remove branch from c59b2ae to 2761cbf Compare February 8, 2017 07:37

stlaz force-pushed the sslv2_explicitly_remove branch from 2761cbf to 53aebe8 Compare February 14, 2017 14:59

stlaz force-pushed the sslv2_explicitly_remove branch from 53aebe8 to b5bea92 Compare February 15, 2017 09:29

tkrizek suggested changes Feb 16, 2017

View reviewed changes

stlaz force-pushed the sslv2_explicitly_remove branch from b5bea92 to 661ed55 Compare February 16, 2017 15:20

tkrizek approved these changes Feb 16, 2017

View reviewed changes

tkrizek added the ack Pull Request approved, can be merged label Feb 16, 2017

MartinBasti added the pushed Pull Request has already been pushed label Feb 17, 2017

MartinBasti closed this Feb 17, 2017

Explicitly remove support of SSLv2 #396

Explicitly remove support of SSLv2 #396

stlaz commented Jan 13, 2017 •

edited by MartinBasti

tiran commented Jan 17, 2017

stlaz commented Jan 19, 2017

HonzaCholasta Feb 7, 2017

stlaz Feb 7, 2017

HonzaCholasta Feb 7, 2017

stlaz Feb 7, 2017

HonzaCholasta Feb 7, 2017 •

edited

stlaz Feb 7, 2017

stlaz commented Feb 7, 2017

HonzaCholasta commented Feb 7, 2017

stlaz commented Feb 8, 2017

HonzaCholasta commented Feb 15, 2017

tkrizek Feb 16, 2017

stlaz Feb 16, 2017

tkrizek Feb 16, 2017

tkrizek Feb 16, 2017

tkrizek Feb 16, 2017

tkrizek commented Feb 16, 2017

tkrizek Feb 16, 2017

MartinBasti commented Feb 17, 2017

Explicitly remove support of SSLv2 #396

Explicitly remove support of SSLv2 #396

Conversation

stlaz commented Jan 13, 2017 • edited by MartinBasti

tiran commented Jan 17, 2017

stlaz commented Jan 19, 2017

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

HonzaCholasta Feb 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stlaz commented Feb 7, 2017

HonzaCholasta commented Feb 7, 2017

stlaz commented Feb 8, 2017

HonzaCholasta commented Feb 15, 2017

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

tkrizek commented Feb 16, 2017

Choose a reason for hiding this comment

MartinBasti commented Feb 17, 2017

stlaz commented Jan 13, 2017 •

edited by MartinBasti

HonzaCholasta Feb 7, 2017 •

edited