-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
minimum_version mismatch #105090
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
This part was changed in response to the deprecation of TLSv1_2. 2875c60 You should change the description text below instead. Line 2691 in c39500d
|
) (cherry picked from commit e5252c6) Co-authored-by: Jocelyn Castellano <[email protected]>
…107038) Co-authored-by: Jocelyn Castellano <[email protected]>
) (cherry picked from commit e5252c6) Co-authored-by: Jocelyn Castellano <[email protected]>
…107039) Co-authored-by: Jocelyn Castellano <[email protected]>
…107040) Co-authored-by: Jocelyn Castellano <[email protected]>
) (cherry picked from commit e5252c6) Co-authored-by: Jocelyn Castellano <[email protected]>
I believe there are still issues here (and my apologies for dropping the ball and not weighing in earlier -- the closing of the issue sent me a notification prompting me to look again). First, I do not think it matters whether or not TLS 1.2 is deprecated. The example is simply showing how to restrict the preferred version used. Second, the text states "and later" but the example sets a maximum_version as well as a minimum_version. Finally, I think a useful example would actually show a range and how both minimum_version and maximum_version could be used together to restrict to that range. For example, a minimum_version of ssl.TLSVersion.TLSv1_2 and a maximum_version of ssl.TLSVersion.TLSv1_3 would restrict to a range of TLS 1.2 and TLS 1.3, even if there are later versions in the future. To match the current text, "The SSL context created above will only allow TLSv1.3 and later (if supported by your system)," the maximum_version would need to be left out. I have submitted the following PR to reflect my thoughts above: gh-105090: Update ssl.rst to show a range of TLS versions supported by a context. |
I believe this was done mistakenly, as in reality there is no "deprecation of TLSv1.2", only some older features including how a connection is configured. Therefore:
That means there's no reason to not allow a TLSv1.2 connection as an example in the docs when connected using the current client_context, so the original wording was right. What was also introduced in said change was a maximum_version that doesn't match the description, so it should have been either left out completely, or the description and configuration options rewritten entirely to demonstrate a different point than before with the OP flags. |
Documentation
The documentation states "The SSL context created above will only allow TLSv1.2 and later..." However, the example code sets the minimum version to TLS 1.3 (not 1.2 as stated).
I believe this line should be changed to:
>>> client_context.minimum_version = ssl.TLSVersion.TLSv1_2
I am happy to create a pull request, if that is helpful.
Linked PRs
The text was updated successfully, but these errors were encountered: