#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)
Change History (31)
comment:1 Changed 9 years ago by
Cc: | Hynek Schlawack added |
---|
Changed 9 years ago by
Attachment: | move-_parseSSL-to-CertificateOptions-6286.patch added |
---|
comment:2 Changed 9 years ago by
Keywords: | review added |
---|
comment:3 Changed 9 years ago by
Owner: | set to Jean-Paul Calderone |
---|
comment:4 Changed 9 years ago by
Owner: | changed from Jean-Paul Calderone to Richard Wall |
---|---|
Status: | new → assigned |
Reviewing...
comment:5 Changed 9 years ago by
Author: | → rwall |
---|---|
Branch: | → branches/sslendpoint-certificateoptions-6286 |
(In [37302]) Branching to 'sslendpoint-certificateoptions-6286'
comment:6 Changed 9 years ago by
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 9 years ago by
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 9 years ago by
Cc: | Richard Wall added |
---|---|
Keywords: | review removed |
Owner: | changed from Richard Wall to Hynek Schlawack |
Status: | assigned → new |
Hynek,
Here's a quick (inexpert) review of your patch.
- Patch applied cleanly to a new branch (source:branches/sslendpoint-certificateoptions-6286)
- Missing news file (I added one to the branch in r37304)
- Build results look fine
- See Itamar's comment (above)
- He's right, the old default was sslmethod=SSL.SSLv23_METHOD
- New default will be TLSv1_METHOD
- So that needs changing for backwards compatibility
- 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 9 years ago by
Thank you!
FTR: We need to fix #6337 before we can use SSLv23
by default.
Changed 9 years ago by
Attachment: | move-_parseSSL-to-CertificateOptions-6286-v2.patch added |
---|
Same as before, using SSLv23 as default method.
comment:10 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Hynek Schlawack deleted |
Changed 9 years ago by
Attachment: | move-_parseSSL-to-CertificateOptions-6286-v3.patch added |
---|
Brownbag fix of skip definition.
comment:13 Changed 9 years ago by
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 9 years ago by
Author: | rwall → hynek |
---|---|
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 9 years ago by
Owner: | changed from zzccflpccl to Jean-Paul Calderone |
---|
comment:16 Changed 9 years ago by
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 9 years ago by
<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 9 years ago by
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 9 years ago by
Attachment: | fix-concat-order-6286.patch added |
---|
Use a more natural concatenation order.
comment:19 Changed 9 years ago by
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 9 years ago by
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 9 years ago by
Owner: | Hynek Schlawack deleted |
---|
comment:22 Changed 9 years ago by
Owner: | set to Jean-Paul Calderone |
---|
comment:23 Changed 9 years ago by
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.
comment:25 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
(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).
Move to CertificateOptions, push test coverage of contractor.