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

Tom Kristensen <2mkristensen@gmail.com> Fri, 14 February 2014 23:14 UTC

Return-Path: <2mkristensen@gmail.com>
X-Original-To: bfcpbis@ietfa.amsl.com
Delivered-To: bfcpbis@ietfa.amsl.com
Received: from localhost (ietfa.amsl.com [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 8CF791A045D for <bfcpbis@ietfa.amsl.com>; Fri, 14 Feb 2014 15:14:51 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: 0.301
X-Spam-Level:
X-Spam-Status: No, score=0.301 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, MANGLED_LIST=2.3, SPF_PASS=-0.001] autolearn=no
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 4wnhfO1z4x_Y for <bfcpbis@ietfa.amsl.com>; Fri, 14 Feb 2014 15:14:45 -0800 (PST)
Received: from mail-qa0-x234.google.com (mail-qa0-x234.google.com [IPv6:2607:f8b0:400d:c00::234]) by ietfa.amsl.com (Postfix) with ESMTP id A5DB21A040B for <bfcpbis@ietf.org>; Fri, 14 Feb 2014 15:14:39 -0800 (PST)
Received: by mail-qa0-f52.google.com with SMTP id j15so19117566qaq.39 for <bfcpbis@ietf.org>; Fri, 14 Feb 2014 15:14:38 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=WgLSB17M5q0QY10rFdb+q6fgMFqxGIfXVgWMXMa6I7I=; b=dnxbu9arGZZhlCP9QBLZFcDEajMzIr/ab0/BJP2WtFuUKy+n+ZcAwQFlFcqiVzBFb6 EldAtLjgOwhCXj2pGYSjhDPi8RF3ey2lyuVpkRVCUCXq8I9u4Jse4INaQN3+NNNgw98I PdYORBFQCmR/R4ntuKQVoIlgNRYSWJbT2Gdt0S3KJDnb4Ydh5W1/im7YI2OD6aid+I81 2WNILBSVf49UhgdiQFH6ZAsifPFyuCcYza+qJyHT8kinGWMFf57bCljDjFpSzPXWjCEm w+KGaBc3QH7o9uT898bjgBtekPOrCta50GHt5NJMuKqXFMegpImzD6Lg9kRZTKZuTpUw vBOg==
MIME-Version: 1.0
X-Received: by 10.140.108.116 with SMTP id i107mr16991623qgf.80.1392419677852; Fri, 14 Feb 2014 15:14:37 -0800 (PST)
Received: by 10.229.2.69 with HTTP; Fri, 14 Feb 2014 15:14:37 -0800 (PST)
In-Reply-To: <CEFACFF4.1A7CE%eckelcu@cisco.com>
References: <CAHBDyN425aOuEVpvy3gkeTdrrySXApugdsb1wnYGJFXBj-A7Hg@mail.gmail.com> <CEFACFF4.1A7CE%eckelcu@cisco.com>
Date: Sat, 15 Feb 2014 00:14:37 +0100
Message-ID: <CAFHv=r90NMANg=VH9PcLv=PoB3a3_xe=MkYVfm-BGghYFL1CQA@mail.gmail.com>
From: Tom Kristensen <2mkristensen@gmail.com>
To: "Charles Eckel (eckelcu)" <eckelcu@cisco.com>, Mary Barnes <mary.ietf.barnes@gmail.com>
Content-Type: multipart/alternative; boundary=001a113aadc246d8cd04f265f80a
Archived-At: http://mailarchive.ietf.org/arch/msg/bfcpbis/dtHcSGVvhjvV0bnzqT6XWtyrBFU
Cc: Richard Barnes <rlb@ipv.sx>, "bfcpbis@ietf.org" <bfcpbis@ietf.org>, "bfcpbis-chairs@tools.ietf.org" <bfcpbis-chairs@tools.ietf.org>, "draft-ietf-bfcpbis-rfc4582bis.authors@tools.ietf.org" <draft-ietf-bfcpbis-rfc4582bis.authors@tools.ietf.org>, Tom Kristensen <tomkrist@cisco.com>
Subject: Re: [bfcpbis] PROTO review comments on draft-ietf-bfcpbis-rfc4582bis-10
X-BeenThere: bfcpbis@ietf.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: BFCPBIS working group discussion list <bfcpbis.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/bfcpbis>, <mailto:bfcpbis-request@ietf.org?subject=unsubscribe>
List-Archive: <http://www.ietf.org/mail-archive/web/bfcpbis/>
List-Post: <mailto:bfcpbis@ietf.org>
List-Help: <mailto:bfcpbis-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/bfcpbis>, <mailto:bfcpbis-request@ietf.org?subject=subscribe>
X-List-Received-Date: Fri, 14 Feb 2014 23:14:51 -0000

On 14 January 2014 20:44, Charles Eckel (eckelcu) <eckelcu@cisco.com> wrote:

>
>  From: Mary Barnes <mary.ietf.barnes@gmail.com>
> 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/
## tomkrist@cisco.com  |  http://www.tandberg.com
###                               |  http://folk.uio.no/tomkri/