Opened 4 years ago

Closed 4 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 asserts 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)

remove-asserts-6288.patch (6.9 KB) - added by Hynek Schlawack 4 years ago.
remove-asserts-6288-v2.patch (11.2 KB) - added by Hynek Schlawack 4 years ago.

Download all attachments as: .zip

Change History (8)

Changed 4 years ago by Hynek Schlawack

Attachment: remove-asserts-6288.patch added

comment:1 Changed 4 years ago by Hynek Schlawack

Keywords: review added
Type: enhancementdefect

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

Keywords: review removed
Owner: set to Hynek Schlawack

Thanks.

From the description:

So either the docs are wrong or this code can be simplified.

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.

Changed 4 years ago by Hynek Schlawack

comment:3 Changed 4 years ago by Hynek Schlawack

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 4 years ago by Tom Prince

Author: tomprince
Branch: branches/remove-asserts-6288

(In [37135]) Branching to remove-asserts-6288.

comment:5 Changed 4 years ago by Tom Prince

Author: tomprincehynek
Keywords: review removed
Owner: set to Tom Prince

This looks good. I'll merge.

comment:6 Changed 4 years ago by Tom Prince

Resolution: fixed
Status: newclosed

(In [37139]) Merge remove-asserts-6288: Remove asserts from OpenSSLCertificateOptions.

Author: hynek Reviewers: exarkun, tom.prince Fixes: #6288

This replaces them with raising ValueError in the appropriate cases. This avoids the checks being removed when running with -O.

Note: See TracTickets for help on using tickets.