Last Call Review of draft-ietf-netconf-sztp-csr-02
review-ietf-netconf-sztp-csr-02-yangdoctors-lc-clarke-2021-06-08-00

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/