[netconf] AD review of draft-ietf-netconf-http-client-server-13

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

Hi Kent,

This is my AD review of draft-ietf-netconf-http-client-server-13.  Another great, easy to read document, thanks.

Moderate level comments:

(1) p 3, sec 1.1.  Relation to other RFCs

                                  crypto-types
                                    ^      ^
                                   /        \
                                  /          \
                         truststore         keystore
                          ^     ^             ^  ^
                          |     +---------+   |  |
                          |               |   |  |
                          |      +------------+  |
   tcp-client-server      |     /         |      |
      ^    ^        ssh-client-server     |      |
      |    |           ^            tls-client-server
      |    |           |              ^     ^        http-client-server
      |    |           |              |     |                 ^
      |    |           |        +-----+     +---------+       |
      |    |           |        |                     |       |
      |    +-----------|--------|--------------+      |       |
      |                |        |              |      |       |
      +-----------+    |        |              |      |       |
                  |    |        |              |      |       |
                  |    |        |              |      |       |
               netconf-client-server       restconf-client-server

Looking at the YANG, I would have thought that http-client-server should have a normative reference to tcp, and tls that isn't illustrated in the diagram above.  E.g., I reviewed this document before tls because I thought that it had no dependency from the diagram.


(2) p 25, sec 4.1.  The "ietf-http-client" YANG Module

   None of the writable data nodes defined in this YANG module are
   considered sensitive or vulnerable in network environments.  The NACM
   "default-deny-write" extension has not been set for any data nodes
   defined in this module.

Should 'client-identity' in http-client-identity-grouping be listed here?  It has a default-deny-write extension.  A similar comment applies for 'proxy-connect'.


(3) p 26, sec 4.2.  The "ietf-http-server" YANG Module

   None of the writable data nodes defined in this YANG module are
   considered sensitive or vulnerable in network environments.  The NACM
   "default-deny-write" extension has not been set for any data nodes
   defined in this module.

Should 'server-name', 'client-authentication', and users/user/basic/password be listed here?  They all have default-deny-write NACM extension statements defined on them.



Minor level comments:

(4) p 1, sec 

   The "Relation to other RFCs" section Section 1.1 contains a self-
   reference to this draft, along with a corresponding Informative
   Reference in the Appendix.

Note to clarify the instruction to the RFC editor.


(5) p 4, sec 1.4.  Conventions

   Various examples used in this document use a placeholder value for
   binary data that has been base64 encoded (e.g., "BASE64VALUE=").
   This placeholder value is used as real base64 encoded structures are
   often many lines long and hence distracting to the example being
   presented.

I couldn't see any usage of BASE64VALUE, perhaps this paragraph can be removed?


(6) p 8, sec 2.1.2.3.  The "http-client-stack-grouping" Grouping

      -  The "tcp-client-grouping" grouping is discussed in
         Section 3.1.2.1 of [I-D.ietf-netconf-tcp-client-server].
      -  The "tls-client-grouping" grouping is discussed in
         Section 3.1.2.1 of [I-D.ietf-netconf-tls-client-server].
      -  The "http-client-grouping" grouping is discussed in
         Section 2.1.2.2 in this document.

Would it be helpful to also include the expanded tree, perhaps in an appendix if it is quite long?


(7) p 8, sec 2.1.3.  Protocol-accessible Nodes

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

Same comment as per other draft review as to whether this paragraph is helpful/required.


(8) p 8, sec 2.2.  Example Usage

   <http-client xmlns="urn:ietf:params:xml:ns:yang:ietf-http-client">
     <client-identity>
       <basic>
         <user-id>bob</user-id>
         <cleartext-password>secret</cleartext-password>
       </basic>
     </client-identity>
   </http-client>

I find this example slightly confusing (relative to the description) because it never identifies the server that it is purportedly connecting to.  This is also true for the connection via a proxy.


(9) p 13, sec 2.3.  YANG Module

         presence
           "Indicates that a proxy server connections have been
            configured.  This statement is present so the mandatory
            descendant nodes do not imply that this node must be
            configured.";

As per a similar comment on the TCP draft, I wonder whether a presence container is needed here, or whether it would be better to move it down to the containers under the case statements?

Regards,
Rob