Re: [netconf] Genart last call review of draft-ietf-netconf-crypto-types-28

[Kent Watsen <[e-mail]>]

Hi Dale,

Thank you for your review!
Please see below for responses to your comments.

Kent


> On Jan 22, 2024, at 10:46 AM, Dale Worley via Datatracker <[e-mail]> wrote:
> 
> Reviewer: Dale Worley
> Review result: Ready with Nits
> 
> 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
> '\'.

Egads, how embarrassing ;)

Fixed!



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

Added the following paragraph (to each of the nine drafts in this suite of drafts):

Please note that the arrows in the diagram point from referencer
to referenced.  For example, the "crypto-types" RFC does not
have any dependencies, whilst the "keystore" RFC depends on the
"crypto-types" RFC.



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

s/Label in Diagram to RFC Mapping/Label to RFC Mapping/

Good enough?


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

Agreed, but since it’s always only used for binary data, I elided the “usually” bit, so the sentence now reads:

	Various examples in this document use "BASE64VALUE="
	as a placeholder value for 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?

Fixed (great catch!)


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

I think I’ll not make this change.


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

Ack.


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

Fixed.


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

For the most part, yes, I see your point.
Maybe s/The/For the/ or s/The/Regarding the/?

In any case, be aware that there exists an IETF-defined template
for the Security Considerations section that is to be used for each
YANG module defined in a draft.  So, if a draft defines the three
modules: ietf-foo-common, ietf-foo-client, and ietf-foo-server, the
Security Considerations section contains the three subsections:

	 The “ietf-foo-common" YANG Module
	 The “ietf-foo-client" YANG Module
	 The “ietf-foo-server" YANG Module

Each containing an instance of the template for that YANG module.


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

This text comes from the aforementioned template.  That said, I agree
that it’s not great.  Perhaps, even better, “*The following* subtrees and
data nodes have particular sensitivities/vulnerabilities”?


> [END]

Thanks again,
Kent