bpo-31453: Add setter for min/max protocol version #5259

tiran · 2018-01-21T14:02:46Z

OpenSSL 1.1 has introduced a new API to set the minimum and maximum
supported protocol version. The API is easier to use than the old
OP_NO_TLS1 option flags, too.

Since OpenSSL has no call to set minimum version to highest supported,
the implementation emulate highest_version = MINIMUM_SUPPORTED and
lowest_version = MAXIMUM_SUPPORTED by figuring out the minumum and
maximum supported version at compile time.

Signed-off-by: Christian Heimes christian@python.org

https://bugs.python.org/issue31453

alex · 2018-01-29T17:25:15Z

Doc/library/ssl.rst

 .. data:: HAS_TLSv1_3

-   Whether the OpenSSL library has built-in support for the TLS 1.3 protocol.


I think this is more correct with "the"

alex · 2018-01-29T17:27:44Z

Doc/library/ssl.rst

+.. attribute:: TLSVersion.MAXIMUM_SUPPORTED
+
+   The minimum or maximum supported SSL or TLS version. The value of the
+   enum value does not reflect the actual minimum or maximum version.


I don't understand what the second sentence means.

I tried to explain that MAXIMUM_SUPPORTED == TLSv1_2 or MAXIMUM_SUPPORTED == TLSv1_3 doesn't give any meaningful result. You cannot use the value of the MAXIMUM_SUPPORTED constant to figure out what the actual maximum supported TLS version is. The values are just arbitrary values.

Maybe something like "this is a magic constant, it will not be equal to the value of the constant for the highest protocol"?

alex · 2018-01-29T17:29:51Z

Doc/library/ssl.rst

@@ -1729,6 +1779,26 @@ to speed up repeated connections from the same clients.

     This features requires OpenSSL 0.9.8f or newer.

+.. attribute:: SSLContext.highest_version


I think this should be maximum_version for consistency with the TLSVersion

The names are based on PEP 543. Also maximum_version and minimum_version are visually similar compared to highest_version and lowest_version.

https://www.python.org/dev/peps/pep-0543/#configuration

:param lowest_supported_version TLSVersion: The minimum version of TLS that should be allowed on TLS connections using this configuration. :param highest_supported_version TLSVersion: The maximum version of TLS that should be allowed on TLS connections using this configuration.

https://www.python.org/dev/peps/pep-0543/#tls-versions

class TLSVersion(Enum): MINIMUM_SUPPORTED = auto() SSLv2 = auto() SSLv3 = auto() TLSv1 = auto() TLSv1_1 = auto() TLSv1_2 = auto() TLSv1_3 = auto() MAXIMUM_SUPPORTED = auto()

alex · 2018-01-29T17:30:14Z

Doc/library/ssl.rst

+
+   .. note::
+
+     This features requires OpenSSL 1.1.0g or newer.


I'm not sure what this means :-)

The feature is not available unless the ssl module is compiled with OpenSSL 1.1.0g or a later version. OpenSSL 1.1.0f and older versions do not have getters.

I think something like "This attribute does not exist unless ..." would be clearer

alex · 2018-01-29T17:33:14Z

Modules/_ssl.c

+#ifdef TLS1_3_VERSION
+    PY_PROTO_TLSv1_3 = TLS1_3_VERSION,
+#else
+    PY_PROTO_TLSv1_3 = 0x304,


Should this exist if OpenSSL doesn't have 1.3?

In PEP 543 all versions are always defined.

alex · 2018-01-29T17:35:53Z

Modules/_ssl.c

+    }
+    if ((v == PY_PROTO_MINIMUM_SUPPORTED) ||
+            (v == PY_PROTO_MAXIMUM_SUPPORTED)) {
+        /* 0: set lowest or highest supported version */


This is confusing to me, how does it know the difference between set_min(min) and set_min(max)?

SSL_CTX_set_min_proto_version(ctx, 0) sets lowest version, SSL_CTX_set_max_proto_version(ctx, 0) sets highest version. The function selects betwen min and max based on the what argument.

(Q.E.D. why I prefer highest/lowest over max/min) :p

if (what == 0) { result = SSL_CTX_set_min_proto_version(self->ctx, v); } else { result = SSL_CTX_set_max_proto_version(self->ctx, v); }

How do you set the minimum version to be the same as the maximum version?

Ah, now I got your. You want to have lowest_version = MAXIUMUM_SUPPORTED. OpenSSL has no API for that. I added more code to emulate it. I also make the attributes read-only unless the context's method support version negotiation.

vadmium · 2018-01-30T06:11:24Z

Doc/library/ssl.rst

+.. class:: TLSVersion
+
+   :class:`enum.IntEnum` collection SSL and TLS versions for
+   :attr:`SSLContext.highest_version` and :attr:`SSLContext.lowest_version`.


Is there a word missing here? Perhaps something like “Enum collection of SSL and TLS versions . . .”?

vadmium · 2018-01-30T06:15:59Z

Doc/library/ssl.rst

+
+   The minimum or maximum supported SSL or TLS version. This is a magic
+   constant, it will not be equal to the value of the constant for the
+   lowest or highest protocol


Plural: These are magic constants (or These are both equal to a magic constant or something)

By magic constant, do you just mean the value(s) are distinct from the other enum values? After reading further down, it sounds like the user is supposed to feed these values into the highest_ and lowest_version attributes, but saying “it will not be equal” seems to contradict this.

vadmium · 2018-01-30T06:18:22Z

Doc/library/ssl.rst

@@ -1729,6 +1780,28 @@ to speed up repeated connections from the same clients.

     This features requires OpenSSL 0.9.8f or newer.

+.. attribute:: SSLContext.highest_version
+
+   An :class:`TLSversion` enum member representing the highest supported


An TLS → A TLS

TLSversion capitalization of V is inconsistent with elsewhere.

vadmium · 2018-01-30T06:20:06Z

Doc/library/ssl.rst

+
+   An :class:`TLSversion` enum member representing the highest supported
+   TLS version. The value defaults to :attr:`TLSVersion.MAXIMUM_SUPPORTED`.
+   The flag and :attr:`SSLContext.options` both affect the supported SSL


Which flag? If you mean that highest_version is a flag, be explicit.

vadmium · 2018-01-30T06:28:51Z

Doc/library/ssl.rst

+
+.. attribute:: SSLContext.lowest_version
+
+   Like :attr:`SSLContext.lowest_version` except it is the lowest


Do you mean like highest_version?

vadmium · 2018-01-30T06:33:01Z

Doc/whatsnew/3.7.rst

@@ -629,6 +629,12 @@ ciphers that have been blocked by OpenSSL security update. Default cipher
 suite selection can be configured on compile time.
 (Contributed by Christian Heimes in :issue:`31429`.)

+OpenSSL 1.1 APIs for setting the minimum and maximum TLS protocol version are
+available as as :attr:`~ssl.SSLContext.lowest_version` and


as as repeated

vadmium · 2018-01-30T06:58:06Z

Doc/library/ssl.rst

@@ -852,7 +852,7 @@ Constants

 .. data:: HAS_ALPN

-   Whether the OpenSSL library has built-in support for the *Application-Layer
+   Whether the OpenSSL library has built-in support for the the *Application-Layer
   Protocol Negotiation* TLS extension as described in :rfc:`7301`.


This change seems detrimental with no benefit. In fact all four of the the additions don’t seem beneficial to me.

vadmium · 2018-01-30T07:30:41Z

Doc/library/ssl.rst

 .. data:: HAS_TLSv1_3

-   Whether the OpenSSL library has built-in support for the TLS 1.3 protocol.
+  Whether the OpenSSL library has built-in support for the TLS 1.3 protocol.

   .. versionadded:: 3.7


Have you tried compiling this? I suspect the varying indentation messes things up.

tiran · 2018-02-22T17:35:38Z

@vadmium thanks Martin, I have addressed your documentation remarks and clarified the documentation

@alex It didn't occur to me that you wanted to set the lowest version to maximum supported. OpenSSL doesn't have an API for that. I had to add some extra code to guess the max and min available versions. Since it might be possible that it's incorrect (different libssl than ssl headers), I'm still using 0 for auto config when possible.

vadmium · 2018-02-24T03:02:23Z

Doc/library/ssl.rst

+.. attribute:: TLSVersion.MAXIMUM_SUPPORTED
+
+   The minimum or maximum supported SSL or TLS version. These are a magic
+   constants. Their values don't reflect the lowest and highest available


I think this is better, but also drop the a: “These are magic constants.”

tiran · 2018-02-26T11:41:44Z

I have correct BPO number from bpo-32609 to bpo-31453

tiran · 2018-02-26T15:09:47Z

@alex I discussed the naming of the new attributes with @Lukasa. Cory doesn't have a strong opinion but leans towards minimum and maximum. Since you also prefer minimum and maximum, the OpenSSL API use min/max and I don' have a strong opinion either, I decided to go for minimum_version and maximum_version.

tiran · 2018-02-26T16:21:43Z

AppVeyor has some issues.

OpenSSL 1.1 has introduced a new API to set the minimum and maximum supported protocol version. The API is easier to use than the old OP_NO_TLS1 option flags, too. Since OpenSSL has no call to set minimum version to highest supported, the implementation emulate maximum_version = MINIMUM_SUPPORTED and minimum_version = MAXIMUM_SUPPORTED by figuring out the minumum and maximum supported version at compile time. Signed-off-by: Christian Heimes <christian@python.org>

miss-islington · 2018-02-27T10:54:46Z

Thanks @tiran for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

bedevere-bot · 2018-02-27T10:54:46Z

@tiran: Please replace # with GH- in the commit message next time. Thanks!

OpenSSL 1.1 has introduced a new API to set the minimum and maximum supported protocol version. The API is easier to use than the old OP_NO_TLS1 option flags, too. Since OpenSSL has no call to set minimum version to highest supported, the implementation emulate maximum_version = MINIMUM_SUPPORTED and minimum_version = MAXIMUM_SUPPORTED by figuring out the minumum and maximum supported version at compile time. Signed-off-by: Christian Heimes <christian@python.org> (cherry picked from commit 698dde1) Co-authored-by: Christian Heimes <christian@python.org>

bedevere-bot · 2018-02-27T10:55:53Z

GH-5926 is a backport of this pull request to the 3.7 branch.

OpenSSL 1.1 has introduced a new API to set the minimum and maximum supported protocol version. The API is easier to use than the old OP_NO_TLS1 option flags, too. Since OpenSSL has no call to set minimum version to highest supported, the implementation emulate maximum_version = MINIMUM_SUPPORTED and minimum_version = MAXIMUM_SUPPORTED by figuring out the minumum and maximum supported version at compile time. Signed-off-by: Christian Heimes <christian@python.org> (cherry picked from commit 698dde1) Co-authored-by: Christian Heimes <christian@python.org>

bedevere-bot added the awaiting merge label Jan 21, 2018

tiran force-pushed the ssl_min_max_proto branch from 9e59bf7 to 3c5dd30 Compare January 21, 2018 15:10

tiran force-pushed the ssl_min_max_proto branch 2 times, most recently from 2093bae to 5930eff Compare January 29, 2018 17:07

tiran requested review from alex, njsmith and pitrou January 29, 2018 17:09

tiran added the type-feature A feature request or enhancement label Jan 29, 2018

alex reviewed Jan 29, 2018

View reviewed changes

tiran force-pushed the ssl_min_max_proto branch from 5930eff to 54697fc Compare January 29, 2018 22:11

vadmium reviewed Jan 30, 2018

View reviewed changes

tiran force-pushed the ssl_min_max_proto branch from 54697fc to 7178f0c Compare February 22, 2018 17:12

This was referenced Feb 22, 2018

bpo-31432: Clarify CERT_NONE/OPTIONAL/REQUIRED doc #3530

Merged

bpo-31453: [WIP] Allow to change TLS protocols on Debian #3662

Closed

vadmium reviewed Feb 24, 2018

View reviewed changes

tiran force-pushed the ssl_min_max_proto branch 5 times, most recently from 1486d71 to 99a1442 Compare February 25, 2018 19:46

tiran changed the title ~~bpo-32609: Add setter for min/max protocol version~~ bpo-31453: Add setter for min/max protocol version Feb 26, 2018

tiran force-pushed the ssl_min_max_proto branch from 99a1442 to 31ce9f3 Compare February 26, 2018 11:41

tiran added the needs backport to 3.7 label Feb 26, 2018

tiran force-pushed the ssl_min_max_proto branch from 31ce9f3 to d32a828 Compare February 26, 2018 15:04

tiran force-pushed the ssl_min_max_proto branch from d32a828 to dc50a26 Compare February 26, 2018 16:20

tiran closed this Feb 26, 2018

tiran reopened this Feb 26, 2018

tiran force-pushed the ssl_min_max_proto branch from dc50a26 to 3926082 Compare February 27, 2018 09:34

tiran merged commit 698dde1 into python:master Feb 27, 2018

bedevere-bot removed the awaiting merge label Feb 27, 2018

tiran deleted the ssl_min_max_proto branch February 27, 2018 10:54

bedevere-bot removed the needs backport to 3.7 label Feb 27, 2018

stratakis mentioned this pull request Aug 28, 2020

[3.6] bpo-41183: Skip ssl tests for disabled versions (GH-16427) #21882

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-31453: Add setter for min/max protocol version #5259

bpo-31453: Add setter for min/max protocol version #5259

tiran commented Jan 21, 2018 •

edited

Loading

alex Jan 29, 2018

alex Jan 29, 2018

tiran Jan 29, 2018

alex Jan 29, 2018

alex Jan 29, 2018

tiran Jan 29, 2018

alex Jan 29, 2018

tiran Jan 29, 2018

alex Jan 29, 2018

alex Jan 29, 2018

tiran Jan 29, 2018

alex Jan 29, 2018

tiran Jan 29, 2018

alex Jan 29, 2018

tiran Feb 22, 2018

vadmium Jan 30, 2018

vadmium Jan 30, 2018

vadmium Jan 30, 2018

vadmium Jan 30, 2018

vadmium Jan 30, 2018

vadmium Jan 30, 2018

vadmium Jan 30, 2018

vadmium Jan 30, 2018

tiran commented Feb 22, 2018

vadmium Feb 24, 2018

tiran commented Feb 26, 2018

tiran commented Feb 26, 2018

tiran commented Feb 26, 2018

miss-islington commented Feb 27, 2018

bedevere-bot commented Feb 27, 2018

bedevere-bot commented Feb 27, 2018

		.. data:: HAS_TLSv1_3

		Whether the OpenSSL library has built-in support for the TLS 1.3 protocol.

		@@ -1729,6 +1779,26 @@ to speed up repeated connections from the same clients.

		This features requires OpenSSL 0.9.8f or newer.

		.. attribute:: SSLContext.highest_version


		.. attribute:: SSLContext.lowest_version

		Like :attr:`SSLContext.lowest_version` except it is the lowest

bpo-31453: Add setter for min/max protocol version #5259

bpo-31453: Add setter for min/max protocol version #5259

Conversation

tiran commented Jan 21, 2018 • edited Loading

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiran commented Feb 22, 2018

Choose a reason for hiding this comment

tiran commented Feb 26, 2018

tiran commented Feb 26, 2018

tiran commented Feb 26, 2018

miss-islington commented Feb 27, 2018

bedevere-bot commented Feb 27, 2018

bedevere-bot commented Feb 27, 2018

tiran commented Jan 21, 2018 •

edited

Loading