[netconf] AD review of draft-ietf-netconf-crypto-types-26

["Rob Wilton (rwilton)" <[e-mail]>]

Hi Kent,

Here are my review comments for draft-ietf-netconf-crypto-types.  This document is well written, so I would like to thank you, the shepherd, and WG for a high-quality document.

I have a few comments, but will note that I'm not a security expert and hence some of my questions/comments may be due to my security ignorance ...


Moderate level comments:                                                                                                                                                                                                                                                                                                                                                                     
                                                                                                                                                                                                                                                                                                                                                                                             
(1) p 14, sec 2.1.4.10.  The "asymmetric-key-pair-with-cert-grouping" Grouping                                                                                                                                                                                                                                                                                                               
                                                                                                                                                                                                                                                                                                                                                                                             
   Comments:                                                                                                                                                                                                                                                                                                                                                                                 
   *  This grouping defines an asymmetric key with at most one                                                                                                                                                                                                                                                                                                                               
      associated certificate, a commonly needed combination in protocol                                                                                                                                                                                                                                                                                                                      
      models.                                                                                                                                                                                                                                                                                                                                                                                
                                                                                                                                                                                                                                                                                                                                                                                             
Will it also always be the case that is has to support a CSR Action?                                                                                                                                                                                                                                                                                                                         
                                                                                                                                                                                                                                                                                                                                                                                             
                                                                                                                                                                                                                                                                                                                                                                                             
(2) p 14, sec 2.1.4.11.  The "asymmetric-key-pair-with-certs-grouping" Grouping                                                                                                                                                                                                                                                                                                              
                                                                                                                                                                                                                                                                                                                                                                                             
   *  This grouping defines an asymmetric key with one or more                                                                                                                                                                                                                                                                                                                               
      associated certificates, a commonly needed combination in                                                                                                                                                                                                                                                                                                                              
      configuration models.                                                                                                                                                                                                                                                                                                                                                                  
                                                                                                                                                                                                                                                                                                                                                                                             
Will it also always be the case that is has to support a CSR Action?                                                                                                                                                                                                                                                                                                                         
                                                                                                                                                                                                                                                                                                                                                                                             
                                                                                                                                                                                                                                                                                                                                                                                             
(3) p 45, sec 3.2.  No Support for Key Generation                                                                                                                                                                                                                                                                                                                                            
                                                                                                                                                                                                                                                                                                                                                                                             
   Early revisions of this document included "rpc" statements for                                                                                                                                                                                                                                                                                                                            
   generating symmetric and asymmetric keys.  These statements were                                                                                                                                                                                                                                                                                                                          
   removed due to an inability to obtain consensus for how to identify                                                                                                                                                                                                                                                                                                                       
   the key-algorithm to use.  Thusly, the solution presented in this
   document only supports keys to be configured via an external client,
   which does not support Security best practice.

There is an obvious risk that the SEC ADs may not accept this, and hence bump this back to the WG to solve, but I still think it is right to try and progress this document now.


(4) p 46, sec 3.5.  Strength of Keys Conveyed

   Implementations SHOULD only use secure transport protocols meeting
   local policy.  A reasonable policy may, e.g., state that only
   ciphersuites listed as "recommended" by the IETF be used (e.g.,
   [RFC7525] for TLS).

Should the security considerations mention that passwords can be stored in cleartext and the risks associated with that (i.e., specifically beyond what is already described in section 3.8)?


(5) p 51, sec 5.2.  Informative References


   [I-D.ietf-netconf-crypto-types]
              Watsen, K., "YANG Data Types and Groupings for
              Cryptography", Work in Progress, Internet-Draft, draft-
              ietf-netconf-crypto-types-25, 19 October 2022,
              <https://www.ietf.org/archive/id/draft-ietf-netconf-
              crypto-types-25.txt>.

An informative reference to itself is strange and should be removed.



Minor level comments:

(6) p 4, sec 1.1.  Relation to other RFCs

                                  crypto-types
                                    ^      ^
                                   /        \
                                  /          \
                         truststore         keystore
                          ^     ^             ^  ^
                          |     +---------+   |  |
                          |               |   |  |
                          |      +------------+  |
   tcp-client-server      |     /         |      |
      ^    ^        ssh-client-server     |      |
      |    |           ^            tls-client-server
      |    |           |              ^     ^        http-client-server
      |    |           |              |     |                 ^
      |    |           |        +-----+     +---------+       |
      |    |           |        |                     |       |
      |    +-----------|--------|--------------+      |       |
      |                |        |              |      |       |
      +-----------+    |        |              |      |       |
                  |    |        |              |      |       |
                  |    |        |              |      |       |
               netconf-client-server       restconf-client-server
   +======================+===========================================+
   |Label in Diagram      | Originating RFC                           |
   +======================+===========================================+
   |crypto-types          | [I-D.ietf-netconf-crypto-types]           |
   +----------------------+-------------------------------------------+
   |truststore            | [I-D.ietf-netconf-trust-anchors]          |
   +----------------------+-------------------------------------------+
   |keystore              | [I-D.ietf-netconf-keystore]               |
   +----------------------+-------------------------------------------+
   |tcp-client-server     | [I-D.ietf-netconf-tcp-client-server]      |
   +----------------------+-------------------------------------------+
   |ssh-client-server     | [I-D.ietf-netconf-ssh-client-server]      |
   +----------------------+-------------------------------------------+
   |tls-client-server     | [I-D.ietf-netconf-tls-client-server]      |
   +----------------------+-------------------------------------------+
   |http-client-server    | [I-D.ietf-netconf-http-client-server]     |
   +----------------------+-------------------------------------------+
   |netconf-client-server | [I-D.ietf-netconf-netconf-client-server]  |
   +----------------------+-------------------------------------------+
   |restconf-client-server| [I-D.ietf-netconf-restconf-client-server] |
   +----------------------+-------------------------------------------+

Rather than having a formal reference to E-D.ietf-netconf-crypto-types, it would probably be better to just label at as "This draft", with a note to change it to "This RFC".


(7) p 6, sec 2.1.2.  Identities

   *  The various "leaf" identities define specific encoding formats.
      The derived identities defined in this document are sufficient for
      the effort described in Section 1.1 but, by nature of them being
      identities, additional derived identities MAY be defined by future
      efforts.

Using the term "leaf" applied to identities could be somewhat confusing, please consider using "terminal" or some other definition instead instead.


(8) p 9, sec 2.1.4.2.  The "password-grouping" Grouping

      -  The "cleartext-password" node can encode any cleartext value.
      -  The "encrypted-password" node's structure is discussed in
         Section 2.1.4.1.

I noted that there is no feature statement for cleartext passwords or cleartext private keys?  E.g., is it plausible that for security purposes a device may want to disallow the use of cleartext passwords or cleartext private keys, without needing to resort to deviations?


(9) p 15, sec 2.2.1.  The "symmetric-key-grouping" and "asymmetric-key-pair-with-
        certs-grouping" Grouping

   Notably, this example illustrates a hidden asymmetric key (ex-hidden-
   asymmetric-key) has been used to encrypt a symmetric key (ex-
   encrypted-one-symmetric-based-symmetric-key) that has been used to
   encrypt another asymmetric key (ex-encrypted-rsa-based-asymmetric-
   key).  Additionally, the symmetric key is also used to encrypt a
   password (ex-encrypted-password).

When I first read this, I thought that this text was just describing the module and hence the prose regarding the hidden key was slightly confusing.  Hence, perhaps consider tweaking this to: 

   Notably, this example module and associated configuration data illustrates ...


(10) p 15, sec 2.2.1.  The "symmetric-key-grouping" and "asymmetric-key-pair-with-
        certs-grouping" Grouping

     description
       "This module illustrates the 'symmetric-key-grouping'
        and 'asymmetric-key-grouping' groupings defined in
        the 'ietf-crypto-types' module defined in RFC AAAA.";

Suggest "This module" -> "This example module".  Just in case anyone ever manually extracts (copies) this module.


(11) p 25, sec 2.3.  YANG Module

     feature p10-based-csrs {
       description
         "Indicates that the erver implements support
          for generating P10-based CSRs, as defined
          in RFC 2986.";
       reference
         "RFC 2986: PKCS #10: Certification Request Syntax
                    Specification Version 1.7";
     }

I'll leave it to the authors discretion but note that the p10-based-csrs feature could be made conditional on the csr-generation feature.


(12) p 26, sec 2.3.  YANG Module

     feature password-encryption {
       description
         "Indicates that the server supports password
          encryption.";
     }

I'm wondering whether this should really be a feature, or whether supporting encrypted passwords should be a base requirement of the module.


(13) p 26, sec 2.3.  YANG Module

     feature private-key-encryption {
       description
         "Indicates that the server supports encryption
          of private keys.";
     }

I'm wondering whether this should really be a feature, or whether supporting encrypted private-keys should be a base requirement of the module.


(14) p 37, sec 2.3.  YANG Module

          This CMS encodes the degenerate form of the SignedData
          structure that is commonly used to disseminate X.509
          certificates and revocation objects (RFC 5280).";
       reference
         "RFC 5280:
            Internet X.509 Public Key Infrastructure Certificate
            and Certificate Revocation List (CRL) Profile.";
     }
     /*****************/
     /*   Groupings   */
     /*****************/

Up to the authors, but for these groupings you could potentially include reference statements back to the specific section that describe there.


(15) p 37, sec 2.3.  YANG Module

            The leaf nodes MUST be direct descendants in the data tree,
            and MAY be direct descendants in the schema tree.";

I presume that the intention here is that intermediate choice/case statements are allowed, but not containers.  I wonder, whether that might also be worth stating (e.g., choice/case statements are allowed, but ...)


(16) p 38, sec 2.3.  YANG Module

            For encrypted keys, the value is the same as it would
            have been if the key were not encrypted.";

I don't understand what this sentence is intending to convey.


(17) p 40, sec 2.3.  YANG Module

     grouping asymmetric-key-pair-grouping {
       description
         "A private key and its associated public key.  Implementations
          SHOULD ensure that the two keys are a matching pair.";

By "SHOULD ensure" does this mean "SHOULD validate", or something else?


(18) p 40, sec 2.3.  YANG Module

            For encrypted keys, the value is the same as it would have
            been if the key were not encrypted.";

Another instance of that sentence that I don't follow.


(19) p 45, sec 3.3.  Unconstrained Public Key Usage

   The "asymmetric-key-pair-grouping" grouping uses the aforementioned
   "public-key-grouping" grouping, and carries the same traits.

For the two cases above, is there any specific advice that could be given.  E.g., they should only be used when ... or use XXX in preference?  Same comment applies to private keys below.



Nit level comments:

(20) p 10, sec 2.1.4.3.  The "symmetric-key-grouping" Grouping

   *  The "key-format" node is an identity-reference to the "symmetric-
      key-format" abstract base identity discussed in Section 2.1.2,
      enabling the symmetric key to be encoded using the format defined
      by any of the derived identities.

Possible better phrasing: ... using any of the formats defined by the derived identities.


(21) p 13, sec 2.1.4.10.  The "asymmetric-key-pair-with-cert-grouping" Grouping

   This section presents a tree diagram [RFC8340] illustrating the
   "asymmetric-key-pair-with-cert-grouping" grouping.  The this diagram
   does not expand the internally used grouping statement(s):

The this diagram -> This tree diagram


(22) p 36, sec 2.3.  YANG Module

          The CMS MUST contain only a single chain of certificates.
          The client or end-entity certificate MUST only authenticate
          to last intermediate CA certificate listed in the chain.

Should this be "to the last intermediate"?


(23) p 36, sec 2.3.  YANG Module

     typedef end-entity-cert-cms {
       type signed-data-cms;
       description
         "A CMS SignedData structure that MUST contain the end
          entity certificate itself, and MAY contain any number
          of intermediate certificates leading up to a trust
          anchor certificate.  The trust anchor certificate
          MAY be included as well.

The previous description references root certificates, this description references the trust anchor certificate.  Are these the same thing, and if so would it help to align the descriptions?

Regards,
Rob