Re: [tram] Adam Roach's Discuss on draft-ietf-tram-stunbis-16: (with DISCUSS and COMMENT)

Marc Petit-Huguenin <marc@petit-huguenin.org> Thu, 03 May 2018 23:37 UTC

Return-Path: <marc@petit-huguenin.org>
X-Original-To: tram@ietfa.amsl.com
Delivered-To: tram@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 7B40512EB8B; Thu, 3 May 2018 16:37:31 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.108
X-Spam-Level:
X-Spam-Status: No, score=-1.108 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, RDNS_NONE=0.793, SPF_PASS=-0.001] autolearn=no autolearn_force=no
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id C6PM6GECYQ9m; Thu, 3 May 2018 16:37:29 -0700 (PDT)
Received: from implementers.org (unknown [IPv6:2001:4b98:dc0:45:216:3eff:fe7f:7abd]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 7CE9712EB5A; Thu, 3 May 2018 16:37:29 -0700 (PDT)
Received: from [IPv6:2001:0:53aa:64c:18b5:3a25:f31a:9fd] (unknown [IPv6:2001:0:53aa:64c:18b5:3a25:f31a:9fd]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "Marc Petit-Huguenin", Issuer "implementers.org" (verified OK)) by implementers.org (Postfix) with ESMTPS id 13084AE844; Fri, 4 May 2018 01:37:25 +0200 (CEST)
From: Marc Petit-Huguenin <marc@petit-huguenin.org>
To: Adam Roach <adam@nostrum.com>, The IESG <iesg@ietf.org>
Cc: tram-chairs@ietf.org, Tolga Asveren <tasveren@rbbn.com>, Gonzalo.Camarillo@ericsson.com, draft-ietf-tram-stunbis@ietf.org, tram@ietf.org
References: <152403138853.31946.14807823535362928987.idtracker@ietfa.amsl.com> <27cb2f70-d907-b61f-bb5a-6b19053238fe@petit-huguenin.org>
Message-ID: <1e8cd5de-06de-6745-fc4d-d15fcdd0b4d9@petit-huguenin.org>
Date: Thu, 3 May 2018 16:37:22 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0
MIME-Version: 1.0
In-Reply-To: <27cb2f70-d907-b61f-bb5a-6b19053238fe@petit-huguenin.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Archived-At: <https://mailarchive.ietf.org/arch/msg/tram/RY5ckk6bqJOdEjzL1xU-z0nqDps>
Subject: Re: [tram] Adam Roach's Discuss on draft-ietf-tram-stunbis-16: (with DISCUSS and COMMENT)
X-BeenThere: tram@ietf.org
X-Mailman-Version: 2.1.22
Precedence: list
List-Id: "Discussing the creation of a Turn Revised And Modernized \(TRAM\) WG, which goal is to consolidate the various initiatives to update TURN and STUN." <tram.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/tram>, <mailto:tram-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/tram/>
List-Post: <mailto:tram@ietf.org>
List-Help: <mailto:tram-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/tram>, <mailto:tram-request@ietf.org?subject=subscribe>
X-List-Received-Date: Thu, 03 May 2018 23:37:35 -0000

On 04/23/2018 03:37 PM, Marc Petit-Huguenin wrote:
> Hi Adam,
> 
> Thanks for the review, please see my responses inline.
> 
> This is also the last review I can process until Saturday 28.  The remaining of the work to do on stunbis is:
> 
> - process Alexey Melnikov review
> - process Process Benjamin Kaduk review
> - add text for Dale Worley point on IPv6 NAT
> - add text explaining the bid-attack protection mechanism
> - work on answer to §6.2.3 below
> - answer additional concerns following my responses
> - final review for language
> - publish -17
> 
> 
> On 04/17/2018 11:03 PM, Adam Roach wrote:
>> Adam Roach has entered the following ballot position for
>> draft-ietf-tram-stunbis-16: Discuss
>>
> 
> [...]
> 
>>
>>
>> ----------------------------------------------------------------------
>> DISCUSS:
>> ----------------------------------------------------------------------
>>
>> Thanks to everyone who worked on this revision of the STUN protocol. Thanks in
>> particular to the ARTART reviewer and to the authors for actively engaging on
>> the points he raised.
>>
>> I have one concern about interoperability and another about the IANA changes
>> that I believe require changes to the document prior to publication.
>>
>> §14.2:
>>
>>>  X-Port is computed by taking the mapped port in host byte order,
>>>  XOR'ing it with the most significant 16 bits of the magic cookie, and
>>>  then the converting the result to network byte order.  If the IP
>>>  address family is IPv4, X-Address is computed by taking the mapped IP
>>>  address in host byte order, XOR'ing it with the magic cookie, and
>>>  converting the result to network byte order.  If the IP address
>>>  family is IPv6, X-Address is computed by taking the mapped IP address
>>>  in host byte order, XOR'ing it with the concatenation of the magic
>>>  cookie and the 96-bit transaction ID, and converting the result to
>>>  network byte order.
>>
>> The discussion of performing operations "in host byte order" is very confusing,
>> and seems likely to cause issues communicating between machines of different
>> endianness. As an implementor, based on this description, I cannot tell
>> whether, given a port of 0x1234 (and operating on a little-endian machine),
>> I'm supposed to do:
>>
>> Port (host order):   34 12
>> Magic Cookie Prefix: 21 12
>> Result (host order): 15 00
>> X-Port (net order):  00 15
>>
>> or:
>>
>> Port (host order):   34 12
>> Magic Cookie Prefix: 12 21
>> Result (host order): 26 33
>> X-Port (net order):  33 26
>>
>> One of these is clearly wrong. I think it's the first one, but I *also* think
>> that the first one is the most straightforward interpretation of the quoted
>> paragraph.
>>
>> The following would seem to be a complete description of the
>> operation without introducing possible confusion about the difference between
>> host and network order:
>>
>>    X-Port is computed by XOR'ing the mapped port with the most significant 16
>>    bits of the magic cookie.  If the IP address family is IPv4, X-Address is
>>    computed XOR'ing the mapped IP with the magic cookie.  If the IP address
>>    family is IPv6, X-Address is computed by XOR'ing the mapped IP address with
>>    the concatenation of the magic cookie and the 96-bit transaction ID. In all
>>    cases, the XOR operation works on its inputs in network byte order (that is,
>>    the order they will be encoded in the message).
>>
>> This makes it clear that the proper operation is:
>>
>> Port:                12 34
>> Magic Cookie Prefix: 21 12
>> Result / X-Port:     33 26
> 
> It seems that you are advocating the opposite of what is traditionally done on that subject:  Always do all the calculations using the computer order and eventually convert to/from the network order when writing/reading a packet.  That basically force all little-endian computers (which is most computers nowadays) to do their work in big-endian format.
> 
> If there is some ambiguity in the text, I am all for fixing it, for example by saying that the 16 bits of the magic cookie, the magic cookie and the concatenation of the magic cookie and the 96-bit transaction ID should all be converted into host byte order before been used with the XOR operation - which is done automatically when loading them in integer variables.
> 
>>
>> ---------------------------------------------------------------------------
>>
>> §17.3.1:
>>
>>>  IANA is requested to update the names for attributes 0x0002, 0x0003,
>>>  0x0004, 0x0005, 0x0007, and 0x000B, and the reference from RFC 5389
>>>  to RFC-to-be for the following STUN methods:
>> ...
>>>  0x0003: (Reserved; prior to [RFC5389] this was CHANGE-REQUEST)
>>
>> The attribute 0x0003 is registered by RFC 5780, and should not be removed by this document.
> 
> Fixed.
> 
>>
>>
>> ----------------------------------------------------------------------
>> COMMENT:
>> ----------------------------------------------------------------------
>>
>> §3:
>>
>> This is almost, but not quite, the boilerplate defined by RFC 8174. Please
>> update to match the text in RFC 8174.
> 
> Already fixed following a previous review.
> 
>>
>> ---------------------------------------------------------------------------
>>
>> §5:
>>
>>>  server to detect if the client will understand certain attributes
>>>  that were added in this revised specification.
>>
>> This is a *bit* misleading, as these attributes were actually added in RFC 5389.
>> Consider: "...that were added to STUN by [RFC5389]."
> 
> Applied.
> 
>>
>> ---------------------------------------------------------------------------
>>
>> §6.1:
>>
>>>  If the agent is sending a request, it SHOULD add a SOFTWARE attribute
>>>  to the request.
>>
>> I believe this would benefit from a pointer to the security section; e.g.:
>> "Note that the inclusion of a SOFTWARE attribute may have security
>> implications; see Section 15.1.2 for details."
> 
> Applied.
> 
>>
>> ---------------------------------------------------------------------------
>>
>> §6.2:
>>
>>>  STUN may be used
>>>  with anycast addresses, but only with UDP and in usages where
>>>  authentication is not used.
>>
>> This bit of operational advice seems out of place in the middle of an
>> implementation discussion, and is quite likely to be missed by its intended
>> audience. Consider relocating it to an "Operational Considerations" section.
> 
> Done.
> 
>>
>> ---------------------------------------------------------------------------
>>
>> §6.2.1:
>>
>>>  As with TCP, the usage of Karn's
>>>  algorithm is RECOMMENDED [KARN87].
>>
>> This normative language means that [KARN87] needs to be a normative reference.
> 
> Done.
> 
>>
>> ---------------------------------------------------------------------------
>>
>> §6.2.3 says:
>>
>>>  Alternatively, a
>>>  client MAY be configured with a set of IP addresses that are trusted;
>>>  if a certificate is received that identifies one of those IP
>>>  addresses, the client considers the identity of the server to be
>>>  verified.
>>
>> Presumably, this means the server supplies a certificate with SubjectAltName
>> containing an iPAddress? Please add text to clarify whether that's the
>> intention.
>>
>> If that *is* the intended meaning, then this behavior in §8.1 seems
>> unnecessarily restrictive:
>>
>>>  A "stuns" URI
>>>  containing an IP address MUST be rejected, unless the domain name is
>>>  provided by the same mechanism that provided the STUN URI, and that
>>>  domain name can be passed to the verification code.
>>
>> Presumably, this is done because certs with iPAddress-form SubjectAltNames are
>> pretty rare (although CAB Forum baseline requirements do explicitly allow
>> their issuance) -- but if the text in §6.2.3 is based on allowing the use of
>> such certs in a TURN deployment, then it seems that URI resolution should be
>> also.
>>
> 
> I am not sure what was the intent there, so I'll work on that later.

We addressed all the other comments, but it would be great if you could suggest some text to address that one.

Thanks.