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

["Konda, Tirumaleswar Reddy" <[e-mail]>]

Hi Ben,

Please see inline

> -----Original Message-----
> From: Benjamin Kaduk <[e-mail]>
> Sent: Saturday, July 13, 2019 7:45 AM
> To: Konda, Tirumaleswar Reddy <[e-mail]>
> Cc: The IESG <[e-mail]>; [e-mail]; draft-ietf-tram-
> [e-mail]; [e-mail]; [e-mail]
> Subject: Re: [tram] Benjamin Kaduk's Discuss on draft-ietf-tram-turnbis-27:
> (with DISCUSS and COMMENT)
> 
> This email originated from outside of the organization. Do not click links or
> open attachments unless you recognize the sender and know the content is
> safe.
> 
> Hi Tiru,
> 
> On Thu, Jul 11, 2019 at 03:47:19PM +0000, Konda, Tirumaleswar Reddy wrote:
> > Hi Ben,
> >
> > Thanks for the detailed review. Please see inline
> 
> Sorry to have made you deduplicate with the reviews that had already come
> in -- I wrote this text fairly early (on just the diff) with intent to return and
> tidy up, but ran out of time and had to just submit what I had.

No problem, the review helped to improve the draft. 

> 
> > > -----Original Message-----
> > > From: tram <[e-mail]> On Behalf Of Benjamin Kaduk via
> > > Datatracker
> > > Sent: Thursday, July 11, 2019 11:10 AM
> > > To: The IESG <[e-mail]>
> > > Cc: [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]
> > > Subject: [tram] Benjamin Kaduk's Discuss on
> > > draft-ietf-tram-turnbis-27: (with DISCUSS and COMMENT)
> > >
> > >
> > >
> > > 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 have updated the draft to address Roman's Discuss.
> >
> > >
> > > I think I'm confused about whether REQUESTED-ADDRESS-FAMILY and
> > > ADDITIONAL-ADDRESS-FAMILY are mutually exclusive.
> >
> > Yes, both attributes are mutually exclusive. It is discussed in Section 7.1,
> Clients MUST NOT include REQUESTED-ADDRESS-FAMILY and ADDITIONAL-
> ADDRESS-FAMILY attributes in the same request.
> 
> Okay.  This seems weird to  me and I wouldn't object if some very blunt text
> was added early on about "when both IPv4 and IPv6 are desired, the
> ADDITIONAL-ADDRESS-FAMILY attribute alone serves to request both
> address types; otherwise the single REQUESTED-ADDRESS-FAMILY attribute
> suffices", but this point clearly is not Discuss-worthy.

This point is already covered in Section 5 (General Behavior) as follows:

TURN, as defined in this specification, supports both IPv4 and IPv6. IPv6 support in TURN includes IPv4-to-IPv6, IPv6-to-IPv6, and IPv6-to-IPv4 relaying. The REQUESTED-ADDRESS-FAMILY attribute allows a client to explicitly request the address type the TURN server will allocate (e.g., an IPv4-only node may request the TURN server to allocate an IPv6 address). The ADDITIONAL-ADDRESS-FAMILY attribute allows a client to request the server to allocate one IPv4 and one IPv6 relay address in a single Allocate request. This saves local ports on the client and reduces the number of messages sent between the client and the TURN server.

> 
> > > Does sending just the
> > > "additional" one secretly mean that you want both v4 and v6?
> >
> > Yes, ADDITIONAL-ADDRESS-FAMILY is used for allocation of both IPv4 and
> IPv6.
> >
> > >
> > > 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.)
> >
> > This "SHOULD" to send SOFTWARE attribute is defined in RFC5766 (As you
> may know, TURN is used in the field for several years) and turnbis does not
> modify the behavior for backward compatibility, and to address the possible
> threat added the following lines:
> >
> > SOFTWARE attribute can reveal the specific software version of the TURN
> client and server to eavesdropper and it might possibly allow attacks against
> vulnerable software that is known to contain security holes. If it is important
> to prevent an eavesdropper from learning the software version, TURN can
> be run over (D)TLS.
> 
> (let's keep this on the secdir thread)

Okay. 

> 
> > >
> > > Section 12
> > >
> > > Do we need to say anything about backwards compatibility with RFC
> > > 5766 peers that use a wider range of channel IDs?
> >
> > Yes, added the following line:
> > Note that the channel number range is not backwards compatible with
> > [RFC5766], and cannot be used in environments where such compatibility
> > is required.
> 
> I see now that Adam also raised this topic, and I'd like to hear his thoughts on
> this proposed text.  Since we normally don't make breaking changes in "bis"
> documents, I'd also like to hear a bit more about what knoweldge we have (if
> any) about implementations that might be affected and are not affected.

Update to the TURN channel number range is done in https://tools.ietf.org/html/rfc7983#section-9.3 to distinguish multiplexed protocols, and the range is updated in turnbis to align with RFC7983 . 

> 
> > >
> > > 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.
> >
> > Good catch, removed conflicting text from Section 12.7.
> >
> > >
> > > 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?
> >
> > You are right, replaced with "obMatJos2gAAAadl7W7PeDU4hKE72jda" (hex)
> > 80 00 00
> 
> It's reassuring to know that I wasn't missing something obvious; thanks!

Thanks for catching it :)

> 
> > >
> > > I'd prefer if the examples showed more usage of MESSAGE-INTEGRITY-
> > > SHA256 (especially, any from the server) and fewer MESSAGE-INTEGRITY.
> >
> > Sure, updated examples.
> >
> > >
> > >
> > > --------------------------------------------------------------------
> > > --
> > > 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.
> >
> > Fixed.
> >
> > >
> > > 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.
> >
> > Yes, use pre-populated trust store on the endpoint to validate the
> certificate path.
> >
> > >
> > > 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.
> >
> > No, STURN third-party authorization provides message integrity for both
> request and response messages (please see
> https://tools.ietf.org/html/rfc7635#section-5).  The mac_key to compute the
> message integrity can change per call and this mechanism is better than the
> long-term password mechanism. Note that WebRTC uses third-party
> authorization mechanism.
> 
> Ah, that's reassuring to learn.  Sorry for the confusion.
> 
> > >
> > > 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)?
> >
> > Yes, cannot read the value as sent on wire.
> 
> I'd  suggest "is unable to access the state of", but that's entirely editorial and
> at your discretion.

Thanks, updated.

> 
> > >
> > > 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?
> >
> > Yes.
> >
> > > (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.
> >
> > You missed the next line, it says the authentication of the request is
> > optional to allow TURN servers provided by the local or access network
> > to accept Allocation requests from new and/or guest users in the
> > network who do not necessarily possess long term credentials for STUN
> > authentication and its security implications are discussed in
> > [RFC8155].
> 
> I saw the next line; the current formulation also allows non-use of
> authentication in other  situations, which is why I suggested an alternate
> wording.
> 
> > The WG spent several cycles discussing the downgrade of a MUST level
> requirement (please see https://tools.ietf.org/html/rfc8155#section-9).
> 
> To be clear, I do not object to removing the MUST for the local-use case; I'm
> concerned with the wording of the text that, as written, also weakens the
> requirement for all other cases.

Got it, updated step as follows:

   1.   The TURN server provided by the local or access network MAY
        allow unauthenticated request in order to accept Allocation
        requests from new and/or guest users in the network who do not
        necessarily possess long term credentials for STUN
        authentication and its security implications are discussed in
        [RFC8155].  Otherwise, the server MUST require that the request
        be authenticated.  If the request is authenticated, the
        authentication MUST be done either using the long-term credential
        mechanism of [I-D.ietf-tram-stunbis] or the STUN Extension for
        Third-Party Authorization [RFC7635] unless the client and server
        agree to use another mechanism through some procedure outside
        the scope of this document.

> 
> > >
> > >    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.
> >
> > Fixed, updated text as follows:
> > If the server does not support the address family requested by the client in
> REQUESTED-ADDRESS-FAMILY or if the allocation of the requested address
> family 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.
> 
> Thanks
> 
> > >    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?
> >
> > Both IPv4 and IPv6 address families.
> 
> I guess once I realize that "ADDITIONAL-ADDRESS-FAMILY" is just code for
> "IPv4 and IPv6" with no other options, this doesn't look as unclear.
> 
> > > We only have the ADDITIONAL-ADDRESS- FAMILY (which contains one
> > > type) and no REQUESTED-ADDRESS-FAMILY.
> >
> > If ADDITIONAL-ADDRESS-FAMILY is used, the server will allocate both IPv4
> and IPv6 relayed transport addresses.
> >
> > >
> > > 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?
> >
> > Good catch, replaced "types" with "type"
> 
> (It needs a definite article, then for "only include the family type")

Done. 

Cheers,
-Tiru

> 
> > >
> > > Section 11.5
> > >
> > > [check that "type and code" is valid in both ICMPv4 and v6]
> >
> > Yes, it is valid.
> 
> Sorry; that was supposed to be a note to myself!
> But yes, luckily for me this is not the point of difference between ICMPv4
> and ICMPv6 that I was vaguely remembering.
> 
> > >
> > > 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.)
> >
> > Yes,  a channel binding is created or refreshed using a ChannelBind
> transaction (Please see Section 12.1).
> 
> Great.  Well, not great that there's a collision of jargon, but it's way too late to
> do anything about, now.
> 
> > >
> > >
> > > 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.
> >
> > Good point, fixed text as follows:
> > Otherwise, if that is not possible for a particular field, the server MUST
> implement the alternate behavior and MUST NOT do anything else for the
> reasons detailed in [RFC7915].
> >
> > >
> > > 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.
> >
> > Suresh raised the same comment, fixed text as follows:
> > If the ICMPv6 packet ("Packet too big") 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.
> 
> Great; t hanks
> 
> > >
> > > Section 13.3
> > >
> > > [Same comment about ICMP packet generation/grammar]
> >
> > Fixed text as follows:
> > If the ICMPv4 packet (Destination Unreachable (Type 3) with Code 4) is
> being sent to the peer, the TURN server SHOULD reduce the MTU reported
> in the ICMP message by 48 bytes to allow room for the overhead of a Data
> indication.
> >
> > >
> > > Section 15
> > >
> > > Maybe note that SIP and WebRTC are the primary consumers of TURN?
> >
> > Yes, it is mentioned in several places in the document that WebRTC and SIP
> use TURN.
> 
> Okay; my fault for only looking at the diff, probably.
> 
> > >
> > >                                                         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".
> >
> > Replaced "UDP" with "application"
> >
> > >
> > > 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.
> >
> > Modified text for clarity as follows:
> >
> > ICMP messages resulting from the UDP datagrams sent to the peer are
> > processed by the server as described in Section 11.5 and forwarded to
> > the client using TURN's mechanism for relevant ICMP types and codes.
> 
> Thanks
> 
> > >
> > > Section 21.16
> > >
> > > I agree with the secdir reviewer that there is at least potential
> > > for username and realm to be sensitive.
> >
> > I proposed to add the following text:
> >
> >    If the TURN client and server use the STUN Extension for Third-Party
> >    Authorization [RFC7635] (for example it is used in WebRTC), the
> >    username does not reveal the real user's identity, the USERNAME
> >    attribute carries an ephemeral and unique key identifier.  If the
> >    TURN client and server use the STUN long-term credential mechanism
> >    and the username reveals the real user's identity, the client needs
> >    to use (D)TLS transport between the client and the server or use the
> >    USERHASH attribute instead of the USERNAME attribute to anonynmize
> >    the username.
> >
> >    If the TURN client and server use the STUN long-term credential
> >    mechanism and realm information is privacy sensitive, TURN can be run
> >    over (D)TLS.  As a reminder, STUN Extension for Third-Party
> >    Authorization does not use realm.
> 
> Excellent; thank you!
> 
> > >
> > > Section 21.4
> > >
> > > Do we need references for Teredo and/or 6to4?
> >
> > https://tools.ietf.org/html/rfc6156 did not add references to Teredo and
> 6to4.
> 
> Okay; just wanted to check.
> 
> > >
> > > 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.)
> >
> > Yes, added RFC6156.
> >
> > >
> > > 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?
> >
> > Yes, several sections in RFC6156 are updated.
> 
> Okay, thanks.
> 
> -Ben