bpo-31432: Clarify CERT_NONE/OPTIONAL/REQUIRED doc by tiran · Pull Request #3530 · python/cpython

tiran · 2017-09-13T05:13:37Z

The documentation for CERT_NONE, CERT_OPTIONAL, and CERT_REQUIRED were
misleading and partly wrong. It fails to explain that OpenSSL behaves
differently in client and server mode. Also OpenSSL does validate the
cert chain everytime. With SSL_VERIFY_NONE a validation error is not
fatal in client mode and does not request a client cert in server mode.

Also discourage people from using CERT_OPTIONAL in client mode.

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

https://bugs.python.org/issue31432

tiran · 2017-09-13T05:15:49Z

Memo to me: drop PROTOCOL_TLS_CLIENT in 2.7 backport.

alex · 2017-09-15T15:12:29Z

Modules/_ssl.c

nit: should be an SSLContext

tiran · 2018-02-22T17:41:48Z

@vadmium Martin, you have helped me with documentation of PR #5259. Could you check this documentation, too?

vadmium · 2018-02-24T02:25:00Z

Doc/library/ssl.rst

What if “anonymous ciphers are enabled”, mentioned under https://docs.python.org/dev/library/ssl.html#verifying-certificates?

I'd rather not explain anonymous cipher suites and just remove them from the documentation completely. Anonymous cipher suites is a misleading term. They don't provide anonymity for the user but allow the server to stay anonymous by not providing any authentication. It's totally insecure and no longer supported in TLS 1.3 for a very good reason.

vadmium · 2018-02-24T02:25:01Z

Doc/library/ssl.rst

What does cert_reqs default to if I call wrap_socket(..., ssl_version=PROTOCOL_TLS_CLIENT)? Judging by the signature and documentation for wrap_socket, CERT_NONE seems to be the default regardless of the protocol setting. If you are only talking about the default value for verify_mode after an SSLContext is constructed, I suggest to mention that under the entry for SSLContext.verify_mode, or the SSLContext constructor, instead of here. (Otherwise, maybe adjust the description of the top-level wrap_socket function.)

For a moment I feared that you have discovered a security bug.

Thankfully the code is robust and doesn't allow you to use PROTOCOL_TLS_CLIENT with default arguments of wrap_socket and SSLSocket. Both ssl.wrap_socket(ssl_version=ssl.PROTOCOL_TLS_CLIENT) and ssl.SSLSocket(ssl_version=ssl.PROTOCOL_TLS_CLIENT) will both fail with error Cannot set verify_mode to CERT_NONE when check_hostname is enabled..

So the insecure default doesn't work here. I consider it a feature. :) I'm going to deprecate ssl.wrap_socket and ssl.SSLSocket() default constructor because they have more fundamental issues.

Okay, now I realize that the PROTOCOL_TLS_CLIENT mode is newer than the old wrap_socket function. It was not clear that wrap_socket doesn’t support this new mode, but that is separate to what you are trying to do here.

tiran · 2018-02-25T09:16:21Z

@vadmium I have removed the misleading note about anonymous ciphers.

willingc · 2018-05-21T04:05:13Z

Hi @vadmium, It looks like this is ready to be merged. Is there anything that prevents doing so at this point? Thanks.

The documentation for CERT_NONE, CERT_OPTIONAL, and CERT_REQUIRED were misleading and partly wrong. It fails to explain that OpenSSL behaves differently in client and server mode. Also OpenSSL does validate the cert chain everytime. With SSL_VERIFY_NONE a validation error is not fatal in client mode and does not request a client cert in server mode. Also discourage people from using CERT_OPTIONAL in client mode. Signed-off-by: Christian Heimes <christian@python.org>

The documentation no longer mentions anonymous ciphers. They are totally insecure and irrelevant for virtually all users. The documentation was also wrong. For anonymous ciphers, CERT_NONE, CERT_OPTIONAL and CERT_REQUIRED all behaved the same. Internally, CERT_REQUIRED = SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT but the last flag is ignored in client mode. When a anonymous cipher is used, clients ignore missing server certs. Signed-off-by: Christian Heimes <christian@python.org>

ned-deily · 2018-06-07T05:06:55Z

@tiran, @vadmium I would prefer to merge this for 3.7.0. If I don't hear any objections in the next day or so, I will merge it.

miss-islington · 2018-06-11T22:59:48Z

Thanks @tiran for the PR, and @ned-deily for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6, 3.7.
🐍🍒⛏🤖

bedevere-bot · 2018-06-11T23:00:10Z

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

The documentation for CERT_NONE, CERT_OPTIONAL, and CERT_REQUIRED were misleading and partly wrong. It fails to explain that OpenSSL behaves differently in client and server mode. Also OpenSSL does validate the cert chain everytime. With SSL_VERIFY_NONE a validation error is not fatal in client mode and does not request a client cert in server mode. Also discourage people from using CERT_OPTIONAL in client mode. (cherry picked from commit ef24b6c) Co-authored-by: Christian Heimes <christian@python.org>

miss-islington · 2018-06-11T23:00:50Z

Sorry, @tiran and @ned-deily, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker ef24b6c54d40e7820456873a6eab6ef57d2bd0db 3.6

miss-islington · 2018-06-11T23:01:51Z

Sorry, @tiran and @ned-deily, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker ef24b6c54d40e7820456873a6eab6ef57d2bd0db 2.7

…H-7649) The documentation for CERT_NONE, CERT_OPTIONAL, and CERT_REQUIRED were misleading and partly wrong. It fails to explain that OpenSSL behaves differently in client and server mode. Also OpenSSL does validate the cert chain everytime. With SSL_VERIFY_NONE a validation error is not fatal in client mode and does not request a client cert in server mode. Also discourage people from using CERT_OPTIONAL in client mode. (cherry picked from commit ef24b6c) Co-authored-by: Christian Heimes <christian@python.org>

The documentation for CERT_NONE, CERT_OPTIONAL, and CERT_REQUIRED were misleading and partly wrong. It fails to explain that OpenSSL behaves differently in client and server mode. Also OpenSSL does validate the cert chain everytime. With SSL_VERIFY_NONE a validation error is not fatal in client mode and does not request a client cert in server mode. Also discourage people from using CERT_OPTIONAL in client mode.

bedevere-bot · 2018-06-12T01:17:26Z

GH-7652 is a backport of this pull request to the 3.6 branch.

…H-7652) The documentation for CERT_NONE, CERT_OPTIONAL, and CERT_REQUIRED were misleading and partly wrong. It fails to explain that OpenSSL behaves differently in client and server mode. Also OpenSSL does validate the cert chain everytime. With SSL_VERIFY_NONE a validation error is not fatal in client mode and does not request a client cert in server mode. Also discourage people from using CERT_OPTIONAL in client mode.

tiran added needs backport to 2.7 docs Documentation in the Doc dir labels Sep 13, 2017

the-knights-who-say-ni added the CLA signed label Sep 13, 2017

bedevere-bot added the awaiting merge label Sep 13, 2017

tiran force-pushed the bpo-31432-cert-doc branch from d5084da to 58d89be Compare September 13, 2017 05:15

tiran force-pushed the bpo-31432-cert-doc branch from 58d89be to 0243bd5 Compare September 15, 2017 15:08

alex reviewed Sep 15, 2017

View reviewed changes

Modules/_ssl.c Outdated

Copy link

Member

alex Sep 15, 2017

Choose a reason for hiding this comment

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

nit: should be an SSLContext

tiran force-pushed the bpo-31432-cert-doc branch from 0243bd5 to 2f4126a Compare September 15, 2017 15:17

Mariatta added needs rebase and removed needs rebase labels Oct 9, 2017

tiran force-pushed the bpo-31432-cert-doc branch 2 times, most recently from 597b493 to 0f3916d Compare January 20, 2018 23:14

tiran force-pushed the bpo-31432-cert-doc branch from 0f3916d to 6dde39e Compare February 22, 2018 17:40

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

tiran requested a review from vadmium February 22, 2018 17:40

vadmium reviewed Feb 24, 2018

View reviewed changes

tiran force-pushed the bpo-31432-cert-doc branch from 6dde39e to 7cb02cf Compare February 25, 2018 09:15

tiran force-pushed the bpo-31432-cert-doc branch from 7cb02cf to 350506a Compare May 14, 2018 18:07

tiran added 2 commits May 22, 2018 12:53

tiran force-pushed the bpo-31432-cert-doc branch from 350506a to 86aa7d2 Compare May 22, 2018 10:53

ned-deily merged commit ef24b6c into python:master Jun 11, 2018

bedevere-bot removed the awaiting merge label Jun 11, 2018

bedevere-bot removed the needs backport to 3.7 label Jun 11, 2018

miss-islington assigned ned-deily Jun 11, 2018

bedevere-bot removed the needs backport to 3.6 label Jun 12, 2018

Uh oh!

Conversation

tiran commented Sep 13, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tiran commented Sep 13, 2017

Uh oh!

alex Sep 15, 2017

Choose a reason for hiding this comment

Uh oh!

tiran commented Feb 22, 2018

Uh oh!

vadmium Feb 24, 2018

Choose a reason for hiding this comment

Uh oh!

tiran Feb 25, 2018

Choose a reason for hiding this comment

Uh oh!

vadmium Feb 24, 2018

Choose a reason for hiding this comment

Uh oh!

tiran Feb 24, 2018

Choose a reason for hiding this comment

Uh oh!

vadmium Mar 20, 2018

Choose a reason for hiding this comment

Uh oh!

tiran commented Feb 25, 2018

Uh oh!

willingc commented May 21, 2018

Uh oh!

ned-deily commented Jun 7, 2018

Uh oh!

miss-islington commented Jun 11, 2018

Uh oh!

bedevere-bot commented Jun 11, 2018

Uh oh!

miss-islington commented Jun 11, 2018

Uh oh!

miss-islington commented Jun 11, 2018

Uh oh!

bedevere-bot commented Jun 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

tiran commented Sep 13, 2017 •

edited by bedevere-bot

Loading