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

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

HI Kent,

I’ve given some replies inline, but in short, I think that what you have is fine and hence I will initiate IETF LC on this draft.

inline …

From: Kent Watsen <[e-mail]>
Date: Wednesday, 20 December 2023 at 00:27
To: Rob Wilton (rwilton) <rwilton=[e-mail]>
Cc: [e-mail] <[e-mail]>, draft-ietf-netconf-http-client-server.all@ietf<mailto:draft-ietf-netconf-http-client-server.all@ietf>.org <[e-mail]>
Subject: Re: [etconf] AD review of draft-ietf-netconf-http-client-server-13
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
     |    |           |              |     |                 ^
     |    |           |        +-----+     +---------+       |
     |    |           |        |                     |       |
     |    +-----------|--------|--------------+      |       |
     |                |        |              |      |       |
     +-----------+    |        |              |      |       |
                 |    |        |              |      |       |
                 |    |        |              |      |       |
              etconf-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?


Yes, probably okay.  I still feel it is a little bit misleading but it would probably make the diagram look quite a lot more complicated, so I’m happy to leave it as is, and see if anyone else complains.


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

Ack.



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


Thanks.


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

This is okay.  I still think that it would be slightly better to have the presence containers under the case statements (and drop the presence statement from the proxy-connect container), but I think that functionality wise they equate to the same thing and I’m happy for you to leave it as is.

Regards,
Rob




Regards,
Rob

Kent