Re: [netconf] WG LC for three drafts

[Kent Watsen <[e-mail]>]

Hi Juergen

Following up on this thread…


>> OLD: Common YANG Data Types for Cryptography
>> NEW: Common YANG Data Types and Groupings for Cryptography
>> 
> 
> Works for me.

Done (and I also took out the word “Common”).


>>> - The certificate-expiration notification of the
>>>   trust-anchor-certs-grouping probably needs adjustment; I assume
>>>   this applies to any certificate the the cert leaf-list.
>> 
>> Yes, a notification could be for any “cert” entry in the leaf-list.  What adjustment is needed?  Is it just the description text?  Or are you thinking that the notification will be unable to identify the specific leaf-list entry that is expiring?
>> 
>> For NETCONF, it seems that the XPath would be able to identity the specific certificate (e.g., [1], [2], etc.).   RESTCONF might be problematic though, maybe the full cert is listed?
>> 
>> Digging deeper, I note that “leaf-list cert” appears only in two groupings:
>> 
>>  1) the "end-entity-certs-grouping” grouping
>>       - this grouping isn’t used anywhere (AFAIK) and could be removed
>> 
>>   2) the "trust-anchor-certs-grouping” grouping
>>       - this grouping is used in the truststore draft, but only in the definition
>>         of the "local-or-truststore-certs-grouping” grouping, and then only to
>>         support the “local” case…
>>       - FWIW, the “truststore” case is a "certificate-bag-ref”, that is, a bag 
>>         of certs...
>>       - at first I was thinking that we could adjust the “local” case to use
>>         the "trust-anchor-cert-cms” typedef but, actually, it already does.
>>         That is, it is a leaf-list of CMS structs…
>>       - another option would be to expand the “local” case to be a “list”
>>         instead of a “leaf-list”.  This would necessitate each cert to have
>>         a “key” (e.g., name), but that is how it is in the truststore's bag
>>         definition, so probably okay...
> 
> Giving certs a name and using a list may make things simpler. But I
> have no overview of all the usage implications (since I did not look
> at the other documents).

Done.  Converted the leaf-list to a list.



>>> - Is there any semantic difference between
>>>   trust-anchor-cert-grouping and end-entity-cert-grouping, i.e., why
>>>   do we need two groupings? Same question for
>>>   trust-anchor-certs-grouping and end-entity-certs-grouping. If
>>>   there is a semantic difference, perhaps this should be highlighted
>>>   in the grouping description.
>> 
>> There is a huge semantic difference (albeit, no syntactic difference), TAs and EEs are used in entirely different ways.
> 
> Yes, but if its the same structure, why do we need two definitions of
> the structure?

It is the same structure, but the description statements differ.



>> That said, the truststore could (really, SHOULD) be used to configure the system-level SSH server as well, as it portends to have security protections beyond regular config.  This is analogous the folks migrating to storing TAs in system-level mechanisms, such as Mac’s “keychain”.
>> 
>> What makes sense to me is to have a future 7317bis that migrates that definition to use the “ts:local-or-truststore-public-keys-grouping” grouping.
> 
> Yes, perhaps it is OK to stay silent about and to keep a record
> somewhere that this needs to be looked at when 7317 is being revised.

I modified the example (s/User/Application/) and removed the "relation to authorized-keys" comment to tamper it down more.


>>> - Do we need the truststore-supported feature? We do not have this
>>>   in other YANG modules. Is the idea to distinguish implementations
>>>   that only import groupings? Even in this case, should this not be
>>>   handled by the yang library somehow?
>> 
>> The "truststore-supported” feature is used to prune out the “truststore” case from the various “local-of truststore” groupings.  Yes, it is for implementations that do not implement the module (they only import the groupings).  I’m unsure how YANG Library could be used differently, that is, it's already currently used to indicate if the module is implemented/imported as well as what features are supported.
>> 
> 
> I assume (have not checked) that you don't toggle the implement bit in
> the yang library if you just import type definitions and groupings.

Correct.  But the implement bit *does* need to be toggled in order for “identity” statements to be defined, along with RPCs and protocol-accessible nodes.


>>> - Do the leaf names always look good in instance data? For example,
>>>   is <truststore-reference> may not be the most descriptive name in
>>>   instance data.
>> 
>> All of the downstream drafts (ssh-, tls-, netconf-, restconf-) have examples illustrating "truststore-reference” in use.  [and the ssh- and tls- drafts have examples illustrating "keystore-reference” in use].
> 
> I guess I will eventually have to look at them...
> 
>>> Perhaps it would help to also have examples that
>>>   show how various groupings are used, not just the bags.
>> 
>> This would entail defining an example module to instantiate those groupings (such as is done in the crypto-types and keystore drafts).  If the goal is to make this draft as readable as possible in a standalone fashion, then perhaps that should be done.  Or perhaps we could just point readers to the examples in the aforementioned drafts?
>> 
> 
> My preference is to point to the other drafts. These are likely
> non-normative references, i.e., they are not problematic process wise

I added examples to the trust-anchors draft.  More work, but better contained...


>>> - I have to think about the 'copy to running' procedure described in
>>>   section 3. Perhaps this is the way to go but who happens if things
>>>   get out of sync?
>> 
>> We previously had "require-instance false” statements (also in the keystore draft), but they were removed (see the Change Log) because it seemed skewed to throwout validation for 99% of the TAs/keys in orderbto support the 1% case for builtin TA/keys.  So we moved to the current approach, which isn’t ideal, but seems better then the alternative...
>> 
>> FWIW, it’s intended (though perhaps not stated) that the server will reject any attempt to configure a new TA certificate.  That is, only a subset of the certificate can appear in <running>.  Any attempt to configure a new cert, or change an exist cert, should cause the commit to fail.   In this way, configurations that refer to the builtin definitions can be assured that the values used are only those approved by the manufacturer. 
>> 
> 
> Spelling this out would make me happy. The server has to enforce
> consistency here.

Spelled out in spades.  Please re-review.


>>> - Why is it useful to define truststore-grouping, i.e., why is this
>>>   not just a part of the truststore container definition? How are
>>>   the reference types going to work if you instantiate the
>>>   truststore-grouping elsewhere?
>> 
>> I use this grouping in a project outside the IETF, where there are a multiplicity of context specific truststores.  I use the features to disable to default “local-or-truststore” cases and then augment-in my own case statement containing a reference to my context-specific truststore.
>> 
> 
> Well, to do this properly, one would have to invent a way to make the
> references to different truststores. You are essentially dropping the
> truststore and adding new ones, no? Still a hack, like cut'n'paste.

Maybe.  Still, it’s useful, and not a hardship to support.



>>> * draft-ietf-netconf-keystore-17.txt
>>> 
>>> - Looking at the overview figure (that is unfortunately to be
>>>   removed), I wonder whether trust-anchors should be renamed to
>>>   truststore as this seems to be the term used by the yang module.
>> 
>> The artwork is helpful, but it’s easy to see that it won’t age well - right?  For instance, already there are other WGs using some of this work and, let’s not forget the “syslog” draft that has been parked in NETMOD for ~2 years due to a MISREF to the TLS draft...
>> 
> 
> I can't follow, artwork is no different than text.

I moved the artwork into the Introduction section for all drafts.  No longer removed by RFC Editor.


>>> - Several pages of yang tree output without explanations is not very
>>>   helpful. I suggest to break this into pieces and to explain for
>>>   each grouping when it may be used (or not used).
>> 
>> This suggestion is in line with the response I gave Tom, regarding it being consistent with what I did in the “sztp-csr” draft.  I think the trick is to rename the section from “Tree Diagram” to “Data Model Overview” and go from there...
>> 
> 
> Short tree diagrams very helpful, multi pages tree diagramm are almost
> pointless (and can always be generated when needed). The art is to
> slice things into meaningful pieces.

I replaced the wall of tree-diagram with new “Data Model Overview” section in all drafts.
This was a ton of work, and I pray that the WGLCs don’t result in many changes that would entail needing to update both the YANG definition and also the draft-text.

BTW, in the course of doing this, it occurred to me that a “yang2rfc” utility could effectively automate the per-module snippet put into each draft.  We might need to add some YANG extensions to support “docstrings” but, otherwise, I think it could be done and would be a huge time-saver for future I-Ds…and also provide a consistency across I-Ds…maybe a good student project?  ;)


>>> - Should the support for encrypted keys be moved to the cryto-types
>>>   module? Is there a reason why we add the feature to have keys
>>>   encrypted here? Perhaps this is sensible, I am just trying to
>>>   check whether this has been given some thought.
>> 
>> In order to persist an encrypted key, it is necessary to reference the other key that was used to encrypt the key (so the server knows how to decrypt the key).  
>> 
>> The expectation is that the other key will be in the keystore, but note that a “choice” node is used for the references, and all the existing cases are protected by “feature” statements, so that, e.g., a “local” key or key in a context-specific instance of the keystore grouping can be used alternatively.
>> 
>> To your point, we could move the essence of the encrypted-key support directly into the key groupings in the crypto-types draft, but leave the “case” statement empty, and then have the keystore draft augment in “case” statements for keys in the keystore, when the keystore module is “implemented”.  Makes sense?
>> 
> 
> I have no strong opinion - it just felt that adding encrypted keys
> later adds additional noise, more groupings to understand and to deal
> with.

Done.  I did exactly what is in my last paragraph.  Much cleaner!


>>> - The text in 5.3 scares me a bit. Migrating root keys may be seen
>>>   as a bad idea by some. If you have a new device with new root
>>>   keys, then you better re-encrypt or re-key instead of patching the
>>>   root key.  Or are you essentially saying that encrypted keys
>>>   should be encrypted with a key-encryption-key instead of the 'root
>>>   key'?
>> 
>> More the latter.  The 2nd-to-last paragraph intends to say this. 
> 
> Perhaps this recommendation should be spelled out clearer and the
> §obscure copy root keys approach be given as a reason to have such
> key-encryption-keys.

Done.  I re-wrote these sections do be much clearer.  Please re-review!


>>> - I am not sure how one implements the 'MUST ensure that the
>>>   strength of the key being configured is not greater than the
>>>   strength of the underlying secure transport' nor am I sure that
>>>   the consequences are really desirable, i.e., I can never move to
>>>   'more secure keys' than what the transport currently allows. I
>>>   think I understand the argument that says a key can't be more
>>>   trustworthy than the transport used to configure it but I do not
>>>   think we should disallow upgrade paths.
>> 
>> Changed to a SHOULD in my local copy.
> 
> Works for me. 

I did it, but then deleted it (ooops).  I now have a “Strength of Configured Keys” Security Considerations section in my local copy of the crypto-types draft.


K.