Re: [core] Roman Danyliw's Discuss on draft-ietf-core-senml-etch-05: (with DISCUSS and COMMENT)

[Ari Keränen <[e-mail]>]

Hi Roman,

Thank you for your review! And apologies for late reply. Here's a PR to address your comments:
https://github.com/core-wg/senml-etch/pull/13/files

See comments and questions inline below.


Cheers,
Ari

> On 4. Sep 2019, at 21.45, Roman Danyliw via Datatracker <[e-mail]> wrote:
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
> 
> (1) Section 5.  It’s helpful that this document notes the relevance of SenML’s
> security and privacy considerations (i.e., Section 13 of RFC8428).  However,
> this references seems circular.  RFC8428 says, “SenML formats alone do not
> provide any security and instead rely on the protocol that carries them to
> provide security.”  This document seems to be that “protocol that carries them”
> implying it  should cover the security mechanisms.  Is it not appropriate to
> suggest that CoAPs can address the (per RFC8428) “confidentiality, data
> integrity, and authentication as appropriate for the usage.”? If not, what
> additional guidance can be provided?

Yes, secure CoAP would be addressing the security requirements in this case. I can make this explicit by adding to the first paragraph:

  CoAP's security mechanisms are used to provide security for the FETCH and (i)PATCH methods.

> (2) Section 5.  It seems like it would be appropriate for the server to support
> access control to restrict the client’s ability access and modify a resource
> with FETCH and (i)PATCH.  My read of “Per “In FETCH and (i)PATCH requests, the
> client can pass arbitrary names to the target resource for manipulation.  The
> resource implementer must take care to only allow access to names that are
> actually part of (or accessible through) the target resource”, doesn’t suggest
> that type of access control.

Yes, access control should be OK here. Do you have any suggestion how to make this more clear here? Note that each SenML name may or may not map to a separate resource (hence the wording "are actually part of (or accessible through) the target resource").


> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> (3) Section 3.1.  A few questions about how general purpose of a query language
> is possible with the FETCH:
> 
> -- (To confirm based on the text “The SenML Records are selected by giving a
> set of names that …”) Does a name always have to be in the Fetch Request (i.e.,
> ‘at least one “n” MUST be in the Fetch Request’)?

Yes. I clarified this with:

A Fetch Pack MUST contain at least one Fetch Record. A Fetch Record MUST
contain a name and/or base name field(s).

> Using the third example in Section 5.1.2 of RFC8428 as a source:
> 
> -- The current text mentions that names and time can be in the Fetch Pack
> request.  Can other fields also be used as part of the criteria, say “v”?  For
> example, would ‘{“n”:” urn:dev:ow:10e2073a01080063”, “v”:”21.5”}’ return the
> three name+time records where the value is 21.5?

The intention was that only name and time could be used to select records; the last paragraph says that all others must be ignored. But looking at this closer I realized now that we probably need to add Unit to the selection criteria to support cases where same sensor name is used with multiple units for different types of measurements. 

But since this would be new functionality, I'll put this into a separate PR (needs likely more work):
https://github.com/core-wg/senml-etch/pull/14/files?utf8=%E2%9C%93&diff=split&w=1

> (4) Section 3.1. If there is no match for a Fetch, how is that signaled back in
> the response?  Is there any guidance to provide on error handling via CoAP
> error codes?

Good catch. Clarified this with:

If no SenML Records match, empty SenML Pack (i.e., array with no
elements) is returned as a response.

> (5) Editorial Nits:
> 
> -- Section 3.1.   s/When SenML Records contain also time values/When SenML
> Records also contain time values/

I realized this was not fully correct and rewrote that part into:
The SenML time field can be used in a Fetch Record to further narrow the
selection of matched SenML Records.