Subject: Re: [tram] I-D Action: draft-ietf-tram-turnbis-06.txt

From: "Tirumaleswar Reddy (tireddy)" <[email-masked]>
Date: Wed, 16 Sep 2015 21:27:06 +0000

Hi Simon,

Thanks for the review, Please see inline.

> -----Original Message-----
> From: Simon Perreault [mailto:[email-masked]]
> Sent: Monday, September 14, 2015 6:05 PM
> To: Tirumaleswar Reddy (tireddy); [email-masked]
> Subject: Re: [tram] I-D Action: draft-ietf-tram-turnbis-06.txt
> 
> Le 2015-09-11 18:28, Tirumaleswar Reddy (tireddy) a écrit :
> > This revision updates PMTUD and adds support for receiving ICMP from
> peer.
> 
> Thanks for this update. Here are a few comments...
> 
> > 10.5.  Receiving an ICMP Packet
> >
> >    When the server receives an ICMP packet at a currently allocated
> >    relayed transport address,
> 
> You can't receive an ICMP packet "at a currently allocated relayed transport
> address".
> 
> Suggestion: s/at a currently allocated relayed transport address//, with
> additional text below.
> 
> >    the server verifies that the type is
> >    either 3, 11 or 12 for an ICMPv4 [RFC0792] packet or either 1, 2, or
> >    3 for an ICMPv6 [RFC4443] packet.  It also verifies that the IP
> >    packet in the ICMP packet payload contain an UDP header.  If either
> 
> s/contain/contains/

Fixed.

> 
> >    of these conditions fail, then the ICMP packet is silently dropped.
> 
> >    The server looks up the allocation associated with the relayed
> >    transport address.
> 
> Replace this sentence with: "The server looks up the allocation whose
> relayed transport address corresponds to the encapsulated packet's source
> IP address and UDP port. If no such allocation exists, the packet is silently
> dropped."

Replaced.

> 
> >    The server then checks to see whether the set of
> >    permissions for the allocation allows the relaying of the ICMP packet
> >    as described in Section 8.
> 
> Section 8 would imply checking the outer packet's source, which is wrong. It
> is very likely that ICMP packets are generated by intermediate routers. The
> router must ignore the outer source and must instead check the
> encapsulated packet's destination.

Yes.

> 
> Reword for clarity: "For ICMP packets, the source IP address MUST NOT be
> checked against the permissions list as it would be for UDP packets.
> Instead, the server extracts the destination IP address from the encapsulated
> IP header.  The server then compares this address with the IP address
> associated with each permission in the list of permissions for the allocation.
> If no match is found, relaying is not permitted, and the server silently
> discards the ICMP packet.  Note that only addresses are compared and port
> numbers are not considered."

Looks good, updated draft.

> 
> >    If relaying is permitted then the server forms and sends a SendError
> >    indication.  The SendError indication MUST contain both an XOR-PEER-
> >    ADDRESS and an ICMP attribute.  The ICMP attribute is set to the
> >    value of the type and code fields from the ICMP packet, and the XOR-
> >    PEER-ADDRESS attribute is set to the destination IP address in the IP
> >    header and to the destination port in the UDP header.  Both are found
> >    in the ICMP packet payload.  The SendError indication is then sent on
> >    the 5-tuple associated with the allocation.
> 
> Two things:
> 
> - The "SendError" method can be confused with a "Send Indication error
> response" (i.e., a message with the Send method and class 0b11). I suggest
> renaming to avoid confusion.

How about ICMPError or ICMPErrorIndication?

> 
> - Is it really necessary to define a new method? I'm thinking that the Data
> method could be reused: just make it transport an ICMP attribute instead of
> a DATA attribute. It would not affect backwards compatibility since older
> clients will drop Data messages that don't contain a DATA attribute (per
> section 10.4).

Yes, but we still have to deal with channels. As per section https://tools.ietf.org/html/draft-ietf-tram-turnbis-06#section-11.5, TURN server cannot use DATA method if channel has been bound to the peer but the client can use Send indications even with channels.

-Tiru

> Simon

---

From: Simon Perreault <[email-masked]>
Date: Wed, 16 Sep 2015 17:29:48 -0400

Le 2015-09-16 17:27, Tirumaleswar Reddy (tireddy) a écrit :
>> - The "SendError" method can be confused with a "Send Indication
>> error response" (i.e., a message with the Send method and class
>> 0b11). I suggest renaming to avoid confusion.
> 

I'm fine with both.

>> - Is it really necessary to define a new method? I'm thinking that
>> the Data method could be reused: just make it transport an ICMP
>> attribute instead of a DATA attribute. It would not affect
>> backwards compatibility since older clients will drop Data messages
>> that don't contain a DATA attribute (per section 10.4).
> 
> Yes, but we still have to deal with channels. As per section
> https://tools.ietf.org/html/draft-ietf-tram-turnbis-06#section-11.5,
> TURN server cannot use DATA method if channel has been bound to the
> peer but the client can use Send indications even with channels.

Couldn't that text be changed? I don't see any reason why Data wouldn't
work even with channels.

Simon

---

From: "Tirumaleswar Reddy (tireddy)" <[email-masked]>
Date: Wed, 16 Sep 2015 21:46:07 +0000

> Sent: Thursday, September 17, 2015 3:00 AM
> >> attribute instead of a DATA attribute. It would not affect backwards
> >> compatibility since older clients will drop Data messages that don't
> >> contain a DATA attribute (per section 10.4).
> >

It can be changed, assuming it does not impact existing TURN client implementations. I will will remove this restriction, use DATA to transport the ICMP attribute and remove SendError. 

-Tiru