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

[Kent Watsen <[e-mail]>]

Hi Rob,

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

Kent // author


> On Jun 30, 2023, at 6:42 AM, Rob Wilton (rwilton) <rwilton=[e-mail]> wrote:
> 
> Hi Kent,
> 
> Here is my AD review of draft-ietf-netconf-netconf-client-server-29.
> 
> I think that this means that you have a full set of AD reviews for all 9 drafts, so you should be able to process/update them together.

Indeed - thank you!


> This draft is also very well written.  It is also, not surprisingly, structurally very similar to the RESTCONF draft, and hence where appropriate, I would like you to regard the comments that I made on the RESTCONF draft as equally applying to this draft and I will assume that they will have the same resolution and be updated in both unless you indicate in your replies otherwise.  Thus, I have not reflagged the same issues inline in the NETCONF draft but did try and include a summary (below) of the main points that I raised in the other reviews that should also be considered here.  Hopefully, this is pragmatic/sufficient, but let me know otherwise.

I fixed this draft alongside the restconf-client-server draft, but let’s see…


> I've also flagged a couple of other minor comments that seemed to be specific to this draft.

Ack.


> Moderate level comments:
> 
> (1) p 0, sec 
> 
>   This document defines two YANG modules, one module to configure a
>   NETCONF client and the other module to configure a NETCONF server.
>   Both modules support both the SSH and TLS transport protocols, and
>   support both standard NETCONF and NETCONF Call Home connections.

Why did you quote the above paragraph?


> I'm assuming that the comments that I've made previously on the other drafts, in particular the RESTCONF draft, will also be taken into consideration for this document, and resolved similarly (rather than raising all my questions again).
> 
> - RFC editor guidance on self references [FIXED]
> - Relationship diagram clarification.  [FIXED]
> - Empty groupings comment   [DISCUSS]
> - Support for on-demand connections rather than just persistent and periodic  [DISCUSS]
> - No endpoints container under grouping netconf-client-app-grouping/listen  [FIXED]
> - "Both the "initiate" and "listen" subtrees must be enabled " is unclear comment  [FIXED]
> - Query about "central" being the best prefix.  [DISCUSS]
> - Use of top level "*-supported" features vs putting them in a separate module.  [DISCUSS]
> - Avoid describing the reason why containers and p-containers exist in the YANG descriptions.  [FIXED]
> - Question about use of p-containers with lists requiring 1 or more entries (could just use a np-container and list of 0 or )  [DISCUSS]
> - Security considerations should list paths from groupings.  [DISCUSS]
> - Include expanded groupings, e.g., in the appendix?  [DISCUSS]


I’m aware of each issue above.  Many are addressed, while others need more discussion...


> Minor level comments:
> 
> (2) p 32, sec 3.2.  Example Usage
> 
>           <netconf-server-parameters>
>             <!-- nothing to configure -->
>           </netconf-server-parameters>
> 
> Possibly elide this, otherwise it implies that the netconf-server-parameter container is needed ...

Elided



> (3) p 43, sec 3.3.  YANG Module
> 
>               uses ncs:netconf-server-grouping {
>                 refine "client-identity-mappings/cert-to-name" {
>                   min-elements 1;
>                   description
>                     "The TLS transport requires a mapping.";
>                 }
>               }
> 
> I note that this refinement appears in the NETCONF module, but an equivalent refinement doesn't exist in the RESTCONF module.  Hence, I wanted to check that was intentional.

It was/is intentional.

RESTCONF clients may alternatively identify themselves at the HTTP-level 
	- i.e., HTTP "Basic" authentication
	- NOT using a TLS-level client-certificate

And therefore the cert-to-name mappings do not have to be configured...



> Nit level comments:
> 
> (4) p 43, sec 3.3.  YANG Module
> 
>                   description
>                     "NETCONF/TLS servers MUST validate client
>                      certificates.  This configures certificates
>                      at the socket-level (i.e. bags), more
>                      discriminating client-certificate checks
>                      SHOULD be implemented by the application.";
> 
> s/, more/. More/?

Indeed.  Fixed.



> (5) p 45, sec 3.3.  YANG Module
> 
>                 refine "client-identity-mappings" {
>                   if-feature "sshcmn:ssh-x509-certs";
>                   description
>                     "Augments in an 'if-feature' statement
>                      ensuring the 'client-identity-mappings'
>                      descendant is enabled only when SSH
>                      supports X.509 certificates.";
>                 }
> 
> s/Augments in/Adds in/.  I would suggest steering clear of using the term augment when not adding in new data nodes.

Point.   Fixed.



> (6) p 46, sec 3.3.  YANG Module
> 
>                   description
>                     "NETCONF/TLS servers MUST validate client
>                      certificates.  This configures certificates
>                      at the socket-level (i.e. bags), more
>                      discriminating client-certificate checks
>                      SHOULD be implemented by the application.";
> 
> s/, more/. More/?

Indeed.  Fixed.


> Regards,
> Rob

Kent