Opened 9 years ago

Last modified 9 months ago

#3278 defect new

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

Reported by: Stas Shtin Owned by:
Priority: normal Milestone:
Component: core Keywords: amp ssl
Cc: antisvin@…, Jean-Paul Calderone Branch:
Author:

Description

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.

Attachments (1)

make_AMP_work_with_proxies.patch (2.1 KB) - added by Stas Shtin 9 years ago.

Download all attachments as: .zip

Change History (10)

Changed 9 years ago by Stas Shtin

comment:1 Changed 9 years ago by 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.

comment:2 Changed 9 years ago by Stas Shtin

Owner: changed from Glyph to Stas Shtin
Status: newassigned

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.

comment:3 Changed 9 years ago by 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.

comment:4 Changed 8 years ago by Glyph

Are you still interested in working on this?

comment:5 Changed 8 years ago by 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.

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

Branch: trunk
Keywords: amp ssl added
Owner: changed from Stas Shtin to Glyph
Status: assignednew

comment:7 Changed 7 years ago by <automation>

Owner: Glyph deleted

comment:8 Changed 9 months ago by warner

We have a need for this too, in Tahoe-LAFS/Foolscap, when used with an I2P listener transport (https://tahoe-lafs.org/trac/tahoe-lafs/ticket/2861).

You can make servers available over I2P, just like you might make a server available over Tor (with a .onion address). However the protocol looks different:

  • plain TCP: the real server listens on a globally-available socket, clients connect directly to it
  • Tor: the real server listens on a locally-bound socket, clients connect over Tor to the local Tor daemon, the local Tor daemon opens a localhost socket to the real server
  • I2P: the real server makes multiple connections *to* the local I2P daemon, and writes a command on each that says "hey, I want to be told when someone connects to your I2P address". Then later, when a client connects to that daemon, the daemon picks one of these waiting connections and sends a response saying "here you go!". The server (inside the I2P server endpoint) then tells the Factory that the connection is done, and glues the new Protocol object into the converted connection/transport. (meanwhile the Endpoint then makes a new connection to replace the one that's just been converted; the number of outstanding connections is the moral equivalent to socket.listen()'s queue-length argument)

So I2P is a bit special, in that you wind up with a tcp.Client connected on both the I2P client side and the I2P server side. Since .startTLS() looks at the transport to decide whether to do a TLS clientHello or serverHello, when we do this with Foolscap, both sides do a clientHello, and TLS negotiation fails.

I see that the current signature of t.i._newtls.ConnectionMixin.startTLS is .startTLS(ctx, normal=True), and that you could pass normal=False to flip things around. We could probably find a hackish way to use that, but it wouldn't be ideal, because the ._tlsClientDefault attribute that it's flipping is private. We (at the foolscap/i2p level) don't have a great way to know whether we've been given a client-like connection or a server-like connection, so we don't know whether we need to flip or not.

Foolscap knows exactly when it wants to do a TLS clientHello vs doing a serverHello (it should exactly match the HTTP-like exchange that immediately precedes the startTLS). "flip or not" isn't the right question, it's "client or server" that we want to express.

So really, what we'd like is .startTLS(ctx, isClient=True) or =False. I'm thinking that this would override the current normal=True argument (which isn't even exposed in t.i.interfaces.ITLSTransport.startTLS(), so I'm not sure if it's public or not).

Other name ideas, to start the bikeshedding early:

  • client= (boolean)
  • both client= and server=, which take booleans (but only one is legal)
  • side= taking "client" or "server" or "normal" or "reversed"

We've also considered things like:

  • a special attribute on the ITransport that tells Foolscap whether to pass in normal=False or not, set only on I2P-generated tcp.Client-yet-Foolscap-server connections
  • something higher up in the Foolscap connection management stack, where the "foolscap connection handler" (which converts string-based connection hints into Endpoints) is enhanced to return tuples of (endpoint, reversed)

comment:9 Changed 9 months ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone added
Note: See TracTickets for help on using tickets.