Subject: [bfcpbis] AD Review of draft-ietf-bfcpbis-rfc4583bis-24

From: Adam Roach <[email-masked]>
Date: Mon, 10 Sep 2018 15:41:40 -0500

This is my AD review of draft-ietf-bfcpbis-rfc4583bis. Thanks to 
everyone who
worked on this document. It looks generally in good shape, but there are two
issues that I think need to be addressed before placing the document 
into IETF
last call. I believe these should be pretty easy to address.

NOTE: If you think I am wrong about either blocking point, please speak 
up. It
is entirely possible that I've misunderstood something, and that the 
things I
believe are issues are actually okay.

---------------------------------------------------------------------------

§4 (BLOCKING):

 >  This document defines five values for the proto field: TCP/BFCP,
 >  TCP/DTLS/BFCP, TCP/TLS/BFCP, UDP/BFCP, and UDP/TLS/BFCP.

Generally, having more ways to do the same things in a protocol leads to 
less
interoperability rather than more. While the rationale for the four-way 
split
caused by TLS-versus-plaintext and UDP-versus-TCP is pretty self 
evident, there
appears to be no rationale in this document for having both 
TCP/DLTS/BFCP and
TCP/TLS/BFCP; more importantly, the document offers no guidance to 
implementors
regarding which to use. This is likely to lead to some implementations 
choosing
one encoding and others choosing the other somewhat arbitrarily. This 
decreases
the chances of the protocol interoperating.

Minimally, please include guidance regarding which of these two variations
implementations should use, and under which conditions. On a first 
glance, it
would appear that the guidance might be that non-ICE uses should use
TCP/TLS/BFCP for maximal compatibility with RFC 4583 implementations, 
and that
ICE uses need to use TCP/DTLS/BFCP, as outlined in section 9.

---------------------------------------------------------------------------

§7.1 (BLOCKING):

 >  When the existing TCP connection is closed and re-established
 >  following the rules in [16], the floor control client MUST send an
 >  offer towards the floor control server in order to re-establish the
 >  connection.

The behavior here is ambiguous in the case that "a=floorctrl:c-s" is 
negotiated.
This needs to be clarified. Perhaps something like:

    If the peers are acting as both floor control client and floor control
    server (i.e., if "a=floorctrl:c-s" has been negotiated), then the 
peer that
    most recently sent a successfully answered offer MUST send an offer.

---------------------------------------------------------------------------

The remaining comments are editorial, and can be handled as part of IETF 
last
call, or prior to last call, at the authors' convenience.

---------------------------------------------------------------------------

Abstract:

 >  This document obsoletes RFC 4583.  Changes from RFC 4583 are
 >  summarized in Section 15.

Nit: update to "Section 14."

---------------------------------------------------------------------------

§1:

 >  Following this model, a
 >  BFCP connection is described as any other media stream by using an
 >  SDP 'm' line, possibly followed by a number of attributes encoded in
 >  'a' lines.

It's probably worth including 'c' and 'b' lines in here as well, or possibly
phrasing it to be more general ("...possibly followed by additional SDP 
lines
that also apply to the BFCP connection")

---------------------------------------------------------------------------

§2:

Please update this section to match the boilerplate in RFC 8174.

---------------------------------------------------------------------------

§3:

 >  conference server will act as floor control server. However, there
 >  are scenarios where both endpoints would be able to act as floor
 >  control server.

Nit: "...act as a floor control server."
                 ^

---------------------------------------------------------------------------

§4:

 >  RFC4571 such that the length field defined in RFC4571 precedes each

Nit: "RFC 4571" (twice). See RFC 7322 §3.5, third bullet.

---------------------------------------------------------------------------

§4:

 >  Similarly, UDP/BFCP is used when BFCP runs directly on top of UDP,
 >  and UDP/TLS/BFCP is used when BFCP runs on top of DTLS, which in turn
 >  runs on top of UDP.

I'm sure there's rationale behind why this is "UDP/TLS/BFCP" rather than
"UDP/DTLS/BFCP". It would benefit the reader to include a one-sentence
explanation.

---------------------------------------------------------------------------

§5.1:

 >  This section defines the SDP 'floorctrl' media-level attribute.  The
 >  attribute is used to determine the floor control role(s) that the
 >  endpoints can take for the BFCP-controlled media streams. As
 >  described in Section 5.1

Nit: "As described in this section..."

---------------------------------------------------------------------------

§5.1, §5.2, §5.3, §5.4:

 >      The Augmented BNF syntax [RFC5234] for the attribute is:
...
 >        ;DIGIT is defined in [RFC5234]
...
 >        ;token is defined in [RFC4566]

The syntax for references in this document are of the style [2] rather than
[RFC5234]. Please either update the references in these sections to 
match the
rest of the document, or (my suggestion) include the following directive 
in your
XML source:

<?rfc symrefs="yes" ?>

---------------------------------------------------------------------------

§5.3:

 >     Although the example was non-normative, it is implemented by some
 >     vendors and occurs in cases where the endpoint is willing to act
 >     as an server.

Nit: "...as a server."

---------------------------------------------------------------------------

§9:

 >  multiple valid candidate pairs, and if BFCF media is shifted between

Nit: BFCP

---------------------------------------------------------------------------

§10:

 >     Note: The use of source-specific SDP parameters [18] is not
 >     defined to BFCP streams.

Nit: "...defined for BFCP streams."

---------------------------------------------------------------------------

§10.1:

 >  o  MUST associate an SDP 'label' attribute (Section 5.3) with the 'm'
 >     line of each BFCP-controlled media stream.

This is a little confusing, as Section 5.3 does not define the 'label'
attribute. I would suggest replacing "(Section 5.3)" with a reference to RFC
4574.

---------------------------------------------------------------------------

§10.2:

 >  When the answerer receives an offer, which contains an 'm' line
 >  describing a BFCP stream, the answerer MUST check whether it supports

"Contains an 'm' line describing a BFCP stream" appears to be a restrictive
clause rather than a parenthetical one. This calls for the use of "that" 
rather
than "which," and the removal of a comma:

    When the answerer receives an offer that contains an 'm' line
    describing a BFCP stream, the answerer MUST check whether it supports

---------------------------------------------------------------------------

§10.4:

 >  o  If the BFCP stream is carried on top of TCP, and if the offerer
 >     does not want to re-establish an existing TCP connection, the
 >     offerer MUST associate an SDP connection attribute with an
 >     'existing' value, with the 'm' line; and

Nit: remove quotes around "existing", and add quotes around "connection".

/a

---

From: Roman Shpount <[email-masked]>
Date: Mon, 10 Sep 2018 17:23:24 -0400

Hi Adam,

On Mon, Sep 10, 2018 at 4:41 PM, Adam Roach <[email-masked]> wrote:

> caused by TLS-versus-plaintext and UDP-versus-TCP is pretty self evident,
> there
> appears to be no rationale in this document for having both TCP/DLTS/BFCP
> and
> implementations should use, and under which conditions. On a first glance,
> it
> TCP/TLS/BFCP for maximal compatibility with RFC 4583 implementations, and
> that

You are correct, the reason there are two protocols is that TCP/TLS/BFCP
needs to be supported for legacy interop with RFC4583 but it does not work
with ICE. Since  ICE is designed as being able to switch between ICE
candidates, including UDP and TCP based candidates, without disrupting the
higher level protocol, any ICE transport, including ICE-TCP, should be
treated as packet-based unreliable transport. As a result, ICE can only
work with UDP/TLS/DTLS/BFC for UDP candidates or TCP/DTLS/BFCP for TCP
candidates. Both of these protocols treat underlying transport as
unreliable with DTLS responsible for packet re-transmission. On the other
hand,  TCP/TLS/BFCP relies on the underlying transport for reliable
in-order delivery, which is not provided by ICE. Furthermore, ICE is not
supported by plain UDP/BFC either, or you will end up with TCP/UDP/BFCP
(different from TCP/BFCP) which everyone found to be even more confusing. I
think some explanation of this would be helpful.

Regards,
_____________
Roman Shpount

---

From: Christer Holmberg <[email-masked]>
Date: Fri, 21 Sep 2018 21:21:33 +0000

Hi Roman,

Could you help providing explanation?

Perhaps a bullet list with each protocol value, and when to use it.

Regards,

Christer

From: Roman Shpount [mailto:[email-masked]]
Sent: 11 September 2018 00:23
To: Adam Roach <[email-masked]>
Cc: [email-masked]; [email-masked]
Subject: Re: [bfcpbis] AD Review of draft-ietf-bfcpbis-rfc4583bis-24

Hi Adam,

On Mon, Sep 10, 2018 at 4:41 PM, Adam Roach <[email-masked]<mailto:[email-masked]>> wrote:

>  TCP/DTLS/BFCP, TCP/TLS/BFCP, UDP/BFCP, and UDP/TLS/BFCP.

Generally, having more ways to do the same things in a protocol leads to less
interoperability rather than more. While the rationale for the four-way split
caused by TLS-versus-plaintext and UDP-versus-TCP is pretty self evident, there
appears to be no rationale in this document for having both TCP/DLTS/BFCP and
TCP/TLS/BFCP; more importantly, the document offers no guidance to implementors
regarding which to use. This is likely to lead to some implementations choosing
one encoding and others choosing the other somewhat arbitrarily. This decreases
the chances of the protocol interoperating.

Minimally, please include guidance regarding which of these two variations
implementations should use, and under which conditions. On a first glance, it
would appear that the guidance might be that non-ICE uses should use
TCP/TLS/BFCP for maximal compatibility with RFC 4583 implementations, and that
ICE uses need to use TCP/DTLS/BFCP, as outlined in section 9.

You are correct, the reason there are two protocols is that TCP/TLS/BFCP needs to be supported for legacy interop with RFC4583 but it does not work with ICE. Since  ICE is designed as being able to switch between ICE candidates, including UDP and TCP based candidates, without disrupting the higher level protocol, any ICE transport, including ICE-TCP, should be treated as packet-based unreliable transport. As a result, ICE can only work with UDP/TLS/DTLS/BFC for UDP candidates or TCP/DTLS/BFCP for TCP candidates. Both of these protocols treat underlying transport as unreliable with DTLS responsible for packet re-transmission. On the other hand,  TCP/TLS/BFCP relies on the underlying transport for reliable in-order delivery, which is not provided by ICE. Furthermore, ICE is not supported by plain UDP/BFC either, or you will end up with TCP/UDP/BFCP (different from TCP/BFCP) which everyone found to be even more confusing. I think some explanation of this would be helpful.

Regards,
_____________
Roman Shpount

---

From: Roman Shpount <[email-masked]>
Date: Fri, 21 Sep 2018 17:48:55 -0400

Christer,

Here is the best I can come up with:

The main use case for TCP/TLS/BFCP is legacy interop. This protocol should
be used if end point needs to communicate with existing equipment that
implements it. This protocol cannot be used with ICE and does not work in
NAT environments. This protocol utilizes TCP for reliable in-order delivery
and relies on these properties of TCP in its implementation, so that
implementation of this protocol is fairly different from  UDP/TLS/BFCP.

The main use case for TCP/DTLS/BFCP is ICE and associated support for NAT
environments. This is the protocol which will be used on the wire and
specified in the m= line when an ICE TCP candidate is used. It is
recommended to use this protocol for new implementations or when endpoints
deployed in NAT environments need to be supported. From implementation
point of view, TCP/DTLS/BFCP protocol compliments UDP/TLS/BFCP since it
allows seamless switch-over between TCP/DTLS/BFCP and UDP/TLS/BFCP within
the same communication session, which is required for ICE support. Both
protocols use the same encryption (DTLS), and the same mechanisms to insure
reliable message delivery. Essentially, TCP/DTLS/BFCP is UDP/TLS/BFCP with
DTLS packets sent over the TCP connection using framing defined in RFC 4571
instead of UDP, so that most of the transport implementation code can be
shared.

Regards,

_____________
Roman Shpount

On Fri, Sep 21, 2018 at 5:21 PM, Christer Holmberg <
[email-masked]> wrote:

> *From:* Roman Shpount [mailto:[email-masked]]
> *Sent:* 11 September 2018 00:23
> *To:* Adam Roach <[email-masked]>
> *Cc:* [email-masked]; [email-masked]
> *Subject:* Re: [bfcpbis] AD Review of draft-ietf-bfcpbis-rfc4583bis-24
>
>
>
> Hi Adam,
>
>
>

---

From: Christer Holmberg <[email-masked]>
Date: Fri, 21 Sep 2018 22:08:29 +0000

Hi,

I have updated the pull request. It should now have addressed everything except the proto value issue.

https://github.com/cdh4u/draft-bfcp-4583bis/pull/10

Regards,

Christer

-----Original Message-----
From: Adam Roach [mailto:[email-masked]] 
Sent: 10 September 2018 23:42
To: [email-masked]
Cc: [email-masked]
Subject: AD Review of draft-ietf-bfcpbis-rfc4583bis-24

This is my AD review of draft-ietf-bfcpbis-rfc4583bis. Thanks to everyone who worked on this document. It looks generally in good shape, but there are two issues that I think need to be addressed before placing the document into IETF last call. I believe these should be pretty easy to address.

NOTE: If you think I am wrong about either blocking point, please speak up. It is entirely possible that I've misunderstood something, and that the things I believe are issues are actually okay.

§4 (BLOCKING):

 >  This document defines five values for the proto field: TCP/BFCP,  >  TCP/DTLS/BFCP, TCP/TLS/BFCP, UDP/BFCP, and UDP/TLS/BFCP.

Generally, having more ways to do the same things in a protocol leads to less interoperability rather than more. While the rationale for the four-way split caused by TLS-versus-plaintext and UDP-versus-TCP is pretty self evident, there appears to be no rationale in this document for having both TCP/DLTS/BFCP and TCP/TLS/BFCP; more importantly, the document offers no guidance to implementors regarding which to use. This is likely to lead to some implementations choosing one encoding and others choosing the other somewhat arbitrarily. This decreases the chances of the protocol interoperating.

Minimally, please include guidance regarding which of these two variations implementations should use, and under which conditions. On a first glance, it would appear that the guidance might be that non-ICE uses should use TCP/TLS/BFCP for maximal compatibility with RFC 4583 implementations, and that ICE uses need to use TCP/DTLS/BFCP, as outlined in section 9.

§7.1 (BLOCKING):

 >  When the existing TCP connection is closed and re-established  >  following the rules in [16], the floor control client MUST send an  >  offer towards the floor control server in order to re-establish the  >  connection.

The behavior here is ambiguous in the case that "a=floorctrl:c-s" is negotiated.
This needs to be clarified. Perhaps something like:

    If the peers are acting as both floor control client and floor control
    server (i.e., if "a=floorctrl:c-s" has been negotiated), then the peer that
    most recently sent a successfully answered offer MUST send an offer.

The remaining comments are editorial, and can be handled as part of IETF last call, or prior to last call, at the authors' convenience.

Abstract:

 >  This document obsoletes RFC 4583.  Changes from RFC 4583 are  >  summarized in Section 15.

§1:

 >  Following this model, a
 >  BFCP connection is described as any other media stream by using an  >  SDP 'm' line, possibly followed by a number of attributes encoded in  >  'a' lines.

It's probably worth including 'c' and 'b' lines in here as well, or possibly phrasing it to be more general ("...possibly followed by additional SDP lines that also apply to the BFCP connection")

§2:

§3:

 >  conference server will act as floor control server. However, there  >  are scenarios where both endpoints would be able to act as floor  >  control server.

§4:

 >  Similarly, UDP/BFCP is used when BFCP runs directly on top of UDP,  >  and UDP/TLS/BFCP is used when BFCP runs on top of DTLS, which in turn  >  runs on top of UDP.

I'm sure there's rationale behind why this is "UDP/TLS/BFCP" rather than "UDP/DTLS/BFCP". It would benefit the reader to include a one-sentence explanation.

§5.1:

 >  This section defines the SDP 'floorctrl' media-level attribute.  The  >  attribute is used to determine the floor control role(s) that the  >  endpoints can take for the BFCP-controlled media streams. As  >  described in Section 5.1

...
 >        ;DIGIT is defined in [RFC5234] ...

The syntax for references in this document are of the style [2] rather than [RFC5234]. Please either update the references in these sections to match the rest of the document, or (my suggestion) include the following directive in your XML source:

<?rfc symrefs="yes" ?>

§5.3:

 >     Although the example was non-normative, it is implemented by some  >     vendors and occurs in cases where the endpoint is willing to act  >     as an server.

Nit: "...as a server."

Nit: BFCP

§10:

 >     Note: The use of source-specific SDP parameters [18] is not  >     defined to BFCP streams.

This is a little confusing, as Section 5.3 does not define the 'label'
attribute. I would suggest replacing "(Section 5.3)" with a reference to RFC 4574.

§10.2:

 >  When the answerer receives an offer, which contains an 'm' line  >  describing a BFCP stream, the answerer MUST check whether it supports

"Contains an 'm' line describing a BFCP stream" appears to be a restrictive clause rather than a parenthetical one. This calls for the use of "that" 
rather
than "which," and the removal of a comma:

§10.4:

 >  o  If the BFCP stream is carried on top of TCP, and if the offerer  >     does not want to re-establish an existing TCP connection, the  >     offerer MUST associate an SDP connection attribute with an  >     'existing' value, with the 'm' line; and

/a

---

From: Christer Holmberg <[email-masked]>
Date: Fri, 21 Sep 2018 22:42:01 +0000

Hi,

Note that the draft already contains SOME descriptions on when to use what.

For example, the “ICE Considerations” section talks about 'UDP/TLS/BFCP' and 'TCP/DTLS/BFCP'. 

What about the following bullet points?

- 'TCP/BFCP' is used for TCP transport without TLS or DTLS encryption, and is backward interoperability with RFC 4583 endpoints. 
- 'TCP/TLS/BFCP' is used when TCP transport with TLS encryption is used, and is backward interoperability with RFC 4583 endpoints that support. 
- 'UDP/BFCP' is used for UDP transport without DTLS encryption.
- 'UDP/TLS/BFCP' and 'TCP/DTLS/BFCP' are used with ICE (Section 9), and can also be used without ICE for communication between endpoints compliant
with this specification (there is no backward interoperability with RFC 4583 endpoints).
 
Regards,

Christer

From: Roman Shpount [mailto:[email-masked]] 
Sent: 22 September 2018 00:49
To: Christer Holmberg <[email-masked]>
Cc: Adam Roach <[email-masked]>; [email-masked]; [email-masked]
Subject: Re: [bfcpbis] AD Review of draft-ietf-bfcpbis-rfc4583bis-24

Christer,

The main use case for TCP/TLS/BFCP is legacy interop. This protocol should be used if end point needs to communicate with existing equipment that implements it. This protocol cannot be used with ICE and does not work in NAT environments. This protocol utilizes TCP for reliable in-order delivery and relies on these properties of TCP in its implementation, so that implementation of this protocol is fairly different from  UDP/TLS/BFCP.

The main use case for TCP/DTLS/BFCP is ICE and associated support for NAT environments. This is the protocol which will be used on the wire and specified in the m= line when an ICE TCP candidate is used. It is recommended to use this protocol for new implementations or when endpoints deployed in NAT environments need to be supported. From implementation point of view, TCP/DTLS/BFCP protocol compliments UDP/TLS/BFCP since it allows seamless switch-over between TCP/DTLS/BFCP and UDP/TLS/BFCP within the same communication session, which is required for ICE support. Both protocols use the same encryption (DTLS), and the same mechanisms to insure reliable message delivery. Essentially, TCP/DTLS/BFCP is UDP/TLS/BFCP with DTLS packets sent over the TCP connection using framing defined in RFC 4571 instead of UDP, so that most of the transport implementation code can be shared.

Regards,

_____________
Roman Shpount

On Fri, Sep 21, 2018 at 5:21 PM, Christer Holmberg <[email-masked]> wrote:
Hi Roman,

 
Regards,
 
Christer

Hi Adam,

 
 
Regards,
_____________
Roman Shpount

---

From: Roman Shpount <[email-masked]>
Date: Fri, 21 Sep 2018 19:36:11 -0400

Hi Christer,

We can use this, but " is backward interoperability with RFC 4583 endpoints
that support" sounds a bit like Yoda to me. Can you correct this?

Regards,

_____________
Roman Shpount

On Fri, Sep 21, 2018 at 6:42 PM, Christer Holmberg <
[email-masked]> wrote:

> - 'TCP/BFCP' is used for TCP transport without TLS or DTLS encryption, and
> is backward interoperability with RFC 4583 endpoints.
> - 'TCP/TLS/BFCP' is used when TCP transport with TLS encryption is used,
> and is backward interoperability with RFC 4583 endpoints that support.
> - 'UDP/TLS/BFCP' and 'TCP/DTLS/BFCP' are used with ICE (Section 9), and
> can also be used without ICE for communication between endpoints compliant

---

From: Christer Holmberg <[email-masked]>
Date: Sat, 22 Sep 2018 06:29:45 +0000

Hi,

>We can use this, but " is backward interoperability with RFC 4583 endpoints that support" sounds a bit like Yoda to me. Can you correct this?

Correct the text I can :)

"...is backward interoperability with RFC 4583 endpoints that support TLS."

Or, does 4583 mandate support of TLS? In that case we don't need the "that support TLS" part.

In addition, I guess we should say "backward compatibility" instead of "backward interoperability".

Regards,

Christer

On Fri, Sep 21, 2018 at 6:42 PM, Christer Holmberg <[email-masked]> wrote:
Hi,

 
Regards,

Christer

Christer,

Regards,

_____________
Roman Shpount

 
 
Regards,
 
Christer

Hi Adam,

 
 
Regards,
_____________
Roman Shpount

---

From: Christer Holmberg <[email-masked]>
Date: Sat, 22 Sep 2018 13:04:49 +0000

Hi,

I have updated the pull request. It now also addresses Adam's issues regarding when to use specific 'm' line proto values.

Regards,

Christer

-----Original Message-----
From: Christer Holmberg [mailto:[email-masked]] 
Sent: 22 September 2018 09:30
To: Roman Shpount <[email-masked]>
Cc: Adam Roach <[email-masked]>; [email-masked]; [email-masked]
Subject: RE: [bfcpbis] AD Review of draft-ietf-bfcpbis-rfc4583bis-24

Hi,

Regards,

Christer

- 'TCP/BFCP' is used for TCP transport without TLS or DTLS encryption, and is backward interoperability with RFC 4583 endpoints. 
- 'TCP/TLS/BFCP' is used when TCP transport with TLS encryption is used, and is backward interoperability with RFC 4583 endpoints that support. 
- 'UDP/BFCP' is used for UDP transport without DTLS encryption.
- 'UDP/TLS/BFCP' and 'TCP/DTLS/BFCP' are used with ICE (Section 9), and can also be used without ICE for communication between endpoints compliant with this specification (there is no backward interoperability with RFC 4583 endpoints).
 
Regards,

Christer

Christer,

Regards,

_____________
Roman Shpount

 
 
Regards,
 
Christer

Hi Adam,

 
 
Regards,
_____________
Roman Shpount