Re: [netconf] Éric Vyncke's Discuss on draft-ietf-netconf-https-notif-13: (with DISCUSS and COMMENT)

[Mahesh Jethanandani <[e-mail]>]

Hi Eric,

We posted a -14 version of the draft that addresses your DISCUSS and COMMENT. Please review.

Thanks.

> On Dec 15, 2022, at 8:49 AM, Kent Watsen <[e-mail]> wrote:
> 
> Hi Ėric,
> 
> Please see below for responses to your valuable comments.
> 
> Kent // co-author
> 
> 
>> On Dec 12, 2022, at 11:38 AM, Éric Vyncke via Datatracker <[e-mail] <mailto:[e-mail]>> wrote:
>> 
>> Éric Vyncke has entered the following ballot position for
>> draft-ietf-netconf-https-notif-13: Discuss
>> 
>> When responding, please keep the subject line intact and reply to all
>> email addresses included in the To and CC lines. (Feel free to cut this
>> introductory paragraph, however.)
>> 
>> 
>> Please refer to https://www.ietf.org/about/groups/iesg/statements/handling-ballot-positions/ <https://www.ietf.org/about/groups/iesg/statements/handling-ballot-positions/> 
>> for more information about how to handle DISCUSS and COMMENT positions.
>> 
>> 
>> The document, along with other ballot positions, can be found here:
>> https://datatracker.ietf.org/doc/draft-ietf-netconf-https-notif/ <https://datatracker.ietf.org/doc/draft-ietf-netconf-https-notif/>
>> 
>> 
>> 
>> ----------------------------------------------------------------------
>> DISCUSS:
>> ----------------------------------------------------------------------
>> 
>> 
>> # Éric Vyncke, INT AD, comments for draft-ietf-netconf-https-notif-13
>> CC @evyncke
>> 
>> Thank you for the work put into this document.
>> 
>> Please find below one blocking DISCUSS points (easy to address), some
>> non-blocking COMMENT points (but replies would be appreciated even if only for
>> my own education).
>> 
>> Special thanks to Qiufang Ma for the shepherd's detailed write-up including the
>> WG consensus *and* the justification of the intended status.
>> 
>> I hope that this review helps to improve the document,
>> 
>> Regards,
>> 
>> -éric
>> 
>> ## DISCUSS
>> 
>> As noted in https://www.ietf.org/blog/handling-iesg-ballot-positions/, a
>> DISCUSS ballot is a request to have a discussion on the following topics:
>> 
>> ### Section 6.1
>> 
>> `The module contains the TCP, TLS and HTTPS parameters` seems to prevent the
>> use of HTTP/3, which relies on QUIC, i.e., UDP. Is it on purpose ? If so, a
>> justification is probably required.
> 
> The parameters mentioned are inherited from draft-ietf-netconf-http-client-server, which was just published to the IESG for consideration.  
> 
> Looking at the "http-client-server" draft, Section 1.1, entitled "Relation to other RFCs" [1], you will notice that the there is a hierarchy of protocol-specific drafts, each pertaining only to its layer in a protocol stack.  More specifically, each draft defines two YANG "groupings", one for the protocol-client and the other for the protocol-server.
> 
> [1] https://www.ietf.org/archive/id/draft-ietf-netconf-http-client-server-12.html#name-relation-to-other-rfcs <https://www.ietf.org/archive/id/draft-ietf-netconf-http-client-server-12.html#name-relation-to-other-rfcs>
> 
> For instance, to define an HTTP-client, the stack is:
> 
>     tcp-client-grouping	// from draft-ietf-netconf-tcp-client-server
>     http-client-grouping	// from draft-ietf-netconf-http-client-server
> 
> To define an HTTPS-client, the stack is:
> 
>     tcp-client-grouping	// from draft-ietf-netconf-tcp-client-server
>     tls-client-grouping	// from draft-ietf-netconf-tls-client-server
>     http-client-grouping	// from draft-ietf-netconf-http-client-server
> 
> Now, to support a QUIC-client, we'd want a stack something like:
> 
>     udp-client-grouping	// from TBD
>     quic-client-grouping	// from TBD
>     http-client-grouping	// from draft-ietf-netconf-http-client-server
> 
> The "TBD" above means that there are currently no drafts in progress for "udp-client-server" or "quic-client-server".   Such drafts may be developed at any time, and "case" statements may be "augmented" into the instances of the groupings defined the "http-client-server" draft.
> 
> All this to say, support for HTTP/3 can be added when desired.
> 
> 
>> Related to the above sentence, the tree has:
>> ```
>>          +--rw https-receiver
>>             +--rw (transport)
>>             |  +--:(tls) {tls-supported}?
>>             |     +--rw tls
>> ```
>> Please, bear with my ignorance of YANG, but does the '?' indicate that the tls
>> sub-tree may not be supported ? Weird for an IETF draft whose title include
>> HTTPS ;-) (or is it to support QUIC ?)
> 
> Yes, the "{foo}?" syntax in YANG tree-diagrams (RFC 8340) means that the node is present only when the specified feature (foo) is enabled.
> 
> The "https-notif" module (defined in this draft) inherits a snippet of YANG from the "ietf-http-client" module (defined in draft-ietf-netconf-http-client-server).   The "http-client-server" draft correctly defines features enabling the various supported transports (e.g., TCP and TLS currently, QUIC when defined) so that the specific transport(s) supported by an application may be specified.
> 
> FWIW, this draft's YANG explicitly disables the ability for HTTP (i.e., w/o TLS), but the tree diagram still renders the only remaining "case" statement with its "feature" statement.  This should be seen as a positive, as it enables other secure transports to be exclusively supported in the future (e.g., a QUIC-only future).  Perhaps this draft's title could be changed, e.g., s/HTTPS/Secure/?
> 
> 
>> Finally, and for sure it is my ignorance about YANG, but I do not see an
>> obvious link between the tree view and the YANG module. Should there be a
>> written explanation in the text ? (this is of course a COMMENT-level point).
> 
> Good catch, this draft is missing a reference to RFC 8340.
> 
> 
> 
>> ### Section 7
>> 
>> The security section is mainly about the YANG RESTCONF/NETCONF template, but
>> does not address the security of sending notifications over HTTP. E.g., as
>> written below, what about TLS mutual authentication ? How is the HTTP server
>> certificate is trusted ?
> 
> HTTP (without TLS) is not supported by this draft.
> 
> The Security Considerations section, as written, accurately addresses the "northbound API" presented by NETCONF and RESTCONF servers.  What is not addressed is the "eastbound API" this draft introduces, which is just "HTTPS" and does not have a mutual-auth requirement.
> 
> Would the fix be to add the following to the Security Considerations section?
> 	- the publisher (the HTTPS-client) MUST authenticate the server.
> 	- the receiver (the HTTPS-server) MUST authenticate the client.
> 
> 
> 
>> ----------------------------------------------------------------------
>> COMMENT:
>> ----------------------------------------------------------------------
>> 
>> 
>> ## COMMENTS
>> 
>> ### Abstract
>> ```
>>   This document requires that the publisher is a "server" (e.g., a
>>   NETCONF or RESTCONF server), but does not assume that the receiver is
>>   a server.
>> ```
>> This last paragraph adds more confusion on the roles... Suggest to remove it.
> 
> The paragraph was added due to comments about how things were confusing before.
> 
> To be clear:
> 
> - the publisher (the HTTPS-client) MUST also be a YANG-based server, i.e., presenting a YANG-based protocol such as NETCONF and RESTCONF.
>  
> - the receiver (the HTTPS-server), is an HTTPS-server without further stipulation
> 
> 
> 
> 
>> ### Roles
>> 
>> Suggest to prefix server always by HTTPS (i.e., "HTTPS server") and use
>> "publisher" for the NETCONF/RESTCONF "server".
> 
> Agreed.  Mark Nottingham provided similar comments, and v14 version of the document (currently in GitHub here <https://github.com/netconf-wg/https-notif/pull/27>) carries the prefix HTTP or NETCONF/RESTCONF before the word “server” to clarify what kind of server is being assumed.
> 
>> 
>> ### Which TLS version ?
>> 
>> I will let the transport/security ADs to ballot a DISCUSS if required, but
>> wouldn't a specification of which HTTP and TLS versions are required ?
> 
> The "http-client-server" draft does not have any "feature" statements enabling configuration to vary the configuration HTTP-layer by HTTP-version.  And hence is unconstrained.
> 
> The "tls-client-server" draft does have "feature" statements enabling configuration to vary by TLS-version, specifically 1.0, 1.1, 1.2, and 1.3.  Note that 1.0 and 1.1 have "status obsolete", and TLS 1.2 has "status deprecated".
> 
> This draft inherits the above stance without imposing any new limitations.
> 
> 
>> Should the certificate validation procedure be specified ?
> 
> No, such is for protocol drafts to define.  This draft, and those it depends on, regard only on how to *configure* the protocol stacks.
> 
> 
> 
>> ### Section 2
>> 
>> Probably linked to my ignorance of the system (a global archicture or a more
>> detailed introduction would be appreciated), but how is the
>> "relay-notifications" known by the publisher ? In the examples, there is not
>> such item.
> 
> There is a typo in Section 2: s/relay-notifications/relay-notification/.
> 
> Search again without the trailing 's'?
> 
> 
>> Minor comment: is it a "path" or a "common prefix" ? Also, doesn't having a
>> common prefix in the URI prevent having two "HTTP servers" for the same
>> subscription. E.g., one for the capabilities and one for the notifications
>> (e.g., for geographica load distribution).
> 
> It is a common prefix.  This was fixed per another comment.
> 
> 
>> 
>> ### Section 3.2
>> 
>> `a publisher can issue an HTTPS GET request` should the 'can' be 'upgraded' to
>> a "SHOULD" or "MAY" ?
> 
> Good grief, as the beginning of that sentence is written, it's more like a MUST ;)
> 
> But equally good, IMO: s/can issue/issues/
> 
> Thoughts?
> 
> 
>> ### Section 3.4
>> 
>> Please replace 'nnn' by the exact Content-Length.
> 
> Or remove "Content-Length" line from the examples?
> 
> 
> 
>> ### Section 4.1
>> 
>> s/HTTPS POST request/HTTP POST request/ ?
> 
> Good catch.
> 
> 
> 
>> ### Section 4.2
>> 
>> ```
>>   ... In case of
>>   corrupted or malformed event, the response should be an appropriate
>>   HTTP error response.
>> ```
>> 
>> Suggest to replace "should" by "SHOULD" or even "MUST".
> 
> Agreed.
> 
> 
>> ## NITS
>> 
>> ## Notes
>> 
>> This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
>> [`ietf-comments` tool][ICT] to automatically convert this review into
>> individual GitHub issues.
>> 
>> [ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md <https://github.com/mnot/ietf-comments/blob/main/format.md>
>> [ICT]: https://github.com/mnot/ietf-comments <https://github.com/mnot/ietf-comments>
> 
> 
> 
> Thanks again!
> Kent


Mahesh Jethanandani
[e-mail]