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

From: Marc Petit-Huguenin <[email-masked]>
Date: Thu, 3 May 2018 16:37:22 -0700

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.

---

From: Adam Roach <[email-masked]>
Date: Mon, 21 May 2018 13:11:27 -0500

Sorry for taking so long to get back to you on this. Two responses below 
-- everything else looks good.

>>> The attribute 0x0003 is registered by RFC 5780, and should not be removed by this document.

Thanks for the change, but the new text still asks IANA to update the 
table so that 0x0003 points to *this* document, instead of continuing to 
point to RFC 5780. Since this document does not do anything with 
CHANGE-REQUEST, this update does not seem correct.

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

I'm not sure what was meant either!

I think we need to untangle what the working group meant to say 
regarding "trusted IP addresses," the way this protocol is intended to 
use certs, and whether the prohibition on using IP addresses in "stuns" 
URIs derives from cert handling or if it has a completely different 
rationale behind it; and, if the former, ensure that those things that 
are prohibited or allowed in certs are similarly prohibited or allowed 
in URIs.

I can suggest some *behavior*, but unless there is some record of what 
the WG meant, any such behavior would need to be discussed by the 
working group, and a consensus would need to be declared by the chairs.

/a