Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#6286 enhancement closed fixed (fixed)

SSL server endpoints string syntax results in creation of DefaultOpenSSLContextFactory

Reported by: Itamar Turner-Trauring Owned by: Tom Prince
Priority: normal Milestone:
Component: core Keywords:
Cc: Hynek Schlawack, Richard Wall Branch: branches/sslendpoint-certificateoptions-6286
branch-diff, diff-cov, branch-cov, buildbot
Author: hynek

Description

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).

Attachments (5)

move-_parseSSL-to-CertificateOptions-6286.patch (6.2 KB) - added by Hynek Schlawack 5 years ago.
Move to CertificateOptions, push test coverage of contractor.
move-_parseSSL-to-CertificateOptions-6286-v2.patch (6.4 KB) - added by Hynek Schlawack 5 years ago.
Same as before, using SSLv23 as default method.
move-_parseSSL-to-CertificateOptions-6286-v3.patch (6.7 KB) - added by Hynek Schlawack 5 years ago.
Brownbag fix of skip definition.
fix-concat-order-6286.patch (2.2 KB) - added by Hynek Schlawack 5 years ago.
Use a more natural concatenation order.
untangle-6286.patch (3.0 KB) - added by Hynek Schlawack 5 years ago.
Untangled as per jp’s request.

Download all attachments as: .zip

Change History (31)

comment:1 Changed 5 years ago by Hynek Schlawack

Cc: Hynek Schlawack added

Changed 5 years ago by Hynek Schlawack

Move to CertificateOptions, push test coverage of contractor.

comment:2 Changed 5 years ago by Hynek Schlawack

Keywords: review added

comment:3 Changed 5 years ago by Tom Prince

Owner: set to Jean-Paul Calderone

comment:4 Changed 5 years ago by Richard Wall

Owner: changed from Jean-Paul Calderone to Richard Wall
Status: newassigned

Reviewing...

comment:5 Changed 5 years ago by Richard Wall

Author: rwall
Branch: branches/sslendpoint-certificateoptions-6286

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

comment:6 Changed 5 years ago by 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.

comment:7 Changed 5 years ago by 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?

comment:8 Changed 5 years ago by Richard Wall

Cc: Richard Wall added
Keywords: review removed
Owner: changed from Richard Wall to Hynek Schlawack
Status: assignednew

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.

comment:9 Changed 5 years ago by Hynek Schlawack

Thank you!

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

Changed 5 years ago by Hynek Schlawack

Same as before, using SSLv23 as default method.

comment:10 Changed 5 years ago by Hynek Schlawack

Keywords: review added
Owner: Hynek Schlawack deleted

comment:11 Changed 5 years ago by Jean-Paul Calderone

The patch has a bug in the skip definition.

Changed 5 years ago by Hynek Schlawack

Brownbag fix of skip definition.

comment:12 Changed 5 years ago by Hynek Schlawack

Thanks JP, fixed!

comment:13 Changed 5 years ago by 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)

comment:14 Changed 5 years ago by Tom Prince

Author: rwallhynek
Owner: set to zzccflpccl

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.

comment:15 Changed 5 years ago by Tom Prince

Owner: changed from zzccflpccl to Jean-Paul Calderone

comment:16 Changed 5 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Hynek Schlawack

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.

comment:17 Changed 5 years ago by 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

comment:18 Changed 5 years ago by 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.

Changed 5 years ago by Hynek Schlawack

Attachment: fix-concat-order-6286.patch added

Use a more natural concatenation order.

comment:19 Changed 5 years ago by 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.

comment:20 Changed 5 years ago by Hynek Schlawack

Keywords: review added

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.

comment:21 Changed 5 years ago by Hynek Schlawack

Owner: Hynek Schlawack deleted

comment:22 Changed 5 years ago by Tom Prince

Owner: set to Jean-Paul Calderone

comment:23 Changed 5 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Tom Prince

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.

Changed 5 years ago by Hynek Schlawack

Attachment: untangle-6286.patch added

Untangled as per jp’s request.

comment:24 Changed 5 years ago by Hynek Schlawack

Keywords: review added

Changes done, ticket filed.

comment:25 Changed 5 years ago by Tom Prince

Resolution: fixed
Status: newclosed

(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).

comment:26 Changed 5 years ago by Tom Prince

Keywords: review removed

This looks good.

Note: See TracTickets for help on using tickets.