OpenSSLCertificateOptions contains asserts

[Hynek Schlawack]

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.

[Jean-Paul Calderone]

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.

[Hynek Schlawack]

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.

[Tom Prince]

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

[Tom Prince]

This looks good. I'll merge.

[Tom Prince]

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