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

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

Hi Kent,

I’ve also responded inline to close the issues, but in summary I’m good with your proposed changes for the two remaining issues.

Please note, given that your relationship diagram is common between all the drafts in the set, then I did flag a comment on this in the HTTP draft – querying why it doesn’t show the dependency on TLS and TCP.   I think that once this is discuss/resolved then I think my AD review on this draft is complete, and it will be ready to send to IETF LC.


From: Kent Watsen <[e-mail]>
Sent: 27 June 2023 21:20
To: Rob Wilton (rwilton) <[e-mail]>
Cc: [e-mail]; [e-mail]
Subject: Re: [netconf] AD review of draft-ietf-netconf-crypto-types-26

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!  :)
[Rob Wilton (rwilton)]
No problem.  I’m currently working through the TLS draft, which should hopefully come this week (possibly even today), with NETCONF and RESTCONF still on my pile.  I’ve started looking at those as well, but I don’t know if I will get them all done this week.

I’m hoping that means that I can send crypto-types, keystore and trust store together as the first batch, then tcp, ssh, tls and http as a second batch, and then finally netconf and restconf together as a final batch.



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?

[Rob Wilton (rwilton)]
Yes, looks good thanks.



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

Thanks,
Rob




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