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:  review-draft-ietf-netconf-crypto-types-28
Reviewer:  Dale R. Worley
Review Date:  2024-01-22
IETF LC End Date:  2024-01-24
IESG Telechat date:  [not known]

Summary:

    This draft is basically ready for publication, but has nits that
    should be fixed before publication.

Nits/editorial comments:

Editorial Note (To be removed by RFC Editor)

   Tree-diagrams in this draft may use the '/' line-folding mode defined
   in RFC 8792.  However, nicer-to-the-eye is when the '//' line-folding
   mode is used.  The AD suggested suggested putting a request here for
   the RFC Editor to help convert "ugly" '/' folded examples to use the
   '//' folding mode.  "Help convert" may be interpreted as, identify
   what looks ugly and ask the authors to make the adjustment.

Throughout this paragraph, slash '/' should be replaced by backslash
'\'.

1.1.  Relation to other RFCs

   The dependency relationship between the primary YANG groupings
   defined in the various RFCs is presented in the below diagram.

Perhaps there is a convention that I am not aware of, but when I see
in the figure e.g.

                                  crypto-types
                                    ^      ^
                                   /        \
                                  /          \
                         truststore         keystore

does that mean that crypto-types contains a reference to truststore,
or does it mean that truststore contains a reference to crypto-types?
The usual convention is that arrows point from referencer to
referenced, but also the usual convention is that the referenced thing
is written physically below the referencer.  Perhaps add an
explanatory sentence.

   Table 1: Label to RFC Mapping

In -28, this caption appears visually to be the caption of both the
dependency diagram at the top of page 5 and the label-to-RFC mapping
table at the bottom of page 5, and so probably should be amended to
describe both of them together.

1.4.  Conventions

   Various examples used in this document use a placeholder value for
   binary data that has been base64 encoded (e.g., "BASE64VALUE=").

This would be clearer if it stated directly that the (only)
placeholder value used is "BASE64VALUE=".  Perhaps

   Various examples in this document use "BASE64VALUE=" as a
   placeholder value for (usually binary) data has has been base64
   encoded.

2.1.2.  Identities

     +-- csr-format
           +-- p10-csr-format {p10-csr-format?}

This construct ends with "?}", whereas a number of other constructs
end with "}?".  Are all of these correct?

2.1.3.  Typedefs

   *  Additionally, all the typedefs define a type for encoding an ASN.1
      [ITU.X680.2021] structure using DER [ITU.X690.2021].

It seems like it would be useful to have a typedef "asn-1-der" that
extends "binary", to be used specifically for DER-encoded ASN.1 data,
and which in turn is extended here.  E.g.

     binary
       +-- asn-1-der
	   +-- csr-info
	   +-- csr
	   +-- x509
	   |  +-- trust-anchor-cert-x509
	   ...

Unfortunately, what would make such an extended type valuable is that
DER-encoded ASH.1 strings are used in a number of RFCs, which means
that this document might not be the best place to introduce such an
extended type.

2.3.  YANG Module

I am no expert on Yang, so my examination of the module itself was
superficial.  The Datatracker says that Yang doctors looked at -18 on
2021-01-12, which presumably means that -19 reflected their report.
The differences between the module in -19 and -28 appear to me to to
be minor.

3.5.  Strength of Keys Conveyed

   ... it is desireable ...

Wiktionary describes "desireable" as "an archaic form of desirable".
The RFC Editor's opinion on this should probably be sought.

    3.10.  The "ietf-crypto-types" YANG Module

The title of this section seems to be uninformative given that 'The
"ietf-crypto-types" YANG Module' is the subject of the entire
document.  Is this title what was intended?

   Some of the readable data nodes defined in this YANG module may be
   considered sensitive or vulnerable in some network environments.  It
   is thus important to control read access (e.g., via get, get-config,
   or notification) to these data nodes.  These are the subtrees and
   data nodes and their sensitivity/vulnerability:

The use of "These" in the last sentence does not have an unambiguous
referent as I read it.  Perhaps "These subtrees/data nodes have these
particular sensitivities/vulnerabilities:"  Similar considerations
apply to the last sentence of:

   Some of the operations in this YANG module may be considered
   sensitive or vulnerable in some network environments.  It is thus
   important to control access to these operations.  These are the
   operations and their sensitivity/vulnerability:

[END]