Telechat Review of draft-ietf-tram-stunbis-16
review-ietf-tram-stunbis-16-genart-telechat-worley-2018-03-29-00

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.

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.

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

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/.

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.

14.4.  USERHASH

   userhash = SHA-256(Opaque(username) ":" Opaque(realm))

I believe that this should be

   userhash = SHA-256(OpaqueString(username) ":" OpaqueString(realm))

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.

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.

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

[END]