Subject: [Gen-art] Gen-ART review of draf-tietf-behave-rfc3489bis-11.txt

From: Miguel Garcia <[email-masked]>
Date: Fri, 26 Oct 2007 13:31:11 +0300

I have been selected as the General Area Review Team (Gen-ART)
reviewer for this draft (for background on Gen-ART, please see
http://www.alvestrand.no/ietf/gen/art/gen-art-FAQ.html).

Please resolve these comments along with any other Last Call comments
you may receive.

Document: draf-tietf-behave-rfc3489bis-11.txt
Reviewer: Miguel Garcia <[email-masked]>
Review Date: 2007-10-26
IETF LC End Date: 2007-10-26

Summary: The document is almost ready for publication as a standards 
track RFC.

Comments: The document is very well written and has an outstanding 
quality. Congratulations to the authors.

I have a number of comments, mainly clarifications that may help 
implementors to better understand the text. Take these comments are your 
own discretion.

- Section 5, Definitions. The last term is "RTO", which is merely 
expanded. It would be nice to have a short description, like in the rest 
of the defined terms.

- Section 6, Figure 3. It would help if you add the bit counter on top 
of the figure, for example:

->                       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|
                         +--+--+-+-+-+-+-+-+-+-+-+-+-+-+

- Section 7.1, the last sentence reads:

    All STUN requests (and responses) sent over UDP MUST be less than the
    path MTU, or 1500 bytes if the MTU is not known.

   The draft does not offer a mechanism to fall back to TCP. I was 
wondering what happens if the request is sent over UDP and the response 
should be larger than the MTU. If there is no solution for this case, 
which is probably more theoretical than practical (due to the nature of 
STUN), it should be listed as a limitation of the protocol.

- Section 7.2.1 refers to a _configurable_ value of RTO:

    First, the initial value for RTO SHOULD be configurable (rather than
    the 3s recommended in RFC 2988) and SHOULD be greater than 100ms.

   I was wondering of the nature of the configurable value. Does the 
word mean that the human user should be able to configure the initial 
RTO value? Or does it mean that STUN should offer applications the 
possibility of configure this value? The former is scary... I don't 
expect humans need to do something related to STUN. The latter is very 
reasonable, so, if the intention is to have the initial RTO value 
configurable to applications, it should be mentioned in the draft.

- Section 7.2.1 refers to the Karn's algorithm. I suggest to add an 
informative reference to Karn's algorithm.

- Section 7.2.1, typo:
   s/by an client/by a client

- Section 7.3, the 4th paragraph uses the "known-but-unexpected 
attributes" and "comprehension-optional attributes" terms. While it is 
obvious what they mean, they haven't been defined earlier, so, I would 
suggest to add the two terms to the Definitions section.

- Section 7.3.1, third paragraph, talks about idempotent requests. I am 
sure if the reader who is not familiar with RFC 2616 will get right what 
it means, although the same sentence tries to get a definition. Perhaps 
this is another term to add to the definitions, and add a reference to 
RFC 2616.

- Section 8, last paragraph, the text reads:

    the agent also checks that the
    message contains a FINGERPRINT attribute and that the attribute
    contains the correct value (see Section 7.3.

   The comment is that Section 7.3 does not have additional information, 
so I suggest to remove the "(see Section 7.3".

- Section 10.2.1.2
  s/INTEGRITY attributed/INTEGRITY attributes

- Section 12, last bullet point:

    o  There were two comprehension-required attributes, RESPONSE-ADDRESS
       and CHANGE-REQUEST, that have been removed from this
       specification;

       *  These attributes are now part of the NAT Behavior Discovery
          usage.

    I noticed that in addition to RESPONSE-ADDRESS and CHANGE-REQUEST, 
also CHANGE-ADDRESS has been removed from the original 3489, although I 
don't know if CHANGE-ADDRESS is comprehension-required or not. In any 
case, it should be also mentioned that the attribute has been removed 
(and is nw part of the NAT Behavior Discovery).

   Additionally, a reference to the "NAT Behavior Discovery usage" 
should be added.

- Section 13, last sentence, add a reference to RFC 3424.

- Section 14.2, the forth paragraph reads:

    If the IP address
    family is IPv6, X-Address is computed by taking the mapped IP address
    in host byte order, XOR'ing it with the magic cookie and the 96-bit
    transaction ID, and converting the result to network byte order.

I am not sure how to XOR the mapped IP address. I suspect the text wants 
to say "XOR'ing it with the *concatenation of the* magic cookie and the 
96-bit transaction ID". I hope it does not mean doing a double XOR 
operation.

- In many places in the spec, starting in Section 14.3, the term UTF-8 
is mentioned. I am missing a normative reference RFC 3629 in every 
occurrence.

- Similarly, in Section 14.4, the term MD5 is written briefly to explain 
how to calculate the key. It would be nice to have some text around that 
includes a normative reference to RFC 1321.

- Section 14.5 reads:

    The FINGERPRINT attribute may be present in all STUN messages.

   I think the "may" can be promoted to a normative "MAY".

- Section 14.5, there is a reference to ITU-V42.1994. I tried to 
download that version of the specification, but it is no longer 
available. It has been superseded by "200203",

http://www.itu.int/rec/T-REC-V.42/en

- Section 14.6, the figures does not have a figure number neither a caption.

- Section 14.6. I have trouble to follow-up on the 401 code, I think the 
draft contradicts itself. The text in Section 14.6 reads:

    401  Unauthorized: The request did not contain the expected MESSAGE-
         INTEGRITY attribute.  The server MAY include the MESSAGE-
         INTEGRITY attribute in its error response.

However, in Section 10.2.2, 4th bullet point, the text reads:

    o  If the username in the USERNAME attribute is not valid, the server
       MUST generate an error response with an error code of 401
       (Unauthorized).

So, it seems that the 401 response does not really mean that the 
MESSAGE-INTEGRITY attribute is not present or is wrong. It may also mean 
that the USERNAME attribute is not valid.

Is this the desired behavior though? Essentially, it will be difficult 
to inspect a response and analyze what the problem is without 
correlating it with the request.

- Section 14.6, below the figure, the text reads:

    The Reserved bits SHOULD be 0, and are for alignment on 32-bit
    boundaries.  Receivers MUST ignore these bits.  The Class represents
    the hundreds digit of the error code.  The value MUST be between 3
    and 6.  The number represents the error code modulo 100, and its
    value MUST be between 0 and 99.

It would be nice to indicate that these values are all written in 
binary. I hope none makes a mistake and codes them in hexadecimal.

- Section 14.6, the following paragraph reads:

    The following error codes, along with their recommended reason
    phrases (in brackets) are defined:

But the reason phrases are not written "in brackets", so delete it.

- Section 14.8 Unlike Section 14.7, the text in 14.8 does not say 
whether the NONE attribute includes the quotes or not. Bearing in mind 
that RFC 2617 apparently requires the usage of quotes in nones, but 
'qdtext' in RFC3261 excludes them, I would suggest to clearly say, as in 
Section 14.7, whether the NONCE attribute includes quotes or not.

- Section 14.8. The text reuses 'qdtext' and 'quoted-pair' from RFC 
3261. If the implementor is familiar with SIP, will most likely 
appreciate it, but if the implementor is not so familiar with SIP, it 
won't help. I wonder if there is a way to express the exactly what it 
is, without referring to RFC 3261.

Furthermore. I noticed that the 'nonce' value in RFC 3261 is defined as 
follows:

nonce               =  "nonce" EQUAL nonce-value
nonce-value         =  quoted-string

So, notice that nonce-value in RFC 3261 does not allow to include a 
'quoted-pair', as indicated in STUN. Therefore, I think the reference to 
RFC 3261 will just confuse people. The BNF of the nonce value attribute 
should be self-described in STUN.

- Section 17.2

In the initial STUN Attributes types, it came to my attention that the 
values 0 and 7 are reserved, because they were used by the original RFC 
3489. I noticed that the values 2, 3, 4, and 5 are not mentioned. But 
then I noticed that values 3, 4, and 5 are registered by the NAT 
Behavior Discovery usage. So, what about value 2, which RFC 3489 assigns 
to the RESPONSE-ADDRESS attribute, and that nowadays seems to be 
deprecated? I think it should be registered by this draft as "reserved; 
was RESPONSE-ADDRESS".

/Miguel

-- 
Miguel A. Garcia           tel:+358-50-4804586
Nokia Siemens Networks     Espoo, Finland

_______________________________________________
Gen-art mailing list
[email-masked]
https://www1.ietf.org/mailman/listinfo/gen-art

---

From: Jonathan Rosenberg <[email-masked]>
Date: Tue, 30 Oct 2007 12:29:51 -0400

Thanks for the review comments. Responses below:

> expanded. It would be nice to have a short description, like in the rest 

added:

         <t hangText="RTO:">Retransmission TimeOut, which defines the
         initial period of time between transmission of a request and
         the first retransmit of that request.</t>

> of the figure, for example:

done.

> STUN), it should be listed as a limitation of the protocol.

There is no solution. No TCP fallback is provided as this is 
fundamentally not possible, given NAT pinholes are specific to UDP and TCP.

Text says:

	<t>All STUN requests (and responses) sent over UDP MUST be
	less than the path MTU, or 1500 bytes if the MTU is not
	known. STUN provides no ability to handle the case where the
	request is under the MTU but the response would be larger than
	the MTU. It is not envisioned that this limitation will be an
	issue for STUN.</t>

>   I was wondering of the nature of the configurable value. Does the word 
> mean that the human user should be able to configure the initial RTO 
> value? Or does it mean that STUN should offer applications the 

It means configurable using whatever means are used to configure the UA. 
Usually its automated, but STUN doesnt REQUIRE automated configuration, 
in which case this could be user adjusted, likely through hidden config 
settings I suspect. I don't think we can or should mandate automated 
configuration.

> informative reference to Karn's algorithm.

ok.

>   s/by an client/by a client

fixed.

> suggest to add the two terms to the Definitions section.

these terms are actually used nowhere else; just here. I think the 
definition is clear from the words.

> this is another term to add to the definitions, and add a reference to 

I added an inline definition of idempotent. Its not used enough to 
warrant definition in the terminology section.

> so I suggest to remove the "(see Section 7.3".

well it does indicate when the check is performed. I changed the text to:

	correct value.  <xref target="sec:receiving-a-stun-message"/>
	describes when in the overall processing of a STUN message the
	FINGERPRINT check is performed. This additional check helps

>  s/INTEGRITY attributed/INTEGRITY attributes

fixed

> (and is nw part of the NAT Behavior Discovery).

fixed.

> 
>   Additionally, a reference to the "NAT Behavior Discovery usage" should 
> be added.

done.

> - Section 13, last sentence, add a reference to RFC 3424.

done.

> 96-bit transaction ID". I hope it does not mean doing a double XOR 

good catch, fixed.

> is mentioned. I am missing a normative reference RFC 3629 in every 

fixed.

> includes a normative reference to RFC 1321.

fixed.

>   I think the "may" can be promoted to a normative "MAY".

fixed

> http://www.itu.int/rec/T-REC-V.42/en

wow - good catch. fixed.

> - Section 14.6, the figures does not have a figure number neither a 

fixed

> correlating it with the request.

This got discussed in the group. There is a DoS attack possible if you 
include a specific error code for invalid username. It allows for 
probing of valid usernames without authenticating yourself. So a generic 
401 is used to cover all cases. The text in 14.6 needs to be adjusted.

> binary. I hope none makes a mistake and codes them in hexadecimal.

I don't follow. Hex is just a way of representing binary values in a 
string format. The protocol fields are binary.

> But the reason phrases are not written "in brackets", so delete it.

fixed

> Section 14.7, whether the NONCE attribute includes quotes or not.

fixed.

> is, without referring to RFC 3261.

you don't need to understand rfc3261; you just need to pull the BNF 
definition from there.

> should be self-described in STUN.

I dislike repeating the same grammar in multiple places; it introduces 
the possibility of making a mistake and then the definition is really 
unclear.

> deprecated? I think it should be registered by this draft as "reserved; 

Yes, great catch.

Thanks,
Jonathan R.

-- 
Jonathan D. Rosenberg, Ph.D.                   600 Lanidex Plaza
Cisco Fellow                                   Parsippany, NJ 07054-2711
Cisco Systems
[email-masked]                              FAX:   (973) 952-5050
http://www.jdrosen.net                         PHONE: (973) 952-5000
http://www.cisco.com

---

From: Miguel Garcia <[email-masked]>
Date: Wed, 31 Oct 2007 09:54:40 +0200

Jonathan:

All clear now. I am satisfied with your explanations.

/Miguel