Opened 3 years ago

Closed 6 weeks ago

#6800 enhancement closed fixed (fixed)

Expand `CertificateOptions` to include a simple API for specifying what protocols to support

Reported by: Jean-Paul Calderone Owned by: hawkowl
Priority: normal Milestone:
Component: core Keywords:
Cc: Hynek Schlawack, David Petar Novakovic Branch: 6800-certificate-options-protocols
branch-diff, diff-cov, branch-cov, buildbot

Description (last modified by Jean-Paul Calderone)

twisted.internet.ssl.CertificateOptions is used to define the connection parameters for a family of protocols. SSL versions 21 and 3 and TLS versions 1.0, 1.1, and 1.2.

The current API for choosing between these protocols is quite baroque. It's also not very expressive.

  • CertificateOptions(method=SSLv2_METHOD) - this negotiates SSLv2
  • CertificateOptions(method=SSLv3_METHOD) - this negotiates SSLv3
  • CertificateOptions(method=TLSv1_METHOD) - this negotiates TLS v1.0.
  • CertificateOptions(method=SSLv23_METHOD) - this negotiates SSLv2, SSLv3, TLSv1.0, TLSv1.1, or TLSv1.2.
  • CertificateOptions(method=None) (or CertificateOptions()) - this negotiates TLSv1.0, TLSv1.1, or TLSv1.22

Given a sufficiently new version of pyOpenSSL, these are also possible:

  • CertificateOptions(method=TLSv1_1_METHOD) - this negotiates TLSv1.1
  • CertificateOptions(method=TLSv1_2_METHOD) - this negotiates TLSv1.2

Notice that out of 5 protocols there are only 7 possible protocol combinations supported (rather than the complete set of 31 combinations3).

I suggest that CertificateOptions(protocols=[...]) is a better API. The list can specify exactly which protocols to support in any combination. This avoids the need for more symbols than there are protocols (eg SSLv23_METHOD) and makes it extremely obvious what protocols are being selected.

It also steps away from the OpenSSL specific terminology of "methods" and it will force us to step away from the pyOpenSSL supplied method constants since will not longer be useful and not all of them (SSLv23_METHOD) will make sense with this API. These are good things since CertificateOptions is not supposed to be OpenSSL-specific.

1: Of course, SSLv2 is disabled in most real-world OpenSSL builds now so all mentions of SSLv2 in this description are for the sake of completeness. It's most likely not possible to ever negotiate this protocol anymore - nor should it be.
2: Prior to the merge of #6772 it actually negotiates only TLSv1.0.
3: Or 15 if we disregard SSLv2

Change History (13)

comment:1 Changed 3 years ago by Jean-Paul Calderone

Description: modified (diff)

comment:2 Changed 3 years ago by Jean-Paul Calderone

Description: modified (diff)

comment:3 Changed 3 years ago by Jean-Paul Calderone

Description: modified (diff)

comment:4 Changed 3 years ago by Hynek Schlawack

Cc: Hynek Schlawack added

comment:5 Changed 2 years ago by Glyph

Apropos of some of my comments on #7752, here's a proposal:

from twisted.python.constants import Names, NamedConstant
class TLSVersion(Names):
    TLSv1 = NamedConstant()
    TLSv1_1 = NamedConstant()
    TLSv1_2 = NamedConstant()

class KnownVulnerableVersion(Names):
    SSLv2 = NamedConstant()
    SSLv3 = NamedConstant()

_versionOrdering = (list(KnownVulnerableVersion.iterconstants()) +

# these will all need fallbacks for older pyOpenSSL probably
                         OP_NO_SSLv2, OP_NO_SSLv3, OP_NO_TLSv1, OP_NO_TLSv1_1,
                         OP_NO_TLSv1_2, Context)

_disableFlags = {
    KnownVulnerableVersion.SSLv2: OP_NO_SSLv2,
    KnownVulnerableVersion.SSLv3: OP_NO_SSLv3,

    TLSVersion.TLSv1: OP_NO_TLSv1,
    TLSVersion.TLSv1_1: OP_NO_TLSv1_1,
    TLSVersion.TLSv1_2: OP_NO_TLSv1_2,

def versionRange(minimum, maximum=TLSVersion.TLSv1_2):
    left = _versionOrdering.index(minimum)
    right = _versionOrdering.index(maximum)+1
    return set(_versionOrdering[left:right])


def _contextAndOptions(versions):
    pyOpenSSL does not expose SSL_set_ssl_method or SSL_get_options, so there's
    literally no way to represent this sanely.  We can't fully create a context
    because then the caller can't modify the options, and we can't modify an
    existing context because we can't change its method *or* read its existing
    ctx = Context(NEGOTIATE_VERSIONS)
    options = 0x0000
    for version in set(_versionOrdering) - set(versions):
        options |= _disableFlags[version]
    return ctx, options

comment:6 Changed 2 years ago by Hynek Schlawack

I’m not happy with the division between class TLSVersion(Names) and class KnownVulnerableVersion(Names): since those sets are in no way final.

Can’t we have

class TLSVersion(Names):
    SSLv2 = NamedConstant()
    SSLv3 = NamedConstant()
    TLSv1 = NamedConstant()
    TLSv1_1 = NamedConstant()
    TLSv1_2 = NamedConstant()

and sort out the current vulnerabilities elsewhere?

Or is it by design, that software that is written using TLSVersion.TLSv1 today breaks and has to be manually fixed once TLSv1 wanders over to KnownVulnerableVersion?

In any case, I’d guess that 99% of users will only use the default anyways.

comment:7 Changed 2 years ago by David Petar Novakovic

Cc: David Petar Novakovic added

comment:8 Changed 2 months ago by hawkowl

Branch: 6800-certificate-options-protocols

comment:9 Changed 2 months ago by hawkowl

Keywords: review added; ssl removed

Took a stab at it, with an API that lukasa proposed.

Please review.

comment:10 Changed 2 months ago by Jean-Paul Calderone

Keywords: review removed

comment:11 Changed 2 months ago by hawkowl

Keywords: review added

I think this is good for another review cycle.

comment:12 Changed 6 weeks ago by Glyph

Keywords: review removed
Owner: set to hawkowl

Reviewed, and approved, modulo small changes -

comment:13 Changed 6 weeks ago by Amber Brown <hawkowl@…>

Resolution: fixed
Status: newclosed

In eb7e522c:

Merge 6800-certificate-options-protocols: Add TLS version selection to CertificateOptions

Author: hawkowl
Reviewer: exarkun, glyph
Fixes: ticket:6800

Note: See TracTickets for help on using tickets.