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

From: "Charles Eckel (eckelcu)" <[email-masked]>
Date: Tue, 14 Jan 2014 19:21:25 +0000

Hi Mary,

Thanks for the comments as part of the writeup. Please see inline.

From: Mary Barnes <[email-masked]<mailto:[email-masked]>>
Date: Wednesday, December 18, 2013 2:38 PM
To: "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>
Cc: Richard Barnes <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>
Subject: [bfcpbis] PROTO review comments on draft-ietf-bfcpbis-rfc4583bis-08

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.

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, the SDP directorate review hit on this as well. It needs to be fixed to align with RFC 4566.

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.

In another thread, I proposed to change from SHOULD to MUST. I am okay with your proposal as well, especially if there are known reasons and/or implementations that already do otherwise.

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

Agreed.

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.

Agreed in principle. There is a discussion of which way to go  on MMUSIC thread in which I proposed MUST was appropriate for servers.

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.

Agreed, and I think MUST be integers is fine.

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.

Yep.

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 think it would be good to tighten up the wording as you suggested.

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.

Works for 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.

Agreed.

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.

What is we were to say that TLS/DTLS is preferred, but other mechanisms are possible and outside the scope of this document.

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

Yep.

10) Related to item 7, this list of changes has no mention of the new "bfcpver" attribute.  That definitely should be on this list.

Good catch.

Editorial nits:

1) Introduction.  "These data includes..." -> "This data includes...."  (data is now grammatically considered to be a singular noun).

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:

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'.

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.

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.

Works for me.

Cheers,
Charles

---

From: Tom Kristensen <[email-masked]>
Date: Thu, 16 Jan 2014 15:47:39 +0100

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.

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:_
 > 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)?
 >
 > 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.

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

Fine with me, will update.

 > "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).

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.

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

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

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

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

OK

 > This document defines two SDP media-level attributes: 'confid' and 
'userid'.

OK

 > 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

---

From: "Charles Eckel (eckelcu)" <[email-masked]>
Date: Fri, 24 Jan 2014 22:41:46 +0000

>updated my comments to align with Charles Eckel's  recent response too.

Thanks for that. More inline.

>(Or something along those lines).

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

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

>in line with the definition of the Version field in [8]."

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".

>mechanism, but other mechanisms exists but are outside the scope of this

This still works for me :)

Cheers,
Charles

>_______________________________________________
>bfcpbis mailing list
>[email-masked]
>https://www.ietf.org/mailman/listinfo/bfcpbis