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

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

Hi Kent,

Please see inline …

I think that there are just some small changes (perhaps mainly deleting support for TLS1.0/TLS1.1) and then we should be good to go.

From: Kent Watsen <[e-mail]>
Date: Monday, 18 December 2023 at 23:11
To: Rob Wilton (rwilton) <rwilton=[e-mail]>
Cc: [e-mail] <[e-mail]>, [e-mail] <[e-mail]>
Subject: Re: [netconf] AD review of draft-ietf-netconf-tls-client-server-33
Hi Rob,

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

Kent // author



On Jun 28, 2023, at 9:09 AM, Rob Wilton (rwilton) <rwilton=[e-mail]> wrote:

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

Good catch!
Fixed in my local copy.



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.

ACK.  And I do mean these values per that understanding.



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?

No valid use-cases that I’m aware of.   Invalid use cases include 1.0/1.1 are still is use, but that doesn’t mean our YANG has to carry that debt forward.   The tls10/tls11 feature/identity statements are not used by any other YANG (unlike the tls12/tls13 statements).

I recommend removing the  tls10/tls11 feature/identity statements from the YANG, and remove the text above about them being NOT RECOMMENDED.

I think that you should just remove them from the model/draft, and only support TLS 1.2 onwards.


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

Indeed.

I had made this change already when responding to your same comment for the SSH draft.

Thanks.


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

To your first point, good catch!   By way of explanation, the “cleartext-private-key” feature was a recent addition, so the default was valid before.  Nonetheless, I’ve removed the default and made the choice “mandatory true”.  Interestingly this was already the case in the SSH draft, which surprised me as I do try to keep them consistent.

To your second point, I had made this change already when responding to your same comment for the SSH draft.


Thanks.


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

Firstly, note that different drafts are listed in the “description” and the “reference” sections.  The correct draft is the one listed in the “reference” section, now RFC 9258.  I fixed this oversight after confirming with Jeff, my co-author, who wrote this section.

Next, for what it should look like, note that both examples in Section 4.2 (in the tls-client-server draft) include the line: `            <target-protocol>8443</target-protocol>`.   Jeff tells me that, though they look like port numbers, these a valid values...

Okay.

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

I’m sympathetic to the concern of including initial versions of the to-be IANA-maintained modules, as they will quickly be out of date.  I’m happy to remove them, from this draft as well as from the ssh-client-server draft, but I’m unwilling to include the scripts used to generate these YANG modules, or even share them with IANA, unless they have a bash-developer on staff.

That being said, what to do?   Can we just have a note to the RFC Editor to remove these YANG modules?

My suggestion is that we keep them in for now, and perhaps flag an IANA/RFC editor comment to see if they have a preference on what to do (and include a note that if they do remove them then the list of normative references could probably be shortened significantly).

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

IDK what’s best, but will say that the underlying IANA registry continues to list the entries marked “obsolete” in the YANG module.


IIRC, there is a difference as to what deprecated means in YANG vs what it means for security ciphers.  I think that I will need to flag this up with the SEC ADs during the IESG review.  I suspect that probably marking deprecated cipher entries in YANG as deprecated, and also as “SHOULD NOT” use in the description is probably the right thing to do.




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

I’ve attached the latest version of the script used to create the IANA module.

Okay, thanks.  I took a quick look, but it looks like it has data paste into the script rather than always pulling it from IANA or the Datatracker.  Hence, I can understand why this script was useful for you initially generating it, but probably isn’t useful to IANA going forward.  I think that IANA having a script here would be most useful for ensuring that the generated file is right though …. Not a problem that you need to solve here and now though.



Minor level comments:

(6) p 4, sec 1.  Introduction

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

Done!




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

Good catch, RFC 8446 says that they’re in “preference order”.
Added an "ordered-by user” statement here.



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

I have removed the “expanded” tree diagrams, for the “common” modules, in both this and the SSH draft.

Yes, the tree diagrams are huge.  With all of the line-wrapping, they are not useful to include in the Appendix.

Ack.  I also think that folks working with YANG will effectively need to know how to generate them anyway.




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

Certainly possible.  It's not difficult to implement support for both if doing one of them already.  Of course, a consuming module can always augment in another if-feature statement to disable one, in case both aren’t supported.

Ack, okay.


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

Ack.  See above.



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

I’m sure that you have noticed these drafts have a similar structure.  So much so that I once mentioned to Juergen that it would be a cool student project to write an “YANG-to-RFC” generator to do the heavy lifting.  Part of this structure is that there exists a section called “Protocol-accessible Nodes”.  In my view, the section should always exist because it makes it explicit (not a “gap” that has to be discovered).

However, I agree that the quoted text doesn’t seem to convey the right sentiment.  Fortunately, I found in the TCP draft an additional sentence being used that resolves the issue for me.  The additional sentence is:

"Thus this module, when implemented, does not itself define any protocol-accessible nodes.”

I’ve updated all the drafts to include this second sentence.

Okay,  thanks.

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

Jeff Hartley wrote this section, but I agree with your analysis.
I’ve replaced the "mandatory true” with a “default sha-256"

Thanks.


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

Fixed via another one of your comments.
The “description” was supposed to say “ietf-tls-external-psk-importer” (now RFC 9258).

Thanks.


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

Done.

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

Done.



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

Done.



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

Done.



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

Indeed.   (This stanza was from Jeff)


Thanks.


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

Same answer, the draft in the “description” statement was wrong (fixed now).



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

Added the sentence:

  "IANA MAY deprecate and/or obsolete identities over time as needed to address security issues found in the algorithms."

To both this and the SSH draft.

Thanks.


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

What is your desired text-update?

My proposal is to mark these sections to be removed by the RFC Editor…

Highlighting them to IANA and RFC editor is fine for now.  We will then figure out if they need to stay in the RFC or can be elided at publication time.


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

No, they’re useless to non-programmers.  See previous comment.


Agreed.


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?

I added the following text to each draft’s “Note to RFC Editor” section:


Tree-diagrams in this draft may use the '/' line-folding mode defined in RFC 8792.

However, nicer-to-the-eye is when the '//' line-folding mode is used.  The AD suggested

suggested putting a request here for the RFC Editor to help convert "ugly" '/' folded

examples to use the '//' folding mode.  "Help convert" may be interpreted as, identify

what looks ugly and ask the authors to make the adjustment.

Good?


I think that I’ve answered this a couple of weeks ago, that I wasn’t really suggesting the // mode as much as just manually changing the indentation of the tree diagrams to make them more readable.  I.e., I don’t think that tree diagrams in RFCs need to be machine readable code.


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

Fixed - thx!



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

Indeed yes - added!



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

Fixed - thx!



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

Yes indeed - fixed!



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

Fixed!  Thank you!



Regards,
Rob

PS: “fixed” in this message means “fixed in my local copy”.  I will
publish all of the drafts to Datatracker together once I've responded
to your comments to the last three drafts (http, netconf, and restconf).

Thanks for all the updates,
Rob

Cheers,
Kent  // author