<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sun, Jul 17, 2016 at 1:25 AM, Yuya Nishihara <span dir="ltr"><<a href="mailto:yuya@tcha.org" target="_blank">yuya@tcha.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="">On Fri, 15 Jul 2016 22:18:46 -0700, Gregory Szorc wrote:<br>
> # HG changeset patch<br>
> # User Gregory Szorc <<a href="mailto:gregory.szorc@gmail.com">gregory.szorc@gmail.com</a>><br>
> # Date 1468554442 25200<br>
> #      Thu Jul 14 20:47:22 2016 -0700<br>
> # Node ID 862344cd55472d3d5233abcb829766c947f7df90<br>
> # Parent  da2cb958a05e738646878e564ad109cfb478ac13<br>
> sslutil: config option to specify TLS protocol version<br>
<br>
</span>The change LGTM per the last discussion. Queued 3, 5, and 6, thanks.<br>
<span class=""><br>
>      # Despite its name, PROTOCOL_SSLv23 selects the highest protocol<br>
>      # that both ends support, including TLS protocols. On legacy stacks,<br>
> -    # the highest it likely goes in TLS 1.0. On modern stacks, it can<br>
> +    # the highest it likely goes is TLS 1.0. On modern stacks, it can<br>
>      # support TLS 1.2.<br>
>      #<br>
>      # The PROTOCOL_TLSv* constants select a specific TLS version<br>
>      # only (as opposed to multiple versions). So the method for<br>
>      # supporting multiple TLS versions is to use PROTOCOL_SSLv23 and<br>
>      # disable protocols via SSLContext.options and OP_NO_* constants.<br>
>      # However, SSLContext.options doesn't work unless we have the<br>
>      # full/real SSLContext available to us.<br>
<br>
</span>Perhaps this comment should be moved to protocolsettings() where<br>
PROTOCOL_SSLv23 is used.<br>
<span class=""><br>
> +    if protocol == 'tls1.0':<br>
> +        # Defaults above are to use TLS 1.0+<br>
> +        pass<br>
> +    elif protocol == 'tls1.1':<br>
> +        options |= ssl.OP_NO_TLSv1<br>
> +    elif protocol == 'tls1.2':<br>
> +        options |= ssl.OP_NO_TLSv1 | ssl.OP_NO_TLSv1_1<br>
<br>
</span>Nit: OP_NO_TLSv1_1 requires OpenSSL 1.0.1+. Better to drop 'tls1.2' from the<br>
configprotocols if unsupported?<br>
<br>
<a href="https://docs.python.org/2.7/library/ssl.html#ssl.OP_NO_TLSv1_1" rel="noreferrer" target="_blank">https://docs.python.org/2.7/library/ssl.html#ssl.OP_NO_TLSv1_1</a></blockquote><div><br></div><div>It looks like a number of constants aren't always defined on the ssl module: <a href="https://hg.python.org/cpython/file/345ec7455b75/Modules/_ssl.c#l4129">https://hg.python.org/cpython/file/345ec7455b75/Modules/_ssl.c#l4129</a><br><br></div><div>I guess I'll send some follow-ups to this series to make the code more robust.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<span class=""><br>
>      if modernssl:<br>
>          # We /could/ use create_default_context() here since it doesn't load<br>
> -        # CAs when configured for client auth.<br>
> -        sslcontext = SSLContext(ssl.PROTOCOL_SSLv23)<br>
> -        # SSLv2 and SSLv3 are broken. Ban them outright.<br>
> -        sslcontext.options |= OP_NO_SSLv2 | OP_NO_SSLv3<br>
> -        # Prevent CRIME<br>
> -        sslcontext.options |= getattr(ssl, 'OP_NO_COMPRESSION', 0)<br>
> +        # CAs when configured for client auth. However, it is hard-coded to<br>
> +        # use ssl.PROTOCOL_SSLv23m which may not be appropriate here.<br>
<br>
</span>Fixed s/PROTOCOL_SSLv23m/PROTOCOL_SSLv23/<br>
</blockquote></div><br></div></div>