Re: [bfcpbis] PROTO review comments on draft-ietf-bfcpbis-rfc4582bis-10

[Tom Kristensen <[e-mail]>]

On 14 January 2014 20:44, Charles Eckel (eckelcu) <[e-mail]> wrote:

>
>  From: Mary Barnes <[e-mail]>
> Date: Thursday, December 19, 2013 12:27 PM
>

[...]

 *General:*
> 1) I still have not had a response from Keith Drage nor Joerg Ott with
> regards to IPR, so I cannot forward the document to the AD even after the
> document is updated until I get those responses.
>
> Hopefully, this is now taken care of!

> 2) Obviously, the WG needs to agree proposed solutions to the points made
> by Joerg:
> http://www.ietf.org/mail-archive/web/bfcpbis/current/msg00249.html
> At this time, I've not seen the authors post any proposals as to how to
> deal with those comments/concerns
>
> This should be solved, with the amount of commenting and proposals that
has been published on the list.

> 3) I have reviewed the document in detail - both the entire document and
> the diffs between RFC4582. I have both comments and nits on text that was
> also in RFC 4582, as well as on the new text, detailed below. Note, that a
> number of nits are around the use of lower case normative words which ought
> to be uppercase to distinguish them from the normal English usage. I think
> there are a number of cases where "may"s for example would be much better
> stated as "can"s.  I've identified some of those below, but I don't think
> it's worth the effort to change them all (nor my effort to identify them
> all). `
>
> I've done a scan and do think the ones you have found should be sufficient
to deal with.

> *Comments/questions:*
>
>  1) Section 3.4, 1st paragraph.  You need to specify what you mean by the
> parenthetical "in a certain way" - that doesn't tell you anything.
>  Something like "depending upon policies, privileges, etc." is probably
> more appropriate.
>
> Well, with the simple example and later reference to RFC 5239, I do think
the explanation of how resources may be used is not needed here. Removed
"(in a certain way)".

> 2) Section 6.2.
> a) 3rd paragraph, 1st sentence: I don't think this is normative since
> you're referring to the normative section.  I suggest "shall form" ->
> "forms".
>
> Fixed.

> b) 4th paragraph. Under what conditions would a server not send an Error
> message or is that statement just specifically sending an error message
> with a parameter value of 10?  I can't see why this isn't a MUST. If it's
> really a should, more text is needed as to what happens if it isn't sent
> (or why not sending the Error message is okay).
>
> I believe this was because it may not be possible to form a valid response
> because it was not possible to parse fields that are required to be
> included in the response, such as the transaction ID.
>

Yes, I have therefore changed to MUST and appended: ",  , given that it is
possible to parse the received message to such an extent that an Error
message may be built." to the sentence.

>     c) 4th para, last sentence: "shall" -> "SHALL"
>
> This is already fixed according to Gonzalo Camarillo's comment.

> d) last para, next to last sentence.  Why isn't this "MUST" request the
> use of DTLS? If it's a SHOULD, then the impacts or criteria under which
> DTLS isn't REQUIRED should be clarified.  I see that the paragraph has
> references to  5018 Section 5.  I think it also should be clarified that
> the "section 6" reference in that paragraph is also in 5018 (as I don't
> think you're referring to section 6 of this document).
>
>
>  I think the server can choose to require DTLS or not. If it requires
> DTLS for the request, it MUST respond the request with "Use DTLS).
>

Keeping the SHOULD, but adding a sentence in addition so that it reads:

  "In <xref target="RFC5018"/>, Section 4 applies, but when using BFCP over
an unreliable transport the floor control server that receives a BFCP
message over UDP (no DTLS) SHOULD request the use of DTLS by generating an
Error message with an Error code with a value of 11 (Use DTLS). A floor
control server that is configured to require DTLS MUST request the use of
DTLS this way. "

3) Section 6.2.1
> a) 1st para, 3rd sentence. I don't think "previous paragraph" is accurate.
>  I think it would be better to include an explicit reference to section 6.2
> or say "previous section".
>
> Yes, cross reference added.

> b) next to last sentence ought to be written normatively
> OLD
>    The default initial
>    interval is set to 500ms and the interval is doubled after each
>    retransmission attempt.
> NEW
>    The default initial
>    interval MUST be set to 500ms and the interval MUST be doubled after
> each
>    retransmission attempt.
>
> Yes, fixed.

> 4) Section 6.2.2, 1st sentence.  What happens if the connection isn't
> treated as closed (since this is a SHOULD and not a MUST)?  Either this is
> a MUST or the reasons one wouldn't treat the connection as closed and what
> happens when it's not treated as closed need to be explained.
>
> True, a bit fuzzy as it was. Fixed.

> 5) Section 6.2.3, indented paragraph.  What happens if the cookie exchange
> mechanisms in DTLS aren't used and/or what other mechanisms could be used?
>   (Note, also that the should ought to be SHOULD).
>
>  Well, by not using that mechanism spoofing is a bit easier. I don't know
what other mechanisms to mention, and will assume the usage are based on
environment and probability for + consequence of such an attack.

> 6) Section 9.1.
> a) 1st para, 2nd sentence. (Similar to comment 9 on rfc4583-bis).  The
> "assumes" needs to a REQUIRED.
> OLD:
>    BFCP assumes that there is an integrity-
>    protected channel between the client and the floor control server
>    that can be used to exchange their self-signed certificates or, more
>    commonly, the fingerprints of these certificates.
>
>  NEW:
>    An initial integrity-protected channel is REQUIRED between the client
>    and the floor control server that can be used to exchange their
>    self-signed certificates or, more commonly, the fingerprints of
>    these certificates.
>
>  OR  this needs to be a SHOULD and then include the text that's in the
> "Note..." in the 3rd paragraph that indicates if there are additional
> authentication mechanisms defined that don't require the integrity
> protected channel.
>
> I'm adopting the new text, but adding "  If TLS/DTLS is used, an initial
..." in front of it.

> b) 2nd paragraph, last sentence.  Under what criteria would a client NOT
> ignore unauthenticated messages or what bad things can happen if a client
> doesn't ignore the messages.  It might be better to make this a MUST.
>
> Agree. Changed to MUST.

> c) 4th paragraph, 2nd sentence.  "SHOULD check" -> "MUST check" or explain
> what might happen if the floor control servers doesn't check that the
> messages aren't using an authorized User ID.
>
>  I cannot see why this won't be checked, so requiring a check sounds
reasonable.

> 7) Section 11.1.
> a) 4th paragraph.  There's three SH OULDs in there that all ought to be
> MUSTs OR the situations under which the action doesn't need to be done
> needs to be specified.
> b) 5th paragraph: I don't think that the use of Queue position field in
> the last sentence prior to the indented text ought to be normative - i.e.,
> I think the " MAY use the Queue Position field" ought to be "can use the
> Queue Position field".
> c) Indented text (1st para) after 5th para.   There are several "may"s:
> (lower case) in this paragraph (and the next note), that probably all ought
> to be "can"s
> d) last paragraph. It's not clear to me whether this "may" ought to be
> normative. If so, it should be a  MAY and perhaps reworded as I think it's
> trying to say that "the floor chair MAY include STATUS-INFO attributes" in
>    Otherwise, a "can" would suffice.
>
> I agree with these issues. Reworded as suggested for issue d).

> 8) Section 12.1.2.  There is no normative language in this section.  It
> needs to be revisited.  For example, 2nd para, 1st sentence.  It would seem
> that "the floor control server will
>    respond with a FloorStatus message or with an Error message"
> ought to be "MUST respond.
>
> Revisited and the MUST is added.

> 9) Section 13.6, 1st para.  Under what circumstances would the server NOT
> generate an Error response (i.e., why is it a SHOULD and not a MUST)?
>
> This is inherited from RFC 4582. I agree that MUST seems appropriate.
>

Fixed. These cases are fixed across the document, since it was pointed out
by Jörg too and recommended being fixed by Charles as well (when reading
RFC 2119).

> 10) Section 13.7, 1st para. Same comment as 9 above.
>
> Fixed.

> 11) Section 16.1.  I think there should be mention of the additional text
> added for handling incorrect payload length.
>
> In Section 16.2.

> 12) Section 16.1. "Requiring timely response".  Shouldn't there be a
> reference to section 8.3 (Timers)?
>
> Yes.
>

Added.


> 13) Appendix A. Has there been anyone that has verified the call flows
> with a tool or in the lab?  If so, who was that?
>
> Not sure, but I expect yes. Hopefully someone can else can respond to this
>

No verification other than review and comparing with actual implementations.

*Nits:*
>
>  1) Section 3.1, last paragraph.  I would prefer this be updated to be
> much more precise in terms of CCM P.  I realize that this document was
> published well before CCMP, so the text in RFC 4582 is based on draft
> versions.
> OLD:
>    Conference control clients using CCMP [17] can specify such floor-
>    related settings by editing the floor-information section of the
>    to-be created conference object provided in the body of a CCMP
>    confRequest/create message issued to the conference control server.
>
>  NEW:
>     Conference control clients using CCMP [17] can specify such floor-
>    related settings in the <floor-information> [RFC6501]element of the
>    to-be created conference object provided in the body of a CCMP
>    confRequest/create message issued to the conference control server.
>
> Adopted.

> 2) Section 3.2, 1st para, 2nd sentence.  "These data include..." -> This
> data includes
>
> OK!

> 3) Section 3.3, last paragraph. "<floor-information> section" ->
>  "<floor-information> element"
>
> OK!

> 4) Section 6.2, 4th para, last sentence: "shall" -> "SHALL"
>
> Already taken care of.

> 5) Section 6.2.3:
> - seventh paragraph. "must then retransmit" -> "MUST then retransmit"
> - indented paragraph. "the Payload Length field may be compared" - > "can
> be compared"
>
> OK!

> 6) Section 8, 2nd indented paragraph. "must be non-zero" -> "MUST be
> non-zero"
>
> Indeed.

> 7) Section 10.1.1. 1st paragraph after 2nd indented para. "must insert" ->
> "MUST insert"
>
> Indeed.

> 8) Section 10.1.2, 5th & 6th paragraphs.  "MAY display" -> "can display"
>
> True, no mandatory language needed here.

> 9) Section 12.2.1. last paragraph. "must insert" -> "MUST insert"
>
> OK!

> 10) Section 12.3.1.   last paragraph. "must insert" -> "MUST insert"
>
> OK!

> 11) Section 16.1.  This is a bit hard to read.  I would suggest you use a
> bulleted or numbered list rather than this hanging list format.
>
> Fixed!

> 12) References:
> a) Per the first nit, a reference to RFC 6501 (XCON Data model) should be
> added to this document.
> b) RFC 4582 should be a normative reference
> c) I also suggest that RFC 5018 is normative as this document relies on
> RFC 5018 for authentication and security.
>
> Sounds good.
>

Yes and fixed.

-- Tom

-- 
# Cisco                         |  http://www.cisco.com/telepresence/
## [e-mail]  |  http://www.tandberg.com
###                               |  http://folk.uio.no/tomkri/