[netconf] Benjamin Kaduk's Discuss on draft-ietf-netconf-subscribed-notifications-24: (with DISCUSS and COMMENT)

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

Benjamin Kaduk has entered the following ballot position for
draft-ietf-netconf-subscribed-notifications-24: 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-subscribed-notifications/



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

Thanks for this document; I just have a few minor "housekeeping" points
that should get resolved before publication.  (Please also note the
substantive comments in the COMMENT section as well, particularly those
relating to the transport requirements and security considerations.)

I'm not sure that we state clearly enough what is required to have a
specification for a transport for notifications.  Specifically (see
COMMENT), in the module we seem to say that the specification must
indicate what the default encoding is to be used if not otherwise
specified, but that's not enumerated as a requirement on such a
specification anywhere I see.  I also think that we could probably
require (as opposed to "highly recommend" in the current security
considerations) that the transport provide message confidentiality and
integrity protection.

I'm also unsure that I properly understand the use of the "reset" RPC --
if it has no effect when transit connectivity already exists, then what
entity is going to call "reset" in the case of publisher timeout when
trying to reach a receiver?  Surely not the publisher itself, since
that would just be publisher-internal functionality with no need for an
external-facing RPC.

I'm also a little unclear on the mechanics of the list of subscriptions
described in Section 3.3.  Does it provide a way for a low-privilege
client (that can only access one or a handful of the subscriptions) to
enumerate all subscriptions that exist, including subscriptions used by
high-privilege clients?  If so, we may want to think about some security
considerations text to that effect; if not, we may want to say that the
access-control will limit which leafs are visible to some clients.

Finally, we have a few instances of "leaf filter-failure-hint" that's of
type "string", providing
             "Information describing where and/or why a provided filter
              was unsupportable for a subscription.";
I don't understand why it's a string as opposed to some form of
machine-readable data.  Is it supposed to be human-readable?  Does that
bring in any internationalization considerations?


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

Section 1.3

   Also note that transport specific transport drafts based on this
   specification MUST detail the life cycle of dynamic subscriptions, as
   well as the lifecycle of configured subscriptions (if supported).

I'm not sure I understand what additional specification is needed for
the lifecycle of configured subscriptions -- doesn't the previous
text tightly tie their lifecycle to the presence of configuration in the
configuration datastore?
nit: please be consistent about "life cycle" vs. "lifecycle" throughout.

Section 2.1

   An event stream is a named entity on a publisher which exposes a
   continuously updating set of YANG encoded event records.  [...]

nit: I don't think "YANG encoded" is well-defined (here and below), in
that YANG structures generate data models but can be encoded into
various formats (like XML and JSON).

Section 2.3

   If the publisher supports the "dscp" feature, then a subscription
   with a "dscp" leaf MUST result in a corresponding [RFC2474] DSCP
   marking being placed within the IP header of any resulting
   notification messages and subscription state change notifications.
   Where TCP is used, a publisher which supports the "dscp" feature
   SHOULD ensure that a subscription's notification messages are
   returned within a single TCP transport session where all traffic
   shares the subscription's "dscp" leaf value.  Where this cannot be
   guaranteed, any "establish subscription" RPC request SHOULD be
   rejected with a "dscp-unavailable" error

I can't decide whether we need to be more explicit in the second and/or
third sentences that this remains contingent on the subscription in
question having a "dscp" leaf.
nit: end sentence with a full stop

I share the TSV-ART reviewer's confusion about how the weighting
(as well as DSCP) functionality is intended to work.

Section 2.4.2.1

   Replay provides the ability to establish a subscription which is also
   capable of passing recently generated event records.  In other words,

nit/editorial: this language could probably be more clear about
"recently generated" meaning "in the past", that is, records for events
that have already happened when the subscription is established.

Section 2.4.3

   any number of times.  Dynamic subscriptions can only be modified via
   this RPC using a transport session connecting to the subscriber.  If

nit(?): is this more readable as "connecting to" or "connecting from"
the subscriber?  (The same wording shows up throughout the document, but
I'll try to just comment once.)

Section 2.4.5

Is there any distinction between "killing a subscription" and
"administratively deleting a subscription"?  It's unclear to me that we
need the extra connotations of the word "kill", here.

Section 2.4.6

   As a result of this mixture, how subscription errors are encoded

nit: "mixture" doesn't seem like the right word to me; "variety" or
"varied population" are the first alternatives that come to mind, though
they are not perfect either.

Is there some sort of "access denied" error that could result from
kill-subscription RPCs?  (The table/artwork only lists
"no-such-subscription".)

Section 2.5

It's interesting to see a slight dichotomy between dynamic and
configured subscriptions, in that (IIUC) for dynamic subscriptions, a
modification event un-suspends a subscription immediately, but for
configured subscriptions the publisher seems to have some latitude to
retain the subscription in the suspended state for some time before
un-suspending it and sending the "subscription-modified" state change
message.

                 In this case, a separate dynamic subscription can be
   established to retransmit the missing event records.

Do you want to put in a pointer to replay-start-time here?

Section 2.5.1

         Event records are only sent to active receivers.  Receivers of
   a configured subscription remain active if both transport
   connectivity can be verified to the receiver, and event records are
   not being dropped due to a publisher buffer overflow.  [...]

nit: "buffer overflow" is a technical term in security circles
indicating a potentially exploitable security flaw; would "publisher
buffer capacity being reached" be an acceptable substitute (here and
below)?

Section 2.7.7

   This notification indicates that all of the event records prior to
   the current time have been passed to a receiver.  It is sent before
   any notification message containing an event record with a timestamp
   later than (1) the "stop-time" or (2) the subscription's start time.

nit(?): this text seems to imply that a notification message with a
timestamp later than the "stop-time" might actually be sent, which IIUC
is not the case.

Section 2.9

nit: the name "supports-vrf" for the feature doesn't really parallel the
other feature names, that don't include a word like "support" and rather
just describe the actual feature.

Section 3.1

Is there any risk of collision in event stream names from
vendor-specific streams?  (We don't seem to create an IANA registry
for them, for example.)

Section 4

   identity subscription-suspended-reason {
      description
       "Problem condition communicated to a receiver as part of a
       'subscription-terminated' notification.";
   }

   identity subscription-terminated-reason {
      description
       "Problem condition communicated to a receiver as part of a
       'subscription-terminated' notification.";
   }

Are these descriptions supposed to be the same?

   identity configurable-encoding {
     description
       "If a transport identity derives from this identity, it means
        that it supports configurable encodings.  An example of a [...]

Is it intended that such future transports (or future encodings?) will
derive from both this and the normal base identity (i.e., "transport")?
I guess I'm asking why this identity does not derive from the
corresponding base, but that's just a style question and not really a
protocol matter, making this question a side note.

     leaf weighting {
       if-feature "qos";
       type uint8 {
          range "0 .. 255";
       }
       description
         "Relative weighting for a subscription. Allows an underlying
          transport layer perform informed load balance allocations
          between various subscriptions";
       reference
         "RFC-7540, section 5.3.2";

Do we want to chase the reference for readers and say that larger
weights get more resources?

     leaf encoding {
       when 'not(../transport) or derived-from(../transport,
       "sn:configurable-encoding")';
       type encoding;
       description
         "The type of encoding for notification messages.   For a
         dynamic subscription, if not included as part of an establish-
         subscription RPC, the encoding will be populated with the
         encoding used by that RPC.  For a configured subscription, if
         not explicitly configured the encoding with be the default
         encoding for an underlying transport.";

Where is the default encoding for an underlying transport specified?
Section 5.3 does not seem to say that a transport specification must
provide this information, nor does Section 1.3 (which mentions that
transport specifications must detail the lifecycle of dynamic
subscriptions), nor does Section 2.5.7 (that discusses the need for a
separate model augmenting /subscriptions/subscription/receivers/receiver
to provide transport details).

       choice notification-message-origin {
         if-feature "configured";
         description
           "Identifies the egress interface on the publisher from which
            notification messages are to be sent.";

nit: given the address-originated case, perhaps "the egress interface"
is not quite correct anymore.

               enum connecting {
                 value 3;
                 if-feature "configured";
                 description
                   "A subscription has been configured, but a
                   'subscription-started' subscription state change
                   notification needs to be successfully received before
                   notification messages are sent.

nit: successful receipt happens on the receiver but sending happens on
the publisher, so there's a bit of a mismatch in the sentence subject
here.  Perhaps we could talk about "successfully sent" state-changes to
resolve things.

             config false;
             mandatory true;
             description
               "Specifies the state of a subscription from the
               perspective of a particular receiver.  With this info it
               is possible to determine whether a subscriber is
               currently generating notification messages intended for
               that receiver.";

nit: is it the subscriber that is generating messages or the publisher?

Section 5.3

   A secure transport is highly recommended and the publisher MUST
   ensure that the receiver has sufficient authorization to perform the
   function they are requesting against the specific subset of content
   involved.

"secure transport" usually means that it provides message
confidentiality, integrity protection, and source authenticity (akin to
the mutual authentication).  This is qualitatively different from
implementing authorization checks, and it's surprising to see them
squashed into the same sentence.

Do we want to say anything about discovery for support of new
transports, and what workflow would be used to negotiate the use of a
new transport?

Section 5.4

I can think of a couple other considerations to mention here:

- Using DNS names for receiver "name" lookup can cause situations where
  the name resolves differently on the publisher and subscriber, so the
  recipient would be different than expected.

- Using the publisher's boot time for deduplication protection on
  replayed messages introduces a dependency on accurate time.  We don't
  have a great secure time story at present, so this is more of a
  "beware of risk" situation than something we can mitigate, but it does
  seem that an attacker that could (e.g.) spoof the NTP traffic to the
  publisher on boot could cause it to send duplicate notifications to
  recipients that requested historical data.

Some other comments on what's already there (which is pretty good;
thanks for it!) follow.

   Container: "/filters"

   o  "stream-subtree-filter": updating a filter could increase the
      computational complexity of all referencing subscriptions.

   o  "stream-xpath-filter": updating a filter could increase the
      computational complexity of all referencing subscriptions.

Do we want to say anything about modifying either of these causing the
set of notifications delivered to recipients to shrink (or become
unmanageably large)?  I guess it may not be necessary, since the
recipient gets a notification about the modification that includes the
details of the filter.

   Container: "/subscriptions"

   o  "excluded-event-records": leaf can provide information about
      filtered event records.  A network operator should have
      permissions to know about such filtering.

To be clear, this is event records filtered either via explicit filter
or via access control restrictions.  Specifically, it can allow a
receiver to learn that there exists access controls that deny it access
to some data, in the case when they did not apply any filtering rules
explicitly.  This potential for information leakage (of the existence of
ACLs) should be mentioned explicitly.

Appendix A

This example transport module does not specify the life cycle of dynamic
subscriptions per Section 1.3, and a couple other things required from a
transport module specification.  Perhaps we are okay claiming that since
this is just an example YANG model and not a full transport example, the
omission is okay, but it may be worth mentioning the omission for
clarity.