Re: [yang-doctors] [Last-Call] Yangdoctors last call review of draft-ietf-netconf-trust-anchors-13

[Kent Watsen <[e-mail]>]

Thank you for your review, Juergen!

Below are responses to your comments.

PS: this response took longer because I had to triage with Security Experts on a couple points on the “crypto-types” draft.

Kent


> On Jan 12, 2021, at 1:41 PM, Jürgen Schönwälder via Datatracker <[e-mail]> wrote:
> 
> Reviewer: Jürgen Schönwälder
> Review result: Ready with Issues
> 
> The crypto modules aim at providing a flexible reusable infrastructure
> of groupings for modeling cryptographic keys and related concepts. The
> flexibility of the definitions provided of course comes with a certain
> amount of complexity.
> 
>> From a YANG perspective, draft-ietf-netconf-trust-anchors-13.txt is in
> a good and close to publish state (a couple of minor issues left).  I
> also tried to understand what is being modeled here and hence I also
> have some questions concerning the concepts modeled and I hope these
> are easy to answer/resolve as well.
> 
> - I have compiled the YANG modules using yanglint 0.16.105.
> 
> - The prefix 'ts' is rather short, the set of two letter prefixes is
>  rather small limited. This comment also applies to the other
>  documents, crypto-types uses 'ct. Perhaps this is not a problem
>  since collisions can be handled but if we go for rather short
>  prefixes, we will have to exercise the collision resolution. (I see
>  that 'ct' has already been used by ietf-complex-types, RFC 6095.) A
>  possible alternative could be to use sec-ct, sec-ts, sec-ks, ...).

Ugh (me, whenever a naming-discussion comes up ;)

<rant>I don’t know why prefixes are registered, as the prefix is defined at the top of each module.  The only value I see is not syntactic but rather for consistency, to aid human readability across disconnected sets of modules.</rant>

Prefixing the prefix with “sec-“ may work for trio of drafts you just reviewed, but it seems undesirable when getting to the client-server drafts, as they're more *protocol* oriented.  If “ks” and “ts” aren’t taken, then maybe we claim “first mover” advantage?  If not, then maybe “kstore” and “tstore” might be meaningful fallbacks?  Assuming we claim “first mover” advantage for “ks” and “ts”, then we just need to handle “ct”, which maybe could be “ctypes” or “cryptypes” or “ct2”?  So, 1) prefix the trio or 2) claim first-mover advantage on two and figure out something for “ct”?  …or maybe 3) keep “ct” also noting that a) “SHOULD NOT” is not a “MUST NOT” (in the text below), b) my “rant” might have some merit, and c) short and terse is nice (per your next comment).



>  RFC 8407 provides the following guidelines (Section 4.2):
> 
>   Prefix values SHOULD be short but are also likely to be unique.
>   Prefix values SHOULD NOT conflict with known modules that have been
>   previously published.
> 
>  Well, having short and terse prefixes is actually nice and enhancing
>  programmer readability.

:)


> - s/is defines a truststore/defines a truststore/

Fixed.


> - s/to be grouped referenced/to be grouped and referenced/

Fixed.


> - In 2.2.1, I was not sure what CA certificates are and what EE
>  certificates are. I then tried to guess EE = end entity cert, but
>  this does not explain CA since the term used in crypto types is
>  trust anchor cert. The description in the XML clarified that my
>  guess was kind of correct. Perhaps explain upfront what these
>  acronyms mean? Or perhaps the acronyms can be avoided by simply
>  spelling things out? They do not appear to be used frequently.

Good point, I expanded to using "trust anchor” and “end entity” (but then had to 's/certificates/certs/' to keep the diagram from overrunning)


> - s/<!-- Entity Certs/<!-- End Entity Certs/

Fixed.


> - s/(Section 2.1.3.2), groupings/(Section 2.1.3.2) groupings/

Fixed.


> - Is the feature truststore-supported really needed? Does the YANG
>  library not already provide the information whether a module has
>  been implemented or just imported to access type and grouping
>  definitions?

This was discussed before.  The bottom of Section 2.1.4 says: "The reason for why the "truststore-grouping" exists separate from the protocol-accessible nodes definition is to enable instances of the truststore to be instantiated in other locations, as may be needed or desired by some modules.”.  Case in point, the MacOS “Keychain Access” utility maintains both system-level and user-level trust anchors.  [UPDATE: you had a similar comment in the keystore draft, which you self-resolved there and then said to ignore this comment.  Just the same, let’s ensure the drafts are clear.]


> - In the YANG module, you seem to use Truststore to refer to
>  /ts:truststore but in the surrounding text you also use just
>  truststore. I am not sure it is necessary to have the capitalized
>  version but if people think its necessary, it makes sense to define
>  the difference and to make sure the proper capitalization is used
>  throughout the document. (If it is necessary somewhere to be
>  explicit, I would rather use /ts:truststore but that may be my
>  subjective preference. Well, I started to use 'central truststore'
>  below, not sure this is better as the term also would benefit from
>  being defined.)

No difference between the capitalized the uncapitalized forms was intended, it’s just me being inconsistent.  Happy to go either way.  You seem to prefer uncapitalized, so now they are.  [also applied to the keystore draft]


> - Does it make sense to be explicit about the fact that the cert/key
>  references are all weak references in the sense that they can exist
>  without the corresponding cert/key being present? In other words, in
>  order to safely delete a cert/key, one would have to check that
>  there are no dangling references left (which is difficult in case
>  references are scattered over multiple (proprietary) data models).
>  For YANG savvy people this may be obvious since there is no
>  require-instance statement in the leafref type definition but not
>  every implementer may recall this - and it would be good to document
>  that using weak references was an explicit design decision and not
>  an oversight.

Correct, there are no "require-instance” statements, but the "require-instance” statement defaults to “true”.  Do you mean the text should be explicit that strong references are enforced?


> - Does it make sense to spell out that using the grouping in other
>  YANG modules requires to define additional reference types since the
>  ones provided by this modules only work for the central truststore
>  store? And this as a consequence may require multiple leafs to refer
>  to keys that exist in different truststores, i.e., having multiple
>  truststores is possible but comes with a cost.

What section are you looking at?  Here’s what’s in the draft now:

In Section 2.1.3.1:

	Additional "case" statements MAY be augmented in if, e.g., there is a need to reference a bag in an alternate location.

In Section 2.1.3.2: 

	Additional "case" statements MAY be augmented in if, e.g., there is a need to reference a bag in an alternate location.

In the YANG module:

  grouping truststore-grouping {
    description
      "A grouping definition that enables use in other contexts.
       Where used, implementations SHOULD augment new 'case'
       statements into the local-or-truststore 'choice'
       statements to supply leafrefs to the model-specific
       location.";





> 
> - Would it make sense to add to all three documents an objectives
>  section that summarizes the design objectives? For this module,
>  a starting point might be this:
> 
>  2.1.  Objectives
> 
>  The design of the truststore data model was guided by the following
>  design objectives:
> 
>  - provide a central truststore for storing keys or certificates
>  - provide support for storing named bags of keys or certificates
>  - provide types that can be used to reference keys or certificates
>    stored in the central truststore
>  - protect the truststore using suitable access control definitions
>  - provide groupings that support locally configured keys or
>    certificates or references to key or certificate bags in the
>    central truststore
>  - provide groupings that can be used to instantiate truststores
>    in other data models (but this requires to introduce additional
>    types to reference keys or certificates)
> 
>  I do not know whether this list is complete but I find it usually
>  helpful to have the design objectives spelled out.

I pasted a modified version of this into the Introduction section.



> - Section 3 talks about populating <running> with built-in trust
>  anchors.
> 
>   In order for the built-in trust anchors to be referenced by
>   configuration, the referenced certificates MUST first be copied into
>   <running>.  The certificates SHOULD be copied into <running> using
>   the same "key" values, so that the server can bind them to the built-
>   in entries.
> 
>  Is the idea that this copy operation is an explicit management
>  operation or can implementations populate <running> with this
>  data automatically?

This item was discussed on list, the result of which is reflected the posted update  as well.



> - Section 4.2 says:
> 
>   This module does not define any RPCs, actions, or notifications, and
>   thus the security consideration for such is not provided here.
> 
>  Well, the module actually instantiates certificate-expiration
>  notifications.

True.   According to RFC 8407, Section 3.7.1, notifications are categorized as a “readable” node, for which the document currently says "None of the readable data nodes defined in this YANG module are considered sensitive or vulnerable in network environments”, which still seems accurate, so I just removed the statement about the module not defining any notifications.


> - Registrant Contact: should be changed to the IESG.

Fixed.



Thank you again for your review!

Kent