Subject: Re: [tram] Genart telechat review of draft-ietf-tram-stunbis-16

From: Marc Petit-Huguenin <[email-masked]>
Date: Mon, 16 Apr 2018 14:49:15 -0700

Thanks again for the review.  Comments inline.

On 03/30/2018 04:45 AM, Dale Worley wrote:
> Reviewer: Dale Worley
> Review result: Ready with Nits
> 
> I am the assigned Gen-ART reviewer for this draft.  The General Area
> Review Team (Gen-ART) reviews all IETF documents being processed by
> the IESG for the IETF Chair.  Please wait for direction from your
> document shepherd or AD before posting a new version of the draft.
> 
> For more information, please see the FAQ at
> <https://wiki.tools.ietf.org/area/gen/wiki/GenArtfaq>.
> 
> Document:  draft-ietf-tram-stunbis-16
> Reviewer:  Dale R. Worley
> Review Date:  2018-03-29
> IETF LC End Date:  2018-02-20
> IESG Telechat date:  2018-04-19
> 
> Summary:
> 
>        This draft is basically ready for publication, but has nits
>        that should be fixed before publication.
> 
> The only interesting item concerns section 17.1, where the assignment
> of meanings to bits in the "security feature set" value is different
> from the assignment in -16.  This is either non-upward-compatible with
> -16, or there is an error in either -16 or -17.
> 
> ----------------------------------------------------------------------
> 
> There is an issue that shows up in several places:  The NAT may
> forward the request using an IP family that is different from the IP
> family that it received the request using.  This means that the
> "source IP family of the request" may depend on whether one is
> speaking of the client or the server.  The draft is cognizant of this,
> and mentions its consequences in sections 6.3.3 and 12.  But this also
> has consequences for ALTERNATE-SERVER:  Section 14.15 says "The IP
> address family MUST be identical to that of the source IP address of
> the request." even though that family might not be usable by the
> client.  The draft doesn't seem to explicitly say that this comes from
> address-switching by the NAT.  It would help if there was a
> higher-level discussion of this matter, pointing to the various
> consequences.

I still do not have text about that but, as this is blocking this response since 2 weeks now, I am releasing it as is and will come back to that after I process the other reviews that accumulated during my time traveling around Europe.

> 
> 3.  Terminology
> 
> This section needs to be updated to reference RFC 8174, including
> updating the paragraph (especially the final two lines) to read as
> specified in RFC 8174:
> 
>       The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL
>       NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED",
>       "MAY", and "OPTIONAL" in this document are to be interpreted as
>       described in BCP 14 [RFC2119] [RFC8174] when, and only when, they
>       appear in all capitals, as shown here.

Updated.

> 
> 6.2.2.  Sending over TCP or TLS-over-TCP
> 
>    o  if multiplexing other application protocols over that port, has
>       finished using that other protocol, and
> 
> The two clauses don't match in grammatical number.  This should be
> either
> 
>    o  if multiplexing other application protocols over that port, has
>       finished using those other protocols, and
> 
> or
> 
>    o  if multiplexing another application protocol over that port, has
>       finished using that other protocol, and

Updated using the former.

> 
> 9.2.4.  Receiving a Request
> 
>       *  If the request contains neither PASSWORD-ALGORITHMS nor
>          PASSWORD- ALGORITHM, then the request is processed as though
>          PASSWORD- ALGORITHM were MD5 (Note that if the original
>          PASSWORD-ALGORITHMS attribute did not contain MD5, this will
>          result in a 400 Bad Request in a later step below).
> 
> There are two places where s/PASSWORD- ALGORITHM/PASSWORD-ALGORITHM/.

Already fixed following a previous review.

> 
> 14.3.  USERNAME
> 
>    The value of USERNAME is a variable-length value containing the
>    authentication username.  It MUST contain a UTF-8 [RFC3629] encoded
>    sequence of less than 509 bytes, and MUST have been processed using
>    the OpaqueString profile [RFC8265].  A compliant implementation MUST
>    be able to parse UTF-8 encoded sequence of less than 763.
> 
> The final "763" is grammatically incorrect.  I think you intend
> s/763/763 bytes/, to parallel other items.

Fixed.

> 
> 14.4.  USERHASH
> 
>    userhash = SHA-256(Opaque(username) ":" Opaque(realm))
> 
> I believe that this should be
> 
>    userhash = SHA-256(OpaqueString(username) ":" OpaqueString(realm))

Fixed.

> 
> 14.5.  MESSAGE-INTEGRITY
> 
>    Petit-Huguenin, et al.  Expires September 6, 2018 [Page 40]
> 
>    Internet-Draft Session Traversal Utilities for NAT (STUN) March 2018
> 
> This bit of text appears as body text in the middle of page 40.

Already fixed following a previous review.

> 
> 14.6.  MESSAGE-INTEGRITY-SHA256
> 
>    The MESSAGE-INTEGRITY-SHA256 attribute contains an HMAC-SHA256
>    [RFC2104] of the STUN message.  The MESSAGE-INTEGRITY-SHA256
>    attribute can be present in any STUN message type.  The MESSAGE-
>    INTEGRITY-SHA256 attribute contains an initial portion of the HMAC-
>    SHA-256 [RFC2104] of the STUN message.  The value will be at most 32
>    bytes and MUST be a positive multiple of 4 bytes.  The HMAC MUST NOT
>    be truncated below a minimum size of 16 bytes.  The value must be the
>    full 32 bytes unless the STUN Usage explicitly specifies that
>    truncation is allowed.  STUN Usages may specify a minimum length
>    longer than 4 bytes.
> 
> This seems to be less clear than it could be.  (And my apologies, some
> of the redundancy was my suggestion!)  Perhaps this is an improvement:
> 
>    The value will be at most 32 bytes, MUST be at least 16 bytes, and
>    MUST be a multiple of 4 bytes.  The value must be the full 32 bytes
>    unless the STUN Usage explicitly specifies that truncation is
>    allowed.  STUN Usages may specify a minimum length longer than 16
>    bytes.

Applied.

> 
> 17.1.  STUN Security Features Registry
> 
>    A STUN Security Feature set defines 24 bit as flags.
> 
>    IANA is requested to create a new registry containing the STUN
>    Security Features that are protected by the bid down attack
>    prevention mechanism described in section Section 9.2.1.
> 
>    The initial STUN Security Features are:
> 
>    Bit 0: Password algorithms
>    Bit 1: Username anonymity
>    Bit 2-23: Unassigned
> 
> Beware that the assignment of features to bits in the security feature
> value has changed!  Bit numbers are assigned from the left/high-order
> end -- see figure 2 in the draft.  So the *values* for these bits are:
> 
> 0x400000   Bit 0: Password algorithms
> 0x200000   Bit 1: Username anonymity
> 
> But the values assigned in -15 were:
> 
>    0x000001: Password algorithms
>    0x000002: Username anonymity

Hmm, that was not the intent, and not how I read the text. Even in Figure 2 less significant bits are on the right.  So the intent is real tat bit0=0x001, bit1 =
0x002, and so on.  All we do is about interoperability, so I added some text that makes that less ambiguous.

-- 
Marc Petit-Huguenin
Email: [email-masked]
Blog: https://marc.petit-huguenin.org
Profile: https://www.linkedin.com/in/petithug

---

From: Alissa Cooper <[email-masked]>
Date: Wed, 18 Apr 2018 21:56:07 -0400

Dale, thanks for your reviews. Marc, thanks for your response. I have entered a No Objection ballot.

Alissa

> On Apr 16, 2018, at 5:49 PM, Marc Petit-Huguenin <[email-masked]> wrote:
> 
> Email: [email-masked] <mailto:[email-masked]>
> Blog: https://marc.petit-huguenin.org <https://marc.petit-huguenin.org/>
> Profile: https://www.linkedin.com/in/petithug <https://www.linkedin.com/in/petithug>
> 
> _______________________________________________
> tram mailing list
> [email-masked] <mailto:[email-masked]>
> https://www.ietf.org/mailman/listinfo/tram <https://www.ietf.org/mailman/listinfo/tram>

---

From: [email-masked] (Dale R. Worley)
Date: Sun, 22 Apr 2018 22:12:26 -0400

My apologies for not responding to this sooner:

Marc Petit-Huguenin <[email-masked]> writes:
> Hmm, that was not the intent, and not how I read the text. Even in
> Figure 2 less significant bits are on the right.  So the intent is
> real tat bit0=0x001, bit1 = 0x002, and so on.  All we do is about
> interoperability, so I added some text that makes that less ambiguous.

(The latest posted version is still -16, so I haven't seen the changes
you refer to.)

>>    Bit 1: Username anonymity

I've skimmed the text again, and it appears that this passage is the
only one that allocates the functions "password algorithms" and
"username anonymity" to specific bits of the security feature value.
Thus, it's critical what "bit 0" means in this context.

Searching for "0" in the document, I can find no outright definition of
the meaning of "bit 0".  However, there are 10 figures in the document,
and 8 of them show the numbering of the bits of various fields, with bit
"0" being the *high-order*, *most-significant*, "left"-most bit of the
field, bit "1" the next-highest-order bit, etc.  This numbering seems to
be conventional in RFCs.

Thus, I recommend that if you really want to persist in calling the
low-order bit of the security features field "bit 0", you should explain
that very clearly in section 17.1.

Dale

---

From: Marc Petit-Huguenin <[email-masked]>
Date: Mon, 23 Apr 2018 10:10:58 -0700

> (The latest posted version is still -16, so I haven't seen the changes

Sorry, I should have pasted the text.  But the point is moot, see below.

> field, bit "1" the next-highest-order bit, etc.  This numbering seems to

I did some research and you were right, starting from the left side is more common.  My main counter-example is in fact in STUN itself as figure 3:

                       0                 1
                       2  3  4 5 6 7 8 9 0 1 2 3 4 5
                      +--+--+-+-+-+-+-+-+-+-+-+-+-+-+
                      |M |M |M|M|M|C|M|M|M|C|M|M|M|M|
                      |11|10|9|8|7|1|6|5|4|0|3|2|1|0|
                      +--+--+-+-+-+-+-+-+-+-+-+-+-+-+

And as the base64 encoding is done on an array of bytes and not an integer, it is in fact simpler to define it that way. The new text in section now reads:

   Bits are assigned starting from the most significant side of the bit
   set, so Bit 0 is the leftmost bit and Bit 23 the rightmost bit.

I also updated the example in Appendix B.1.

---

From: [email-masked] (Dale R. Worley)
Date: Mon, 23 Apr 2018 21:38:49 -0400

Marc Petit-Huguenin <[email-masked]> writes:
> I did some research and you were right, starting from the left side is
> more common.  My main counter-example is in fact in STUN itself as
> figure 3:
>
>                        0                 1
[...]

That's an interesting complication.  As you've noted, it's nearly
universal for IETF documents to use big-endian numbering.

But the above example is paradoxical, as the bits are numbered in a
big-endian way across the top of the diagram, from 2 to 15, while being
numbered in a littl-endian way is the boxes, from 11 to 0.  I had
noticed that the top numbers didn't start at 0 (so I didn't count this
figure in my "8 out of 10"), but I overlooked the numbers in the boxes.

Dale