Subject: [Gen-art] Genart last call review of draft-ietf-cbor-cddl-control-05

From: Theresa Enghardt via Datatracker <[email-masked]>
Date: Thu, 16 Sep 2021 11:12:54 -0700

Reviewer: Theresa Enghardt
Review result: Ready with Nits

I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair.  Please treat these comments just
like any other last call comments.

For more information, please see the FAQ at

<https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.

Document: draft-ietf-cbor-cddl-control-05
Reviewer: Theresa Enghardt
Review Date: 2021-09-16
IETF LC End Date: 2021-09-21
IESG Telechat date: Not scheduled for a telechat

Summary: This document is fairly clear and concise, and basically ready for
publication. I noticed a few minor issues and nits that could further enhance
document clarity.

Major issues: None.

Minor issues:

Section 1:
Please consider adding a brief description of what a control operator is, how
it can be used, and what its components are.

.feature is described as "Detecting feature use in extension points"
(introduction) VS "indicating the use of a non-basic feature in an instance"
(abstract) - How can a control operator detect feature use, doesn't it merely
indicate? If both of these descriptions are meant to conved the same meaning,
please consider changing the "purpose" of .feature to something like "Name of a
feature that is used", or, ideally, find a different word to capture what a
feature is.

Section 2:
"CDDL as defined in [RFC8610] does not have any mechanisms to compute
   literals.  As an 80 % solution, this specification adds three control
   operators"
What are the missing 20%? Please consider briefly mentioning the limitations of
the new operators.

"Not all tools may be able to work with non-unique targets or controllers."
Here, "tool" refers to "CDDL tool" as used in RFC8610, correct? If so, please
consider making all references to "tool" in this document more precise.

Section 2.3:
Please consider adding the outcome of the .det operation in the example in
Figure 3: The definition of dedenting includes "determining the smallest amount
of left-most blank space (number of leading space characters) in all the
non-blank lines". If I understand correctly, taking this definition literally,
the operation ("oid" .det cbor-tags-oid) would result in no space being removed
at all, because the string "oid" does not contain anly left-most blank spaces.
But I suspect that the example is intended to show dedenting of the contents of
the cbor-tags-oid variable. Perhaps I'm missing something about CDDL here that
was discussed in RFC8610.

Section 7 (Security considerations):
Can there be any additional security concerns if CDDL specifications can
contain ABNF or "arbitrary" features? While this document obviously can't go
into specifics, maybe it's worth calling out that one needs to pay specific
attention if these control operators are used.

Nits/editorial comments:

Section 2.2
"concatenating the target text string ""foo"""
Is foo intended to be in two double quotes, or should there only be one pair of
quotes?

Section 3
"by defining a ".abnf" control operator"
Should this say 'an ".abnf" control operator' instead?

---

From: Carsten Bormann <[email-masked]>
Date: Fri, 17 Sep 2021 17:28:02 +0200

Hi Theresa,

thank you for your review, which highlights a number of needed improvements.

> it can be used, and what its components are.

I’m not so sure about that.  This document really is not useful without some knowledge of RFC 8610, and the use of control operators is detailed there.  Paraphrasing a normative document in another normative document always has this danger of subtly changing things…

I added a reference to Section 3.8 of RFC8610 to the first sentence.

Now https://github.com/cbor-wg/cddl-control/commit/036f6d8

> feature that is used", or, ideally, find a different word to capture what a

Good point.
I don’t think losing the term “extension point”, so I changed this into:

| .feature | Indicate name of feature used (extension point) |

It is always hard to cram all the intended meaning into a table cell, but I think this should be closer.

Now https://github.com/cbor-wg/cddl-control/commit/909b407

> What are the missing 20%? Please consider briefly mentioning the limitations of

Indeed, that wording was noticed by others, too.

https://github.com/cbor-wg/cddl-control/commit/87ad909 changes this into:

-literals.  As an 80 % solution, this specification adds three control
+literals.  To cover a large part of the use cases, this specification adds three control

I think it should be obvious that numeric addition and string concatenation (possibly dedented) does not cover everything that a full-blown expression language could (which would need to wait for CDDL 2.0 if we were going to create that).

> consider making all references to "tool" in this document more precise.

I put this into the terminology (after the reference to RFC 8610 terminology):

+"Tool" refers to tools along the lines of that described in {{Appendix F of -cddl}}.

Now https://github.com/cbor-wg/cddl-control/commit/6241afe

> at all, because the string "oid" does not contain anly left-most blank spaces.

The text says that "both arguments (target and controller) are independently dedented before the concatenation takes place”, so that rules out this interpretation.
But examples always help, so I added:

For the first rule in {{exa-det}}, the result is
equivalent to {{exa-det-result}}.

~~~~ cddl
oid = bytes .abnfb 'oid
oid = 1*arc
roid = *arc
arc = [nlsb] %x00-7f
nlsb = %x81-ff *%x80-ff
'
~~~~
{: #exa-det-result title="Dedenting example: result of first .det”}

Now https://github.com/cbor-wg/cddl-control/commit/de9bc15

> attention if these control operators are used.

Isn’t that true for any extension to any language?
I’d prefer to highlight specific security considerations when I’m aware of them.
(I wouldn’t go as far as RFC 5234, though:

5.  Security Considerations

   Security is truly believed to be irrelevant to this document.

:-)

> Is foo intended to be in two double quotes, or should there only be one pair of

This is an artifact of the way xml2rfc handles <tt> elements.
There currently is no way to mark the document up in a way such that both plain text and html versions look good, and I opted to prefer the HTML to look good (and correct).
(RFC 8949 has a long explanation in several paragraphs of Section 1.2 how the plain text sometimes will look weird; I was hoping I wouldn’t have to repeat this here.)
There is a proposed change to xml2rfc that will change the specifics, so I would like to wait for how that plays out before making changes.

> Should this say 'an ".abnf" control operator' instead?

Actually, we say out loud “dot a b n f control operator”, so “a” corresponds to the speech pattern.

The changes are collected in the “TE-…” commits in https://github.com/cbor-wg/cddl-control/pull/11

Grüße, Carsten

---

From: Theresa Enghardt <[email-masked]>
Date: Fri, 17 Sep 2021 15:07:06 -0700

Hi Carsten,

Thanks for the reply and changes, they look good to me.

> I’d prefer to highlight specific security considerations when I’m aware of them.

Probably true, but I guess I prefer to state the obvious, especially 
when it comes to security, to be safer rather than sorry. Also I like it 
when a document that "builds" on another document still shows that the 
authors considered the implications of what they added.

That said, I won't argue with your reasoning :)

> Actually, we say out loud “dot a b n f control operator”, so “a” corresponds to the speech pattern.

Thanks, I learned something in both of these cases.

Best,
Theresa