Re: [netconf] Yangdoctors last call review of draft-ietf-netconf-keystore-20

[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 14, 2021, at 12:00 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-keystore-20.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.
> 
> - Compared to the other two I-Ds in the batch, this I-D has a more
>  verbose introduction (appreciated) and it also has a terminology
>  section (which never hurts). I do not know whether Kent has the
>  energy to align the I-Ds in their intro style.

Not much, text-contributions help immensely.

FWIW, I did enhance the trust-anchor’s intro based on your review of that draft. 



> - s/in Examples (Section 2.2)./in Section 2.2./

Fixed.


> - Is the feature keystore-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? OK, I know see that this is used to make definitions
>  conditional, hence it makes sense. This means that my comment on
>  truststore-supported in the other review can be ignored, I found
>  the answer.

Please see my response to your comment in that draft’s review.  The net-net appears to us just needing to ensure things are clear enough.


> - My question concerning two-letter prefixes applies to this I-D as
>  well.

Let’s continue the discussion for this item in the trust-anchors as well.


> - In the YANG module, you seem to use Keystore to refer to
>  /ks:keystore but in the surrounding text you also use just
>  keystore. 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 /ks:keystore but that may be my
>  subjective preference. In the other I-D review, I used the term
>  'central truststore', which is not a good term either, the term
>  'well-known keystore' may be a better alternative.)

As mentioned in my response to your review of the trust-anchors draft, I uncapitalized all “Keystores” (unless the word is beginning a sentence).


> - Many of the groupings [use] either symmetric-key-ref or asymmetric-key-ref
>  and while the groupings seem to offer flexibility to instantiate
>  other keystores, I have some doubts that this actually works unless
>  you augment in other reference types. Looking at the
>  ex-keystore-usage module, I find in there the usage of
>  ks:symmetric-key-ref and ks:asymmetric-key-ref and they refer to the
>  well-known keystore, not the one defined in the example module.

Correct, these typedefs and all the groupings that use them are applicable only when the module is implemented.  When not implemented, the typedefs become useless and the "keystore-supported” feature is not set so as to disable these references, and different “case” statements need to be augmented into the “local-or-keystore” groupings.  

I think the issue is that the text isn’t clear enough; the latest update attempts to correct that (both here and in the trust-anchors draft)

BTW, “keystore-supported” may not be the best name.  “keystore-implemented” is closer, but maybe “keystore-module-implemented” or “central-keystore-implemented would be better still?   One question is if the “central” (or “global” or default?) prefix should be a term used throughout the draft.   The latest update has text like “… the keystore, when this module is implemented”, but this might be better if, e.g., “… the central keystore, when this module is implemented”.  Thoughts? 


>  YANG's reference mechanism via leafrefs is not really supporting
>  well what you try to do. I understand the flexibility you want to
>  achieve but it seems YANG 1.1 does not really support this well
>  enough. What you would need is a leafref type that can be "anchored"
>  at different places but we don't really have this...

I don’t understand this comment.  But looking to my previous comment, a way has been devised that works.  It may be kludgy relative to what you have in mind, but it seems to work.


>  You hint at this in the definition of the keystore-grouping:
> 
>         "Grouping definition enables use in other contexts.  If ever
>          done, implementations SHOULD augment new 'case' statements
>          into local-or-keystore 'choice' statements to supply leafrefs
>          to the new location.";
> 
>  It seems the SHOULD really is a 'must' (I do not care about
>  capitalization); if you do not add your own leafrefs, things will
>  not work or be majorly surprising.

Fixed (also in truststore)


>  If I am correct, then there should be stronger warnings upfront that
>  simply reusing the groupings is not enough and that the example
>  module is actually an incomplete example...
> 
>  The same may apply to some of the groupings in the truststore
>  drafts.

Yes, on both points (see previous comments and the updates)


> - There is a live discussion concerning the built-in keys, which
>  obviously applies here as well. Perhaps the conclusion is that what
>  we have is the best solution. This is just here as a reminder in
>  case there are changes.

That conversation primarily revolved around built-in trust anchors.  The same update applied to the truststore draft has been applied here.


> - Section 4 points to keys being compromised 'when in transit' but I
>  think we also want to protect keys at rest, i.e., sitting in a
>  backup.

Actually, Section 4 is all about protecting keys at rest.  What text are you looking at?


> - I am wondering whether key encryption also applies to the related
>  truststore document.

No, because trust anchors (certs and/or raw public keys) are *public*, and hence there are no secrets that need to be protected.


> - Expand RMA in "RMA scenarios" or simply avoid the acronym (its only
>  used once).

Removed the “RMA” acronym.


> - s/"default-deny-all)/"default-deny-all")/

Fixed.

> - Section 4.3 talks about _highly_ restricted access mechanisms and
>  _highly_ authorized clients and I am usually a bit confused what
>  _highly_ means. But I am YANG doctor, not a security reviewer. ;-)

Trying to say that, e.g., the NACM should only allow a "crypto officer" access to the MK’s cleartext value.



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

Same change as with truststore draft (copied below):

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