StartTLS doesn't work in AMP when both peers are TCP clients.

[Stas Shtin]

In my application clients are connected to server that sometimes acts as a proxy for them. Secure communication is one of requirements, and data shouldn't be readable by server as well. When I send AMP's StartTLS command that should go from client A to client B through the proxy, I get an error from OpenSSL. After looking at TCP dump, I've found out that both clients are starting TLS connection by sending client hello, which leads to "Bad message type" failure.

In my opinion, AMP should determine whether a peer should act as client or server for TLS connections depending on it being sender or receiver of StartTLS command, but not client or server for the TCP connection as it currently is. Otherwise it can't be used in non-trivial setups without monkey-patching.

For client side, twisted.internet.tcp.BaseClient.startTLS method is used to initiate TLS connection. It has an optional parameter client=1. So we can make the receiver of StartTLS amp command to act as server even if it on client side of connection. On server side, t.i.t.Server.startTLS is called that can in turn be used as TLS client, not server. But Server's optional parameter is server=1, so they have different method signature and duplicated code instead.

I've made a patch that fixes the problem that should explain previous paragraph more clearly.

[glyph]

This is a really interesting use-case! Thanks for raising it; I'm pretty sure that I'll want it eventually. However, this patch has a few issues:

• No unit tests. Please see our wiki page on how to develop for Twisted.
• It looks like that change in tcp.py is pretty severely incompatible. Compatibility, and appropriate deprecation warnings, especially in such a core API, is really important. There is a draft of the compatibility policy on the wiki, which has been superseded by recent mailing list discussion, but is broadly accurate. However, the way that flag to startTLS works seems pretty awful. I think that change should be dealt with separately, as it is likely to be tricky, but affects a lot more protocols than just AMP.

Normally I'd reassign this to you, but you don't seem to show up in the list. Trac nonsense, I suppose. If you can try accepting the ticket until you've had a chance to deal with some of this feedback, I'd apprecaite it, since my queue is incredibly long.

[Stas Shtin]

Well, I haven't written any unit tests because I've wanted to get some feedback first. I didn't expect it to go to trunk as is 'cause I'm aware of UQDS and stuff, but it's just that code is more illustrative then words :-)

I've run all twisted test suite and it didn't give any errors. I guess that's because the tests were relying on default behaviour, not API, and default behaviour didn't change. The only place that I've found actually calling transport.startTLS with two arguments in the whole twisted codebase was in twisted.test.test_ssl, yet somehow there was no failures. The backwards incompatibility shouldn't be a huge problem IMHO as it's probably needed only in a rare case like mine or with client's and server's roles reversed (or for two servers using client as a proxy, but that's just too perverse;-)) .

What I dislike about proposed change is that if server connection was using that argument and wasn't passing it as a keyword, then behaviour silently changes to opposite and we can't really detect it. So maybe a better solution is necessary. I think we may keep compatibility by adding a boolean attribute to ITLSConnection, something like isTlsClient and adding warnings to users of current API without breaking behaviour. Though that's more tricky then my naive fix.

Trac was probably caching users list and didn't show my recently added account, I'm accepting this ticket and will work on a better patch after receiving some more feedback.

[Stas Shtin]

I was wrong when I've said that this patch doesn't give any errors in tests. It fails some SSL and AMP tests, so I'll have to look for a better solution.

[glyph]

Are you still interested in working on this?

[Stas Shtin]

For my own needs a small monkey-patch worked fine. It was backwards-incompatible, since current twisted implementation of startTLS method has invalid behavior here. Looks like an acceptable solution would be to provide an alternative to startTLS method that would resolve the client or server mode selection correctly and deprecating the old method. But I don't need that for my own purposes, and nobody else was interested in discussion. So I guess it could be closed as wontfix then.