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

From: Benjamin Kaduk via Datatracker <[email-masked]>
Date: Tue, 07 May 2019 13:58:47 -0700

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.

---

From: "Reshad Rahman (rrahman)" <[email-masked]>
Date: Fri, 10 May 2019 21:55:08 +0000

Hi,

Thanks for the review, please see inline.

On 2019-05-07, 4:59 PM, "Benjamin Kaduk via Datatracker" <[email-masked]> wrote:

    

    Section 3.2 states:

<RR> RFC8040 is the RESTCONF RFC and subscribed-notifications is not transport specific. Since this document is for RESTCONF, this is why we have added this statement in this document.

    
<RR> You are correct, this text can be removed.   I can replace it with a reference to the section in subscribed-notifications, would that work for you?

<RR> There are no "long-lived RESTCONF sessions". Terminating (a) does not terminate the subscription.
    
    Section 2
    
    Please use the boilerplate from RFC 8174.
<RR> Ack, will do in next rev.
    
    Section 3

    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.
<RR> The augmentation to subscribed-notifications is mentioned because it is needed for RESTCONF. Are you suggesting we change this to "...accomplished as described in  [I-D.ietf-netconf-yang-push] Section 4.4."?
    
    Section 3.1
    
                                                             Where quick
    
<RR> The subscriber is the one making that determination since this is to recognize the loss of the publisher.
    
    nit: It's interesting that we seem to be using both "subscriber" and
    "receiver" to talk about the TLS client, in the same sentence.
<RR> Will change to subscriber.

    
    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?
<RR> Yes this is the intent since it all depends on what the subscriber needs/wishes.
    
    Section 3.3
    
<RR> Yes this section is for RPC-level errors, it references 2.4.6 of subscribed-notifications which provided a list of possible errors.

    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.
<RR>Good catch,  I think this should be "reason" from yang-push, will check.    

    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.
<RR> I'll move the acronym to the section text

    nit: I might suggest some language about "order within the notifications
    stream" for this state change, but that's purely editorial.
<RR> So you would like this text changed to clarify the order of events?

    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.
<RR> What you describe above is true for netconf (all done on same session. 
However restconf doesn't work the same way (no long-lived sessions), so subsequent RPCs could actually be on a different transport connection.
I do recall the secdir comments and your comments to my response, I will include the text you had suggested to justify the SHOULD.
    
       o  Loss of TLS heartbeat

    Section 9
    
<RR> Section 9 says "The subscription URI is implementation specific ...". I can add text, as you suggested, to recommend that the uri values not be predictable.

    Appendix A
    
<RR> I will add a note.

       {
    
    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?
<RR> You are correct, will change to 10
    
<RR> Correct again.    
    
Regards,
Reshad.

---

From: Benjamin Kaduk <[email-masked]>
Date: Wed, 15 May 2019 12:12:20 -0500

On Fri, May 10, 2019 at 09:55:08PM +0000, Reshad Rahman (rrahman) wrote:
> <RR> RFC8040 is the RESTCONF RFC and subscribed-notifications is not
>   transport specific. Since this document is for RESTCONF, this is why we

Ah, of course; I failed to internalize that RFC 8040 is RESTCONF-specific
in my quick check of the reference.

>   have added this statement in this document.

So I agree that this document is the only reasonable place to convey this
information, but the current presentation still feels rather odd to me.
Just to check my understanding, the situation could be summarized as
"[subscribed-notifications] defines a "streams" container, and 8040
describes a different "streams" container, but if both containers are
present/supported, then anything in the one from [subscribed-notifications]
should also be visible in the one from 8040"?

To me, this has a large component of "document A specifying the interaction
of documents B and C", which normally would require an "Updates"
relationship to either B or C (or, if B or C are not RFCs yet, just moving
the text).  I understand that in this case things are somewhat different,
as document A (i.e., this document) is specifying the transport scheme for
document B (subscribed-notifications) using the protocol from document C.
As such, anything that describes interactions about document B that are
specific to when it is carried over document C must be in this document,
but this specific matter feels less like something about how the protocol
is encoded (i.e., the main focus of this document) and rather an
interaction between protcol features.

I don't have a great proposal for anything specific to change that would
alleviate my concern, though (and as such I will stop holding this as a
Discuss point), though I hope my colleagues on the IESG may have some
additional thoughts.  The best thing I can come up with right now would be
to say:

  In the case when the RESTCONF binding specified by this document is used
  to convey the "streams" container from ietf-restconf-monitoring.yang
  (i.e., that feature is supported), any event streams contained therein
  are also expected to be present in the "streams" container of
  ietf-restconf-monitoring.yang.

Feedback is welcome!

> <RR> You are correct, this text can be removed.   I can replace it with a reference to the section in subscribed-notifications, would that work for you?

That works for me; thanks.

Okay.  This was not clear to me, a non-expert in the field, but if you
think it's clear to the target audience I won't ask for any text changes
to clarify.

> <RR> The augmentation to subscribed-notifications is mentioned because it is needed for RESTCONF. Are you suggesting we change this to "...accomplished as described in  [I-D.ietf-netconf-yang-push]…

That's my suggestion, yes (but feel free to ignore it).

> <RR> The subscriber is the one making that determination since this is to recognize the loss of the publisher.

Okay.  I think that means all the technical pieces work together as needed,
but there could perhaps be more precision in the text about the expected
behavior.  (Note that this is a non-blocking comment, of course.)

> <RR> Yes this is the intent since it all depends on what the subscriber needs/wishes.

It may be worth mentioning explicitly that this is to be
implementation-specific, but it's your call.

> <RR> Yes this section is for RPC-level errors, it references 2.4.6 of subscribed-notifications which provided a list of possible errors.

Thanks for checking.

> <RR>Good catch,  I think this should be "reason" from yang-push, will check.    

Thanks; it's good to know there's probably a reason why I was confused :)

> <RR> So you would like this text changed to clarify the order of events?

If I was writing it, I'd say "allows the receiver to know exactly when,
within the stream of events, the new terms of the subscription have been
applied [...]".  But you're writing it, not me, so my opinion doesn't count
for much.

> I do recall the secdir comments and your comments to my response, I will include the text you had suggested to justify the SHOULD.

Thanks.

> <RR> Section 9 says "The subscription URI is implementation specific ...". I can add text, as you suggested, to recommend that the uri values not be predictable.

Please do -- I think this is easy to do and improves the robustness of the
system, so we should recommend implementations to do it even if there's not
a strict functional requirement to do so.

>     placeholders and not indicative of expected usage.

Thanks!

-Ben

---

From: "Reshad Rahman (rrahman)" <[email-masked]>
Date: Thu, 16 May 2019 16:47:18 +0000

Hi,

Please see inline.

On 2019-05-15, 1:15 PM, "Benjamin Kaduk" <[email-masked]> wrote:

    

    
    
  <RR2> I'm ok with this text, will check with co-authors. 
  
    Feedback is welcome!

    
    
    Okay.  This was not clear to me, a non-expert in the field, but if you
    think it's clear to the target audience I won't ask for any text changes
    to clarify.
<RR2> I'll mull it over, right now I'm leaning towards no additional text since understanding RESTCONF is needed.

    That's my suggestion, yes (but feel free to ignore it).
<RR2> I see value in keeping the augmentation to subscribed-notifications in the text, I'll discuss with co-authors.

    Okay.  I think that means all the technical pieces work together as needed,
    but there could perhaps be more precision in the text about the expected
    behavior.  (Note that this is a non-blocking comment, of course.)
<RR> I will (try to) clarify the text.

    It may be worth mentioning explicitly that this is to be
    implementation-specific, but it's your call.
<RR2> I will mention it explicitly.

    Thanks for checking.

    
    
    If I was writing it, I'd say "allows the receiver to know exactly when,
    within the stream of events, the new terms of the subscription have been
    applied [...]".  But you're writing it, not me, so my opinion doesn't count
    for much.
<RR2> This text does seem clearer.

    Thanks.

<RR2> Will do.

Regards,
Reshad.

    Thanks!
    
    -Ben