Opened 16 years ago

Closed 13 years ago

#2153 defect closed fixed (fixed)

Remove hard dependency on pyopenssl from t.p.amp

Reported by: paulg Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: therve Branch: branches/amp-without-ssl-2153-2
branch-diff, diff-cov, branch-cov, buildbot
Author: glyph

Description

as of right now, you can't load t.p.amp if you don't have pyopenssl installed. imho, it'd be best if those who didn't need tls didn't need to satisfy this dep.

cheers, -p

Change History (30)

comment:1 Changed 16 years ago by Glyph

Priority: normallowest

This should probably be fixed, but I personally don't care (I have openssl installed everywhere, even in the embedded environment amp is deployed in).

comment:2 Changed 15 years ago by therve

Owner: changed from Glyph to therve

comment:3 Changed 15 years ago by therve

(In [21492]) Refactor ssl imports and skip ssl tests.

Refs #2153

comment:4 Changed 15 years ago by therve

Cc: therve added
Keywords: review added
Owner: therve deleted
Priority: lowesthighest

This is ready to review in amp-without-ssl-2153. Of course, the problem is that we don't have a buildbot slave without PyOpenSSL. Maybe we should add one at some point...

comment:5 Changed 15 years ago by therve

Keywords: review removed
Owner: set to therve
Priority: highesthigh

exarkun is tough with me.

comment:6 Changed 15 years ago by therve

(In [21493]) Manage lack of ssl in StartTLS

Refs #2153

comment:7 Changed 15 years ago by therve

Keywords: review added
Owner: therve deleted
Priority: highhighest

This is back to review in amp-without-ssl-2153.

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

Keywords: review removed
Owner: set to therve

problemsFromTransport is obsolete now, it seems - connectionLost will be called with an OpenSSL exception (or, sometimes, a Failure - blech; unrelated), so AMP doesn't need _sslVerifyProblems or problemsFromTransport anymore. Not sure if you want to try to tackle that in this branch. If so, cool; if not, maybe just keep using problemsFromTransport from _sslverify and one of us (or, who knows! maybe someone else) can get rid of it entirely for a separate ticket (file one if you don't fix itt here).

As mentioned on the channel, it seems better if callRemote(StartTLS, ...) would return a Deferred which fails with RuntimeError, rather than raising it synchronously.

comment:9 Changed 15 years ago by therve

(In [21603]) Return a failure instead of raising an exception.

Refs #2153

comment:10 Changed 15 years ago by therve

Keywords: review added
Owner: therve deleted

I don't understand the ssl problem enough for now, I opened #2874. Back to review.

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

Keywords: review removed
Owner: set to therve

If we don't put problemsFromTransport into twisted.internet.ssl, we won't have to bother with a deprecation before removing it. :) It still seems better if amp keeps importing it from the private location until we fix it completely. Does that sound reasonable?

I hacked doc/core/examples/amp{server,client}.py to test the no-SSL case. If the client is missing SSL support, it behaves basically how one would hope. If the server is missing SSL support though, it gets pretty messy. The server logs this traceback:

Traceback (most recent call last):
  File "/home/exarkun/Projects/Twisted/trunk/twisted/protocols/basic.py", line 416, in stringReceived
    self.state = statehandler(string)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/protocols/amp.py", line 1790, in proto_key
    self.ampBoxReceived(self._currentBox)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/protocols/amp.py", line 702, in ampBoxReceived
    self.dispatchCommand(box).addCallbacks(
  File "/home/exarkun/Projects/Twisted/trunk/twisted/internet/defer.py", line 187, in addCallbacks
    self._runCallbacks()
--- <exception caught here> ---
  File "/home/exarkun/Projects/Twisted/trunk/twisted/internet/defer.py", line 325, in _runCallbacks
    self.result = callback(self.result, *args, **kw)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/protocols/amp.py", line 677, in sendAnswer
    answerBox._sendTo(self)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/protocols/amp.py", line 1394, in _sendTo
    proto._startTLS(self.certificate, self.verify)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/protocols/amp.py", line 1692, in _startTLS
    self.transport.startTLS(certificate.options(*verifyAuthorities))
  File "/home/exarkun/Projects/Twisted/trunk/twisted/protocols/amp.py", line 1355, in options
    sharedDN = ssl.DN(CN='TEMPORARY CERTIFICATE')
exceptions.AttributeError: 'NoneType' object has no attribute 'DN'

and the client logs this:

2007-11-06 08:25:09-0500 [AMP,client] AMP connection lost (HOST:IPv4Address(TCP, '127.0.0.1', 55500) PEER:IPv4Address(TCP, '127.0.0.1', 1234))
2007-11-06 08:25:09-0500 [AMP,client] Unhandled Error
        Traceback (most recent call last):
        Failure: twisted.internet.error.PeerVerifyError: Peer rejected our certificate for an unknown reason.

It'd be better for the StartTLS command to be rejected with a meaningful error, instead of letting negotiation begin and then fail.

comment:12 Changed 15 years ago by Jean-Paul Calderone

Oh, and I got distracted and forgot to mention that test_callRemoteError doesn't return the Deferred it uses with assertFailure (like a couple other tests in that module incorrectly do as well).

comment:13 Changed 15 years ago by therve

(In [21648]) Manage lack of ssl in server, move problemsFromTransport back.

Refs #2153

comment:14 Changed 15 years ago by therve

Keywords: review added
Owner: therve deleted

Back to review in amp-without-ssl-2153.

comment:15 Changed 15 years ago by Jean-Paul Calderone

Owner: set to Jean-Paul Calderone
Status: newassigned

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

Keywords: review removed

I always feel bad when I look at AMP code. I think this is because it's very unclear what the intended extension points are in the API. I think I have a slightly better understanding of where the intended places in the API for doing customization are (just because I've talked to glyph face to face about this code a fair bit), so I'm going to try for a solution which better fits with those ideas. If I can, I'll also document some of them, but I dunno how well that will turn out.

comment:17 Changed 15 years ago by Jean-Paul Calderone

Keywords: review added
Owner: changed from Jean-Paul Calderone to Glyph
Status: assignednew

Nevermind. AMP is too misfactored to implement this in any way which doesn't suck. I'm not motivated to tackle the massive task of actually improving it.

comment:18 Changed 15 years ago by Glyph

Keywords: review removed
Status: newassigned

#2810 should radically improve AMP's factoring. I'm not sure it's sufficient for this branch, but it should at least clear out enough of the brush to allow us to refactor the TLS box stuff as well. I'll have another look at it after that.

comment:19 Changed 14 years ago by Glyph

I think we can implement SSL in AMP with only one import from twisted.internet.ssl.

For connectionLost, I think the right thing to do would be to make problemsFromTransport a method, let's say, checkTLS (name suggestions welcome) on ITLSTransport, and then check to see if ITLSTransport is provided by the transport.

Similarly, Certificate.peerFromTransport is probably backwards. ITLSTransport providers should know about the SSL implementation, but the SSL implementation shouldn't really have to know about transports. This is the direction that the imports go in, since posixbase imports ssl but _sslverify imports nothing from internet except defer and error. peerFromTransport and hostFromTransport could be ITLSTransport.peerCertificate and hostCertificate; then AMP would simply relay that property from protocol to transport for compatibility.

The default StartTLS responder should be registered dynamically on the AMP instance in makeConnection. Right now there's no facility for instance-level registration of responders, but that would be fairly easy to implement. (This is the one thing that actually was made easier by #2810.) This is, however, purely for compatibility, and it might be good to deprecate it. StartTLS is a pretty lame command; the actual SSL support should involve actual verification and authentication, i.e. real security. Separate ticket and all that though.

(Implemented this way, regardless of the command used, in the cases where SSL wasn't supported, rather than having a new SSL_ERROR error type, users would simply get UNHANDLED, which is a more sensible default for "you can't do that".)

_NoCertificate would then be the only thing left that actually imports stuff from twisted.internet.ssl. It seems like generating a temporary, self-signed PrivateCertificate ought to be the sort of thing that could be directly supported by the ssl module, rather than importing DN and KeyPair and so on. It does seem to be a bit of a stretch to put that on the transport, but maybe not. If there were a default that made sense on the transport, then we could get it down to zero imports, and perhaps establish a more sensible idiom for optional SSL support for other protocols.

Thoughts?

comment:20 Changed 14 years ago by Glyph

Owner: changed from Glyph to therve
Status: assignednew

comment:21 in reply to:  19 ; Changed 14 years ago by therve

Owner: changed from therve to Glyph

Replying to glyph:

I think we can implement SSL in AMP with only one import from twisted.internet.ssl.

For connectionLost, I think the right thing to do would be to make problemsFromTransport a method, let's say, checkTLS (name suggestions welcome) on ITLSTransport, and then check to see if ITLSTransport is provided by the transport.

That's seems reasonable. For the name, maybe something that indicate errors would be better. checkTLSErrors ? Also, I wondered one thing: is there a real possibility to get multiple errors on one transport ?

Similarly, Certificate.peerFromTransport is probably backwards. ITLSTransport providers should know about the SSL implementation, but the SSL implementation shouldn't really have to know about transports. This is the direction that the imports go in, since posixbase imports ssl but _sslverify imports nothing from internet except defer and error. peerFromTransport and hostFromTransport could be ITLSTransport.peerCertificate and hostCertificate; then AMP would simply relay that property from protocol to transport for compatibility.

Agreed.

The default StartTLS responder should be registered dynamically on the AMP instance in makeConnection. Right now there's no facility for instance-level registration of responders, but that would be fairly easy to implement. (This is the one thing that actually was made easier by #2810.) This is, however, purely for compatibility, and it might be good to deprecate it. StartTLS is a pretty lame command; the actual SSL support should involve actual verification and authentication, i.e. real security. Separate ticket and all that though.

OK, this is little bit less clear than the rest, but I trust of the feasibility of the thing :)

(Implemented this way, regardless of the command used, in the cases where SSL wasn't supported, rather than having a new SSL_ERROR error type, users would simply get UNHANDLED, which is a more sensible default for "you can't do that".)

_NoCertificate would then be the only thing left that actually imports stuff from twisted.internet.ssl. It seems like generating a temporary, self-signed PrivateCertificate ought to be the sort of thing that could be directly supported by the ssl module, rather than importing DN and KeyPair and so on. It does seem to be a bit of a stretch to put that on the transport, but maybe not. If there were a default that made sense on the transport, then we could get it down to zero imports, and perhaps establish a more sensible idiom for optional SSL support for other protocols.

We could probably move some stuff in the ssl module, yes. Although, it seems to me that NoCertificate.options is only accessed once we are in a ssl context, so it probably matters less to remove ssl thing from it. The thing done there is a bit specific, I'm not sure it'll fit so well on the transport.

What are the next step? Probably create tickets for first 2 points?

comment:22 in reply to:  21 Changed 14 years ago by Glyph

Replying to therve:

What are the next step? Probably create tickets for first 2 points?

Please do. I think they should be straightforward / uncontroversial to get merged, except for the whole "adding methods to interfaces" thing, but hopefully we can keep handwringing about that to a minimum... are there even any external implementations of this (does the M2Crypto thing use it?)

comment:23 Changed 14 years ago by Jean-Paul Calderone

There's no reason for {{problemsFromTransport}}} to continue to exist. Please don't make it a method of anything. Instead, just remove it entirely.

comment:24 in reply to:  23 Changed 14 years ago by Glyph

Replying to exarkun:

There's no reason for {{problemsFromTransport}}} to continue to exist. Please don't make it a method of anything. Instead, just remove it entirely.

Sorry; got carried away there just thinking about how to smoosh the code around. The previous comment on this ticket, plus ticket #2874, indicate the correct course of action. The relevant, closed ticket describing when it became redundant and unnecessary is #778.

comment:25 Changed 13 years ago by Glyph

Author: glyph
Branch: branches/amp-without-ssl-2153-2

(In [26531]) Branching to 'amp-without-ssl-2153-2'

comment:26 Changed 13 years ago by Glyph

Keywords: review added
Owner: Glyph deleted

Builds should be at http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/amp-without-ssl-2153-2

This is a simpler implementation, since we can completely forget about problemsFromTransport. As JP suggested, errors are reported in such a way that you can tell what's working on.

comment:27 Changed 13 years ago by therve

Keywords: review removed
Owner: set to Glyph

Please update copyright notices and merge. Thanks!

(Got a successful build in http://buildbot.twistedmatrix.com/builders/py2.5-without-modules/builds/91)

comment:28 Changed 13 years ago by Glyph

(In [26535]) re #2153 Update copyright statements.

comment:29 Changed 13 years ago by Glyph

Resolution: fixed
Status: newclosed

(In [26536]) Allow AMP to be imported without PyOpenSSL installed.

Since OpenSSL is not available everywhere, this lets you use AMP in more environments.

Author: glyph

Reviewer: therve

Fixes #2153

comment:30 Changed 11 years ago by <automation>

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