Subject: Re: [BEHAVE] WGLC: draft-ietf-behave-turn-tcp-04

From: "Alfred E. Heggestad" <[email-masked]>
Date: Fri, 07 Aug 2009 19:00:08 +0200

Dan Wing wrote:
> BEHAVE is starting a 3 week working group last call (WGLC), ending August 19,
> for 
> 
>   draft-ietf-behave-turn-tcp-04
>   "Traversal Using Relays around NAT (TURN) Extensions for TCP Allocations"
> 
> Please send technical comments to the BEHAVE list, and editorial comments to
> the authors.
> 

Here are my WGLC review comments on draft-ietf-behave-turn-tcp-04:

General:

  The document is well written and easy to read. 

=== Technical ===

5.2. Receiving a Connect Request

   "Otherwise, the server MUST initiate an outgoing TCP connection.  The
    local endpoint is the relayed transport address associated with the
    allocation. ..."

  ---> why does the local endpoint have to be the relayed transport address?
       is this a technical requirement to make the TURN/TCP protocol work?
       Normally an outgoing TCP connection using BSD sockets will use a
       random port as local endpoint. It is possible to use the same port
       as the TCP socket is listening on, but will require that things are
       initialized in the correct order. You should check that this does
       cause problems for implementors (ref: ICE tcp-SO discussion)

5.4. Receiving a ConnectionBind Request

  ---> 5th paragraph: "...Connect or Listen request..."

  I think the "Listen" request is not defined anymore ...

=== Editorial ===

Abstract, line 2:

    "to allows a" --> "to allow a"

    - some of the text in the abstract is bold in the HTML version

    last line:  "STUN Server"  -->  "TURN Server"

1. Introduction:

    "It also allows the client to initiate connections from that allocation to peers"

    would like to suggest instead:

    "It also allows the client to initiate outgoing TCP connections from that allocation to peers"
                                           ^^^^^^^^^^^^

    ", and accept connection requests from peers made towards that allocation"

    would like to suggest instead:

    ", and accept incoming TCP connections from peers made towards that allocation"

4.4 Receiving a Connection:

    line 12: "..attribute, mirrorred from ..."
                           ^^^^^^^^^
    - (typo)

- eof -

---

From: Simon Perreault <[email-masked]>
Date: Fri, 07 Aug 2009 16:17:55 -0400

> cause problems for implementors (ref: ICE tcp-SO discussion)

Here's one case where this is needed: when two TURN servers are talking 
to each other, the client needs to install a permission on each server 
for the remote socket. So each client needs to be know in advance where 
the connection is going to come from.

> I think the "Listen" request is not defined anymore ...

Right. Will be removed.

> - some of the text in the abstract is bold in the HTML version

Yes, this is because the HTML version is generated from the text version 
by heuristics. It got this part wrong.

> last line: "STUN Server" --> "TURN Server"

Right. Strange how this was never noticed before...

> ", and accept incoming TCP connections from peers made towards that

Sure, why not.

Simon
-- 
DNS64 open-source   --> http://ecdysis.viagenie.ca
STUN/TURN server    --> http://numb.viagenie.ca
vCard 4.0           --> http://www.vcarddav.org

---

From: Dave Thaler <[email-masked]>
Date: Sat, 8 Aug 2009 18:29:03 +0000

I have reviewed this doc and have a number of comments.
A marked-up PDF with my full comments is at
http://research.microsoft.com/en-us/um/people/dthaler/draft-ietf-behave-turn-tcp-04.pdf

Summary of non-editorial-only comments:

1) Are data connections to be closed gracefully or abortively,
   and does this depend on how the corresponding (peer vs client)
   connection was closed?

2) When error 447 is returned, section 4.3 says the client MUST
   wait at least 10 seconds before retrying.  I think this is
   for the _same_ destination IP/port, right?  If the client
   has other destination IP/port pairs to try, they can be
   tried immediately, correct?

3) Section 4.6 notes that no keepalive mechanism is provided
   and an application can do this at a higher layer.  For
   NATs between a TURN client and a TURN server, why wouldn't
   TCP keepalives be a possibility?

4) If I understand the 3rd paragraph of section 5.2, correctly,
   there's an important limitation/constraint that should be
   called out early (e.g. in the overview).  Normally, TCP
   clients can open multiple parallel connections (e.g., to
   a web server) to the same destination address/port.  To
   accomplish this with TURN, it seems that the client needs
   multiple control connections so it can get multiple
   allocations, since it can only have one connection to a
   given peer address/port per allocation, correct?

5) The 5th paragraph of Section 5.2 also implies that it's
   illegal for a TURN server to refuse to initiate connections
   based on the XOR-PEER-ADDRESS value.  For example, if it
   refuses to initiate connections to 127.0.0.1 it would be
   violating a MUST.  I'd like to see this relaxed.

6) The draft talks about timing out a peer connection after
   30 seconds in section 5.3 if the client never does a
   ConnectionBind for a connection initiated by a peer.
   However, section 5.2 has no corresponding rule.  This
   means a client can send a Connect request, and then
   never open a data connection, and the server will never
   time out the peer connection.  The same rule needs to be
   added to 5.2 to address this.

7) Continuing on the same point, the security considerations
   section needs to add a discussion of a memory DoS attack
   due to the TURN server having to buffer data from a peer
   while waiting for a ConnectionBind (which may never happen).
   Currently it has references to docs that contain
   discussion of bandwidth DoS attacks, but not memory
   DoS attacks, as far as I can find.

-Dave

> -----Original Message-----
> From: [email-masked] [mailto:[email-masked]] On
> Behalf Of Dan Wing
> Sent: Thursday, July 30, 2009 1:49 AM
> To: [email-masked]
> Cc: [email-masked]; 'Behave Chairs'
> Subject: [BEHAVE] WGLC: draft-ietf-behave-turn-tcp-04
>
> Thanks,
> -d
> _______________________________________________
> Behave mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/behave

---

From: Ari Keranen <[email-masked]>
Date: Wed, 19 Aug 2009 13:21:24 +0300

Hi,

One technical issue in draft-ietf-behave-turn-tcp-04 caught my attention:

In section 4.5., "Sending and Receiving Data" is says:

    [...] Data on a data connection MUST NOT
    be interpreted as STUN messages.

I think this is too heavy restriction. What about if the peer wants to 
send some STUN messages on the path via the TURN server and would need 
the TURN client to parse them as STUN?

I would rather make this "SHOULD NOT" to be future proof. Or even 
removing the whole restriction should not do much harm, right?

Cheers,
Ari

---

From: Simon Perreault <[email-masked]>
Date: Wed, 19 Aug 2009 07:45:17 -0400

Le 2009-08-19 06:21, Ari Keranen a écrit :
> Or even

I have no opinion about this. Unless somebody else has something to say, 
I'll remove the passage.

Simon

---

From: "Dan Wing" <[email-masked]>
Date: Fri, 21 Aug 2009 17:14:04 -0700

> From: [email-masked] 
> [mailto:[email-masked]] On Behalf Of Ari Keranen
> Sent: Wednesday, August 19, 2009 3:21 AM
> To: [email-masked]
> Cc: [email-masked]
> Subject: Re: [BEHAVE] WGLC: draft-ietf-behave-turn-tcp-04
> 
> Hi,
> 

How would the TURN server identify such traffic that it should
respond to?  Thinking aloud, one solution is for the local TURN
client to tell the TURN server which STUN username(s) indicate 
the STUN message is intended for the TURN server.

> removing the whole restriction should not do much harm, right?

The reason for this passage is so that ICE connectivity checks
can be performed, through the TURN server, between the endpoints.
If the TURN server 'eats' (processes) the incoming messages
from the remote peer, the connectivity checks will fail.

This is really something that belongs in the TURN specification
because I believe it is not unique to TURN-TCP.  If anything, the 
TURN specification should say something like: 

  In the absence of an standards-track enhancement to identify 
  STUN traffic intended for the TURN server, data on a data
  connection MUST NOT be interpreted as a STUN message.

Thoughts?

(CC'ing TURN authors.)

-d

---

From: Ari Keranen <[email-masked]>
Date: Mon, 24 Aug 2009 10:55:50 +0300

> the STUN message is intended for the TURN server.

Actually, my point was about TURN clients (section 4, and the 
subsections, I assume, are about "client processing"). That is, a TURN 
client should be able to interpret relayed data as STUN messages. I do 
understand why this same restriction was in the section 5.4, since 
section 5 is about the server part.

I think the TURN-TCP server can, and should, always just blindly relay 
the data on the data connection. If there's need for STUN signaling with 
the server, the control connection should be used for that.

> from the remote peer, the connectivity checks will fail.

Yes, that's why section 5.4 is OK with the restriction. Probably this 
text was supposed to be only in section 5.4 but accidentally slipped to 
section 4.5 too?

>   connection MUST NOT be interpreted as a STUN message.

In the TURN-UDP case you don't have a "data connection" in the same 
sense as with TURN-TCP, so I think this is actually TURN-TCP specific. 
Although data sent by a peer should never be interpreted as STUN by the 
server, but I think this is somewhat clear from the current TURN-UDP draft.

Cheers,
Ari

---

From: Simon Perreault <[email-masked]>
Date: Mon, 24 Aug 2009 10:26:56 -0400

Thank you very much for your comments.

> 1) Are data connections to be closed gracefully or abortively,

The draft is intentionally silent about this. It is expected that "closing a
connection" will be done by following Figure 6 in RFC 793 (the arrow labeled
CLOSE below the ESTAB state), but the server is free to do otherwise as long as
it is valid TCP.

Do you have a reason in mind why we should be more explicit?

>    and does this depend on how the corresponding (peer vs client)

Note that requiring this would probably not be implementable in non-root user-space.

>    tried immediately, correct?

Correct. I have change the wording:

The client MAY retry *with the same XOR-PEER-ADDRESS attribute*, but MUST wait
at least 10 seconds.

>    TCP keepalives be a possibility?

Because in non-root user-space, TCP keepalives as provided by the OS are nearly
worthless in many important cases because they cannot be setup per-socket. They
are setup system-wide and you need to be root to change the timeout.

Basically it's another item in the list of things with STUN/TURN that are the
way they are because of the requirement that a server be implementable in
non-root user-space.

>    given peer address/port per allocation, correct?

Yes, this is correct. It follows naturally from the fact that a 5-tuple uniquely
identifies a connection, and that the local address and port are fixed from the
moment the allocation is created.

How about adding the following sentence to the third paragraph of section 3?

"Note that a maximum of one connection to a given peer (address and port
combination) can be established per allocation."

>    violating a MUST.  I'd like to see this relaxed.

How about this modified text?

"Otherwise, *and if the new connection is permitted by local policy,* the server
MUST initiate an outgoing TCP connection."

This lets the server filter by peer address as well as other criteria.

>    added to 5.2 to address this.

Good catch. I've copied the paragraph in question to the end of section 5.2.

>    while waiting for a ConnectionBind (which may never happen).

Actually, this highlights a bug in the current spec (section 5.2, paragraph 7):

   The server MUST buffer any data received from the peer.
   Data MUST NOT be lost.  It is up to the server to adjust its
   advertised TCP receive window should the buffer size become a
   limiting factor.

If it is not not lose data, the server must rely on the peer obeying the TCP
window. This is bad. Instead, I propose the following:

   The server MUST buffer any data received from the peer.
   Data MUST NOT be lost unless the buffer is about to exceed a limit defined
   by local policy, in which case the data connection MUST be closed.  The
   server adjusts its advertised TCP receive window to reflect the amount of
   empty buffer space.

Similar wording would be applied to section 5.3, paragraph 3.

How about adding the following text to the security considerations section?

    After a TCP connection is established between the server and a peer, and
    before a ConnectionBind request is received from the client, the server
    buffers all data received from the peer. This protocol specification lets
    the server drop the connection if the buffer size is about to exceed a
    limit defined by local policy. This policy should ensure that memory
    resources are not exceeded.

>    DoS attacks, as far as I can find.

The above paragraph would indeed be much improved with such a reference. Could
you provide one?

Thanks,
Simon

---

From: Dave Thaler <[email-masked]>
Date: Mon, 24 Aug 2009 16:50:28 +0000

> Do you have a reason in mind why we should be more explicit?

I think you should be explicit that either is ok, rather than it
looking like an oversight that will generate questions like mine.

> Note that requiring this would probably not be implementable in non-

I wouldn't go there, as that's very OS-specific.

> The client MAY retry *with the same XOR-PEER-ADDRESS attribute*, but

Looks ok.

> are setup system-wide and you need to be root to change the timeout.

Um, no.  Again, this is OS specific, and in fact it's per-socket
configurable on Windows:
http://msdn.microsoft.com/en-us/library/dd877220(VS.85).aspx

This should be rephrased to allow for the possibility
that the OS supports per-socket TCP keepalive intervals.

> combination) can be established per allocation."

Ok.

> This lets the server filter by peer address as well as other criteria.

Yep, that looks great.

> Good catch. I've copied the paragraph in question to the end of section

Ok.

> Similar wording would be applied to section 5.3, paragraph 3.

Ok. I would assume it's legal for the server to just not do a read()
until after a ConnectionBind request is received from the client,
thereby making the OS handle the buffering, right?

> The above paragraph would indeed be much improved with such a

How about [RFC4732] section 2.1?

-Dave

---

From: Simon Perreault <[email-masked]>
Date: Mon, 24 Aug 2009 13:09:38 -0400

> http://msdn.microsoft.com/en-us/library/dd877220(VS.85).aspx

Hence the "in many important cases".

> that the OS supports per-socket TCP keepalive intervals.

I'm fine with a MAY. I'll add this to the draft.

> thereby making the OS handle the buffering, right?

This is exactly what I had in mind.

> How about [RFC4732] section 2.1?

Thanks. I'll link to 2.1.3, which is more specifically about resource exhaustion.

Simon