Re: [tram] AD Evaluation of draft-ietf-tram-turnbis-23

[Gonzalo Camarillo <[e-mail]>]

Thanks for the review, Magnus!

Authors, could you please look into Magnus's comments and provide him
with responses?

Thanks,

Gonzalo

On 05-Apr-19 10:11, Magnus Westerlund wrote:
> Hi,
> 
> One addition.
> 
> Section 16.13.  ICMP Attribute
> 
> This attribute is flawed as it lacks a data part to carry values certain
> codes have. I specifically think of ICMPv6 Type = 2, Packet Too Big.
> That message has a 32-bit unsigned integer carrying the MTU of the Next
> Hop Link that causes the ICMP message to be sent. That really should be
> replicated back to the client.
> 
> There is also the question if ICMP messages for Parameter Problem should
> included the pointer?
> 
> I really think there need to be a 32-bit data field with the ICMP
> attribute.
> 
> Cheers
> 
> Magnus
> 
> 
> On 2019-04-04 16:00, Magnus Westerlund wrote:
>> Hi,
>>
>> I have now finished the AD review. There are a lot of details that I
>> think needs fixing or at least feedback on. I also think there are a
>> couple of more serious issues here. Especially regarding the handling of
>> MTU and fragmentation, as well as IP header fields in address family
>> translation and with non UDP transport. We need to conclude on these
>> before we go to IETF last call.
>>
>> My Review comments are:
>>
>> A. Section 1: "The client can arrange
>>    for the server to relay packets to and from certain other hosts
>>    (called peers) and can control aspects of how the relaying is done."
>>
>> I don't think this is an accurate description of what Turn does. One may
>> easily get the impression that the whole packet TURN receives are
>> relayed, rather than the transport protocol data part and some of the
>> header information are relayed between the client <-> peer, with a
>> knowledge of the peer transport address for the client. I think that
>> should be clarified in the introduction.
>>
>> B. Section 2.7:
>>
>> "Applications using TCP can more or less
>>    ignore this issue because fragmentation avoidance is now a standard
>>    part of TCP, but applications using UDP (and thus any application
>>    using this version of TURN) must handle fragmentation avoidance
>>    themselves."
>>
>> Lower case "must", and as the document uses RFC 2119, this must have the
>> meaning of "MUST". A reformulation here is likely in order.
>>
>> C. Section 2.7:
>>
>>    In this approach, the application exploits the
>>    fact that the IP specification [RFC0791] specifies that IP packets up
>>    to 576 bytes should never need to be fragmented.
>>
>>    As a guideline, sending a maximum of 500 bytes of application data in
>>    a single TURN message (by the client on the client-to-server leg) or
>>    a UDP datagram (by the peer on the peer-to-server leg) will generally
>>    avoid IP fragmentation.  To further reduce the chance of
>>    fragmentation, it is recommended that the client use ChannelData
>>    messages when transferring significant volumes of data, since the
>>    overhead of the ChannelData message is less than Send and Data
>>    indications.
>>
>> In that first paragraph I think it is worth discussing the IPv6
>> requirement of supporting a 1280 minimal size.
>>
>> Secondly, I think recommending only a 500 MTU is way to conservative.
>> Those studies I have seen appears to indicate that at least 1220 bytes
>> UDP payloads should be be working as well as 500 bytes. See for example
>> figure 12 in this:
>> https://static.googleusercontent.com/media/research.google.com/sv//pubs/archive/46403.pdf
>>
>>
>> D. Section 2.7:
>>
>>    [I-D.ietf-tram-stun-pmtud] is an implementation of [RFC4821] that
>>    uses STUN to discover the path MTU, and so might be a suitable
>>    approach to be used in conjunction with a TURN server that supports
>>    the DONT-FRAGMENT attribute.  When the client includes the DONT-
>>    FRAGMENT attribute in a Send indication, this tells the server to set
>>    the DF bit in the resulting UDP datagram that it sends to the peer.
>>    Since some servers may be unable to set the DF bit, the client should
>>    also include this attribute in the Allocate request -- any server
>>    that does not support the DONT-FRAGMENT attribute will indicate this
>>    by rejecting the Allocate request.
>>
>> I want to verify that the WG is working on resolving the issues with the
>> STUN PMTUD document. I definitely sees the point of the recommendation,
>> but only if the WG will conclude on that document in a reasonable time
>> frame.
>>
>> E. Section 3:
>>
>>    Server-Reflexive Transport Address:  A transport address on the
>>       "public side" of a NAT.  This address is allocated by the NAT to
>>       correspond to a specific host transport address.
>>
>> Shouldn't "public side" be replace with "external side"? That is the
>> terminology used by the Behave RFCs.
>>
>> F. Section 3:
>>
>>    5-tuple:  The combination (client IP address and port, server IP
>>       address and port, and transport protocol (currently one of UDP,
>>       TCP, or (D)TLS)) used to communicate between the client and the
>>       server.
>>
>> I see some issues that the 5-tuple aggregates the transport protocol
>> with the security protocol. I agree from the TURN protocol perspective
>> the protocol combination is the important one. However, writing like you
>> do here you obfuscate both that when security is in place you have a
>> combination, as well as that there are 4 alternatives defined by this
>> document.
>>
>> G. Section 4.1:
>>
>>    The "turn" and "turns" URI schemes are used to designate a TURN
>>    server (also known as a relay) on Internet hosts accessible using the
>>    TURN protocol.  The TURN protocol supports sending messages over UDP,
>>    TCP, TLS-over-TCP or DTLS-over-UDP.  The "turns" URI scheme MUST be
>>    used when TURN is run over TLS-over-TCP or in DTLS-over-UDP, and the
>>    "turn" scheme MUST be used otherwise.  The required <host> part of
>>    the "turn" URI denotes the TURN server host.  The <port> part, if
>>    present, denotes the port on which the TURN server is awaiting
>>    connection requests.  If it is absent, the default port is 3478 for
>>    both UDP and TCP.  The default port for TURN over TLS and TURN over
>>    DTLS is 5349.
>>
>> First, I was wondering if this document needed to update RFC 7065? The
>> high level obvious is the reference, but otherwise on the surface it
>> doesn't look necessary. My main complaint when reading was that it was
>> not obvious what not specifying the transport parameter means. Then I
>> saw this in Section 3.1 of RFC 7065:
>>
>>    The <host>, <port>, and <transport> components are passed without
>>    modification to the [RFC5928] algorithm.  <secure> is set to false if
>>    <scheme> is equal to "turn", and set to true if <scheme> is equal to
>>    "turns" and passed to the [RFC5928] algorithm with the other
>>    components.
>>
>> After some digging I figured out that RFC5928 is actually updated by RFC
>> 7350 that defines the resolution and SRV records for DTLS.
>>
>> Can you please update the text in Section 4 to also reference RFC 7350
>> so that one doesn't miss this definitions.
>>
>> H. Section 5:
>>
>>    If the server requires requests to be
>>    authenticated, then the server's administrator MUST choose a realm
>>    value that will uniquely identify the username and password
>>    combination that the client must use, even if the client uses
>>    multiple servers under different administrations.
>>
>> Just to verify, TURN does not allow a single server address to host
>> multiple realms do it? Each Realm needs its own unique server transport
>> address?
>>
>> I. Section 5:
>>
>>    For each Allocate request, the server SHOULD generate a
>>    new random nonce when the allocation is first attempted following the
>>    randomness recommendations in [RFC4086] and SHOULD expire the nonce
>>    at least once every hour during the lifetime of the allocation.
>>
>> So, I think this text due to its use of RFC 2119 language is a bit
>> confusing in its relation to STUNbis. To me it is unclear if the above
>> overrides completely what is written in Section 5 of STUNbis:
>>
>>    To indicate that it supports this specification, a server MUST
>>    prepend the NONCE attribute value with the character string composed
>>    of "obMatJos2" concatenated with the (4 character) Base64 [RFC4648]
>>    encoding of the 24 bit STUN Security Features as defined in
>>    Section 18.1.  The 24 bit Security Feature set is encoded as 3 bytes,
>>    with bit 0 as the most significant bit of the first byte and bit 23
>>    as the least significant bit of the third byte.  If no security
>>    features are used, then a byte array with all 24 bits set to zero
>>    MUST be encoded instead.  For the remainder of this document the term
>>    "nonce cookie" will refer to the complete 13 character string
>>    prepended to the NONCE attribute value.
>>
>> Or is the randomness recommendations in TURNbis only for the NONCE
>> attribute value part?
>>
>> Secondly, I interpret the text to say that compared to STUN, where a
>> Nonce can be long lived after the initial Long Term Credential
>> establishment, in TURN for each allocation one will have to to the
>> Allocate, expired nonce->here is your new nonce, resend allocate using
>> new nonce, success dance.
>>
>> I think part of the problem with the above text in TURNbis is that the
>> sentences prior to the quoted one appear to be rehashing of the STUNbis
>> requirements. Then the quoted part that actually changes behavior
>> without being clear about it. I think it would be good to make it clear
>> what is a change compared to STUNbis.
>>
>> J. Section 6:
>>
>>    Note that, rather than storing the password explicitly, for
>>    security reasons, it may be desirable for the server to store the key
>>    value, which is a secure hash over the username, realm, and password
>>    (see [I-D.ietf-tram-stunbis]).
>>
>> Why isn't this recommendation stronger. Are there any reasons why a
>> server would actually store the plain text password at all?
>>
>> K. Section 7.1:
>>
>>    The client first picks a host transport address.  It is RECOMMENDED
>>    that the client pick a currently unused transport address, typically
>>    by allowing the underlying OS to pick a currently unused port for a
>>    new socket.
>>
>> I think talk about socket in this context should be skipped as it
>> otherwise have been eliminated.
>>
>> L. Section 7.1:
>>
>>   The client then picks a transport protocol to use between the client
>>    and the server.  The transport protocol MUST be one of UDP, TCP, TLS-
>>    over-TCP or DTLS-over-UDP.
>>
>> I find it strange that this indicates that one should pick the transport
>> protocol to use client to server without considering the server
>> capabilities. Especially as the client configuration or server discovery
>> procedures will indicate transport protocol support.
>>
>> Secondly, what is the motivation for the MUST in the second sentence?
>> This is appears to have strange interactions with any future extensions
>> and have no impact. A client must obvious chose a client to server
>> transport protocol it supports, and one that the server supports or the
>> client hope it will support.
>>
>> M. Section 7.1:
>>
>>    The client also picks a server transport address, which SHOULD be
>>    done as follows.  The client uses one or more procedures described in
>>    [RFC8155] to discover a TURN server and uses the TURN server
>>    resolution mechanism defined in [RFC5928] to get a list of server
>>    transport addresses that can be tried to create a TURN allocation.
>>
>> 1) So RFC 8155 states that it has no recommendation on server selection,
>> nor on how to related local configuration to any auto-discovery servers.
>> To me it appears that the protocol utilizing TURN should consider what
>> makes sense in their deployments. Thus, does the need for such
>> consideration needs to be highlighted somewhere in this specification.
>>
>> 2) The RFC5928 reference  should be complemented with an RFC 7350
>> reference.
>>
>> N. Section 7.1:
>>
>>  Clients MUST NOT include a REQUESTED-ADDRESS-
>>    FAMILY attribute in an Allocate request that contains a RESERVATION-
>>    TOKEN attribute, for the reasons outlined in [RFC6156].
>>
>> Considering that this document obsoletes RFC 6156, I think it is bad
>> form to reference that document, rather than to move the necessary
>> motivation into this document.
>>
>> O. Section 7.1:
>>
>>    If the client wishes to obtain a relayed transport address of a
>>    specific address type then it includes a REQUESTED-ADDRESS-FAMILY
>>    attribute in the request.
>>
>> I don't find anything describing what the default behavior is if one
>> doesn't include the attribute, but the TURN server supports multiple
>> address families.
>>
>> I see that bullet 7 in Section 7.2 indicates that IPv4 is default.
>> Should the text in 7.1 be explicit about this default?
>>
>> P. Section 7.2:
>>
>>          Otherwise, if the attribute is included but
>>         specifies a protocol other that UDP, the server rejects the
>>         request with a 442 (Unsupported Transport Protocol) error.
>>
>> Assuming that you don't support an extension for that transport
>> protocol. Should that be made explicit?
>>
>> Q. Section 8.3:
>>
>>    If the client receives a success response to its Refresh request with
>>    a non-zero lifetime, it updates its copy of the allocation data
>>    structure with the time-to-expiry value contained in the response.
>>
>>    If the client receives a 437 (Allocation Mismatch) error response to
>>    a request to delete the allocation, then the allocation no longer
>>    exists and it should consider its request as having effectively
>>    succeeded.
>>
>> So there are no considerations for the client if it receives an error
>> response to a refresh request when Lifetime was non-zero?
>>
>> I am bit surprised by that. I assume that there are certain cases when
>> the request could be resent, and others where the meaning is that the
>> allocation is gone.
>>
>> R. Section 11.5:
>>
>>    When the server receives an ICMP packet, 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 contains a UDP header.  If
>>    either of these conditions fail, then the ICMP packet is silently
>>    dropped.
>>
>> Why is not ICMPv6 Type 4 (Parameter Problem) included when Parameter
>> Problem are for ICMPv4?
>>
>> S. Section 12.5:
>>
>>    Over TCP and TLS-over-TCP, the ChannelData message MUST be padded to
>>    a multiple of four bytes in order to ensure the alignment of
>>    subsequent messages.  The padding is not reflected in the length
>>    field of the ChannelData message, so the actual size of a ChannelData
>>    message (including padding) is (4 + Length) rounded up to the nearest
>>    multiple of 4.  Over UDP, the padding is not required but MAY be
>>    included.
>>
>> Are there anywhere written what one may or may not include of the TURN
>> related messages in a single UDP datagram. Because it is technically
>> feasible I belive to include two Send Indications in one UDP datagram to
>> the server.
>>
>> T. Section 13.1:
>>
>>    Flow Label
>>
>>       Preferred behavior: The relay sets the Flow label to 0.  The relay
>>       can choose to set the Flow label to a different value if it
>>       supports the IPv6 Flow Label field [RFC6437].
>>
>>       Alternate behavior: The relay sets the Flow label to the default
>>       value for outgoing packets.
>>
>> I think this section should be improved in two ways.
>>
>> First of all I think it should clarify that each allocation to peer flow
>> is its own IPv6 Flow in perspective of the IPv6 Flow label.
>>
>> Secondly, I think in the spirit of RFC 6473 the preferred behavior
>> should be to set the IPv6 flow label if the implementation is capable,
>> or leave it to the underlying network stack to actually set it. Compared
>> to RFC 7915, the TURN server actually have an understanding of what are
>> different flows without having examining additional protocol layers.
>>
>> Thirdly, I don't understand what the alternative behavior actually is.
>> This as it is either setting it to 0, or classifying things as flow and
>> then provide it with a random flow label. I think the right alternative
>> behavior is to set it 0.
>>
>> U. Section 13. 1,2,3:
>>
>>       For both preferred and alternate behavior, the DONT-FRAGMENT
>>       attribute MUST be ignored by the server.
>>
>> I don't understand why just because one performs v4<->v6 or v6 to v4
>> translation or v6 to v6 relaying now can ignore the DONT-FRAGEMENT
>> attribute. If the client has request to not fragment and the server is
>> unable to do that when forwarding to the peer it needs to generate an
>> ICMP attribute response saying the packet is to big. Otherwise the relay
>> breaks the Path MTU discovery mechanisms.
>>
>> V. Section 13.2:
>>
>> Fragmentation:
>>
>>       If the incoming packet included a Fragment header and the outgoing
>>       packet size exceeds the outgoing link's MTU, the relay MUST
>>       fragment the outgoing packet into fragments of no more than 1280
>>       bytes.  The relay sets the fields of the Fragment header as
>>       appropriate for a packet originating from the server.
>>
>> Is really the intention here to refragment the packet without providing
>> any indication that the practical MTU is smaller than what the peer or
>> client thinks it is?
>>
>> W. Section 14:
>>
>>       Preferred Behavior: Set the outgoing value to the incoming value,
>>       UNLESS the server is doing Active Queue Management, the incoming
>>       ECN field is ECT(1) (=0b01) or ECT(0) (=0b10), and the server
>>       wishes to indicate that congestion has been experienced, in which
>>       case set the outgoing value to CE (=0b11).
>>
>> First of all do you have any special meaning with all capital UNLESS?
>>
>> Secondly, I don't know how wise it is to have partial definitions of
>> what the ECN behavior is. Maybe simpler to say that that the relay is
>> performing AQM it can support the ECN marking behavior as defined in RFC
>> 3168.
>>
>> X. Section 14:
>>
>> How are these IP level fields handled when the transport is TCP or
>> TLS/TCP? There there are not necessary a one to one mapping between
>> datagrams and TCP packets. And if there are no support then there are
>> need for clarification of behaviors when using TCP based transport.
>>
>> Y. Section 16.4.  DATA
>>
>>    The DATA attribute is present in all Send and Data indications.  The
>>    value portion of this attribute is variable length and consists of
>>    the application data (that is, the data that would immediately follow
>>    the UDP header if the data was been sent directly between the client
>>    and the peer).  If the length of this attribute is not a multiple of
>>    4, then padding must be added after this attribute.
>>
>> Can you please clarify if one would get all the remaining bytes of the
>> IP packet payload or only the number of bytes the UDP length field
>> indiactes?
>>
>> The reason I ask is that there is possibility that there are a
>> discrepancy between these fields, something the proposal for an
>> experimental specification for UDP options attempt to utilize.
>> https://datatracker.ietf.org/doc/draft-ietf-tsvwg-udp-options/
>>
>> Note, I only want you to clarify what is actually included. I would
>> assume using the UDP length field makes sense in this case.
>>
>> Z. The use of "relay" in the meaning of "Turn Server". There are many
>> occurrences of relay in this meaning. Either make it explicit to be a
>> synonym or change it to server.
>>
>> AA. Section 16.7
>>
>> rffu is lacking a definition. I would suggest making it upper case and
>> copy the definition from 16.8.
>>
>> AB. Section 16.12:
>>
>> Rsvd in Format figure does not match definition below which uses "Reserved".
>>
>> AC. Section 16.12:
>>
>> Reason Phrase (variable). This field is under defined. First of all the
>> format of the text string is not defined, which it is in Stunbis
>> Error-Code attribute is defined as.
>>
>>    The reason phrase MUST be a UTF-8 [RFC3629] encoded
>>    sequence of less than 128 characters (which can be as long as 509
>>    bytes when encoding them or 763 bytes when decoding them).
>>
>> Secondly, I think it might be worth pointing out the padding here. It is
>> not clear if there is any form of string termination and how the padding
>> which will be interpreted as NULL characters 0-3 depending on the
>> alignment. I note this later issue appears to be existing in STUNBis also.
>>
>> AD. Section 16.2:
>>
>> Why isn't the text here say that this attribute may also be used to
>> indicate the desired lifetime in allocation requests?
>>
>> AE. Section 16.9: DONT-FRAGMENT
>>
>> I think it would be good to indicate the use of the attribute in
>> Allocate Requests also.
>>
>> AF. Section  24.2
>>
>> [Protocol-Numbers]
>>               "IANA Protocol Numbers Registry", 2005,
>>               <http://www.iana.org/assignments/protocol-numbers>.
>>
>> This is a normative reference based on how it is used in Section 16.8.
>>
>> AG. Seciton 18.
>>
>>    Sometime before the 20 minute lifetime is up, the client refreshes
>>    the allocation.  This is done using a Refresh request.  As before,
>>    the client includes the latest username, realm, and nonce values in
>>    the request.  The client also includes the SOFTWARE attribute,
>>    following the recommended practice of always including this attribute
>>    in Allocate and Refresh messages.  When the server receives the
>>    Refresh request, it notices that the nonce value has expired, and so
>>    replies with 438 (Stale Nonce) error given a new nonce value.  The
>>    client then reattempts the request, this time with the new nonce
>>    value.  This second attempt is accepted, and the server replies with
>>    a success response.  Note that the client did not include a LIFETIME
>>    attribute in the request, so the server refreshes the allocation for
>>    the default lifetime of 10 minutes (as can be seen by the LIFETIME
>>    attribute in the success response).
>>
>> I notice that this part nor the text before appears to discuss the
>> permission lifetime that will expire after 5 minutes. Thus, it needs to
>> be refreshed prior to the Allocation.
>>
>> AH. Section 18.
>>
>> I am missing any example having to do with Address Family interactions.
>>
>> AI. DTLS Cipher Suits
>>
>> Am I understanding it correct that the definition of what DTLS cipher
>> suit that a TURN Server currently is to support is the ones in RFC 7525?
>>
>> In that case, I think the wording is a bit weak as the only reference I
>> find are in Section 2.1:
>>
>> If (D)TLS transport is
>>    used between the TURN client and the TURN server, the guidance given
>>    in [RFC7525] MUST be followed to avoid attacks on (D)TLS.
>>
>>
>> Cheers
>>
>> Magnus Westerlund 
>>
>> ----------------------------------------------------------------------
>> Network Architecture & Protocols, Ericsson Research
>> ----------------------------------------------------------------------
>> Ericsson AB                 | Phone  [phone]
>> Torshamnsgatan 23           | Mobile [phone]
>> SE-164 80 Stockholm, Sweden | mailto: [e-mail]
>> ----------------------------------------------------------------------
>>
>>
>> _______________________________________________
>> tram mailing list
>> [e-mail]
>> https://www.ietf.org/mailman/listinfo/tram
>>
>