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

[Kent Watsen <[e-mail]>]

Hi Rob,

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

Kent // author


> On Jun 26, 2023, at 12:36 PM, Rob Wilton (rwilton) <rwilton=[e-mail]> wrote:
> 
> 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.


First, I note that Section 2.1.2.2. was wrong, but fixed it as follows:

	OLD:
                 The "http-client-grouping" defines the configuration for just
                 "HTTP" part of a protocol stack.  It does not, for instance,
                 define any configuration for the "TCP" or "TLS" protocol layers
                 (for that, see <xref target="http-client-stack-grouping"/>).

	NEW:
                The "http-client-grouping" primarily (not including
                the proxy configuration) defines the configuration for just
                "HTTP" part of a protocol stack.  It does not, for instance,
                define any configuration for the "TCP" or "TLS" protocol layers
                (for that, see <xref target="http-client-stack-grouping"/>).

With this understanding in place, please take another look at the paragraph just above the diagram, reproduced below:

             The dependency relationship between the primary YANG groupings
             defined in the various RFCs is presented in the below diagram.
             In some cases, a draft may define secondary groupings that
             introduce dependencies not illustrated in the diagram.

Note in particular the word “primary” in the first sentence, and also the entirety of the last sentence.

My feeling is that this text allows the dependencies in the artwork to be valid / okay - agreed?



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

Fixed/added


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

Fixed/added


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

Per reviews in other drafts, the following note to the RFC Editor will now appear in all the drafts:

          The "Relation to other RFCs" section <xref target="collective-effort"/> contains
          a self-reference to this draft, along with a corresponding reference in
          the Appendix.  Please replace the self-reference in this section with "This RFC"
          (or similar) and remove the self-reference in the "Normative/Informative References"
          section, whichever it is in.



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

Removed.  Also removed in the restconf-client-server draft.



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

The issue isn’t that they’re long, but that they line-wrap so much as to become unreadable.

The WG made a decision a long time back to just have the unexpanded trees in the *-client-server modules because of this.


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

Same response as before, I added a second sentence to make the section easier to read.


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

The grouping “http-client-grouping” is what is commonly put into a container called “http-client-parameters”, which would be after, e.g., “tcp-client-parameters” and “tls-client-parameters”.   As such, the "http-client-grouping” itself doesn’t specify the HTTP server being connected to.

Elsewhere in the module there is the grouping "http-client-stack-grouping” that defines the whole “stack” (tcp+http and also tcp+tls+http), but this grouping does not have an example for it.

You are right that the text preceding the example is misleading.  To address this, I added the second sentence below:

          The following example illustrates the case where the HTTP client 
          connects directly to an HTTP server.  Note, the information identifying
          the remote server (e.g., its hostname) would be configured in the
          "tcp-client-grouping" (not shown).



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

The “case” statement itself is “mandatory true”.  The presence statement is needed here because the "proxy-connect” node is optional to configure.


> Regards,
> Rob

Kent