Opened 5 years ago

Last modified 5 years ago

#5690 defect new

The invoker of callRemote(t.p.amp.StartTLS) should always be a TLS client () and the responder should always be a TLS server

Reported by: habnabit Owned by: habnabit
Priority: normal Milestone:
Component: core Keywords: amp
Cc: Glyph Branch:
Author:

Description (last modified by Glyph)

Currently, if callRemote(StartTLS) is called on the TCP server side of an AMP connection, the TLS negotiation will be initiated in the wrong direction: the server thinks that it's responding to a handshake from a client, due to the fact that the TLS layer will "helpfully" call set_accept_state or set_connect_state based on the private, internal state that Twisted maintains about whether this is a client or server connection.

This means the handshake will fail if the TLS handshake bytes the server send arrives at the client in the same packet as some boxes - which it will, of course, because callRemote(StartTLS) also sends a box. This usually produces an incomprehensible error message about some length limit being exceeded, since the AMP parser attempts to read some not-AMP bytes.

transport.startTLS() doesn't have quite enough information to do this the right way - it doesn't know whether you're calling startTLS to begin or respond to a handshake - but this can be transparently handled by AMP, since if you're using callRemote you are by definition the initiator.

(Note: the code which originally lead to this bug report was incorrect, and if you want this functionality, it's possible that you're doing it wrong too. Generally speaking, clients should always be the ones to initiate TLS; in particular, the way that clients and servers verify each others certificates, and what they do with the results of that verification, is very different. You really only want to call callRemote(StartTLS) from the server if you are intentionally reversing the roles of client and server on the TCP connection.)

Change History (4)

comment:1 Changed 5 years ago by habnabit

Description: modified (diff)
Summary: t.p.amp.BinaryBoxProtocol doesn't stop parsing after startTLScallRemote(t.p.amp.StartTLS) should always succeed for clients and servers

Changed from the old issue:

When BinaryBoxProtocol._startTLS is called while parsing buffered data, the transport's startTLS is called, but parsing continues. In some cases, the buffer can contain both a box which calls _startTLS upon being received as well as the start of the TLS negotiation. The result is that the start of the TLS handshake is parsed as a box and negotiation never completes.

comment:2 Changed 5 years ago by Jean-Paul Calderone

Can you please clarify a couple points about this ticket?

  1. The summary seems superficially false. Start TLS should not succeed in many cases - the most obvious of which is when a certificate does not pass the verification required.
  2. Why "_should_" the client be the one to initiate the StartTLS in the first place?

comment:3 Changed 5 years ago by Glyph

Description: modified (diff)
Summary: callRemote(t.p.amp.StartTLS) should always succeed for clients and serversThe invoker of callRemote(t.p.amp.StartTLS) should always be a TLS client () and the responder should always be a TLS server

comment:4 in reply to:  2 Changed 5 years ago by Glyph

Replying to exarkun:

Can you please clarify a couple points about this ticket?

  1. The summary seems superficially false. Start TLS should not succeed in many cases - the most obvious of which is when a certificate does not pass the verification required.

You're right. I wonder if habnabit understood me now when I described the issue to him on IRC, because the description came out pretty mangled. I have rewritten it in an attempt to explain better.

  1. Why "_should_" the client be the one to initiate the StartTLS in the first place?

To answer this I added the "note" at the end. Clients are generally the ones that initiate TLS, there are assumptions all over the protocol that this is so; it's just usually a mistake to have the server start it.

Note: See TracTickets for help on using tickets.