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

Kent Watsen <kent+ietf@watsen.net> Sat, 27 January 2024 04:14 UTC

Return-Path: <0100018d49202096-656d9f2f-71f6-4c74-9680-7e0f1fcd2b86-000000@amazonses.watsen.net>
X-Original-To: netconf@ietfa.amsl.com
Delivered-To: netconf@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id ACD4DC14F60A; Fri, 26 Jan 2024 20:14:23 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.909
X-Spam-Level:
X-Spam-Status: No, score=-1.909 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, HTML_MESSAGE=0.001, RCVD_IN_MSPIKE_H2=-0.001, RCVD_IN_ZEN_BLOCKED_OPENDNS=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01] autolearn=unavailable autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=amazonses.com
Received: from mail.ietf.org ([50.223.129.194]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id b7IIsKRHDXYF; Fri, 26 Jan 2024 20:14:20 -0800 (PST)
Received: from a48-95.smtp-out.amazonses.com (a48-95.smtp-out.amazonses.com [54.240.48.95]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 09EFBC14F6FD; Fri, 26 Jan 2024 20:14:19 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/simple; s=224i4yxa5dv7c2xz3womw6peuasteono; d=amazonses.com; t=1706328858; h=From:Message-Id:Content-Type:Mime-Version:Subject:Date:In-Reply-To:Cc:To:References:Feedback-ID; bh=r5JIUKr4vApXjq4YE8D9trw/m8LxnbWyqzEdsdiTZaQ=; b=PNNFhxP4pXvNRfi354n3IZwBSbElXHN+sLaErkcap7fQBePUbBnHzw6SRPcrx/RM 9GVjME1otcSwExOOAFcrKUWI0QW++z57m4sOKRxo92mC5A+FUm7urR+4IkQqlvjpFu2 RhHB95A9PnUY8To/n87vah4tZlWgiQTUjzlYouGo=
From: Kent Watsen <kent+ietf@watsen.net>
Message-ID: <0100018d49202096-656d9f2f-71f6-4c74-9680-7e0f1fcd2b86-000000@email.amazonses.com>
Content-Type: multipart/alternative; boundary="Apple-Mail=_927DA7EF-60A6-4B3B-BD65-3C716F676FE5"
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.600.7\))
Date: Sat, 27 Jan 2024 04:14:18 +0000
In-Reply-To: <LV8PR11MB8536590D4BF46493C5CBAE52B5792@LV8PR11MB8536.namprd11.prod.outlook.com>
Cc: "Rob Wilton (rwilton)" <rwilton=40cisco.com@dmarc.ietf.org>, "draft-ietf-netconf-tls-client-server.all@ietf.org" <draft-ietf-netconf-tls-client-server.all@ietf.org>, "netconf@ietf.org" <netconf@ietf.org>
To: "Rob Wilton (rwilton)" <rwilton@cisco.com>
References: <BY5PR11MB4196C94A3F111E771CA8B3E9B524A@BY5PR11MB4196.namprd11.prod.outlook.com> <0100018c7f33075d-6c0682a3-f6e1-4878-b188-782b364eb446-000000@email.amazonses.com> <LV8PR11MB8536590D4BF46493C5CBAE52B5792@LV8PR11MB8536.namprd11.prod.outlook.com>
X-Mailer: Apple Mail (2.3731.600.7)
Feedback-ID: 1.us-east-1.DKmIRZFhhsBhtmFMNikgwZUWVrODEw9qVcPhqJEI2DA=:AmazonSES
X-SES-Outgoing: 2024.01.27-54.240.48.95
Archived-At: <https://mailarchive.ietf.org/arch/msg/netconf/eAPbYwNpny_epY9ihYbcMW0DwqE>
Subject: Re: [netconf] AD review of draft-ietf-netconf-tls-client-server-33
X-BeenThere: netconf@ietf.org
X-Mailman-Version: 2.1.39
Precedence: list
List-Id: NETCONF WG list <netconf.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/netconf>, <mailto:netconf-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/netconf/>
List-Post: <mailto:netconf@ietf.org>
List-Help: <mailto:netconf-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/netconf>, <mailto:netconf-request@ietf.org?subject=subscribe>
X-List-Received-Date: Sat, 27 Jan 2024 04:14:23 -0000

Hi Rob,

Thanks for your review.
More responses below.

Kent


> On Jan 26, 2024, at 9:53 AM, Rob Wilton (rwilton) <rwilton@cisco.com> wrote:
> 
> 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 <kent+ietf@watsen.net <mailto:kent+ietf@watsen.net>>
> Date: Monday, 18 December 2023 at 23:11
> To: Rob Wilton (rwilton) <rwilton=40cisco.com@dmarc.ietf.org <mailto:rwilton=40cisco.com@dmarc.ietf.org>>
> Cc: draft-ietf-netconf-tls-client-server.all@ietf.org <mailto:draft-ietf-netconf-tls-client-server.all@ietf.org> <draft-ietf-netconf-tls-client-server.all@ietf.org <mailto:draft-ietf-netconf-tls-client-server.all@ietf.org>>, netconf@ietf.org <mailto:netconf@ietf.org> <netconf@ietf.org <mailto:netconf@ietf.org>>
> 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=40cisco.com@dmarc.ietf.org <mailto:rwilton=40cisco.com@dmarc.ietf.org>> 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.

TLS 1.0 and 1.1 removed, per above.

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

This is inconsistent with the preference stated in the SSH review.
In any case, I updated the Note to Editor to remove Section A (YANG Modules for IANA) entirely.
However, in doing so, it will break the Introduction and Security Considerations sections, as the ref these sections…

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

Ack.


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

Ack.
 

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

Ack

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

Good point, but I’m getting tired to tweak the text in 9 documents now...

I will tell Editor to just make more readable later, when they come to bug me about them...


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