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

[Dave Thaler <[e-mail]>]

Simon Perreault write:
> Dave Thaler wrote, on 08/08/09 02:29 PM:
> > 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?

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

> >    and does this depend on how the corresponding (peer vs client)
> >    connection was closed?
>
> Note that requiring this would probably not be implementable in non-
> root user-space.

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

> > 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?
>
> Correct. I have change the wording:
>
> The client MAY retry *with the same XOR-PEER-ADDRESS attribute*, but
> MUST wait
> at least 10 seconds.

Looks ok.

> > 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?
>
> 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.

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.

> 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.
>
> > 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?
>
> 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."

Ok.

> > 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.
>
> 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.

Yep, that looks great.

> > 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.
>
> Good catch. I've copied the paragraph in question to the end of section
> 5.2.

Ok.

> > 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).
>
> 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.

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?

> 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.
>
> >    Currently it has references to docs that contain
> >    discussion of bandwidth DoS attacks, but not memory
> >    DoS attacks, as far as I can find.
>
> The above paragraph would indeed be much improved with such a
> reference. Could
> you provide one?

How about [RFC4732] section 2.1?

-Dave