Patchwork sslutil: clarify internal documentation

login
register
mail settings
Submitter Matt Harbison
Date March 31, 2017, 1:32 a.m.
Message ID <9505a8771bb00e56230e.1490923976@Envy>
Download mbox | patch
Permalink /patch/19853/
State Accepted
Headers show

Comments

Matt Harbison - March 31, 2017, 1:32 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1490795674 14400
#      Wed Mar 29 09:54:34 2017 -0400
# Node ID 9505a8771bb00e56230e4c4b265e8369e659a54f
# Parent  2632df096fc0ac7582382b1f94ea4b9ad0bce8f2
sslutil: clarify internal documentation

I ran into this python issue with an incomplete certificate chain on Windows
recently, and this is the clarification that came from that experimenting.  The
comment I left on the bug tracker [1] with a reference to the CPython code [2]
indicates that the original problem I had is a different bug, but happened to
be mentioned under issue20916 on the Python bug tracker.

[1] https://bz.mercurial-scm.org/show_bug.cgi?id=5313#c7
[2] https://hg.python.org/cpython/file/v2.7.12/Modules/_ssl.c#l628
Ryan McElroy - March 31, 2017, 10:11 a.m.
On 3/31/17 2:32 AM, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1490795674 14400
> #      Wed Mar 29 09:54:34 2017 -0400
> # Node ID 9505a8771bb00e56230e4c4b265e8369e659a54f
> # Parent  2632df096fc0ac7582382b1f94ea4b9ad0bce8f2
> sslutil: clarify internal documentation
>
> I ran into this python issue with an incomplete certificate chain on Windows
> recently, and this is the clarification that came from that experimenting.  The
> comment I left on the bug tracker [1] with a reference to the CPython code [2]
> indicates that the original problem I had is a different bug, but happened to
> be mentioned under issue20916 on the Python bug tracker.

This looks like a good clarification to me. Marked at pre-reviewed in 
patchwork.

>
> [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__bz.mercurial-2Dscm.org_show-5Fbug.cgi-3Fid-3D5313-23c7&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=Jw8rundaE7TbmqBYd1txIQ&m=iK6FTLLdR8XoO7DBC_cybhU5iy0JJFk1_nX69IELeQo&s=c6IpMyl5Lj5dkIDbpvIOghBgB-v5qZLgtvPOKyS8jkM&e=
> [2] https://urldefense.proofpoint.com/v2/url?u=https-3A__hg.python.org_cpython_file_v2.7.12_Modules_-5Fssl.c-23l628&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=Jw8rundaE7TbmqBYd1txIQ&m=iK6FTLLdR8XoO7DBC_cybhU5iy0JJFk1_nX69IELeQo&s=2hYPqWC9CvlufBnn24ngNt9Fd8P8i1fU86saihe0FTM&e=
>
> diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
> --- a/mercurial/sslutil.py
> +++ b/mercurial/sslutil.py
> @@ -414,8 +414,10 @@
>           # a hint to the user.
>           # Only modern ssl module exposes SSLContext.get_ca_certs() so we can
>           # only show this warning if modern ssl is available.
> -        # The exception handler is here because of
> -        # https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.python.org_issue20916&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=Jw8rundaE7TbmqBYd1txIQ&m=iK6FTLLdR8XoO7DBC_cybhU5iy0JJFk1_nX69IELeQo&s=QNXlhziGIzRjCA4SsOAJ6TJ9-sd6Xx7T9772Y01YpJo&e= .
> +        # The exception handler is here to handle bugs around cert attributes:
> +        # https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.python.org_issue20916-23msg213479&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=Jw8rundaE7TbmqBYd1txIQ&m=iK6FTLLdR8XoO7DBC_cybhU5iy0JJFk1_nX69IELeQo&s=4e_4aFyB-W1agNiXm6RNvSOlp0Hex2GjWOzbfPXXYws&e= .  (See issues5313.)
> +        # When the main 20916 bug occurs, 'sslcontext.get_ca_certs()' is a
> +        # non-empty list, but the following conditional is otherwise True.
>           try:
>               if (caloaded and settings['verifymode'] == ssl.CERT_REQUIRED and
>                   modernssl and not sslcontext.get_ca_certs()):
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=Jw8rundaE7TbmqBYd1txIQ&m=iK6FTLLdR8XoO7DBC_cybhU5iy0JJFk1_nX69IELeQo&s=HSqjsFw1fqi9rRnK3Pbh9Jln4nbdzkFVaL9jngxBu3A&e=
Yuya Nishihara - March 31, 2017, 2:43 p.m.
On Fri, 31 Mar 2017 11:11:17 +0100, Ryan McElroy wrote:
> On 3/31/17 2:32 AM, Matt Harbison wrote:
> > # HG changeset patch
> > # User Matt Harbison <matt_harbison@yahoo.com>
> > # Date 1490795674 14400
> > #      Wed Mar 29 09:54:34 2017 -0400
> > # Node ID 9505a8771bb00e56230e4c4b265e8369e659a54f
> > # Parent  2632df096fc0ac7582382b1f94ea4b9ad0bce8f2
> > sslutil: clarify internal documentation
> >
> > I ran into this python issue with an incomplete certificate chain on Windows
> > recently, and this is the clarification that came from that experimenting.  The
> > comment I left on the bug tracker [1] with a reference to the CPython code [2]
> > indicates that the original problem I had is a different bug, but happened to
> > be mentioned under issue20916 on the Python bug tracker.
> 
> This looks like a good clarification to me. Marked at pre-reviewed in 
> patchwork.

Yeah, queued, thanks.

Patch

diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
--- a/mercurial/sslutil.py
+++ b/mercurial/sslutil.py
@@ -414,8 +414,10 @@ 
         # a hint to the user.
         # Only modern ssl module exposes SSLContext.get_ca_certs() so we can
         # only show this warning if modern ssl is available.
-        # The exception handler is here because of
-        # https://bugs.python.org/issue20916.
+        # The exception handler is here to handle bugs around cert attributes:
+        # https://bugs.python.org/issue20916#msg213479.  (See issues5313.)
+        # When the main 20916 bug occurs, 'sslcontext.get_ca_certs()' is a
+        # non-empty list, but the following conditional is otherwise True.
         try:
             if (caloaded and settings['verifymode'] == ssl.CERT_REQUIRED and
                 modernssl and not sslcontext.get_ca_certs()):