[tram] Benjamin Kaduk's Discuss on draft-ietf-tram-turnbis-27: (with DISCUSS and COMMENT)

Benjamin Kaduk via Datatracker <noreply@ietf.org> Thu, 11 July 2019 05:39 UTC

Return-Path: <noreply@ietf.org>
X-Original-To: tram@ietf.org
Delivered-To: tram@ietfa.amsl.com
Received: from ietfa.amsl.com (localhost [IPv6:::1]) by ietfa.amsl.com (Postfix) with ESMTP id 51858120018; Wed, 10 Jul 2019 22:39:54 -0700 (PDT)
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
From: Benjamin Kaduk via Datatracker <noreply@ietf.org>
To: "The IESG" <iesg@ietf.org>
Cc: draft-ietf-tram-turnbis@ietf.org, Brandon Williams <brandon.williams@akamai.com>, tram-chairs@ietf.org, brandon.williams@akamai.com, tram@ietf.org
X-Test-IDTracker: no
X-IETF-IDTracker: 6.98.3
Auto-Submitted: auto-generated
Precedence: bulk
Reply-To: Benjamin Kaduk <kaduk@mit.edu>
Message-ID: <156282359426.15056.17118634579091500165.idtracker@ietfa.amsl.com>
Date: Wed, 10 Jul 2019 22:39:54 -0700
Archived-At: <https://mailarchive.ietf.org/arch/msg/tram/sJiV27rypfT-rApbcYOJmf51h3k>
Subject: [tram] Benjamin Kaduk's Discuss on draft-ietf-tram-turnbis-27: (with DISCUSS and COMMENT)
X-BeenThere: tram@ietf.org
X-Mailman-Version: 2.1.29
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, 11 Jul 2019 05:39:55 -0000

Benjamin Kaduk has entered the following ballot position for
draft-ietf-tram-turnbis-27: Discuss

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)


Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.


The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-tram-turnbis/



----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

I support Roman's Discuss.

I think I'm confused about whether REQUESTED-ADDRESS-FAMILY and
ADDITIONAL-ADDRESS-FAMILY are mutually exclusive.  Does sending just the
"additional" one secretly mean that you want both v4 and v6?

Section 5

I see that the secdir reviewer asked about why it is a "SHOULD" to send
SOFTWARE, and only got a response that it is defined in stunbis, but not
about why it is recommended.  There remain some privacy/security
considerations with it, and we should either point to the stunbis
security considerations or not recommend using it.  (FINGERPRINT is,
IIRC, less interesting from a security perspective as it's just about
confirming that given traffic is in fact STUN and not something else.)

Section 12

Do we need to say anything about backwards compatibility with RFC 5766
peers that use a wider range of channel IDs?

Section 12.7

   When the server receives an ICMP packet, the server processes it as
   described in Section 11.5.  A Data indication MUST be sent regardless
   of whether there is a channel bound to the peer that was the
   destination of the UDP datagram that triggered the reception of the
   ICMP packet.

This MUST seems potentially in conflict with the previous discussion
about permissions checks in Section 11.5.

Section 20

Looking at the
new nonce in the example ("obMatJos2AAABadl7W7PeDU4hKE72jda"), and
noting that it starts with the nonce cookie, help me decode the security
feature bits.  The magic prefix is "obMatJos2" so the capability bits
are encoded as "AAAB", which decodes to (hex) 00 00 01.  But stunbis
says that the bits are ordered from 0 (MSB of first byte) to 23 (LSB of
last byte), so this would have bit 23 set, which is in contrast to the
registry marking bit 0 as the password-algorithms feature.  Where am I
messing this up?

I'd prefer if the examples showed more usage of MESSAGE-INTEGRITY-SHA256
(especially, any from the server) and fewer MESSAGE-INTEGRITY.


----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

Thanks for keeping the diff from RFC 5766 relatively contained!
I only reviewed the diff due to time constraints, but in general support
reviewing the full document for internal consistency and any remarks
that have not aged well, especially with respect to IANA considerations.

Section 2

RFC 8174 has an updated version of the BCP 14 boilerplate text.

Section 3.1

I appreciate the response to the secdir review, but even the linked
stunbis section does not say much about which PKI's trust root is
expected to be used.  I'm forced to assume the Web PKI but don't have
much grounds for that assumption.

Section 3.2

The secdir reviewer's comments seem to have some relevance.
It seems like even when the third-party authorization mechanism is used,
the client is still limited to "the server is authenticated because it
gives me the service I asked for", which is not really documented
anywhere yet.  The long-term password mechanism at least generates an
HMAC integrity tag on messages from server to client, which can provide
some level of authentication via key confirmation.

Section  3.7

                                       If the TURN server carrying out
   packet translation from IPv4-to-IPv6 cannot get the Don't Fragment
   (DF) bit in the IPv4 header, it MUST reject the Allocate request with
   DONT-FRAGMENT attribute.

nit: does "cannot get" mean something like "read the value as sent on
the wire" (e.g., due to implementation/API limitations)?

Section 6

   o  the relayed transport address or addresses;
   [...]

   [...]
   address.  The relayed transport address MUST be unique across all
   allocations, so it can be used to uniquely identify the allocation.

nit: there's maybe a singular/plural mismatch going on here, but I don't
have any good ideas.

Section 7.1

   If the client wishes to obtain one IPv6 and one IPv4 relayed
   transport address then it includes an ADDITIONAL-ADDRESS-FAMILY
   attribute in the request.  This attribute specifies that the server
   must allocate both address types.  The attribute value in the
   ADDITIONAL-ADDRESS-FAMILY MUST be set to 0x02 (IPv6 address family).
   Clients MUST NOT include REQUESTED-ADDRESS-FAMILY and ADDITIONAL-
   ADDRESS-FAMILY attributes in the same request.  [...]

This reads like setting REQUESTED-ADDRESS-FAMILY and
ADDITIONAL-ADDRESS-FAMILY are mutually exclusive.  Is that the correct
reading?  (Naively, one might expect that the thing named "additional"
is applied on top of the base-level request, so I have to ask.)

Section 7.2

Why do we only have a SHOULD-level requirement for authentication of
Allocate requests where 5766 had MUST?  Could we say "may allow
unauthenticated requests in order to support [use case] but otherwise
MUST require authentication" (with better wording)?  I suppose this has
probably been discussed ad nauseum already, so feel free to point me
at the archives before getting into an actual discussion.

   7.   If the server does not support the address family requested by
        the client in REQUESTED-ADDRESS-FAMILY or is disabled by local
        policy, it MUST generate an Allocate error response, and it MUST
        include an ERROR-CODE attribute with the 440 (Address Family not
        Supported) response code.  [...]

nit: "or is disabled" needs a subject.

   9.   The server checks if the request contains an ADDITIONAL-ADDRESS-
        FAMILY attribute.  If yes, and the attribute value is 0x01 (IPv4
        address family), then the server rejects the request with a 400
        (Bad Request) error.  Otherwise, the server checks if it can
        allocate relayed transport addresses of both address types.  If
        the server cannot satisfy the request, then the server rejects

What are "both" address types?  We only have the
ADDITIONAL-ADDRESS-FAMILY (which contains one type) and no
REQUESTED-ADDRESS-FAMILY.

Section 8.1

   When refreshing a dual allocation, the client includes REQUESTED-
   ADDRESS-FAMILY attribute indicating the address family type that
   should be refreshed.  If no REQUESTED-ADDRESS-FAMILY is included then
   the request should be treated as applying to all current allocations.
   The client MUST only include family types it previously allocated and
   has not yet deleted.  [...]

I'm a bit confused about "only include family types" plural; I thought
there could only be at most one REQUESTED-ADDRESS-FAMILY and that it
would only indicate one family type.  So how could  there be multiple
types getting included by the client?

Section 11.5

[check that "type and code" is valid in both ICMPv4 and v6]

Section 12

   Reserved values may be used in the future by other protocols.  When
   the client uses channel binding, it MUST comply with the
   demultiplexing scheme discussed above.

"channel binding" is a term of art in the security world; is this usage
intended to roughly mean "channel multiplexing"?
(I do see that the subsections are talking about the specific
ChannelBind request, so I assume so; I just want to double check.)


Section 13

   The descriptions below have two parts: a preferred behavior and an
   alternate behavior.  The server SHOULD implement the preferred
   behavior.  Otherwise, the server MUST implement the alternate
   behavior and MUST NOT do anything else for the reasons detailed in
   [RFC7915].  [...]

RFC 5766 had this split on a per-field basis, but this text suggests
that if the preferred behavior is not possible for a given field, then
the alternate behavior is used for the entire packet.
I note that section 14 does retain the "for a particular field" language
for UDP-to-UDP relaying, as do section 15 for TCP-to-UDP relaying
and section 16 for UDP-to-TCP relaying.

Section 13.2

Is this flow label text consistent with both the inbound and outbound
translation directions (specifically, using relayed/peer addresses in
the 5-tuple to identify the flow)?

      If the incoming packet did not include a Fragment header and the
      outgoing packet size exceeds the outgoing link's MTU, the TURN
      server drops the outgoing packet and send an ICMP message of type
      2 code 0 ("Packet too big") to the sender of the incoming packet.
       If the packet is being sent to the peer, the TURN server reduces
      the MTU reported in the ICMP message by 48 bytes to allow room for
      the overhead of a Data indication.

(nit?) "If the packet is being send to the peer" seems to grammatically
refer to the outgoing packet that would exceed link MTU, as opposed to
the ICMP message being generated.  But I think it's supposed to refer to
the ICMP message being generated, which has not yet been referred to as
a "packet" in this text.

Section 13.3

[Same comment about ICMP packet generation/grammar]

Section 15

Maybe note that SIP and WebRTC are the primary consumers of TURN?

                                                        Even if both
   TCP-AO and UDP authentication would be used between TURN client and
   server, it would not change the end-to-end security properties of the
   UDP payload being relayed.  [...]

I wonder if we even need to say "UDP" in "payload being relayed".

Section 16

   Fragmentation

      Preferred Behavior: Any fragmented packets are reassembled in the
      server and then forwarded to the client over the TCP connection.
      ICMP messages resulting from the UDP datagrams sent to the peer
      MUST be forwarded to the client using TURN's mechanism for
      relevant ICMP types and codes.

As in section 12.7, I wonder if this MUST could be seen as overriding
other logic for ICMP handling.

Section 21.16

I agree with the secdir reviewer that there is at least potential for
username and realm to be sensitive.

Section 21.4

Do we need references for Teredo and/or 6to4?

Section 22

   The codepoints for the STUN error codes defined in this specification
   are listed in Section 19.  [IANA is requested to update the reference
   from [RFC5766] to RFC-to-be for the STUN error codes listed in
   Section 19.]

(I think some of them are from RFC 6156 as well as from 5766.)

Section 23

It seems prudent to revisit these listed IAB Considerations for whether
the situation has changed in the past ten years (though I don't have any
specific points that I would change).

Section 25

(nit) are these really "updates" to RFC 6156 so much as incorporating its
content wholesale into the core TURN specification?