Re: [Gen-art] [Last-Call] Genart last call review of draft-ietf-core-senml-data-ct-04

[Carsten Bormann <[e-mail]>]

Hi Christer,

thank you for this review.

One interesting side note I have from reading the review is:
The CoRE WG, which produced this specification, is probably best known for maintaining the CoAP protocol, but it also has some other specifications, in particular with respect to data formats.

One example is the RFC 6690 link-format, which is well tied-in to the CoAP discovery mechanisms (RFC 7252 /.well-known/core, CoRE resource discovery).

SenML is the other major data format, with CoRAL coming up — both of these formats are really independent of CoAP, but of course have been designed to fit well with CoAP-based environments.

With that additional context, let’s dive in:

> Summary: I have reviewed the document. I have one technical comment, but the
> rest is mostly editorial. Related to that, I do think the document could use
> some editorial clean-up, e.g., when it comes to consistent terminology. I think
> it is also good not to assume that the reader knows CoAP, and to make sure the
> appropriate references/explanations are present when CoAP is referred to.
> 
> Major issues: N/A
> 
> Minor issues:
> 
> Q1 (TECHNICAL):
> 
> What happens if the receiver does not support the "ct" value? Is it a
> server-error? If so, what response code is used? I think that should be
> specified.

SenML is a data format; the specific protocol using which SenML data are passed around and how recipients of SenML would react in such a protocol to data they cannot process is not specified in SenML.  However, with respect to the extension point of adding new field names, Section 4.4 of RFC 8428 says:

  The SenML format can be extended with further custom fields.  […]   
  Implementations MUST ignore fields they don't recognize
  unless that field has a label name that ends with the "_" character,
  in which case an error MUST be generated.

So, as far as this can go while staying protocol-agnostic, the answer to this question is already fully specified in the base SenML document.

> Nits/editorial comments:
> 
> Q2 (EDITORIAL):
> 
> The text should use consistent terminology. See below for a few examples:
> 
> The Abstract says:
> 
>  "The Sensor Measurement Lists (SenML) media type supports multiple
>  types of values, from numbers to text strings and arbitrary binary
>  data values.  In order to simplify processing of the data values,
>  this document proposes to specify a new SenML field for indicating
>  the Content-Format of the data."
> 
> First the text talks about types of values, and then suddenly the
> Content-Format of the data.

Indeed, the abstract is a rather unfortunate contraction of the text in the introduction, which states explicitly that this document is about just one type of data, the “vd” field, i.e. binary data (see Section 4.2 of RFC 8428).

So we fixed the abstract:

OLD:
In order to simplify processing of the data values, this document proposes
to specify a new SenML field for indicating the Content-Format of the
data.
NEW:
In order to facilitate processing of binary data values, this document
specifies a pair of new SenML fields for indicating the
Content-Format of those binary data values, i.e., their Internet media
type including parameters as well as any Content-Coding applied.

Now in https://github.com/core-wg/senml-data-ct/commit/cc54f6b

> Content-Format is the name of the new field - that is not what you are
> indicating. You are using the new field to indicate something.

The name of the new field is “ct” (or “bct” for the corresponding base field).  
It is used to indicate the Content-Format, a precise definition of which for the purposes of this document follows in Section 2.

> Also, "Content-Format" is also used by CoAP, so please check that it is clear
> what is referred to whenever mentioned.
> 
> The text in Section 1 says:
> 
>  "To facilitate automatic interpretation it is useful to be able to
>  indicate an Internet media type and content-coding right in the SenML
>  Record."
> 
> ...and, the test in Section 7 says:
> 
> "The indication of a media type in the data does not exempt a consuming
> application from properly checking its inputs."
> 
> Now the text suddenly talks about "an Internet media type and content-coding",
> when it earlier (in the Abstract) talked about value of type.

Again, the abstract is excessively condensed here, see fix above.

> Q3 (EDITORIAL):
> 
> The text says:
> 
> "The CoAP Content-Format (Section 12.3 of [RFC7252]) provides just this
> information"
> 
> I think it would be good with a little introduction on how CoAP is related to
> all this.

The abstract of RFC 8428 says:

…  A simple sensor, such as a
  temperature sensor, could use one of these media types in protocols
  such as HTTP or the Constrained Application Protocol (CoAP) to
  transport the measurements of the sensor or to be configured.

(talking about the SenML media types.)


> 
> Also "provides just this information" probably needs some re-wording.

  To facilitate automatic interpretation it is useful to be able to
  indicate an Internet media type and content-coding right in the SenML
  Record.  The CoAP Content-Format (Section 12.3 of [RFC7252]) provides
  just this information; …

OK, we expanded this a bit into:

OLD:
Content-Format ({{Section 12.3 of -coap}}) provides just this
information; enclosing a Content-Format number (in this case number 60 as
defined for content-type application/cbor in {{-cbor}}) in the Record is
illustrated in {{ex-2}}. All registered CoAP Content-Formats are listed
NEW:
Content-Format ({{Section 12.3 of -coap}}) provides this
information in the form of a single unsigned integer; enclosing a Content-Format number (in this case number 60 as
defined for content-type application/cbor in {{-cbor}}) in the Record is
illustrated in {{ex-2}}. All registered CoAP Content-Formatn numbers are listed

Now in
https://github.com/core-wg/senml-data-ct/commit/09c105c
and
https://github.com/core-wg/senml-data-ct/commit/57e4713

> Q4 (EDITORIAL):
> 
> Section 6 contains the ABNF for the new fields.
> 
> Is there a reason you don't define them in the same way as the basic field is
> defined in RFC 8428 (there is no ABNF)?

Yes.  The field values for the fields in RFC 8428 have a rather simple structure (see Table 1 in Section 4.3); there was no need to provide ABNF.  A Content-Format-Spec can get complicated; there is no single standard that can be used to reference the ABNF for it from.
So we define it here.

> Q5 (EDITORIAL):
> 
> The text in Section 7 says:
> 
> "The indication of a media type in the data does not exempt a consuming
> application from properly checking its inputs."
> 
> I assume that by "its inputs" you refer to "received SenML data".
> 
> Shouldn't properly checking inputs be a generic CoAP security consideration?

It sure is, and this security consideration is maybe stating the obvious, but from the point of view of examining the change this spec brings to the security considerations of SenML, this is one of the significant items.

When stating the obvious, there is always a danger that this is misunderstood to mean something different, but I think the current text is innocuous here.


I have prepared a -05 on the basis of this review and the AD review; I’ll submit this once my computer has risen up from installing a mandatory security update...

Grüße, Carsten