Re: [Cbor] Benjamin Kaduk's No Objection on draft-ietf-cbor-cddl-control-06: (with COMMENT)

Benjamin Kaduk <kaduk@mit.edu> Fri, 22 October 2021 01:53 UTC

Return-Path: <kaduk@mit.edu>
X-Original-To: cbor@ietfa.amsl.com
Delivered-To: cbor@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id C017E3A0898; Thu, 21 Oct 2021 18:53:03 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.499
X-Spam-Level:
X-Spam-Status: No, score=-1.499 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, KHOP_HELO_FCRDNS=0.399, SPF_HELO_NONE=0.001, SPF_NONE=0.001] autolearn=no autolearn_force=no
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id v6eQkWr2pQj4; Thu, 21 Oct 2021 18:52:58 -0700 (PDT)
Received: from outgoing.mit.edu (outgoing-auth-1.mit.edu [18.9.28.11]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 888263A048A; Thu, 21 Oct 2021 18:52:57 -0700 (PDT)
Received: from kduck.mit.edu ([24.16.140.251]) (authenticated bits=56) (User authenticated as kaduk@ATHENA.MIT.EDU) by outgoing.mit.edu (8.14.7/8.12.4) with ESMTP id 19M1qhDT001975 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 21 Oct 2021 21:52:49 -0400
Date: Thu, 21 Oct 2021 18:52:43 -0700
From: Benjamin Kaduk <kaduk@mit.edu>
To: Carsten Bormann <cabo@tzi.org>
Cc: The IESG <iesg@ietf.org>, Christian =?iso-8859-1?Q?Ams=FCss?= <christian@amsuess.com>, cbor@ietf.org, draft-ietf-cbor-cddl-control@ietf.org, cbor-chairs@ietf.org
Message-ID: <20211022015243.GS88762@kduck.mit.edu>
References: <163479859923.26389.9202872336721142535@ietfa.amsl.com> <8D259A3B-48A6-402B-B1E2-E85971BEFD05@tzi.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <8D259A3B-48A6-402B-B1E2-E85971BEFD05@tzi.org>
Archived-At: <https://mailarchive.ietf.org/arch/msg/cbor/YluYedYuhjtdGKsh80lv_egXbMY>
Subject: Re: [Cbor] Benjamin Kaduk's No Objection on draft-ietf-cbor-cddl-control-06: (with COMMENT)
X-BeenThere: cbor@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: "Concise Binary Object Representation \(CBOR\)" <cbor.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/cbor>, <mailto:cbor-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/cbor/>
List-Post: <mailto:cbor@ietf.org>
List-Help: <mailto:cbor-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/cbor>, <mailto:cbor-request@ietf.org?subject=subscribe>
X-List-Received-Date: Fri, 22 Oct 2021 01:53:04 -0000

Hi Carsten,

On Thu, Oct 21, 2021 at 01:39:09PM +0200, Carsten Bormann wrote:
> Hi Ben,
> 
> thank you for these detailed and thoughtful comments!
> 
> All the resulting changes below are now in
> https://github.com/cbor-wg/cddl-control/pull/13
> 
> > ----------------------------------------------------------------------
> > COMMENT:
> > ----------------------------------------------------------------------
> > 
> > Does the inclusion of RFC 7405 as a normative reference imply that the
> > %s"" construct is part of the core grammar used by .abnf and .abnfb?
> 
> Yes, that is the intention.
> 
> > My understanding was that just saying "ABNF" did not by default include
> > the case-sensitivity functionality (regardless of what references are
> > present), and so that it might be appropriate for us to say something
> > like "the ABNF control operators defined by this document allow use of
> > the case-sensitive extensions defined in [RFC7405]".
> 
> Interesting!  I live in a world that has pretty much completed the transition, and so I thought just always referencing 5234 and 7405 together would be sufficient indication.

That is reassuring to hear; it's possible that I should re-calibrate myself
(since I mostly only encounter ABNF while doing IESG reviews).

> I put into terminology:
> 
>    The term ABNF in this specification stands for the combination of
>    [RFC5234] and [RFC7405], i.e., the ABNF control operators defined by
>    this document allow use of the case-sensitive extensions defined in
>    [RFC7405].

Thanks; that is well said.

> > Section 2.1
> > 
> >   interval<BASE> = (
> >     BASE => int             ; lower bound
> >     (BASE .plus 1) => int   ; upper bound
> >     ? (BASE .plus 2) => int ; tolerance
> > 
> > I'm having a really hard time coming up for a use case where making the
> > tolerance depend on the base of the interval by an affine transformation
> > is useful.  It feels a bit contrived as an example.
> 
> Oh.  The BASE is just the encoding base, i.e., if base is 400, the three numbers used as labels here are 400, 401, 402 — no interaction with the semantics of an interval.

Oh!  I guess I should have brushed up on my CDDL more before reviewing, at
least when it's after 2200h local time.

> Now:
> 
>    The example in Figure 1 contains the generic definition of a CDDL
>    group "interval" that gives a lower and an upper bound and optionally
>    a tolerance.  The parameter BASE allows the non-conflicting use of
>    multiple of these interval groups in one map, by assigning different
>    labels to the entries of the interval. "rect" combines two of these
>    interval groups into a map, one group for the X dimension (using 0,
>    1, and 2 as labels) and one for Y dimension (using 3, 4, and 5 as
>    labels).

Thanks!

> > Section 4
> > 
> >   (enable/disable).  The latter approach could for instance be used for
> >   a JSON/CBOR switch, as illustrated in Figure 9.
> > 
> > I'd suggest something like "as illustrated in Figure 9 using SenML
> > [RFC8428] as the example data model used with both JSON and CBOR".  (The
> > main goal being to get the 8428 reference in.)
> 
> Nice!  I put that in.
> 
> > Section 6
> > 
> > The shepherd writeup indicates that the author has a complete
> > implementation, so it's surprising to not see it mentioned in this
> > Implementation Status section [that is to be removed prior to
> > publication as an RFC, so this comment itself is somewhat pointless].
> 
> That is actually the implementation mentioned as
> 
>    An early implementation of the control operator .feature has been
>    available in the CDDL tool described in Appendix F of [RFC8610] since
>    version 0.8.11.  […]

Ah.  I guess I was assuming that "early" and "complete" were incompatible,
which is not guaranteed to be a valid assumption.

> > Section 7
> > 
> > I would suggest that the ABNF security considerations would also imply,
> > except that both referenced documents disclaim any such considerations
> > (not even "what you actually describe may not be what you think you are
> > describing" or "ABNF just says whether a given string matches the
> > constraints of a given construction, and does not require a unique
> > interpretation of the string").
> 
> Yep, I was about to write “The security considerations of … apply”, and then read those...
> 
> I have now added:
> 
>    While both [RFC5234] and [RFC7405] state that security is truly
>    believed to be irrelevant to the respective document, the use of
>    formal description techniques cannot only simplify, but sometimes
>    also complicate a specification.  This can lead to security problems
>    in implementations and in the specification itself.  As with CDDL
>    itself, ABNF should be judiciously applied, and overly complex (or
>    "cute") constructions should be avoided.
> 
> I think that should really be a standard disclaimer on all FDT techniques.
> (I haven’t added “If the FDT specification does not make boring reading, you are holding it wrong.”)

:)

> > I might also consider mentioning that the behavior of the "floor" function
> > (for converting floating-point values to integers) on negative inputs
> > invariably surprises some people (i.e., it is not "round to zero").
> 
> I’d rather avoid that surprise be being more explicit:
> 
>  target; converting from a floating point number to an integer selects
>  its floor (the largest integer less than or equal to the floating
> -point number).
> +point number, i.e., rounding down).

I left a suggestion on the PR.

Thanks for this -- it, and the PR, look good.

-Ben