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

[Kent Watsen <[e-mail]>]

[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 <[e-mail]> wrote:
> 
> 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.

Thank you.


> 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.

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.


> ===
> 
> 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).

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



> ===
> 
> 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.

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?


> ===
> 
> Section 2.2
> 
> s/so that the the device/so that the device/

Fixed!


> ===
> 
> 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.

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



> ===
> 
> 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.

'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 =="
     }
   }


> ===
> 
> 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.

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>)


> ===
> 
> 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?

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



> 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.

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

Better?


> ===
> 
> Section 3.1.2:
> 
> 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