[netconf] AD review of draft-ietf-netconf-tls-client-server-33

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

Hi Kent,

Here is my AD review of draft-ietf-netconf-tls-client-server-33

This is another well written document, thank you.  The comments from my review are:

Moderate level comments:                                                                                                                                                                                                                                                                                                                                                                     
                                                                                                                                                                                                                                                                                                                                                                                             
(1) p 3, sec 1.  Introduction                                                                                                                                                                                                                                                                                                                                                                
                                                                                                                                                                                                                                                                                                                                                                                             
   Any version of TLS may be configured.  TLS 1.0 [RFC2246] and TLS 1.1                                                                                                                                                                                                                                                                                                                      
   [RFC4346] are historic and hence the YANG "feature" statements                                                                                                                                                                                                                                                                                                                            
   enabling them are marked "status obsolete".  TLS 1.2 [RFC5246] is                                                                                                                                                                                                                                                                                                                         
   obsoleted by TLS 1.3 [RFC8446] but still in common use, and hence its                                                                                                                                                                                                                                                                                                                     
   "feature" statement is marked "status deprecated".  All the feature                                                                                                                                                                                                                                                                                                                       
   statements for 1.0, 1.1, and 1.3 have "description" statements                                                                                                                                                                                                                                                                                                                            
   stating that it is NOT RECOMMENDED to enable obsolete protocol                                                                                                                                                                                                                                                                                                                            
   versions.                                                                                                                                                                                                                                                                                                                                                                                 
                                                                                                                                                                                                                                                                                                                                                                                             
s/and 1.3/and 1.2/?                                                                                                                                                                                                                                                                                                                                                                          
                                                                                                                                                                                                                                                                                                                                                                                             
In the context of this paragraph, it is worth noting that draft-ietf-netmod-yang-module-versioning refines the meaning of deprecated and obsolete (depending on whether that document progresses).  I.e., from that document the expectation is that implementations would implement deprecated nodes or explicitly deviate not-support them, and must not implement obsolete nodes.  One of the reasons for this approach is to ensure that the client can understand always build the exact schema used by the server without ambiguity over deprecated/obsolete nodes.                                                                                                                                                                                                                                                                                                                                                        
                                                                                                                                                                                                                                                                                                                                                                                             
Hence, I think that there is perhaps a general question as to whether we should be supporting TLS 1.0 or TLS 1.1 at all in this model?  I did briefly ask the SEC ADs yesterday.  Only one responded indicating that they would prefer if it wasn't present (but also said that he probably wouldn't DISCUSS on this if they were included).  Do we know if there are likely use cases that still need TLS 1.0 or TLS 1.1 to justify keeping them in?                                                                                                                                                                                                                                                                                                                                     
                                                                                                                                                                                                                                                                                                                                                                                             
                                                                                                                                                                                                                                                                                                                                                                                             
(2) p 17, sec 2.3.  YANG Module                                                                                                                                                                                                                                                                                                                                                              
                                                                                                                                                                                                                                                                                                                                                                                             
         leaf bits {                                                                                                                                                                                                                                                                                                                                                                         
           type uint16;                                                                                                                                                                                                                                                                                                                                                                      
           description                                                                                                                                                                                                                                                                                                                                                                       
             "Specifies the number of bits in the key to create.                                                                                                                                                                                                                                                                                                                             
              For RSA keys, the minimum size is 1024 bits and                                                                                                                                                                                                                                                                                                                                
              the default is 3072 bits. Generally, 3072 bits is                                                                                                                                                                                                                                                                                                                              
              considered sufficient. DSA keys must be exactly 1024                                                                                                                                                                                                                                                                                                                           
              bits as specified by FIPS 186-2.  For elliptical                                                                                                                                                                                                                                                                                                                               
              keys, the 'bits' value determines the key length                                                                                                                                                                                                                                                                                                                               
              of the curve (e.g., 256, 384 or 521), where valid                                                                                                                                                                                                                                                                                                                              
              values supported by the server are conveyed via an                                                                                                                                                                                                                                                                                                                             
              unspecified mechanism.  For some public algorithms,                                                                                                                                                                                                                                                                                                                            
              the keys have a fixed length and the 'bits' value,                                                                                                                                                                                                                                                                                                                             
              if specified, will be ignored.";                                                                                                                                                                                                                                                                                                                                               
         }                                                                                                                                                                                                                                                                                                                                                                                   
                                                                                                                                                                                                                                                                                                                                                                                             
Wouldn't it be safer to fail the RPC request rather than ignore an incorrect bits value?                                                                                                                                                                                                                                                                                                     
                                                                                                                                                                                                                                                                                                                                                                                             
                                                                                                                                                                                                                                                                                                                                                                                             
(3) p 17, sec 2.3.  YANG Module                                                                                                                                                                                                                                                                                                                                                              
                                                                                                                                                                                                                                                                                                                                                                                             
         choice private-key-encoding {                                                                                                                                                                                                                                                                                                                                                       
           default cleartext;                                                                                                                                                                                                                                                                                                                                                                
           description                                                                                                                                                                                                                                                                                                                                                                       
             "A choice amongst optional private key handling.";                                                                                                                                                                                                                                                                                                                              
           case cleartext {                                                                                                                                                                                                                                                                                                                                                                  
             if-feature "ct:cleartext-private-keys";
             leaf cleartext {
               type empty;
               description
                 "Indicates that the private key is to be returned
                  as a cleartext value.";
             }
           }

I was wondering whether it makes sense to have a default value referencing a case that is conditional on if-feature?  From a quick look at RFC 7950, the behaviour seems to be unspecified in the case that the feature is not supported.

I also note that the 'private-key-encoding' doesn't turn up in the instance data (because it is a choice), so reading the XML doesn't make it obvious that it is the private key that is encoded.  Hence, might it be helpful to include a private-key-encoding container?


(4) p 33, sec 3.3.  YANG Module

               leaf target-protocol {
                 type uint16;
                 description
                   "As per Section 3.1 of I-D.
                    ietf-tls-external-psk-guidance:
                    The protocol for which a PSK is imported for use.";
                 reference
                   "I-D.ietf-tls-external-psk-importer:
                              Importing External PSKs for TLS";
               }

It wasn't clear to me exactly what the target-protocol value looks like here, i.e., where to find the list of allowed values.


(5) p 67, sec Appendix A.  YANG Modules for IANA

   Following are the complete contents to the initial IANA-maintained
   YANG module.  Please note that the date "2022-06-16" reflects the day
   on which the extraction occurred.

As per my comments on the SSH draft, I wonder whether including this copy in this document is actually helpful, or just means that it is more likely that someone could end up using an outdated version rather than getting the latest version from IANA.  I.e., perhaps include is for now, but ask the RFC editor to remove it at publication (at which point IANA should generate a current version).

It also means that this document ends up with a lot of normative references, which although not necessarily a problem, I wonder how useful/meaningful they really are.

In the generated module, some of the initial cipher entries are obsolete.  Would it be better for those to always be excluded from the initial list of identities?  Many cipher entries are also deprecated.  Will the YANG status deprecated be sufficient to warn users that these are deprecated?  E.g., the description could also flag that they are deprecated and SHOULD NOT be used (if that is appropriate)?

Note, I have not checked the generated list of identities in detail (but can review the generating script if that is helpful).



Minor level comments:

(6) p 4, sec 1.  Introduction

Please also mention the YANG modules defined from IANA registries that this document creates.


(7) p 16, sec 2.3.  YANG Module

     grouping hello-params-grouping {
       description
         "A reusable grouping for TLS hello message parameters.";
       reference
         "RFC 5246: The Transport Layer Security (TLS) Protocol
                    Version 1.2
          RFC 8446: The Transport Layer Security (TLS) Protocol
                    Version 1.3";
       container tls-versions {
         description
           "Parameters regarding TLS versions.";
         leaf-list tls-version {
           type identityref {
             base tls-version-base;
           }
           description
             "Acceptable TLS protocol versions.

Presumably this does not need to be ordered-by user because the later versions are always preferable over older versions?


(8) p 19, sec 3.1.2.1.  The "tls-client-grouping" Grouping

   The following tree diagram [RFC8340] illustrates the "tls-client-
   grouping" grouping:

I note that for the common module grouping you provide both unexpanded and expanded tree diagrams for the groupings, but for the client and server modules you only provide the unexpanded grouping.  Is this just because of the length of the expanded grouping tree-diagrams?  If they are too long, then would it be helpful to perhaps included them in an appendix?


(9) p 20, sec 3.1.2.1.  The "tls-client-grouping" Grouping

       |  +-- ca-certs! {server-auth-x509-cert}?
       |  |  +---u ts:inline-or-truststore-certs-grouping
       |  +-- ee-certs! {server-auth-x509-cert}?
       |  |  +---u ts:inline-or-truststore-certs-grouping

Not a crypto expert, so this may be a daft question, but I noted that both ca-certs and ee-certs use the same if-feature statement (also in the server model).  Is it always expected to be the case that if an implementation supports ca-certs then it would also support ee-certs?


(10) p 21, sec 3.1.2.1.  The "tls-client-grouping" Grouping

   *  The "server-authentication" node configures trust anchors for
      authenticating the TLS server, with each option enabled by a
      "feature" statement.

As per my previous comment, I just wanted to highlight that two of the options are enabled by the same feature statement, which might be okay.


(11) p 21, sec 3.1.3.  Protocol-accessible Nodes

   The "ietf-tls-client" module defines only "grouping" statements that
   are used by other modules to instantiate protocol-accessible nodes.

Flagging as per previous comments on the other drafts, is this section needed/helpful, or should it be elided?


(12) p 32, sec 3.3.  YANG Module

               leaf hash {
                 type tlscmn:epsk-supported-hash;
                 mandatory true;
                 description
                   "As per Section 4.2.11 of RFC 8446, for externally
                    established PSKs, the Hash algorithm MUST be set
                    when the PSK is established or default to SHA-256
                    if no such algorithm is defined.  The server MUST
                    ensure that it selects a compatible PSK (if any)
                    and cipher suite.  Each PSK MUST only be used with
                    a single hash function.";
                 reference
                   "RFC 8446: The Transport Layer Security (TLS)
                              Protocol Version 1.3";

I note that the description indicates that it should default to SHA-256, but this leaf is marked as mandatory with no default set instead?


(13) p 33, sec 3.3.  YANG Module

               leaf target-kdf {
                 type uint16;
                 description
                   "As per Section 3.1 of I-D.
                    ietf-tls-external-psk-guidance:
                    The specific Key Derivation Function (KDF) for which
                    a PSK is imported for use.";
                 reference
                   "I-D.ietf-tls-external-psk-importer:
                              Importing External PSKs for TLS";
               }

For these two, draft ietf-tls-external-psk-guidance has no section 3.1, were these two descriptions (for target-protocol and target-kdf) meant to be referring to section 3.1 of ietf-tls-external-psk-importer instead?


(14) p 46, sec 4.3.  YANG Module

     feature server-ident-tls12-psk {
       description
         "Indicates that the server supports identifying itself
          using TLS-1.2 PSKs (pre-shared or pairwise-symmetric keys).";
       reference
         "RFC 4279:
            Pre-Shared Key Ciphersuites for Transport Layer Security
            (TLS)";
     }

Would it make sense for this feature to be conditional on the tls12 feature in the common module?


(15) p 46, sec 4.3.  YANG Module

     feature server-ident-tls13-epsk {
       description
         "Indicates that the server supports identifying itself
          using TLS-1.3 External PSKs (pre-shared keys).";
       reference
         "RFC 8446:
            The Transport Layer Security (TLS) Protocol Version 1.3";
     }

Would it make sense for this feature to be conditional on the tls13 feature in the common module?


(16) p 47, sec 4.3.  YANG Module

     feature client-auth-tls12-psk {
       description
         "Indicates that the server supports authenticating clients
          using PSKs (pre-shared or pairwise-symmetric keys).";
       reference
         "RFC 4279:
            Pre-Shared Key Ciphersuites for Transport Layer Security
            (TLS)";
     }

Would it make sense for this feature to be conditional on the tls12 feature in the common module?


(17) p 47, sec 4.3.  YANG Module

     feature client-auth-tls13-epsk {
       description
         "Indicates that the server supports authenticating clients
          using TLS-1.3 External PSKs (pre-shared keys).";
       reference
         "RFC 8446:
            The Transport Layer Security (TLS) Protocol Version 1.3";
     }

Would it make sense for this feature to be conditional on the tls13 feature in the common module?


(18) p 49, sec 4.3.  YANG Module

               leaf id_hint {
                 type string;
                 description
                   "The key 'psk_identity_hint' value used in the TLS
                    'ServerKeyExchange' message.";
                 reference
                   "RFC 4279: Pre-Shared Key Ciphersuites for
                              Transport Layer Security (TLS)";
               }

Would id-hint be better that id_hint for consistency with how we generally see YANG identifiers?


(19) p 51, sec 4.3.  YANG Module

               leaf target-protocol {
                 type uint16;
                 description
                   "As per Section 3.1 of I-D.
                    ietf-tls-external-psk-guidance: The protocol
                    for which a PSK is imported for use.";
                 reference
                   "I-D.ietf-tls-external-psk-importer:
                              Importing External PSKs for TLS";
               }
               leaf target-kdf {
                 type uint16;
                 description
                   "As per Section 3.1 of I-D.
                    ietf-tls-external-psk-guidance: The specific Key
                    Derivation Function (KDF) for which a PSK is
                    imported for use.";
                 reference
                   "I-D.ietf-tls-external-psk-importer:
                              Importing External PSKs for TLS";
               }

Same question as previously asked about whether section 3.1 and ietf-tls-external-psk-guidance is the correct reference here.


(20) p 55, sec 5.1.  The "iana-tls-cipher-suite-algs" Module

   YANG identities are not security-sensitive, as they are statically
   defined in the publicly-accessible YANG module.

Is it worth adding any text to highlight that these identities represent algorithms that may be deprecated or obsoleted for security reasons?  I.e., these identities feel a bit different than the usual kind.


(21) p 59, sec 6.3.  The "iana-tls-cipher-suite-algs" Module

   *  Please note that this module was created on June 16th, 2022, and
      that additional entries may have been added in the interim before
      this document's publication.  If this is that case, IANA may
      either publish just an updated module containing the new entries,
      or publish the initial module as is immediately followed by a
      "revision" containing the additional algorithm names.

As per comments below, I propose that IANA just publishes the initial version at the point that this document becomes an RFC.


(22) p 65, sec Appendix A.  YANG Modules for IANA

   The module contained in this section was generated by scripts using
   the contents of the associated sub-registry as they existed on June
   16th, 2022.

I presume that these scripts will be available and owned by IANA?



Nit level comments:

(23) p 9, sec 2.1.4.  Protocol-accessible Nodes

          |     |     +---w (encrypted-by-choice)
          |     |        +--:(symmetric-key-ref)
          |     |        |        {central-keystore-supported,symmetric\
   -keys}?
          |     |        |  +---w symmetric-key-ref?
          |     |        |          ks:symmetric-key-ref
          |     |        +--:(asymmetric-key-ref)
          |     |                 {central-keystore-supported,asymmetri\
   c-keys}?
          |     |           +---w asymmetric-key-ref?

For readability, perhaps worth asking the RFC editor to manually fold the symmetric-keys and asymmetric-keys features onto a new line rather than relying on RFC 8792?


(24) p 13, sec 2.3.  YANG Module

     feature tls12 {
       status "deprecated";
       description
         "TLS Protocol Version 1.2 is supported  TLS 1.2 is obsolete
          and thus it is NOT RECOMMENDED to enable this feature.";
       reference
         "RFC 5246: The Transport Layer Security (TLS) Protocol
                    Version 1.2";
     }

s/supported  TLS/supported.  TLS/


(25) p 15, sec 2.3.  YANG Module

     identity tls13 {
       if-feature "tls13";
       base tls-version-base;
       description
         "TLS Protocol Version 1.3.";
       reference
         "RFC 8446: The Transport Layer Security (TLS)
                    Protocol Version 1.3";
     }

Do you want a "     // Typedefs" since this isn't an identity?


(26) p 29, sec 3.3.  YANG Module

       container client-identity {
         nacm:default-deny-write;
         presence
           "Indicates that a TLS-level client identity has been
            configured.  This statement is present so the mandatory
            descendant do not imply that this node must be configured.";
         description
           "Identity credentials the TLS client MAY present when
            establishing a connection to a TLS server.  If not
            configured, then client authentication is presumed to
            occur a protocol layer above TLS.  When configured,
            and requested by the TLS server when establishing a

s/occur a protocol layer/occur in a protocol layer/


(27) p 35, sec 3.3.  YANG Module

           description
             "Indicates that the TLS client can authenticate TLS servers
              using configure PSKs (pre-shared or pairwise-symmetric
              keys).

s/configure/configured/?


(28) p 39, sec 4.1.2.1.  The "tls-server-grouping" Grouping

   *  The "keepalives" node, which must be enabled by a feature,
      configures a flag enabling the TLS client to test the aliveness of
      the TLS server, as well as a "presence" container for testing the
      aliveness of the TLSi client.  The aliveness-tests occurs at the
      TLS protocol layer.

s/TLSi/TLS/?

Regards,
Rob