Patchwork [5,of,5,V2] sslutil: remove redundant check of sslsocket.cipher()

login
register
mail settings
Submitter Gregory Szorc
Date May 15, 2016, 11:18 p.m.
Message ID <cb9a5ca5880b23dcc0d9.1463354322@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/15128/
State Accepted
Headers show

Comments

Gregory Szorc - May 15, 2016, 11:18 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1463338249 25200
#      Sun May 15 11:50:49 2016 -0700
# Node ID cb9a5ca5880b23dcc0d9a8b64a98e332052f2de2
# Parent  e69e18b04ba5d395d50671e69d6762c5107554bc
sslutil: remove redundant check of sslsocket.cipher()

We are doing this check in both wrapsocket() and validatesocket().

The check was added to the validator in 4bb59919c905 and the commit
message justifies the redundancy with a "might." The check in
wrapsocket() was added in 0cc4ad757c77, which appears to be part of
the same series. I'm going to argue the redundancy isn't needed.

I choose to keep the check in wrapsocket() because it is working
around a bug in Python's wrap_socket() and I feel the check for
the bug should live next to the function call exhibiting the bug.
Augie Fackler - May 24, 2016, 3:05 p.m.
On Sun, May 15, 2016 at 04:18:42PM -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1463338249 25200
> #      Sun May 15 11:50:49 2016 -0700
> # Node ID cb9a5ca5880b23dcc0d9a8b64a98e332052f2de2
> # Parent  e69e18b04ba5d395d50671e69d6762c5107554bc
> sslutil: remove redundant check of sslsocket.cipher()

Nice work. I especially like the class becoming a function. Queued.

>
> We are doing this check in both wrapsocket() and validatesocket().
>
> The check was added to the validator in 4bb59919c905 and the commit
> message justifies the redundancy with a "might." The check in
> wrapsocket() was added in 0cc4ad757c77, which appears to be part of
> the same series. I'm going to argue the redundancy isn't needed.
>
> I choose to keep the check in wrapsocket() because it is working
> around a bug in Python's wrap_socket() and I feel the check for
> the bug should live next to the function call exhibiting the bug.
>
> diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
> --- a/mercurial/sslutil.py
> +++ b/mercurial/sslutil.py
> @@ -294,18 +294,16 @@ def sslkwargs(ui, host):
>  def validatesocket(sock, strict=False):
>      """Validate a socket meets security requiremnets.
>
>      The passed socket must have been created with ``wrapsocket()``.
>      """
>      host = sock._hgstate['hostname']
>      ui = sock._hgstate['ui']
>
> -    if not sock.cipher(): # work around http://bugs.python.org/issue13721
> -        raise error.Abort(_('%s ssl connection error') % host)
>      try:
>          peercert = sock.getpeercert(True)
>          peercert2 = sock.getpeercert()
>      except AttributeError:
>          raise error.Abort(_('%s ssl connection error') % host)
>
>      if not peercert:
>          raise error.Abort(_('%s certificate error: '
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
--- a/mercurial/sslutil.py
+++ b/mercurial/sslutil.py
@@ -294,18 +294,16 @@  def sslkwargs(ui, host):
 def validatesocket(sock, strict=False):
     """Validate a socket meets security requiremnets.
 
     The passed socket must have been created with ``wrapsocket()``.
     """
     host = sock._hgstate['hostname']
     ui = sock._hgstate['ui']
 
-    if not sock.cipher(): # work around http://bugs.python.org/issue13721
-        raise error.Abort(_('%s ssl connection error') % host)
     try:
         peercert = sock.getpeercert(True)
         peercert2 = sock.getpeercert()
     except AttributeError:
         raise error.Abort(_('%s ssl connection error') % host)
 
     if not peercert:
         raise error.Abort(_('%s certificate error: '