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

[Kent Watsen <[e-mail]>]

Hi Rob,

Looking at the five reviews you’ve shared in the last few days, I want to start by sincerely thanking you for your extensive and thoughtful comments!  :)



> Hi Kent,
>  
> Sorry, after a long delay … I have finally got back to you with replies.  I think that there are only a couple of comment still open.
>  
> I’ve also checked the changes between -26 and -27 and they all look fine to me.

I’m well aware the size of these documents, despite efforts to keep them short  :sigh:

Responses to your comments inlined below.

Kent


> Regards,
> Rob
>  
>  
>  
>  
> From: Kent Watsen <[e-mail] <mailto:[e-mail]>> 
> Sent: 24 March 2023 21:12
> To: Rob Wilton (rwilton) <[e-mail] <mailto:[e-mail]>>
> Cc: [e-mail] <mailto:[e-mail]>; [e-mail] <mailto:[e-mail]>
> Subject: Re: [netconf] AD review of draft-ietf-netconf-crypto-types-26
>  
> 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] <mailto: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.
>  
> [Rob Wilton (rwilton)] 
> Okay, this is fine.
>  
> I wonder whether the granularity of the features, i.e., being on or off at a module level may ultimately cause longer term modelling issues when used in groupings.  I appreciate that refinements using if-feature statements can be used to mitigate this, but that needs to be considered when the module is being designed.  But this not an issue that needs to be solved here, perhaps one for future YANG Next discussion.

So far I’ve yet to hear a complaint from implementors regarding the number or placement of the if-feature statements.  For my own application, the features placed where they are has allowed me to quickly/nicely turn-off unsupported parts of the configuration.

Resolved with no 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?          
>  
> Same response as for above.
>  
> [Rob Wilton (rwilton)] 
> Ack.

Resolved with no 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.
>  
> 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]).
>  
> [Rob Wilton (rwilton)] 
> Yes, except s/Thusly, thei/Hence, the/.

Fixed in my local copy (I will publish the full draft-set together once these reviews are complete.)


> (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"?
>  
> [Rob Wilton (rwilton)] 
> Sorry, my comment was unclear due to my commenting tool.
>  
> Section 3.6. covered “Encrypting Passwords”.  What I was asking is whether the security considerations section of the document should highlight that the model allows implementations to enable support for cleartext-keys, and warning of the security considerations associated with that?


I did split the last paragraph of that section into a new section called "Use of Recommended Ciphersuites”.

To you main point, I added this new section:

   Cleartext Passwords and Keys

           The module contained within this document enables, only when
            specific "feature" statements are enabled, for the cleartext
            value of passwords and keys to be stored in the configuration
            database.  Storing cleartext values for passwords and keys is
            NOT RECOMMENDED.

Good?

 

> (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 <https://www.ietf.org/archive/id/draft-ietf-netconf-%0b%C2%A0%C2%A0%C2%A0%C2%A0%C2%A0%C2%A0%C2%A0%C2%A0%C2%A0%C2%A0%C2%A0%C2%A0%C2%A0crypto-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.
>  
> [Rob Wilton (rwilton)] 
> Ah, okay.  From that text it wasn’t clear that you were asking the RFC Editor to fix this.  Please can you update the paragraph above in this doc, and the others, to make the request more explicit.  I.e., replace with This RFC and delete the self-reference.


Added the following to the “Editorial Note” section in each draft:

	Please replace the self-reference in this section with "This RFC” (or similar)
	and remove the self-reference in the "Normative/Informative References”
	section, whichever it is in.

Good?



> 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.
> [Rob Wilton (rwilton)] 
> Okay.

Resolved by 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".
> [Rob Wilton (rwilton)]
> Okay.

Resolved.


> (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.
> [Rob Wilton (rwilton)] 
> Thanks.

Resolved.


 
> 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?}
>  
> [Rob Wilton (rwilton)] 
> Okay.

Resolved.


> (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.
> [Rob Wilton (rwilton)] 
> Thanks.

Resolved.


> (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.
> [Rob Wilton (rwilton)] 
> Thanks.

Resolved.


> (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.
> [Rob Wilton (rwilton)] 
> Makes sense.  Thanks for the clarification.

Resolved.


> (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.
> [Rob Wilton (rwilton)] 
> Okay.  We can defer this one to the Sec ADs review.

Resolved (for now)


> (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.
> [Rob Wilton (rwilton)] 
> Agreed.


Resolved (for now)


> (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".
> [Rob Wilton (rwilton)] 
> Thanks.

Resolved.


> (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.
> [Rob Wilton (rwilton)] 
> Thanks.

Resolved.



> (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)
> [Rob Wilton (rwilton)] 
> Yes, thanks.


Resolved.


> (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?
> [Rob Wilton (rwilton)] 
> No, I think that what you have is fine.

Thanks.  Resolved.



> (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.
> [Rob Wilton (rwilton)] 
> Okay.

Resolved.


> 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.
> [Rob Wilton (rwilton)] 
> Ack.

All three points resolved.


> Thanks,
> Rob


Thank You!

Kent