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

From: Peter Saint-Andre <[email-masked]>
Date: Mon, 02 Apr 2018 15:59:45 -0700

Reviewer: Peter Saint-Andre
Review result: Ready with Nits

Section 1 states:

   Implementations and deployments of a STUN usage using TLS or DTLS
   should follow the recommendations in [RFC7525].

Wouldn't the security considerations be a better location for that text?

And given that this specification cites RFC 8174, I suggest changing
"should" to "SHOULD".

(I suggest that the authors review the usage of lowercase and uppercase
requirements keywords, because there might be inconsistencies, e.g., in
the first paragraphs of Section 6.1 and Section 6.2.)

In Section 4, please consider spelling out "SIP" on first use and
including a reference to RFC 3261.

The first paragaraph of Section 6.2.3 restates recommendations from RFC
7525; why not simply reference that specification?

Section 6.3.4 states:

   o  If the error code is 500 through 599, the client MAY resend the
      request; clients that do so MUST limit the number of times they do
      this.

It is reasonable to provide guidance as to the number of re-sends?

Section 9.1.1 and other sections invoke the OpaqueString profile of the
PRECIS FreeformClass; it might be helpful to mention that the profile is
used to handle Unicode characters outside the ASCII range, and that no
changes result if only ASCII characters are used.

In Section 14.2, please consider spelling out "ALG" on first use.

A reference to RFC 6151 seems appropriate regarding the fact that this
specification retains the use of MD5; in particular, the usage here is
actually HMAC-MD5, which is still sanctioned by RFC 6151.

---

From: Marc Petit-Huguenin <[email-masked]>
Date: Mon, 16 Apr 2018 16:43:25 -0700

Hi Peter,

Thanks for the review and sorry for the delay in responding, I was traveling for the last 4 weeks.

See my responses inline.

> Wouldn't the security considerations be a better location for that text?

Moved there.

> And given that this specification cites RFC 8174, I suggest changing

Done.

> the first paragraphs of Section 6.1 and Section 6.2.)

These sections are identical to RFC 5389 and so I am reluctant to modify them as they already went through an IESG review.  I reviewed the lowercase keywords in these two sections and did not find any specific case that would create an interoperability issue, but please let me know if you think otherwise.

> including a reference to RFC 3261.

Done.

> 7525; why not simply reference that specification?

The original text in RFC5389 said this:

" When STUN is run by itself over TLS-over-TCP, the
  TLS_RSA_WITH_AES_128_CBC_SHA ciphersuite MUST be implemented at a
  minimum. [...]"

The new text is an attempt at updating it in the same spirit of giving minimal instructions and complementing them with a reference to RFC 7525 - which was the reason for the reference to RFC 7525 there.

So I kept the text there, followed by the following paragraph, in addition of moving the original last paragraph in the Security Consideration section:

" These recommendations are just a part of the the recommendations in
  [RFC7525] that implementations and deployments of a STUN usage using
  TLS or DTLS SHOULD follow."

> It is reasonable to provide guidance as to the number of re-sends?

Same issue here, that's a section that is unmodified from RFC 5389.  As long as the client does not enter an endless loop of retransmission, choosing different numbers of re-sends does not affect interoperability.

> changes result if only ASCII characters are used.

Hmm.  But in that case the application has first to test that the string is in the ASCII range.  Isn't that better to always assume that the input string will contain Unicode, and let the implementation of OpaqueString short-circuit the processing in the case of an all-ASCII string?

> In Section 14.2, please consider spelling out "ALG" on first use.

Done.

> actually HMAC-MD5, which is still sanctioned by RFC 6151.

Done.

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

---

From: Peter Saint-Andre <[email-masked]>
Date: Mon, 16 Apr 2018 18:22:17 -0600

Hi Marc, a few further comments inline.

>> Review result: Ready with Nits

<snip/>

>   TLS or DTLS SHOULD follow."

I would instead suggest that we do something like Section 2 of RFC 7590
for XMPP:

   The best current practices documented in the "Recommendations for
   Secure Use of TLS and DTLS" [RFC7525] are included here by reference.
   Instead of repeating those recommendations here, this document mostly
   provides supplementary information regarding secure implementation
   and deployment of XMPP technologies.

Here's the rationale: RFC 7525 is likely to be updated/replaced more
quickly than STUNbis. If STUNbis recommends a particular cipher suite
that 7525bis stops recommending, in the absence of STUNter will STUN
implementations keep following STUNbis or will they upgrade to whatever
7525bis recommends? I suggest it will be the former, which is not what
we want.

> Same issue here, that's a section that is unmodified from RFC 5389. 

I understand. Now is our chance to fix it. :-)

>  As long as the client does not enter an endless loop of retransmission, choosing different numbers of re-sends does not affect interoperability.

Choosing different numbers is OK, but choosing an infinite number is
not. Can we provide guidance as to how many is too many? 10? 50? 100?

> Hmm.  But in that case the application has first to test that the string is in the ASCII range.  Isn't that better to always assume that the input string will contain Unicode, and let the implement…

Yes, I meant that as an informational note to implementers so they would
worry less. But it's not necessary and I think you can safely ignore it.

Peter

---

From: Eric Rescorla <[email-masked]>
Date: Mon, 16 Apr 2018 20:12:06 -0700

On Mon, Apr 16, 2018 at 5:22 PM, Peter Saint-Andre <[email-masked]>
wrote:

> > The new text is an attempt at updating it in the same spirit of giving
> minimal instructions and complementing them with a reference to RFC 7525 -
> which was the reason for the reference to RFC 7525 there.
> >
> > So I kept the text there, followed by the following paragraph, in
> addition of moving the original last paragraph in the Security
> Consideration section:
> >

I forgot about this in my review, but you should also profile ciphers for
TLS 1.3.

-Ekr

> >  As long as the client does not enter an endless loop of retransmission,
> choosing different numbers of re-sends does not affect interoperability.
>
> > Hmm.  But in that case the application has first to test that the string
> is in the ASCII range.  Isn't that better to always assume that the input
> string will contain Unicode, and let the implementation of OpaqueString
> short-circuit the processing in the case of an all-ASCII string?
>

---

From: Marc Petit-Huguenin <[email-masked]>
Date: Tue, 17 Apr 2018 03:00:09 -0700

> 7525bis recommends? I suggest it will be the former, which is not what

All right, makes sense.  I'll add something like this on my next round of reviews, most likely this Friday.

> not. Can we provide guidance as to how many is too many? 10? 50? 100?

Well, the text already states that an infinite number of of re-sends is not compliant.  Anyway, I am not sure how to determine a reasonable number, but I'll try.

---

From: Marc Petit-Huguenin <[email-masked]>
Date: Tue, 17 Apr 2018 03:02:37 -0700

> I forgot about this in my review, but you should also profile ciphers for

Do you have any suggestion for these, or a pointer to a document that I can use to find these?

Thanks.

---

From: Peter Saint-Andre <[email-masked]>
Date: Tue, 17 Apr 2018 09:11:28 -0600

> Do you have any suggestion for these, or a pointer to a document that I can use to find these?

Off-topic: it sounds like we might need to start work on 7525bis...

Peter

---

From: Julien ÉLIE <[email-masked]>
Date: Tue, 17 Apr 2018 21:43:55 +0200

Hi Marc,
>>> So I kept the text there, followed by the following paragraph,
>>> in addition of moving the original last paragraph in the Security
>>> Consideration section:
>>>
> All right, makes sense.  I'll add something like this on my next
> round of reviews, most likely this Friday.

If you're going to add some wording about including TLS best current 
practices, maybe you could re-use what we came up with during final RFC 
edition of RFC 8143 <https://tools.ietf.org/html/rfc8143>:

3.  Recommendations

    The best current practices documented in [BCP195] apply here.
    Therefore, NNTP implementations and deployments compliant with this
    document are REQUIRED to comply with [BCP195] as well.

    Instead of repeating those recommendations here, this document mostly
    provides supplementary information regarding secure implementation
    and deployment of NNTP technologies.

Notably, the RFC Editor prefers referencing RFC 7525 as BCP 195 even 
though there currently is only one RFC in this BCP series.  As other 
RFCs may be added in BCP 195, and we want to follow all best practices 
that apply, the reference should be the BCP series.

-- 
Julien ÉLIE

« – C'est joli cette avenue le long de la mer… Ça s'appelle
     comment ?
   – La promenade des Bretons. » (Astérix)

---

From: Marc Petit-Huguenin <[email-masked]>
Date: Sat, 21 Apr 2018 14:02:48 -0700

On 04/17/2018 12:43 PM, Julien ÉLIE wrote:
> If you're going to add some wording about including TLS best current practices, maybe you could re-use what we came up with during final RFC edition of RFC 8143 <https://tools.ietf.org/html/rfc8143>:
> 
> 3.  Recommendations
> 
> Notably, the RFC Editor prefers referencing RFC 7525 as BCP 195 even though there currently is only one RFC in this BCP series.  As other RFCs may be added in BCP 195, and we want to follow all best practices that apply, the reference should be the BCP series.
> 

Thanks, I used the BCP as reference, and changed the SHOULD to MUST.

---

From: Marc Petit-Huguenin <[email-masked]>
Date: Sat, 21 Apr 2018 14:07:12 -0700

> I forgot about this in my review, but you should also profile ciphers for

As suggested by Julien Élie, I now use BCP195 as reference.  Would that, and a future update of that BCP covering TLS 1.3 satisfy your comment?

Thanks.

---

From: Marc Petit-Huguenin <[email-masked]>
Date: Sat, 21 Apr 2018 14:39:28 -0700

> All right, makes sense.  I'll add something like this on my next round of reviews, most likely this Friday.

Done, see my other emails to Julien Élie and Ekr.

> Well, the text already states that an infinite number of of re-sends is not compliant.  Anyway, I am not sure how to determine a reasonable number, but I'll try.

I chose 4 as the maximum number of retransmissions.  I based that on error code 508 in TURN that defines a delay of 60 seconds between retransmissions, so we now get a delay of 5 minutes before the client gives up in case of insufficient capacity.  The new text reads like this:

"o  If the error code is 500 through 599, the client MAY resend the
    request; clients that do so MUST limit the number of times they do
    this.  Unless a specific error code specifies a different value,
    the number of retransmissions MUST be limited to 4."

---

From: Peter Saint-Andre <[email-masked]>
Date: Sun, 22 Apr 2018 21:30:34 -0600

>     the number of retransmissions MUST be limited to 4."

"SHOULD be limited to 4" seems fine, but MUST is OK too.

Thanks for looking into this and making the fix.

Peter

---

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

> "SHOULD be limited to 4" seems fine, but MUST is OK too.

Changed to SHOULD.

Thanks.