Subject: [rtcweb] AD review of draft-ietf-rtcweb-data-protocol

From: Richard Barnes <[email-masked]>
Date: Fri, 8 Aug 2014 16:27:31 -0400

I have reviewed this document in preparation for IETF LC.  In general, it
looks good, and I have requested LC.  There are some comments below that
should be addressed along with IETF LC comments.
--Richard

MAJOR:

Section 4. "To avoid glare..."
The term "glare" is undefined in this document, and will probably be
unfamiliar to many readers.  Please add it to the Terminology section.

Section 4. "When using SCTP over DTLS..."
This qualification is unnecessary, since data channels always use
SCTP/DTLS/UDP:
"""
   The DTLS encapsulation of SCTP packets as described in
   [I-D.ietf-tsvwg-sctp-dtls-encaps] MUST be used.
"""
So this should be "Since data channels use the DTLS encapsulation of
SCTP..."

Section 6. "... all parameters in the DATA_CHANNEL_OPEN message are valid"
The receiver MUST fail if the parameters are invalid, but you never define
what validation needs to be performed.  For example, is the DCEP layer
supposed to verify that the Protocol and Label fields are valid UTF-8?

Section 10.2. [I-D.ietf-rtcweb-data-channel]
This reference needs to be normative, since, after all, this is a protocol
for negotiating data channels.

MINOR / EDITORIAL:

Section 5.1. [packet diagram]
The diagram seems to imply that the protocol field starts on a 4-octet
boundary, even though this is not true.  It might be good if you could find
a way to reformat the diagram to make this clearer.

Section 5.1. "The suggested value of this field for IANA is 0x03"
You're defining the initial contents of this registry.  Just say it.
Likewise for the same text in Section 5.2.

Section 6. "an Stream identifier" -> "a stream identifier"

---

From: Michael Tuexen <[email-masked]>
Date: Thu, 21 Aug 2014 21:57:36 +0200

On 08 Aug 2014, at 22:27, Richard Barnes <[email-masked]> wrote:

> I have reviewed this document in preparation for IETF LC.  In general, it looks good, and I have requested LC.  There are some comments below that should be addressed along with IETF LC comments.
> --Richard
Hi Richard,

thank you very much for the review. See my comments in-line.

Best regards
Michael
> The term "glare" is undefined in this document, and will probably be unfamiliar to many readers.  Please add it to the Terminology section.
Right. It was introduced by a co-author and I hadn't read it before. Maybe the best is to
explicitly state what the problem is. So what about using

   To avoid collisions where both sides try to open a Data Channel with
   the same Stream Identifiers, each side MUST use Streams with either
   even or odd Stream Identifiers when sending a DATA_CHANNEL_OPEN
   message.  When using SCTP over DTLS
   [I-D.ietf-tsvwg-sctp-dtls-encaps], the method used to determine which
   side uses odd or even is based on the underlying DTLS connection
   role: the side acting as the DTLS client MUST use Streams with even
   Stream Identifiers, the side acting as the DTLS server MUST use
   Streams with odd Stream Identifiers.

   Note: There is no attempt to ensure uniqueness for the label; if both
   sides open a Data Channel labeled "x" at the same time, there will be
   two Data Channels labeled "x" - one on an even Stream pair, one on an
   odd pair.

   The protocol field is to ease cross-application interoperation
   ("federation") by identifying the user data being passed with an
   IANA-registered string ('WebSocket Subprotocol Name Registry' defined
   in [RFC6455]), and may be useful for homogeneous applications which
   may create more than one type of Data Channel.  Please note that
   there is also no attempt to ensure uniqueness for the protocol field.

> This qualification is unnecessary, since data channels always use SCTP/DTLS/UDP:
> """
> So this should be "Since data channels use the DTLS encapsulation of SCTP..."
The text is written in a way that you could use DCEP over SCTP not over DTLS.
That is why we have the qualification cited above and also this text in the
security considerations:

   This protocol does not provide privacy, integrity or authentication.
   It needs to be used as part of a protocol suite that contains all
   these things.  Such a protocol suite is specified in
   [I-D.ietf-tsvwg-sctp-dtls-encaps].
So is it OK to keep the current text?
> The receiver MUST fail if the parameters are invalid, but you never define what validation needs to be performed.  For example, is the DCEP layer supposed to verify that the Protocol and Label fields are valid UTF-8?
All parameters have to be checked (normal input validation of a receiver, I would say):
ChannelType: Is it one of the supported values?
Priority: Is it a value the receiver wants to work with? There are four values specified.
Reliability Parameter: Is it one of the supported values?
Label Length, Protocol Length: Does the packet contain exactly Label Length + Protocol Length + 12 bytes?
Label: Valid UTF-8?
Protocol: Valid UTF-8?

I would expect any receiver to do the above checks as normal input validation. If you prefer,
I can add explicit text.
> This reference needs to be normative, since, after all, this is a protocol for negotiating data channels.
OK.
> The diagram seems to imply that the protocol field starts on a 4-octet boundary, even though this is not true.  It might be good if you could find a way to reformat the diagram to make this clearer.
If you can point me to an example figure you like, please do. We used this kind
of figure in several RFC for SCTP and don't assume that the figure implies
that the packet starts on a 4 byte boundary. It just shows 32 bits in a row,
starting from the beginning of the packet.
> You're defining the initial contents of this registry.  Just say it.  Likewise for the same text in Section 5.2.
OK. What about using in Section 5.1

   Message Type: 1 byte (unsigned integer)
      This field holds the IANA defined message type for the
      DATA_CHANNEL_OPEN message.  The value of this field is 0x03 as
      specified in Section 8.2.1.

and in Section 5.2

   Message Type: 1 byte (unsigned integer)
      This field holds the IANA defined message type for the
      DATA_CHANNEL_ACK message.  The value of this field is 0x02 as
      specified in Section 8.2.1.
> Section 6. "an Stream identifier" -> "a stream identifier"
I would prefer to change it to a Stream Identifier, since we define 'Stream Identifier' in the Terminology section.
> 
> _______________________________________________
> rtcweb mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/rtcweb