Opened 16 years ago

Closed 16 years ago

#2171 defect closed fixed (fixed)

Bad ssl check

Reported by: ilia Owned by:
Priority: highest Milestone:
Component: words Keywords:
Cc: ilia, spiv, Ralph Meijer Branch:
Author:

Description

     if self.wanted:
            if not ssl.supported:
"twisted/words/protocols/jabber/xmlstream.py" 
line:326

should be

     if self.wanted:
            if not ssl or not ssl.supported:

To properly handle 'no ssl' case

Change History (12)

comment:1 Changed 16 years ago by spiv

Cc: spiv added

Actually, I think it should be simply

if ssl is None:

instead of

if not ssl.supported:

The way ssl is imported will ensure it is None if it's not supported.

comment:2 Changed 16 years ago by Ralph Meijer

Keywords: review added
Owner: changed from Jean-Paul Calderone to spiv
Priority: highhighest

Fixed in source:branches/xmlstream-ssl-import-2171. This time, I made sure tests pass both when OpenSSL is there, and when not. We really should have a buildbot slave with OpenSSL not installed.

Please review.

comment:3 Changed 16 years ago by Ralph Meijer

Cc: ilia Ralph Meijer added

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

I'd like to see some work on #1795, since without a buildslave testing this configuration it is unlikely to remain working for very long. It's also of somewhat questionable value to have only /some/ parts of Twisted work without OpenSSL. As long as use is restricted to a narrow area, this might be workable, but who knows when you'll stumble on something that falls over.

comment:5 Changed 16 years ago by Ralph Meijer

As stated, I would welcome such work.

Depending on the XML server configuration, TLS negotiation may not be required, so we can simply establish a connection if OpenSSL were not available. I thought it to be nice to make it work like that. The tests for the case that OpenSSL is not available at least work in the case it actually is available, faking its absense. I just made a small error in the implementation that is now corrected.

Or is your point that OpenSSL should simply be required for Twisted Core?

Also, do your comments affect the review of this ticket?

comment:6 Changed 16 years ago by ilia

Maybe you mean the same, in my case, SSL is not needed twisted.words jabber connections. How to disable it?

comment:7 in reply to:  5 Changed 16 years ago by Jean-Paul Calderone

Replying to ralphm:

As stated, I would welcome such work.

:)

Depending on the XML server configuration, TLS negotiation may not be required, so we can simply establish a connection if OpenSSL were not available. I thought it to be nice to make it work like that. The tests for the case that OpenSSL is not available at least work in the case it actually is available, faking its absense. I just made a small error in the implementation that is now corrected.

Quite so, but automated testing is needed to ensure it remains fixed.

Or is your point that OpenSSL should simply be required for Twisted Core?

Nah.

Also, do your comments affect the review of this ticket?

I haven't looked at the code in the branch. I'm just pointing out that without automated tests, few of our development procedures make sense.

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

Keywords: review removed
Owner: changed from spiv to Ralph Meijer

Glancing at this ticket again, I remembered why ssl.supported exists. The change in this branch won't work reliably on Python 2.3, where it is possible to import twisted.internet.ssl even when OpenSSL is present.

So you will need to do both checks, or otherwise fix the detection code, in order to support Python 2.3 and later versions.

comment:9 Changed 16 years ago by Ralph Meijer

Keywords: review added
Owner: Ralph Meijer deleted

Actually, I was already using the idiom as documented in t.i.ssl:

try:
    from twisted.internet import ssl
except ImportError:
    ssl = None
if ssl and not ssl.supported:
    ssl = None

However, I wasn't inspecting the ssl variable properly, which this change should fix.

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

Keywords: review removed
Owner: set to Ralph Meijer

Change looks okay, except SSL is an acronym; all lower-case is okay sometimes, but Ssl is weird. Should be changed to SSL in savedSsl.

Not exactly sure what the scope of this ticket is meant to be. I suppose it is to make the jabber tests pass in the absence of OpenSSL, but since the jabber tests are mixed in with other words tests and running them in isolation is hard, this goal underwhelms, since test_msn fails if OpenSSL isn't present.

Also, an only slightly related point: importing OpenSSL to get TLSv1_METHOD is unnecessary, since TLSv1_METHOD is the default for CertificateOptions. Changing this doesn't quite do enough to allow testWantedSupported to pass without OpenSSL, but it should be possible to to provide enough of the CertificateOptions and TLS APIs so that it could pass without OpenSSL. What the point of this would be, I'm not sure, since you can't actually use this code without OpenSSL, so it doesn't really matter if the tests pass or run.

Anyhow, just fix the capitalization issue and then the branch is fine to merge, unless you feel like fixing test_msn as well (then put it up for review again afterwards).

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

Resolution: fixed
Status: newclosed

Fixed by r18506

comment:12 Changed 11 years ago by <automation>

Owner: Ralph Meijer deleted
Note: See TracTickets for help on using tickets.