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

[Kent Watsen <[e-mail]>]

Hi Rob,

Thank you for your review.
Please see below for comments.

Kent // author


> On Mar 22, 2023, at 5:39 AM, Rob Wilton (rwilton) <rwilton=[e-mail]> wrote:
> 
> 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 ...

Few are experts.   Even the experts don't claim to be experts  ;)


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

Nope, the action does not always have to be supported:
  1) the "generate-csr" action has a feature-statement on it
  2) the "description" statement clarifies that it is only available when the associated 'public-key-format' node's value is 'subject-public-key-info-format'.  
  3) Consuming modules can place additional if-feature statements if more granularity is desired, i.e., the action is enabled for some (not all) keys.


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

Same response as for above.



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

Actually, this paragraph is no longer completely true, as later on we put the key-generation RPCs into the SSH- and TLS- client-server drafts (see "rpc generate-public-key" in ietf-[ssh/tls]-common.yang.  How about this?

NEW:

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
   generically identify the key-algorithm to use.  Thusly, thei solution
   presented in this document only supports keys to be configured via an
   external client.  Separate protocol-specific modules can present
   protocol-specific key-generation RPCs (e.g., "generate-public-key" in
   [I-D.ietf-netconf-ssh-client-server] and
   [I-D.ietf-netconf-tls-client-server]).



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

I think that this is supposed to be that section, albeit the title doesn't seem to line-up with the text.  IIRC, this paragraph got munged in a Sec Dir review.   The general statement is, e.g., don't transfer a 256-key in a 128-protected tunnel.  I can rewrite that paragraph here, but then what title to use for this text?  - "Use of Recommended Ciphersuites"?



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

This has come up before.  The issue stems from a common source file being included into each draft's Introduction section.  I could fork-off custom-text for each draft, but instead chose to put the following into each drat's "Editorial Note" section:

   The "Relation to other RFCs" section Section 1.1 contains a self-
   reference to this draft, along with a corresponding Informative
   Reference in the Appendix.

Effectively asking the copy editor to make the fix.


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

Please see previous comment.



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

Now "terminal".



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

Yes it is plausible.   I added an "feature" statement for the three "cleartext" cases.

Heads-up: I discovered that the list of features in the documents was out-of-date.  Also, there were mismatches in the naming convention.  Both issues corrected, with the result as follows:

Features:
  +-- one-symmetric-key-format
  +-- one-asymmetric-key-format
  +-- symmetrically-encrypted-value-format
  +-- asymmetrically-encrypted-value-format
  +-- cms-enveloped-data-format
  +-- cms-encrypted-data-format
  +-- csr-generation
  +-- p10-based-csrs
  +-- certificate-expiration-notification
  +-- cleartext-passwords
  +-- encrypted-passwords
  +-- cleartext-symmetric-keys
  +-- hidden-symmetric-keys
  +-- encrypted-symmetric-keys
  +-- cleartext-private-keys
  +-- hidden-private-keys
  +-- encrypted-private-keys

Also, since I discovered missing features in Section 2.1.1, I thought to check if any identities were missing in the next section (Section 2.1.2) and, sure enough, I found two that were missing, added to the end of the list:

     +-- csr-format
           +-- p10-csr-format {p10-csr-format?}



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

Updated - thanks!

Also fixed the Section-title, as 3 (not 2) groupings are illustrated by the example.


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

Good point - done.



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

Not exactly, let me explain...

First, recall I mentioned above that mismatches in feature-naming convention were found?   "p10-based-csrs" was one of them.  The fix was to rename it (s/p10-based-csrs/p10-csr-format/) to make sense where used:

  identity p10-csr-format {
    if-feature "p10-csr-format";
    ...
  }

And elsewhere:

    action generate-csr {
      if-feature "csr-generation";
      ...
    }


So you see, one feature controls if an identity-based format exists, and the other feature controls if the action exists.  Different formats and different actions MAY be defined in the future, and thus orthogonal.



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

That would be a hardline stance to take.  AFAIK, no-one has implemented encrypting passwords, symmetric-keys, or asymmetric keys - the effort being significant (needing to also support a "hidden" key)...

For clarity before proceeding, here's the full grouping in my local repo:

  grouping password-grouping {
    description
      "A password that may be encrypted.";
    choice password-type {
      nacm:default-deny-write;
      mandatory true;
      description
        "Choice between password types.";
      case cleartext-password {
        if-feature "cleartext-passwords";
        leaf cleartext-password {
          nacm:default-deny-all;
          type string;
          description
            "The cleartext value of the password.";
        } 
      }
      case encrypted-password {
        if-feature "encrypted-passwords";
        container encrypted-password {
          description
            "A container for the encrypted password value.";
          uses encrypted-value-grouping;
        }
      }
    } 
  }

PS: per earlier comment about mismatched naming conventions, this feature was renamed.  s/password-encryption/encrypted-passwords/.


The "choice" is "mandatory true" and (per earlier response).  Both `case` statements defined here (other cases may be augmented in) must be explicitly switched-on.   So it becomes a matter of local-policy.   I imagine most server implementations starting with what's easy (cleartext-passwords), but then switch to encrypted-passwords when a requirement to do so comes in.




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

See previous response.

Same goes for what was the "symmetric-key-encryption" feature too.




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


Added a reference to RFC 5652, Section 5.2 for both typedefs: "trust-anchor-cert-cms" and "end-entity-cert-cms".



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

Yes, exactly that.  Text updated.



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

NEW:

         For encrypted keys, the value is the decrypted key's
         format (i.e., the 'encrypted-value-format' conveys the
         encrypted key's format.";

Better?   (Fixed in both locations where text occurs)




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

Essentially, but I was trying to not use the word "validate" as it needs to be implemented outside of YANG-level validation.  Do you think text should change?



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

Yep, fixed it before (see earlier response)



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

Ugh.  Not really.  Folks just select what is appropriate for their use-case.


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

Fixed in three places where the text occurs - thanks!



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

Fixed!



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

Fixed!



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

These are similar but different concepts.  

Only in the trivial case is the root and trust-anchor certs the same.



> Regards,
> Rob

Kent // author