SSL server endpoints string syntax results in creation of DefaultOpenSSLContextFactory

[Itamar Turner-Trauring]

twisted.internet.endpoints._parseSSL is the function used to parse string syntax for SSL endpoints. It creates a DefaultOpenSSLContextFactory. A more useful thing to do would be to create a twisted.internet.ssl.CertificateOptions instance, since that is going to be where we concentrate future development (e.g. #2061).

[Hynek Schlawack]

Move to CertificateOptions, push test coverage of contractor.

[Richard Wall]

Reviewing...

[Richard Wall]

(In [37302]) Branching to 'sslendpoint-certificateoptions-6286'

[Itamar Turner-Trauring]

Note that IIRC current ssl endpoint defaults to SSLv23 (backwards compat TLS1+SSL3) with SSL2 disabled. New code in patch defaults to TLSv1 method. So that should be fixed if my memory is correct.

[Hynek Schlawack]

Itamar is right.

But since TLSv1 is from 1999…what would it take to make it the (more secure…) default for such an high-level function?

[Richard Wall]

Hynek,

Here's a quick (inexpert) review of your patch.

1. Patch applied cleanly to a new branch (source:branches/sslendpoint-certificateoptions-6286)
2. Missing news file (I added one to the branch in r37304)
3. ​Build results look fine
4. See Itamar's comment (above)
   1. He's right, the old default was ​sslmethod=SSL.SSLv23_METHOD
   2. New default will be ​TLSv1_METHOD
   3. So that needs changing for backwards compatibility
   4. And maybe a test to demonstrate that SSL.SSLv23_METHOD is the desired default.

Please address point 4 and resubmit for review.

Thanks.

-RichardW.

[Hynek Schlawack]

Thank you!

FTR: We need to fix #6337 before we can use SSLv23 by default.

[Hynek Schlawack]

Same as before, using SSLv23 as default method.

[Jean-Paul Calderone]

The patch has a bug in the skip definition.

[Hynek Schlawack]

Brownbag fix of skip definition.

[Hynek Schlawack]

Thanks JP, fixed!

[Tom Prince]

As a note, when attaching a patch against a ticket that has a branch, please make the patch against the branch, rather than trunk. (I've dealt with it here already)

[Tom Prince]

The build results look good. The code change look correct.

This code looks like it enables SSL.OP_SINGLE_DH_USE and sets a session-id, which DefaultOpenSSLContextFactory doesn't. I suspect neither of these changes is going to be an issue, but I'm not sure.

[Jean-Paul Calderone]

I'm not worried about either of those changes. They're minimally disruptive (ie, neither of them will make connections stop succeeding or compromise any of the security properties of those connections). Applications that require such precise control over SSL parameters are surely already using their own context factories anyway.

[Tom Prince]

<exarkun> I only glanced at the actual code changes.  If you're happy with those, I'm happy with the SSL changes you pointed out would be made.
<tomprince> k.
<tomprince> I *think* those are only differences between the two factories. (and the verify depth, but the CertificatOptions docstring claims it sets it to the default)
<exarkun> Hum
<exarkun> CertificateOptions also disables session tickets by default (increases interoperability)
<exarkun> Uh
<exarkun> Oh
<exarkun> and CertificateOptions has lots of confusing verification logic
<exarkun> requireCertificate is True
<exarkun> but verify is False
<exarkun> and caCerts is None
<exarkun> okay, which means it doesn't require a cert, which is the same
<exarkun> uhh cert chains, why are you such a mess
<exarkun> eh I'm pretty sure the DefaultOpenSSLContextFactory use_certificate_file call has the same meaning as the CertificateOptions use_certificate call
<exarkun> does loadPEM work with two PEMs smashed together?
<exarkun> yea looks like
<exarkun> so probably fine
<exarkun> although looking at the code
<exarkun> PrivateCertificate.load(certPEM + keyPEM).options(...) is probably a somewhat nicer spelling

[Hynek Schlawack]

I’m not entirely sure how to proceed here since the review tag has been removed without further actual criticism. Is it ready to merge or should I change something?

I’m not sure what to make of

<exarkun> PrivateCertificate.load(certPEM + keyPEM).options(...) is probably a somewhat nicer spelling

Is it about the order of the concatenation? If so, I’ll attach a quick fix.

[Hynek Schlawack]

Use a more natural concatenation order.

[Jean-Paul Calderone]

Is it about the order of the concatenation? If so, I’ll attach a quick fix.

No, the order doesn't matter. It's about using PrivateCertificate.options instead of CertificateOptions.

[Hynek Schlawack]

As per discussion on IRC, the approach with PrivateCertificate.options doesn’t work with the current API.

Since nothing is left open, merging would be appreciated.

[Jean-Paul Calderone]

This code is correct:

    cf = ssl.CertificateOptions(
        privateKey=ssl.PrivateCertificate.loadPEM(
            keyPEM + certPEM).privateKey.original,
        certificate=ssl.Certificate.loadPEM(certPEM).original, **kw)

but quite a mess. Please clean it up as much as possible (eg, split it into multiple statements and by not loading certPEM twice). Also file a ticket about how PrivateCertificate.options is greatly hampered in its utility by the lack of support for any options other than certificate authorities.

Otherwise, fine, please merge.

[Hynek Schlawack]

Untangled as per jp’s request.

[Hynek Schlawack]

Changes done, ticket filed.

[Tom Prince]

(In [37463]) Merge sslendpoint-certificateoptions-6286: Use twisted.internet.ssl.CertificateOptions in strports.

Author: hynek Reviewers: rwall, tom.prince, exarkun Fixes: #6286 Refs: #2601, #6377

twisted.internet.endpoints._parseSSL is the function used to parse string syntax for SSL endpoints. It creates a DefaultOpenSSLContextFactory. A more useful thing to do would be to create a twisted.internet.ssl.CertificateOptions instance, since that is going to be where we concentrate future development (e.g. #2061).

[Tom Prince]

This looks good.