Subject: [yang-doctors] Yangdoctors last call review of draft-ietf-netconf-sztp-csr-02

From: Joe Clarke via Datatracker <[email-masked]>
Date: Tue, 08 Jun 2021 11:16:38 -0700

Reviewer: Joe Clarke
Review result: Ready with Nits

I have been asked to perform a last-call review of this draft on behalf of YANG
Doctors.  This draft specifies a YANG module for conveying a CSR in a Secure
Zero Touch Provisioning Bootstrapping Request.  It augments the
"get-bootstrapping-data" RPC defined in RFC 8572.  Overall, I found the draft
(and module) ready with the following small issues/nits.

Editorial Note:

You mention XXXX and AAAA, and you also use BBBB in the module
ietf-yang-structure-ext.  I know this section will be removed, but you may want
to mention this to make sure that substitution is done.

===

Section 2.1

The YANG tree here deviates from the output produced by pyang
--tree-print-structures --tree-line-length=69 --tree-module-name-prefix.  In
particular, the "flags" column is missing from each element under the augment
branch (e.g., the 'w' that appears in pyang's output's flags field).

===

Section 2.2

You have this text: Some implementation may choose to convey it inside a script
(e.g., SZTP's "pre-configuration-script")

I'd like to see a reference to RFC 8572 here on where this is defined.  I
realize I'm being pedantic here, but there are a number of SZTP documents, and
I think having a reference helps.

===

Section 2.2

s/so that the the device/so that the device/

===

Module overall:

I noticed that the module itself differs in various formatting from pyang -f
yang.  There are some line and spacing differences, and I don't think it would
hurt to canonicalize the module in the draft.

===

Module's main description:

You mention SZTP's 'onboarding-information' response, but this isn't a formal
YANG node.  That node is conveyed-information.  Is it worth clarifying that
here in the module description like you did in draft text in section 2.2?  I
have the same comment in the description of "container csr".  The quoted and
hyphenated "onboarding-information" might make one think that there is a node
that contains this.

===

Under leaf cmc's description:

s/is the TaggedCertificationRequest and it a bodyPartId/is the
TaggedCertificationRequest and it consists of a bodyPartId/

There are two instances of this, and I wasn't sure exactly what you wanted to
say here.  This was my attempt to make it readable.

===

While you indicate that the algorithm-selected node is mandatory, should this
not be the case for the selected-algorithm container that contains it since as
you state in the description for cert-req-info, it is an error if the
SubjectPublicKeyInfo's algorithm does not match the selected-algorithm.  Could
an implementer not return this container at all and still be compliant to the
module while causing unexpected behavior for the client?

In leaf cert-req-info's description:

The word “another” (with respect to the 400 Server Error) doesn’t follow here
unless one refers to the csr-support presence container above.  I think
dropping it might make this text a bit less confusing (or copying similar text
from the csr-support node’s description.

===

Section 3.1.2:

s/In cases where it it not possible/In cases where it is not possible/

s/key rather then generate/key rather than generate/

---

From: Kent Watsen <[email-masked]>
Date: Tue, 15 Jun 2021 20:06:45 +0000

[Sean, please note the question to you below]

Hi Joe,

Thank you for your review!

Diffs can be found here: https://www.ietf.org/rfcdiff?url2=draft-ietf-netconf-sztp-csr-03.txt <https://www.ietf.org/rfcdiff?url2=draft-ietf-netconf-sztp-csr-03.txt>.

Please see below for comments.

Kent

> On Jun 8, 2021, at 2:16 PM, Joe Clarke via Datatracker <[email-masked]> wrote:
> 
> Reviewer: Joe Clarke

Thank you.

> to mention this to make sure that substitution is done.

XXXX and AAAA are both discussed in the "Editorial Note (To be removed by RFC Editor)” section found at the beginning of the document.

Good catch on BBBB, but even more importantly, “RFC BBBB" is now “RFC 8791”, so I made that final designation instead.

> branch (e.g., the 'w' that appears in pyang's output's flags field).

True.  I uncharacteristically used `yanglint` to generate that tree diagram.  I reverted back to `pyang` and now the flags appear correctly.

> I think having a reference helps.

I added the sentence "SZTP onboarding information is described in Section 2.2 of [RFC8572].”  to the end of that paragraph.  Is this what you had in mind?

> s/so that the the device/so that the device/

Fixed!

> hurt to canonicalize the module in the draft.

Thanks - updated to better align with `pyang -f yang --keep-comments --yang-line-length 69 ietf-sztp-csr@*.yang`.

> hyphenated "onboarding-information" might make one think that there is a node

'onboarding-information’ *is* a formal node - it is described here: https://datatracker.ietf.org/doc/html/rfc8572#section-2.2.  

Just the same, I converted “ ‘onboarding-information’ ” to “onboarding information” (without the hyphen or single quotes, as it is also defined as a term that way in RFC 8572.

The clarification "onboarding-information" (encoded inside the "conveyed-information" node)" found in 2.2 was/is to help the reader with the following example snippet that doesn’t actually show the "onboarding-information” node because it is hidden inside the “base64encodedvalue==“ value.  Makes sense?

   {
     "ietf-sztp-bootstrap-server:output" : {
       "reporting-level": "verbose",
       "conveyed-information": “base64encodedvalue =="
     }
   }

> say here.  This was my attempt to make it readable.

Reading the description statement for "leaf cmc” shows that it describes three structures, each for a different condition.

I agree that the text for the last two structures is highly similar, though not exactly the same.  Perhaps the text could be simplified.  Sean, what do you think?  (Search for “leaf cmc” here: https://datatracker.ietf.org/doc/html/draft-ietf-netconf-sztp-csr-03 <https://datatracker.ietf.org/doc/html/draft-ietf-netconf-sztp-csr-03>)

> module while causing unexpected behavior for the client?

I think it’s okay.  I just tested and validation also fails if the "selected-algorithm” node is missing.

> from the csr-support node’s description.

Replaced “another 400 (Bad Request) response” with "a 400 (Bad Request) response containingvanother 'request-info' structure.” 

Better?

> s/In cases where it it not possible/In cases where it is not possible/
Fixed.

> s/key rather then generate/key rather than generate/

Fixed.

Thanks again!

Kent

---

From: "Joe Clarke (jclarke)" <[email-masked]>
Date: Tue, 15 Jun 2021 20:49:22 +0000

===

Section 2.2

Perfect.  That's exactly what I was looking for.

===

   {

Ah, I had my head buried in the YANG modules, and I didn't read the RFC8572 text closely enough.  I had missed the "conveyed-info" module.  Regardless, I think the module update is fine.

Better?

Yes.  Well, it's better in the diff.  You have a typo above :-).

Thanks!

Joe

---

From: Sean Turner <[email-masked]>
Date: Wed, 16 Jun 2021 14:05:53 -0400

> On Jun 15, 2021, at 16:06, Kent Watsen <[email-masked]> wrote:
> 
> I agree that the text for the last two structures is highly similar, though not exactly the same.  Perhaps the text could be simplified.  Sean, what do you think?  (Search for “leaf cmc” here: https://datatracker.ietf.org/doc/html/draft-ietf-netconf-sztp-csr-03)

This text explains which of the CMC’s ASN.1 fields need to be present (see RFC 6402 page 12 “PKIData” and the subfields there). I wrote it kind of like a decision tree because there are three options that need to be addressed. The 1st decision drives the later choices. There is some commonality at the 2nd between the 2nd and 3rd choice, but there’s multiple differences in the 1st two choices. Basically, I couldn’t figure out a way to write this without marching from the beginning to the end for each one of the three options.

spt