Opened 9 years ago
Closed 9 years ago
#6288 defect closed fixed (fixed)
OpenSSLCertificateOptions contains asserts
Reported by: | Hynek Schlawack | Owned by: | Tom Prince |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | core | Keywords: | |
Cc: | Hynek Schlawack | Branch: |
branches/remove-asserts-6288
branch-diff, diff-cov, branch-cov, buildbot |
Author: | hynek |
Description
There are two assert
s in the constructor of twisted.internet._sslverify.OpenSSLCertificateOptions
.
Additionally the second assert
is not in sync with the documentation which says for caCerts
:
Only used if verify is True, and if verify is True, this must be specified.
It checks only for caCerts
being set if verify
is True
though.
This also begs the question what the verify
option is good for anyway. Interestingly, the only place where self.verify
is used is _makeContext
and caCert
is checked before usage:
if self.verify: verifyFlags = SSL.VERIFY_PEER if self.requireCertificate: verifyFlags |= SSL.VERIFY_FAIL_IF_NO_PEER_CERT if self.verifyOnce: verifyFlags |= SSL.VERIFY_CLIENT_ONCE if self.caCerts: store = ctx.get_cert_store() for cert in self.caCerts: store.add_cert(cert)
So either the docs are wrong or this code can be simplified.
I’ll attach a proposed patch which assumes the docs to be correct thus making it possible to optimize that if
away and tighten the parameter check. The tests don't mind.
Attachments (2)
Change History (8)
Changed 9 years ago by
Attachment: | remove-asserts-6288.patch added |
---|
comment:1 Changed 9 years ago by
Keywords: | review added |
---|---|
Type: | enhancement → defect |
comment:2 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Hynek Schlawack |
Changed 9 years ago by
Attachment: | remove-asserts-6288-v2.patch added |
---|
comment:3 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Hynek Schlawack deleted |
Thanks for the feedback!
I've attached another attempt which also clarifies the doc string a bit – and matches the style of verify. It’s purely syntax in the latter case though (and the reflow is has been caused by adding proper epydoc markup, not the indent).
Let me know what you think.
comment:4 Changed 9 years ago by
Author: | → tomprince |
---|---|
Branch: | → branches/remove-asserts-6288 |
(In [37135]) Branching to remove-asserts-6288.
comment:5 Changed 9 years ago by
Author: | tomprince → hynek |
---|---|
Keywords: | review removed |
Owner: | set to Tom Prince |
This looks good. I'll merge.
comment:6 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
Thanks.
From the description:
This doesn't seem right to me. The logic seemed to make sense before, and the documentation agreed with the implementation (at least the non-assert part of the implementation).
If it helps, here's a re-phrasing. If verify is False, caCerts will be ignored. If verify is True, if a caCerts value is given, it will be used.
Apart from that, it's better if test methods only make one assertion (so you can see all the problems after a single test run), and the last statement of
test_constructorEnforcesNeitherOrBothCaCertsAndVerify
should be an assertion, not creation of an object that is then thrown away without being inspected at all.