[netconf] Benjamin Kaduk's Discuss on draft-ietf-netconf-restconf-notif-13: (with DISCUSS and COMMENT)

[Benjamin Kaduk via Datatracker <[e-mail]>]

Benjamin Kaduk has entered the following ballot position for
draft-ietf-netconf-restconf-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/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.


The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-netconf-restconf-notif/



----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

Thanks for the well-written document!  I  just have some boring housecleaning
points that should be easy to resolve.

Section 3.2 states:

   Subscribers can learn what event streams a RESTCONF server supports
   by querying the "streams" container of ietf-subscribed-
   notification.yang in
   [I-D.draft-ietf-netconf-subscribed-notifications].  Support for the
   "streams" container of ietf-restconf-monitoring.yang in [RFC8040] is
   not required.  If it is supported, the event streams which are in the
   "streams" container of ietf-subscribed-notifications.yang SHOULD also
   be in the "streams" container of ietf-restconf-monitoring.yang.

This "SHOULD" seems to be attempting to impose a normative requirement
on specifications that implement
draft-ietf-netconf-subscribed-notifications and RFC 8040 streams,
without regard to whether they implement this specification.  It seems
better-placed in draft-ietf-netconf-subscribed-notifications.

Similarly, when Section 4 writes:

   To meet subscription quality of service promises, the publisher MUST
   take any existing subscription "dscp" and apply it to the DSCP
   marking in the IP header.

that seems to be duplicating a normative requirement from the core
subscribed-notifications document.  (And I'm sure Magnus will have
further follow-up about how DSCP markings are per-connection for the
stream transports we have available, as well.)


----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

The core subscribed-notifications document notes that dynamic
subscriptions only last as long as the underlying transport.  In this
document, we have two connections in Figure 1, which could potentially
be separate TCP/TLS connections (or HTTP/2 streams).  In the "two TCP
connections" case, does terminating either one cause the cessation of
the subscription, or just (b)?

Section 2

Please use the boilerplate from RFC 8174.

Section 3

                                      YANG datastore subscription is
   accomplished via augmentations to
   [I-D.draft-ietf-netconf-subscribed-notifications] as described within
   [I-D.ietf-netconf-yang-push] Section 4.4.

I see some reviewers commented that things were a bit terse/arcane; I
might suggest that "via augmentations to [subscribed notifications]" is
not really adding much here, and the yang-push RPCs are the important
part.

Section 3.1

                                                         Where quick
   recognition of the loss of a publisher is required, a subscriber
   SHOULD use a TLS heartbeat [RFC6520], just from receiver to
   publisher, to track HTTP session continuity.

TLS heartbeats require initiation by the TLS client, by virtue of
including the HeartbeatExtension in the ClientHello.  Who is going to be
making the determination that quick recognition is required, and if
that's the publisher, how does the subscriber know to use TLS
heartbeats?

nit: It's interesting that we seem to be using both "subscriber" and
"receiver" to talk about the TLS client, in the same sentence.

side note: per https://github.com/openssl/openssl/pull/1928, OpenSSL
will not support TLS or DTLS heartbeats of any form in its next major
release (3.0.0).

   Loss of the heartbeat MUST result in any subscription related TCP
   sessions between those endpoints being torn down.  A subscriber can
   then attempt to re-establish the dynamic subscription by using the
   procedure described in Section 3.

RFC 6520 does not specify retransmit numbers or intervals to be used to
determine that a peer is nonresponsive (i.e., "lost"), so this text
seems under-specified.  Is the intent to leave these decisions to be
implementation-specific?

Section 3.3

I see that draft-ietf-netconf-subscribed-notifications (section 2.4.6)
admonishes us to check for transport-layer errors (and ACLs!) before
RPC-level errors; is the text here about "fails to serve the RPC
request" and our description of error handling consistent with the
separate transport-layer error handling?  (I think it can be, with a
careful reading of "one of the reasons indicated in [] Section 2.4.6",
but perhaps other readings are possible.)

      The yang-data included within "error-info" SHOULD NOT include the
      optional leaf "error-reason", as such a leaf would be redundant
      with information that is already placed within the
      "error-app-tag".

I'm not sure where this "error-reason" leaf is defined -- I don't see it
in any of subscribed-notifications, yang-push, or RFC 8040.

Section 3.4

I'm not sure that I've previously encountered using the section heading
to introduce an acronym (as is done here for SSE).  I bet the RFC Editor
will do the right thing, though.

   o  In addition to an RPC response for a "modify-subscription" RPC
      traveling over (a), a "subscription-modified" state change
      notification MUST be sent within (b).  This allows the receiver to
      know exactly when the new terms of the subscription have been
      applied to the notification messages.  See arrow (c).

nit: I might suggest some language about "order within the notifications
stream" for this state change, but that's purely editorial.

   o  In addition to any required access permissions (e.g., NACM), RPCs
      modify-subscription, resync-subscription and delete-subscription
      SHOULD only be allowed by the same RESTCONF username [RFC8040]
      which invoked establish-subscription.

I'm confused about this "SHOULD" (the secdir reviewer noted it as well)
-- in my understanding, the core subscribed-notifications requires that
such RPCs must be done on the same transport connection as the one used
to create a dynamic subscription (i.e., line (a) in Figure 1), and since
RFC 8040 authenticated client identities are at the connection level,
there could never be any change of username across the calls.

   o  Loss of TLS heartbeat

(As noted above, this is under-specified at present.)

Section 9

I would suggest also recommending that the 'uri' values not have a
predictable structure or be guessable.  While we do have solid access
control in place via NACM/etc., there is still a risk of side-channel
leakage if there's a distinction between "resource does not exist" and
"not authorized".  (FYI we had a long discussion about unguessable URIs
in the context of draft-ietf-acme-acme, which had a much weaker
access-control story and could in some sense be said to use "capability
URIs", if anyone wants to do some background reading.)
(One might see also draft-gont-numeric-ids-sec-considerations.)

I see that the subscription-id type is only of type uint32 in
draft-ietf-netconf-subscribed-notifications, which to some extent limits
the unguessability property to the URIs and not as much for the IDs
themselves (though randomization within a 32-bit space is not without
value).

Appendix A

Consistent with my suggestion in the Security Considerations, I'd change
the returned URI here or at least note that the "22", "23", etc. are
placeholders and not indicative of expected usage.

(This snippet from A.1.1)

   {
      "ietf-subscribed-notifications:input": {
         "stream-xpath-filter": "/example-module:foo/",
         "stream": "NETCONF",
         "dscp": "10"
      }
   }

The ambiguity of "10" has been noted elsewhere, but since it's a uint8
(range "0..63") wouldn't it be a JSON number, not a JSON string?

Similarly, subscription IDs are uint32, which IIUC gets encoded as a
number.