Document: draft-ietf-behave-rfc3489bis-11.txt Reviewer: Miguel Garcia Review Date: 2007-10-26, Updated 2007-10-31 IETF LC End Date: 2007-10-26 Summary: Ready. Comments (10/31): All clear now. I am satisfied with your explanations. -------------------------------------------------------------------------------------- Comments (10/26): 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".