Subject: Re: [BEHAVE] TURN-12 Comments/Typos

From: Philip Matthews <[email-masked]>
Date: Wed, 4 Feb 2009 13:01:10 -0500

Hi Scott:

Here are replies to your remaining comments.

- Philip

On Wed, 14-Jan-09, at 13:39 , Scott Godin wrote:

> A couple of additional notes:
>
> Section 11 - "To prevent race conditions, the client MUST wait 5
> muntes after the channel binding expires before attempting to bind the
> channel number to a different transport address or the transport
> address to a different channel number".  I think this statement should
> be expanded to explicitly say that this is within the context of an
> allocation.  The client should be able to re-use the transport address
> in a channel binding of a new allocation, anytime after the previous
> allocation that it was used in has been destroyed.

Section 11 already says:
    Channel bindings are specific to an allocation, so that the use of a
    channel number or peer transport address in a channel binding in one
    allocation has no impact on their use in a different allocation.  If
    an allocation expires, all its channel bindings expire with it.

This is also mentioned in the Overview.
One of the problems with an earlier version of the spec was that it  
repeated things over and over again. So I would prefer to say it only  
once.

>
>
> Radical Ideal - In writing my client library, I would like to mask as
> many of the lower level TURN details as possible.  This means that
> masking the details behind channel binding, and channel messaging vs
> Data/Send Ind data sending mechanism is desirable.  I don't see any
> draw backs of using the channel binding mechanism over the Data/Send
> Ind method, and it uses lower bandwidth, so I'm considering that my
> client API will just always use this mechanism.  This leads me to
> think - do we really need both mechanisms?  Supporting only the
> Channel Binding mechanism would really simplify this draft, you would
> be able to get rid of the following concepts:  Create/Refresh
> Permission (done explicily via Channel Binding), Data Indication
> sending/handling, Send Indication sending/handling.  I'm not really
> understanding the value of offering both mechanisms.  With previous
> versions (pre-Create Permission), it made sense to support both
> mechanisms in order to avoid having the queue data to send in the
> client waiting for the channel bind response, but now you need to wait
> for the permission to be installed before you send data, so some
> queuing will be required anyway.

The Data indication is needed for the situation where a server  
receives data from an (address,port) combination where a permission  
has been installed to that address, but the client has been sending to  
a different port. This can happen, for example, when the peer is  
behind a symmetric NAT.

Send indications are needed when the DONT-FRAGMENT attribute is used.

> Thanks,
> Scott

---

From: Philip Matthews <[email-masked]>
Date: Wed, 4 Feb 2009 17:36:26 -0500

Hi Scott:

Thanks for your comments on TURN. My apologies for being so slow to  
respond to your message. Unfortunately, TURN is not currently a part  
of my day job, and I am have been very busy with other things in the  
evenings.

- Philip

On Wed, 7-Jan-09, at 21:01 , Scott Godin wrote:

> I'm in the process updating reTurn (opensource Turn server in
> resiprocate project) to the latest turn-12, and have the following
> comments/typos after reviewing the draft.
>
> 1.  section 1, 4th paragraph, "When the client send...." - should be
> "...sends..."

Fixed.

>
>
> 2.  section 2.1, last paragraph, ".., the client may send and received
> packets..." - should be "...receive..."

Fixed.

>
>
> 3.  section 2.6, list "The ECN field is may be reset..." - remove "is"

Fixed.

>
>
> 4.  section 5, 3rd last paragraph indicates that username, password,
> realm and nonce are used to verify subsequent requests - but according
> to section 4 paragraph 6 the only information authentication
> information used to verify subsequent requests is the username.  I
> think one of these sections needs to be re-worded to avoid confusion.
> Perhaps section 4 should indicate that username, realm and password
> should be stored and verified across all requests for an allocation -
> this will cover cases where the same username exists in two different
> realms.  However note that if password hash is stored instead of the
> plain password, then we don't really need to store and compare realm,
> since it is hashed into the password.

I think both paragraphs are accurate. The username is not allowed to  
change, and to authenticate the request, extra authentication  
information is required. In fact paragraph 4 in section 4 says that  
all request should be authenticated and mentions the realm value.

To make this clearer, I have moved paragraph 6 in section 4 to follow  
immediately after paragraph 4.

>
>
> 5.  section 6.2 - step 5 "... request contains a EVEN-PORT
> attribute..." - should be "...contains an EVEN-PORT attribute..."

Fixed.

>
>
> 6.  section 7.2 - first step indicates that "the additional username
> check of section 4" should be complete.  This comment needs to be
> brought inline with the wording decided for comment 4.
>
> 7.  section 9, 2nd paragraph - missing space "... inSection..."

Fixed.

>
>
> 8.  section 9.2 is missing the steps/checks to process similar to
> Allocate and Refresh requests - it would be good to add these for
> consistency.  It should also include: Long Term Cred check, existing
> allocation check,  and the required auth (username, etc.) checks.

The text for an Allocate and Refresh request is very explicit because  
the processing of these requests (especially the Allocate request) is  
rather complex and there are some less-obvious situations that needed  
to be discussed. By contrast, the processing of the other requests and  
indications are much more straightforward. For example, the need to do  
the existing allocation check seems pretty obvious (not to mention  
that it is said explicitly in section 4).

I added a sentence mentioning the authentication check. I also added a  
sentence saying that, if the server considers the IP addresses to be  
out-of-range, then it should return 400 (Bad Request).

However, I don't feel like doing a rewrite of this section unless  
others see a need.

>
>
> 9.  section 10, 1st paragraph - "...for sending and receive data
> from..." - should read "...for sending and receiving data..."

Fixed.

>
>
> 10.  section 10.2 - should include a step to ensure that an allocation
> exists for the 5-tuple, else message is dropped

This is said explicitly in section 4. Furthermore, it would be  
impossible for an implementator to forget this, since the server could  
not check permissions or have a source address for the UDP datagram if  
no allocation existed.

>
>
> 11.  section 10.2, 4th paragraph, last sentence - "...the server MUST
> NOT refresh the permission due to the receipt of the Send
> indication..." - should be removed since Send Indication never refresh
> permissions now

I believe that this sentence was added at the request of Cullen, who  
wanted to make it very clear that Send indications did not refresh  
permissions. However, I agree that the MUST NOT looks a bit strange. I  
have reworded this sentence as follows:
    Note that a Send indication never causes the server to refresh the  
permission.

>
>
> 12.  section 10 indicates that Data attribute is mandatory in a Send
> Indication - should we allow no data attribute to indicate that a 0
> length UDP packet?  Note the ChannelBinding section discusses sending
> a 0 length packet (section 11.6 last paragraph).  Perhaps this could
> also be accomplished using a Data attribute containing a data length
> of 0 - but I think there should be some mention of this in section 10.

I don't really see a use for it, but to be consistent with the  
ChannelData message, I have added a sentence to the sections  
describing the processing of a Send indication and a Data indication  
to say that zero bytes of data are allowed in a DATA attribute. Since  
I think this will be very uncommon, I don't feel that the optimization  
of omitting the DATA attribute to indicate this is needed at this time.

>
>
> 13. section 11, paragraph 5 - "...or may choose never to bind a
> channel it." - should be "...channel to it."

Fixed.

>
>
> 14. section 11.2 - check list should be numeric to be consistent with
> Allocate and Refresh requests.  Should also contain Long Term Cred
> check, existing allocation check,  and the required auth (username,
> etc.) checks

I have added a sentence about authenticating the packet.

>
>
> 15. section 11.5 - I'm not sure I understand the need to pad the
> ChannelData message to a multiple of four bytes.  TCP implementations
> are capable of reading in the exact right number of bytes from the
> stream.  Can this requirement be removed?

It has been a long-standing property of STUN (and thus TURN) that both  
messages and the attributes within messages are aligned on four-byte  
boundaries. The requirement to pad ChannelData messages over TCP was  
inserted to keep this property.

Do you have a technical reason why you think it is important to remove  
this padding?

>
>
> 16.  Is there are reason we don't just make the default permission and
> channel binding lifetime the same, to facilitate use of the
> ChannelBind refresh to accomplish both refreshes without worrying
> about the difference?

When the mechanism to age-out channels was first introduced, there was  
a request that the lifetime for channels be longer than that for  
permissions. As I recall, it was felt that there would be some race  
conditions introduced if the two timeout intervals were the same.

Personally, I now feel that there would be no problems in making these  
two intervals the same and I agree with you that things would be  
slightly cleaner. But I am not sure it is worthwhile making the change  
at this late date.

>
>
> My apologies if my questions/suggestions have already been discussed.
>
> Thanks,
> Scott Godin
> _______________________________________________
> Behave mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/behave
>

---

From: Scott Godin <[email-masked]>
Date: Thu, 5 Feb 2009 09:36:31 -0500

Hi Philip,

A few responses....

> This is said explicitly in section 4. Furthermore, it would be impossible
> for an implementator to forget this, since the server could not check
> permissions or have a source address for the UDP datagram if no allocation
> existed.

This step is mentioned in the processing of other methods.  Many of my
suggestions are to improve the overall consistency in the document
when describing the processing required for various methods.

> Do you have a technical reason why you think it is important to remove this
> padding?

My only reason was that it adds unnecessary complexity for
implementors.  However, I can buy your argument that you would like to
keep ChannelData messages somewhat consistent with StunMessage
packing.

> When the mechanism to age-out channels was first introduced, there was a
> request that the lifetime for channels be longer than that for permissions.
> As I recall, it was felt that there would be some race conditions introduced
> if the two timeout intervals were the same.
>
> Personally, I now feel that there would be no problems in making these two
> intervals the same and I agree with you that things would be slightly
> cleaner. But I am not sure it is worthwhile making the change at this late
> date.

I would definitely support pushing this change through, given that a
good counter argument is not presented.  Reading some other postings
on the list, it seems others feel the same way.

Thanks,
Scott

---

From: Scott Godin <[email-masked]>
Date: Thu, 5 Feb 2009 09:40:12 -0500

Hi Philip,

>> Radical Idea - In writing my client library, I would like to mask as
> The Data indication is needed for the situation where a server receives data
> from an (address,port) combination where a permission has been installed to
> that address, but the client has been sending to a different port. This can
> happen, for example, when the peer is behind a symmetric NAT.
>

Good points - thanks.

Scott

---

From: Philip Matthews <[email-masked]>
Date: Thu, 5 Feb 2009 18:49:00 -0600

On Thu, 5-Feb-09, at 08:36 , Scott Godin wrote:

>> impossible

OK. In the sections describing Refresh, CreatePermission,  
ChannelBinding, and Send, I have added a reference to section 4. As a  
consequence, I removed the explicit mention of some of the section 4  
processing from the Refresh section.

> keep ChannelData messages somewhat consistent with StunMessage

OK.

> on the list, it seems others feel the same way.

OK. Let's see what others say.

- Philip