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

["Eric Voit (evoit)" <[e-mail]>]

Hi Benjamin,

> From: Benjamin Kaduk, April 30, 2019 5:11 PM
> 
> 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 provide more info in-line below with your comment.  But here is a summary...

For dynamic subscriptions, the default encoding is normally specified by whatever is used within the establish-subscription RPC.  So there is not a need for a default here as the subscriber has already encoded in the RPC what it wants.  

For configured subscriptions, the default (and only) encoding is often driven by the transport selection.  And this is defined by existing transport RFCs which themselves define supported encodings (e.g., "NETCONF + XML" , "RESTCONF + JSON").  

But encodings can vary (e.g., the use of CBOR within draft-birkholz-yang-push-coap-problemstatement).  Supporting this need is the purpose of the identity "configurable-encoding" in the YANG model.   And this identity does say that "Further details for any specific configurable encoding would be explored in a transport document based on this specification."  I guess this information could be repeated as a separate requirement in Section 5.3 if you feel this is insufficiently defined.

> 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 for sure like the idea of encryption. And I absolutely like the idea of signatures too.  However several years ago on based on work with MSDC / cloud providers desiring telemetry, there were requests for deployments where the volume of telemetry generated could overwhelm the CPU of the host router.  And as the telemetry was being delivered within a fully protected MSDC domain/environment, these MSDC providers said they wanted *all* the router data more than they needed message confidentiality and integrity protection.  I.e., the wanted to have more data rather than being constrained the host CPU doing functions which reduced the volume of data being delivered.    

So personally, I want low volume telemetry with full security protections applied.  But by leaving it at "highly recommend" we don't unintentionally inhibit applicability to MSDC implementations in a protected domain.  They explicitly said they didn't want it.

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

The reset RPC of section 2.5.5 is YANG model exposed operational knob based on the YANG 1.1 language construct "action":
https://tools.ietf.org/html/rfc7950#section-7.15 

An operational interface can call this action.  I.e., an entity with the proper administrative privileges can access "reset".    This means a REST interface could be exposed upon a controller for that publisher.  And when somebody find that the subscription isn't responsive (for a variety of reasons) that REST interface can be called, and the publisher can start trying to make connections (such as RFC 8071 call home connections) to a receiver.

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

Your functional requirements completely valid.  I believe they are covered by a set of other YANG RFC which dictate who/what can access various elements of YANG data.  Good example documents to look at here are ones like:

RFC-8341 - Network Configuration Access Control Model

An understanding of these documents are assumed, and listed in places like the security considerations in Section 5.4.
 
> 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?

There are many reasons why a filter might fail.  The authors didn't believe that there was enough operational experience to sufficiently document the universe of failure types across the set of interested parties.  As a result, the option seemed to be to provide no guidance on the failure reason, or to let the vendors provide some guidance (albeit just a 'string').  So at this point with the specification, it would be up to the vendor to determine whether it is human readable, or whether internationalization considerations should be covered.  Hopefully with enough deployments this might be revisited at a future date.

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

For the most part it does.  However there is a relationship for exactly when to establish transport connectivity based on the state of each receiver of a configured subscription.

As an example of what should be specified, see Section 5.2 of
https://datatracker.ietf.org/doc/draft-ietf-netconf-netconf-event-notifications/10/
Here you can see how NETCONF call home is invoked at the proper points in the configured subscription state machine.

> nit: please be consistent about "life cycle" vs. "lifecycle" throughout.

Fixed nit to "lifecycle".

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

This is true.  I made the two occurrences of "YANG encoded" into "YANG defined".
 
> 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.

The sentences were already long, it feels to me like it would be more confusing to put in more words here. 

> nit: end sentence with a full stop

Period added.

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

Short answer:  Earlier versions of this draft made explicit parallels of the weighting method to HTTP2 RFC-7450 section 5.3.2.  The operation of weighting is exactly the same HTTP2 dependency weighting.  There is additional definition of how this is supposed to work in Section 4 of draft-ietf-netconf-restconf-notif (which is also currently up for review).

Long answer: TSV-ART is absolutely correct that a publisher might generate a lot of traffic.  In many ways, high traffic volumes would be a successful outcome for this work.   As a result, mitigating different dimensions of this volume risk has been a design goal since this effort’s inception.  And this is the reason robust QoS mechanisms were included.  This includes both DSCP and weighting.

Here is a quick summary of some QoS mechanisms available in this draft...

1. The publisher is allowed to decline a dynamic subscription.  One of these reasons is that the incremental traffic generated by a particular incremental subscription will be too high.  There are errors back to the subscriber indicating this condition exist.
2. A publisher can suspend any subscription for capacity reasons, and a receiver must be able to gracefully accept this suspension. 
3. Much like with HTTP2 streams, higher priority subscriptions intended for a particular receiver can be dequeued first from a publisher. 
4. Much like with HTTP2 streams, proportional subscription dequeuing intended for a particular receiver can be performed a publisher.
5. DSCP markings can be placed on subscribed data.
6. Mechanisms for detecting and reacting to different types of subscribed data loss have been embedded.
7. Methods have been included to ensure a configured receiver “ok’s” information push before subscribed data is sent. (This should reduce one DDoS vector.)
8. Keep alive mechanisms are established for different transport types, so that subscribed data isn’t being sent when the is no receiver listening.

Mechanism (4) is in question here.   As defined by draft-ietf-netconf-restconf-notif, this is supposed to work identically to HTTP2 RFC-7450 section 5.3.2.   However there were WG members who did not want to include a functional requirement normative reference to that HTTP2 transport in this document however.  Therefore the reference to HTP2 was removed.

In the end, I don't think we can afford to get rid of support for (4) from the specification.   This weighting capability is quite powerful, and can easily be added to GRPC based subscription alternatives.

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

Tweaked to "event records generated in the recent past"
 
> 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.)

I expect that it should be clear enough either way.  I believe leaving the current text should not impact implementations.

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

We chose to use a different RPC name so that administrative access control permissions differences between the kill-subscription and delete-subscription would be explicit and obvious.  And once we had a new RPC name, it seemed easier for the reader to continue using the "kill" word for that RPC.

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

Changed to 'variety'.

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

Access denied when an RPC fails access control is part of the base transport error types for the RPC.   I.e., transports like NETCONF and RESTCONF already have non-subscription-specific errors like this one already covered.

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

I thought getting to that level of detail in the intro was confusing for this intro section.
 
> 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)?

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

You are correct that it is not sent.  But the event record does exist on the stream.  

I think it obvious, but if you want I could add the following sentence to the end of the subsequent paragraph.  "If a stop-time has been exceed during replay, no subsequent event records are sent following the sending of a "replay-completed" notification."


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

This is true.  Several years ago when someone proposed this name, there was a reason.  But I can't remember what it is right now.  So hopefully we can leave it so as not to break someone's thinking.

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

In theory yes.  But it has not proven to be an issue with RFC-5277 streams, so it doesn't seem worth it to do something new now.   So in this document, we have one IETF stream "NETCONF" defined in Section 2.1.  Hopefully it is enough to hard-code this.   We can always revisit if IETF groups want to start defining streams

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

Good catch.   Fixed the 'subscription-suspended' description.
 
>    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.

Yes.  Future identities can derive from configurable-encoding

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

I put in "Larger weights get more resources." as sentence two of the description.

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

For many transports, the encoding is redundant information.  The reason is that transport RFCs themselves define supported encodings (e.g., "NETCONF + XML" , "RESTCONF + JSON").  And WG members who built those transport RFCs did not want to allow operators to misconfigure anything here.  (As an aside, this desire to significantly reduce the opportunity for misconfiguration is what drove the interesting 'when' statement above.)

But encodings can vary.  This is the purpose of the identity "configurable-encoding" in the YANG model.   And this identity does say that "Further details for any specific configurable encoding would be explored in a transport document based on this specification."  I guess this information could be repeated in Section 5.3 if necessary.

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

Every alternative I come up with seems more problematic.   I believe readers should be able to understand based on the cases below.
 
>                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.

This wording is as intended.  Basically it is highly desirable for configured receivers will need to acknowledgement of successful receipt of a "subscription-started" before sending event records.  This helps prevent DDOS attacks.  The mechanism for gaining an acknowledgement varies by transport.  As an example of what we have been thinking about here, see

https://datatracker.ietf.org/doc/draft-ietf-netconf-restconf-notif/04/   Section 3.1.2 
Hopefully this type of mechanism will be revived in future drafts.

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

Good catch.  Changed to 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.

Tweaked to "A secure transport is highly recommended.  Beyond this, the publisher MUST".

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

Not at this point.  

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

I tweaked and inserted the two paragraphs from you...

"Using DNS names for configured subscription receiver "name" lookup can cause situations where the name resolves unexpectedly differently on the publisher, 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."


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

Yes, that was the thinking.

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

Yes.  

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

Yes.  

>This potential for information leakage (of the
> existence of
> ACLs) should be mentioned explicitly.

Added the sentence: "Improper configuration could provide a receiver with information leakage consisting of the dropping of event records."


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

I added as the second sentence
" This example is not intended to be a complete transport model."

Thanks again for the review, it was excellently done.

Eric