Re: [netconf] WG LC for three drafts

[Kent Watsen <[e-mail]>]

HI Juergen,

Thank you for your review!

Responses below.

Kent


> On Jun 17, 2020, at 6:53 AM, Juergen Schoenwaelder <[e-mail]> wrote:
> 
> I have reviewed the I-Ds today and here are my comments:
> 
> * general comments
> 
>  - These drafts needs serious reviews by security domain experts.
>    This all looks reasonable to me but since this is all about
>    security, we need to set the bar high to get things right.

Undoubtedly, though things are dramatically simpler now without trying to define the “algorithm” nodes.   


>  - I think the work is in a good shape but the documents need another
>    round of tweaks to get them ready.

Good to hear.  

FWIW, I’m aware of several implementations, of the higher-level drafts, which bodes well to these models being close.  I can’t vouch that every node has been implemented, but huge swaths have.


> * draft-ietf-netconf-crypto-types-15.txt
> 
>  - I agree with others that this document lacks introductory
>    material.

It’s a fair point.

Being a WG document, text-contributions would be greatly appreciated.  Perhaps the WG could divvy up sections?


>  - Perhaps the title should reflect that this document is not
>    just defining types but also groupings.

I’ve ping-ponged on this a few times myself.  

The idea was worse before, when the “algorithm” nodes were being defined, as then there were identities in the mix as well, which, if all were reflected in the title, was a mouthful.  At that time I silently resolved that “types” could be thought of in the general, as opposed to the specifically “typedefs”.  But now that the “algorithm” nodes are out, it seems more reasonable, how about:

OLD: Common YANG Data Types for Cryptography
NEW: Common YANG Data Types and Groupings for Cryptography


>  - The public-key-grouping says that it captures a public key and its
>    associated algorithm but I am not sure how this is done or is the
>    idea that every algorithm requires a different public-key-format?
> 
>  - The asymmetric-key-pair-grouping says that it captures a private
>    key and its associated public key and algorithm but I am not sure
>    how this is done or is the idea that every algorithm requires a
>    different private-key-format? I also wonder how this grouping
>    captures the associated public key.

Both of the above comments were mentioned by Eric and fixed here https://github.com/netconf-wg/crypto-types/commit/690c016b201241e13a1e324b49ba5e9db0d6c417 <https://github.com/netconf-wg/crypto-types/commit/690c016b201241e13a1e324b49ba5e9db0d6c417>.



>  - Do we have to be prepared for alternate cert formats in the
>    future?

In theory, yes; in practice, unlikely.  Of course, we should attempt to do it right, so...

Searching for the strings “certificate” and “X.509”, I think the issue is just description text asserting things must be X.509 when not true.  For instance, CMS allows for non-X.509 certs and so it’s unnecessary for the text to say otherwise.  Is this your sentiment as well?

[update: I just saw Rich Salz’s comment on this.  It’s true the world is skewed to X.509-based certs.  Do we still attempt to makes the minimum description text adjustments?]



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


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

The “*-certs-grouping” groupings were discussed above, and maybe will be removed but, if they stay, the same comment applies (huge semantic difference).


>  - I have not checked the examples.

All examples are validated each time the draft is built.



> * draft-ietf-netconf-trust-anchors-10.txt
> 
>  - I agree with others that this document lacks introductory
>    material. I am personally not a fan of one sentence sections
>    but that may be just a personal style / preference issue.

Same response as above.


>  - The idea that there are groupings that support locally defined
>    crypto keys or references to a centralized trust store really
>    deserves to be explained upfront. Yes, I can reverse engineer this
>    from the definitions, but people who need to decide whether they
>    want to implement this module may not have the time and energy to
>    do this. Notice that the abstract only talks about bags that can
>    be referenced.

This goes with the previous comment, but it helps a lot to have the specific omission called out.


>  - How does the storage of user public ssh keys suggested in the
>    example usage section relate to the data model in RFC 7317?

Firstly, note that the ct:ssh-public-key-format identity defines the same structure as RFC 7317, but I think your question may be more along the lines of if there is a redundancy, to which, I don’t think so, at least not necessarily…

RFC 7317 regards system-level users, e.g., is SSH-ing directly to a server, whereas here, as well in the ssh-client-server draft, the SSH-server may be application-level.

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.


>  - The authors listed on the document and in the yang module are
>    different.

Fixed in my local copy.


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


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


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


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



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


>  - I have not checked the examples.

All examples are validated each time the draft is built.



> * 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 appreciate the introductory text that this I-D has (compared to
>    the others).

Good to know, thanks.


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



>  - I am not sure I understand the examples, more precisely, how
>       <keystore-reference>rsa-asymmetric-key</keystore-reference>
>    or
>       <keystore-reference>
>         <asymmetric-key>rsa-asymmetric-key</asymmetric-key>
>         <certificate>ex-rsa-cert</certificate>
>       </keystore-reference>
>    will be interpreted.

I don’t follow.  I think that you’re in Section 3.2.2...the paragraph preceding the example says:

   The following example illustrates what two configured keys, one local
   and the other remote, might look like.  This example consistent with
   other examples above (i.e., the referenced key is in an example above).

Probably that text should say “…in the example found in Section 3.2.1”, but maybe your comment about something else?
  


>  - What is the informative reference to RFC8342 in the YANG module?

That comment is a holdover from -06, when there was a “require-instance false” in the YANG module.  I just removed that RFC8342 reference in my local copy.


>  - Do we need the keystore-supported feature? See discussion above.

Same response as above for the "truststore-supported” feature.


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


>  - Why is it useful to define keystore-grouping, i.e., why is this
>    not just a part of the keystore container definition? How are the
>    reference types going to work if you instantiate the
>    keystore-grouping elsewhere?

Same response as above for the "truststore-grouping”.


>  - I have to think about the 'copy to running' procedure described in
>    section 4. Perhaps this is the way to go but who happens if things
>    get out of sync? What is a system provided cert is exceeding its
>    lifetime (does this even lead to a stream of notifications)?

In addition to the comment above regarding Section 3 in the truststore draft…

Things getting out of sync here is less likely (then they are in truststore), in that builtin keys are undoubtedly going to be “hidden” and therefore any attempt to config a builtin key under a different name will fail, as a hidden key (where the secret key data is missing) cannot be configured.

We do want to support users adding user-defined certificates (e.g., LDevID), as is illustrated in Section 4.


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


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


>  - I have not checked the examples.

All examples are validated each time the draft is built.


Kent // contributor


> 
> /js
> 
> On Tue, Jun 02, 2020 at 04:48:07PM -0700, Mahesh Jethanandani wrote:
>> NETCONF WG,
>> 
>> The authors of
>> 
>> - draft-ietf-netconf-crypto-types
>> - draft-ietf-netconf-keystore
>> - draft-ietf-netconf-trust-anchors
>> 
>> have indicated that these drafts are ready for Last Call (LC).
>> 
>> This kicks of a 2 week WG LC for the three drafts. Please review and send any comments to the WG mailing list or by responding to this e-mail. Comments can be statements such as, I read/reviewed the document and believe it is ready for publication, or I have concerns about the document. For the latter, please indicate what your concerns are. 
>> 
>> Any reports on implementation status or plans to implement are also very useful.
>> 
>> Thanks.
>> 
>> Mahesh Jethanandani (as co-chair)
>> [e-mail]
>> 
>> 
>> 
>> _______________________________________________
>> netconf mailing list
>> [e-mail]
>> https://www.ietf.org/mailman/listinfo/netconf
> 
> -- 
> Juergen Schoenwaelder           Jacobs University Bremen gGmbH
> Phone: [phone]         Campus Ring 1 | 28759 Bremen | Germany
> Fax:   [phone]         <https://www.jacobs-university.de/>
> 
> _______________________________________________
> netconf mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/netconf