cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation31 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

.. data:: HAS_TLSv1_3

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

Choose a reason for hiding this comment

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

I think this is more correct with "the"

.. 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.

Choose a reason for hiding this comment

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

I don't understand what the second sentence means.

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

Maybe something like "this is a magic constant, it will not be equal to the value of the constant for the highest 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

Choose a reason for hiding this comment

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

I think this should be maximum_version for consistency with the TLSVersion

Choose a reason for hiding this comment

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

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


.. note::

This features requires OpenSSL 1.1.0g or newer.

Choose a reason for hiding this comment

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

I'm not sure what this means :-)

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Should this exist if OpenSSL doesn't have 1.3?

Choose a reason for hiding this comment

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

In PEP 543 all versions are always defined.

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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);
    }

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

.. class:: TLSVersion

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

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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.

@@ -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

Choose a reason for hiding this comment

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

An TLS → A TLS

TLSversion capitalization of V is inconsistent with elsewhere.


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

Choose a reason for hiding this comment

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

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


.. attribute:: SSLContext.lowest_version

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

Choose a reason for hiding this comment

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

Do you mean like highest_version?

@@ -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

Choose a reason for hiding this comment

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

as as repeated

@@ -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`.

Choose a reason for hiding this comment

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

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

.. 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

Choose a reason for hiding this comment

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

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

@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.

This was referenced

Feb 22, 2018

.. 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

Choose a reason for hiding this comment

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

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

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

@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.

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

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

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

tiran deleted the ssl_min_max_proto branch

February 27, 2018 10:54

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Feb 27, 2018

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.

Signed-off-by: Christian Heimes christian@python.org (cherry picked from commit 698dde1)

Co-authored-by: Christian Heimes christian@python.org

miss-islington added a commit that referenced this pull request

Feb 27, 2018

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.

Signed-off-by: Christian Heimes christian@python.org (cherry picked from commit 698dde1)

Co-authored-by: Christian Heimes christian@python.org