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

[tiran]

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 [email protected]

https://bugs.python.org/issue31453

[alex]

I think this is more correct with "the"

[alex]

I don't understand what the second sentence means.

[tiran]

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.

[alex]

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

[alex]

I think this should be maximum_version for consistency with the TLSVersion

[tiran]

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]

I'm not sure what this means :-)

[tiran]

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.

[alex]

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

[alex]

Should this exist if OpenSSL doesn't have 1.3?

[tiran]

In PEP 543 all versions are always defined.

[alex]

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

[tiran]

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

[alex]

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

[tiran]

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]

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

[vadmium]

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]

An TLS → A TLS

TLSversion capitalization of V is inconsistent with elsewhere.

[vadmium]

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

[vadmium]

Do you mean like highest_version?

[vadmium]

as as repeated

[vadmium]

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

[vadmium]

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

[tiran]

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

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

[tiran]

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

[tiran]

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

AppVeyor has some issues.

[miss-islington]

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

[bedevere-bot]

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

[bedevere-bot]

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