[core] Benjamin Kaduk's Discuss on draft-ietf-core-senml-14: (with DISCUSS and COMMENT)

Ari Keränen

2018-05-07 17:55:59 UTC

Thank you for the review Benjamin!

We have now submitted a new revision of the SenML draft that addresses all the IESG review comments:
https://tools.ietf.org/html/draft-ietf-core-senml-15

For answers to your review comments, please see below.

Thanks,
Ari

Post by Benjamin Kaduk
Benjamin Kaduk has entered the following ballot position for
draft-ietf-core-senml-14: Discuss
When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)
Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.
https://datatracker.ietf.org/doc/draft-ietf-core-senml/
----------------------------------------------------------------------
----------------------------------------------------------------------
I agree with Ben's DISCUSS.

This was addressed in the updated security considerations text:
https://github.com/core-wg/senml-spec/pull/106/commits/b911090ce2d12d394d243658f636522bb3dcc335

https://tools.ietf.org/html/draft-ietf-core-senml-15#section-13

Post by Benjamin Kaduk
Additionally, I have serious reservations about introducing the
concept of "base fields" that apply to subsequent array elemnets
unless overridden. It seems to violate an abstraction barrier for
at least some of the serialization formats, and prevents snippets
from being composable and commutable absent the
resolution/normalization process. It does not seem like the markup
language and the document contain suffient safeguards against misuse
to prevent security holes (both sensor data and commands could be
misinterpreted). It seems that some substantially expanded text
should be added on the hazards of the non-resolved format and giving
guidance on when resolution/normalization must be performed in order
to avoid correctness and security issues.

The base values feature is something that the WG decided is important to have. We added now text about the security implications and remedies, also taking into account Alexey's follow-ups in this thread: https://github.com/core-wg/senml-spec/pull/109/files

Post by Benjamin Kaduk
There also seem to be sizeable risks associated with the semantics
for time values. In particular, both with the use of an
implicit-"now-ish", and with positive and negative values being
interpreted with respect to a different absolute time base. (The
involvement of base time is a further complication -- I do not
remember any discussion of the interaction of a (positive) base time
and a negative regular time, for example. I also do not remember
any discussion of how the "now-ish" semantics make it actively
harmful to do things like store-and-forward or archive SenML data
(again, absent normalization), or what sort of granularity the
"now-ish" semantics are expected to adhere to. (Is "yesterday"
still considered "roughly now"?) I understand the desire for this
sort of semantics, but the current specification seems to leave many
potential problems exposed.

We clarified the "now" issue (also to address other comments on this topic). The new text in section 4.5.3 says:

Obviously, "now"-referenced SenML records are only useful within a
specific communication context (e.g., based on information on when
the SenML pack, or a specific record in a SensML stream, was sent) or
together with some other context information that can be used for
deriving a meaning of "now"; the expectation for any archival use is
that they will be processed into UTC-referenced records before that
context would cease to be available. This specification deliberately
leaves the accuracy of "now" very vague as it is determined by the
overall systems that use SenML. In a system where a sensor without
wall-clock time sends a SenML record with a "now"-referenced time
over a high speed RS 485 link to an embedded system with accurate
time that resolves "now" based on the time of reception, the
resulting time uncertainty could be within 1 ms. At the other
extreme, a deployment that sends SenML wind speed readings over a LEO
satellite link from a mountain valley might have resulting reception
time values that are easily a dozen minutes off the actual time of
the sensor reading, with the time uncertainty depending on satellite
locations and conditions.

Post by Benjamin Kaduk
----------------------------------------------------------------------
----------------------------------------------------------------------
Section 4.4
Just "Considerations" is an unusual subject title.

We re-structruted this part a bit and added sub-titles to this section and split it into "Extensibility" (4.4) and "Records and Their Fields (4.5). Should be more clear & usual now.

Post by Benjamin Kaduk
Having no Unit and no Base Unit is allowed, but you don't say what
the semantics are in that case (presumably just a dimensionless
counter for integers, with units not really being applicable to
non-numeric types). Interestingly, Section 5.1.7 deems it fit to
use "/" for the unit for a boolean value, even though "/" is
supposed to be a (continuous/floating-point) ratio.

The application using SenML may have additional ways to convey the unit information (where needed), as is the case with e.g., LwM2M use of SenML. We clarified this in the text now:

If the Record has no Unit, the Base Unit is used as the Unit. Having
no Unit and no Base Unit is allowed; any information that may be
required about units applicable to the value then needs to be provided
by the application context.

Post by Benjamin Kaduk
Section 4.5
Just to double-check: you really do want to privilege this
specification's version for eternity, for the purpose of being
omitted from resolved records?

Yes.

Post by Benjamin Kaduk
Section 12.1 is there not some other units registry we can use? I
fear begetting https://xkcd.com/927/ . Also, how is/should the
table be sorted?

We are not trying to replace all other such registries. In WISHI work, we haven’t encountered such a registry either. (The problem this one solves is to give ASCII names to units that are useful in SenML.)

The table is in no particular order (and doesn’t have to be); the current order seemed useful to the authors.

Post by Benjamin Kaduk
Also in Section 12.1, number 9, is the need for case sensitivity in
Units (or otherwise?) normatively covered anywhere? If not, should
it?

We added to end of section 3:

All comparisons of text strings
are performed byte-by-byte (and therefore necessarily case-sensitive).

Post by Benjamin Kaduk
Section 12
Are Base fields supposed to get negative CBOR labels
(and not other fields)? Is this mentioned explicitly somewhere?
(Yes, I know that the intent is for no more CBOR lablels to be
allocated, but that is only a SHOULD-level requirement.)

Added to section 6: "The base values are given negative CBOR labels and others non-negative labels."

Post by Benjamin Kaduk
In
Extensions that are mandatory to understand to correctly process the
Pack MUST have a label name that ends with the '_' character.
should that say something about "mandatory to understand but not
defined in this document"? (Also in 12.3.1 et seq?)

This specification doesn't actually define yet any extensions and all the labels in this specification are by definition mandatory to understand, so we don't think this is necessary.

Post by Benjamin Kaduk
Section 13
Why are we talking about "executable content" at all? It seems
quite unrelated to the rest of the document.

This was requested by Alexey due to fact that media type reviews often ask about this.