Re: [bfcpbis] PROTO review comments on draft-ietf-bfcpbis-rfc4583bis-08

"Charles Eckel (eckelcu)" <eckelcu@cisco.com> Fri, 24 January 2014 22:42 UTC

Return-Path: <eckelcu@cisco.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 432C51A01E0 for <bfcpbis@ietfa.amsl.com>; Fri, 24 Jan 2014 14:42:11 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -12.736
X-Spam-Level:
X-Spam-Status: No, score=-12.736 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, MANGLED_LIST=2.3, RCVD_IN_DNSWL_HI=-5, RP_MATCHES_RCVD=-0.535, SPF_PASS=-0.001, USER_IN_DEF_DKIM_WL=-7.5] autolearn=ham
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 f923IVilCEz2 for <bfcpbis@ietfa.amsl.com>; Fri, 24 Jan 2014 14:42:08 -0800 (PST)
Received: from rcdn-iport-7.cisco.com (rcdn-iport-7.cisco.com [173.37.86.78]) by ietfa.amsl.com (Postfix) with ESMTP id 4468F1A010A for <bfcpbis@ietf.org>; Fri, 24 Jan 2014 14:42:08 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=cisco.com; i=@cisco.com; l=11393; q=dns/txt; s=iport; t=1390603327; x=1391812927; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-id:content-transfer-encoding: mime-version; bh=IHt2V4zgzbYTpTDdrKa7C2Gcc92UKvbna5QwpPPrcPU=; b=QtJ3yNOX8IVvfxFcm8cR55TaKepElYV6vlITamynaHJuUfRi+lPdQ6zz AxYQCSjZXqjlmmJ/JjNuTPpllO34fuIKvzTDS+bNTuTrDZQ4OytsKVnnE sBK2QMUo1YvgEAoBj9iDLwHWC7nlg/zbyLmOsbZX5GjygTgK0UqAWuI0x A=;
X-IronPort-Anti-Spam-Filtered: true
X-IronPort-Anti-Spam-Result: AggFAO/r4lKtJV2Z/2dsb2JhbABYAoMMOFa7b0+BCBZ0giUBAQEDAQEBASRHCwULAgEIGBYYJwslAgQBDQUbh2IIDckgEwSOSkIHEguEGwEDmCeSHoMtgWoHFyI
X-IronPort-AV: E=Sophos;i="4.95,715,1384300800"; d="scan'208";a="299576065"
Received: from rcdn-core-2.cisco.com ([173.37.93.153]) by rcdn-iport-7.cisco.com with ESMTP; 24 Jan 2014 22:41:48 +0000
Received: from xhc-rcd-x05.cisco.com (xhc-rcd-x05.cisco.com [173.37.183.79]) by rcdn-core-2.cisco.com (8.14.5/8.14.5) with ESMTP id s0OMflG7021895 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=FAIL); Fri, 24 Jan 2014 22:41:48 GMT
Received: from xmb-aln-x08.cisco.com ([169.254.3.191]) by xhc-rcd-x05.cisco.com ([173.37.183.79]) with mapi id 14.03.0123.003; Fri, 24 Jan 2014 16:41:47 -0600
From: "Charles Eckel (eckelcu)" <eckelcu@cisco.com>
To: "Tom Kristensen (tomkrist)" <tomkrist@cisco.com>, Mary Barnes <mary.ietf.barnes@gmail.com>
Thread-Topic: [bfcpbis] PROTO review comments on draft-ietf-bfcpbis-rfc4583bis-08
Thread-Index: AQHPGVV2eo89ZoxirUulY9xQVEy7Ww==
Date: Fri, 24 Jan 2014 22:41:46 +0000
Message-ID: <CF0823C7.1BA53%eckelcu@cisco.com>
References: <CAHBDyN58t7b0jJC3UcWB8fEASSApjje_Raz-06k88n90ZoKK7w@mail.gmail.com> <52D7F10B.5070401@cisco.com>
In-Reply-To: <52D7F10B.5070401@cisco.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
user-agent: Microsoft-MacOutlook/14.3.9.131030
x-originating-ip: [10.21.123.4]
Content-Type: text/plain; charset="Windows-1252"
Content-ID: <8BEB791B0F740C41ADAEA77E72166D94@emea.cisco.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
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-rfc4583bis.authors@tools.ietf.org" <draft-ietf-bfcpbis-rfc4583bis.authors@tools.ietf.org>
Subject: Re: [bfcpbis] PROTO review comments on draft-ietf-bfcpbis-rfc4583bis-08
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, 24 Jan 2014 22:42:11 -0000

On 1/16/14 6:47 AM, "Tom Kristensen (tomkrist)" <tomkrist@cisco.com> wrote:

>I went through and commented on this in my favourite editor (Emacs
>obviously), but forgot to paste it in and respond until now. I have
>updated my comments to align with Charles Eckel's  recent response too.

Thanks for that. More inline.

>
>Inline below.
>
>
>On 12/18/2013 11:38 PM, Mary Barnes wrote:
> > Hi all,
> >
> > I have agreed to shepherd this document on behalf of the WG. I have
> > reviewed this document in preparation for doing the PROTO write-up.
> >
> > I think the document needs a bit of work before it's ready to progress.
> > I have a few questions/comments, as well as editorial nits which I
>think
> > would improve readability and ease of understanding.
> >
> > _General Comments:_
> >
> > The document requires an SDP directorate review prior to progressing. I
> > have requested that of the MMUSIC WG chairs. At the time of this
>review,
> > Ali Begen has agreed to do that review.
>
>Yes, I have commented on his issues on the MMUSIC mailing list.
>
> > _Comments/Questions:_
> >
> > 1) I am slightly puzzled by the following in section 3:
> >
> > m=<media> <port> <transport> <fmt> ...
> >
> > Section 5.14 of RFC 4566 states the following:
> >
> > m=<media> <port> <proto> <fmt> ...
> >
> > So, this looks to be like the RFC 2327 ABNF for m lines is being used
> > (rather than RFC 4566 which is the normative SDP specification for this
> > document)?
> >
> > m=<media> <port> <transport> <fmt list>
> >
> > Note, that the IANA registration section appears to be based on RFC
>4566
> > as the values are specified as being applicable to the 'pro to' field.
> > So, my guess is that this is a bug from RFC 4583.
>
>Yes, an inherited bug that will be fixed without complications.
>
> > 2) Section 3, next to last paragraph. I think it would be more precise
> > to reword this as:
> >
> > OLD:
> >
> > The fmt (format) list is ignored for BFCP. The fmt list of BFCP 'm'
> > lines SHOULD contain a single "*" character.
> >
> > NEW:
> >
> > The fmt (format) list is not applicable to BFCP. The fmt list of 'm'
> > lines in the case of any proto field value related to BFCP SHOULD
> > contain a single "*" character. If the the fmt list contains any other
> > value it is ignored.
>
>Agree. That is a more precise wording for the fmt part.
>
>Charles Eckel touched upon this as well and was okay with this,
>especially if there are known reasons and/or implementations that
>already do otherwise. Well, once upon a time there was a SIP based
>product from a (large french) vendor that used 0 (I think) - don't think
>that product is still maintained. But it might be out there in the wild
>still. Will fix.
>
> > 3) Section 4, paragraph above table 1, 1st sentence. I suggest for
> > clarification to make the following change:
> >
> > OLD
> >
> > MUST include one in the corresponding media description
> >
> > NEW
> >
> > MUST include a 'floorctrl' attribute in the corresponding media
>description
>
>Fine with me, will update.
>
> > 4) Section 4, 3rd paragraph from the end:
> >
> > "Endpoints that use the offer/answer model to establish BFCP
>connections
> > MUST support the 'floorctrl' attribute. A floor control server acting
>as
> > an offerer or as an answerer SHOULD include the attribute in its
>session
> > descriptions."
> >
> > Since the latter is a SHOULD what happens if the attribute is NOT
> > included? Under what circumstances is it okay for the server not to
> > include it? IF bad things happen if it's not included, then this
> > probably ought to be a MUST. Or, if there's reasonable situations in
> > which the floor control server doesn't include the attribute, those
> > should be explained or at least an example provided.
>
>The default behaviour/assumption if the 'floorctrl' attribute is not
>used is explained in the paragraph below, i.e. offerer == client and
>answerer == server.
>
>I agree that the formulation "is not used in an offer/answer exchange"
>might point towards usage other than offer/answer, but the sentence
>continues with explaining what the offerer and answerer will assume. So
>fine with me. OK?
>
>If not, I will not object using MUST here - as also proposed by Charles
>Eckel.
>
> > 5) Section 7.
> >
> > a) 1st paragraph after ABNF. "..versions SHOULD be integers..". I would
> > think that ought to be a MUST. What happens if the token isn't an
> > integer and what was the motivation for not defining the version as an
> > integer versus a token? If non-integers can be used, the scope of what
> > is acceptable for the field needs to be explained.
>
>I agree, this will be upgraded to a MUST if people doesn't object.
>
>And if this will ever change non-integer versions must be specified in
>another draft/RFC and this draft probably have to be updated as well -
>since semantics for specific versions are specified later in Section 7.
>
> > b) 2nd paragraph after ABNF. I can see why supporting the version is a
> > SHOULD (since it is a new field) and RFC 4583 did not define a version,
> > but I think that needs to be more clearly documented and I think it
> > ought to be REQUIRED for anyone that is using UDP and obviously, older
> > implementations of TCP (i.e., those only compliant to RFC 4583 and not
> > this document) ought to be the only ones that aren't using the version
> > field.
>
>Is it sufficient to merge in the following sentence after the second
>sentence in that paragraph?
>"However, endpoints that supports RFCXXXX, and not only the RFC 4583
>subset, is REQUIRED to support and use the 'bfcpver' attribute."
>(Or something along those lines).

Works for me, though should be "endpoints Š are".

>
>A complicating factor here is that some pre-standard implementations, as
>experienced on recent SIPit and SuperOp test events, is out there and is
>not using the 'bfcpver' attribute. However, demanding that
>implementations following the new RFC from this draft have to use the
>version attribute should be perfectly fine.

Agreed, and stating that when omitted the default is "1" for TCP and "2"
for UDP addresses the problem of pre-standard UDP implementations as well.

>
> > c) last paragraph. Related to b, I would think that in the case of
> > unreliable transports, it's not that an endpoint MUST "assume". An
> > endpoint MUST "use" and signal a version number of 2. In the case of a
> > reliable transport, if the endpoint does not signal a
> > bfcpversion-attribute, then the floor control server MUST use a value
>of
> > 1. I'm recalling a fair amount of mailing list discussion on this one,
> > so if you can point to the thread where this text was agreed, I can let
> > this one go. I still think it's not specified quite right, but I can
> > live with it.
>
>I can't find too much discussion about this, but I agree that the word
>"assume" is not good here. What about reformulating this last paragraph
>to:
>
>"If a 'bfcpver' attribute is not present, default values are based on
>the transport specified in the m-line (Section 3). When used over an
>unreliable transport, an endpoint MUST use a value of "2", and when used
>over a reliable transport, an endpoint MUST use a value of "1". This is
>in line with the definition of the Version field in [8]."
>
>Any better?

Better, but how about:

"If a 'bfcpver' attribute is not present, default values are inferred from
the transport specified in the m-line (Section 3). In accordance with
definition of the Version field in [8], when used over a reliable
transport the default value is "1", and when used over an unreliable
transport the default value is "2".


>
> > 6) Section 8. 1st sentence. I think this should be a "can use TCP or
> > UDP" and not a "may use TCP or UDP". Note, there is the perennial
>debate
> > with regards to whether something is normative even when it's not CAPS.
> > I prefer to just use a different word to remove any confusion,
> > particularly when a different word is actually more correct.
>
>Well explained, the change is fine with me.
>
> > 7) Section 8.1, 4th paragraph, 1st sentence and last sentence. I think
> > the two "SHOULD generate and offer" ought to be a MUST generate an
>offer
> > OR you need to specify the circumstances under which a client would not
> > generate an offer.
>
>OK
>
> > 8) Section 9, 1st paragraph. You're going to have to be more specific
> > about the potential mechanisms for authentication - at least provide an
> > example of mechanisms - i.e., I don't think "some mechanism" will fly
> > with the SecDir folks.
>
>Too bad, since this is inherited from RFC 4583 - but of course that text
>wasn't perfect ;)
>Anyway, isn't the mechanisms meant here mentioned in the paragraphs
>below? If so, might extending to "using some mechanism, as explained
>below" be a valid solution in front of the SecDir review?
>
>Or as Charles Eckel proposed, pointing towards TLS/DTLS as the preferred
>mechanism, but other mechanisms exists but are outside the scope of this
>document.

This still works for me :)

Cheers,
Charles

>
> > 9) Section 11, 2nd paragraph. I think that the assumes in the following
> > statement needs to be a required. I suggest the following change:
> >
> > OLD
> >
> > BFCP assumes that an initial integrity-protected channel is used to
> > exchange...
> >
> > NEW
> >
> > An initial integrity-protected channel is REQUIRED for BFCP to
>exchange...
>
>Yes, I'll adopt that one. No assumption, we will require!
>
> > 10) Related to item 7, this list of changes has no mention of the new
> > "bfcpver" attribute. That definitely should be on this list.
>
>Indeed! It's missing since it arrived late, but that's just a poor
>excuse...
>
>
> > _Editorial nits:_
> >
> > 1) Introduction. "These data includes..." -> "This data includes...."
> > (data is now grammatically considered to be a singular noun).
>
>OK
>
> > 2) Section 3. 1st paragraph after the indented text. This could be
> > written more concisely (and per technical doc standards not as the 1st
> > person) as follows (and per comment 1) above, I think this should be
>the
> > 'proto' field:
> >
> > OLD:
> >
> > We define four new values for the transport field:
> >
> > NEW:
> >
> > This document defines four values for the proto field:
>
>OK
>
> > 3) Section 5. I suggest the following change:
> >
> > OLD:
> >
> > We define the 'confid' and the 'userid' SDP media-level attributes.
> >
> > NEW:
> >
> > This document defines two SDP media-level attributes: 'confid' and
>'userid'.
>
>OK
>
> > 4) Section 6. I suggest the following change:
> >
> > OLD:
> >
> > We define the 'floorid' SDP media-level attribute.
> >
> > NEW:
> >
> > This document defines the 'floorid' SDP media-level attribute.
>
>OK
>
> > 5) Section 13. I found this really, really hard to read. I suggest you
> > use a bulleted or numbered list versus this hanging list style.
>
>Sure, I'll tweak the XML curses and make it more readable.
>
>
>-- Tom
>_______________________________________________
>bfcpbis mailing list
>bfcpbis@ietf.org
>https://www.ietf.org/mailman/listinfo/bfcpbis