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

["Reshad Rahman (rrahman)" <[e-mail]>]

Hi,

Please see inline.

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

    On Fri, May 10, 2019 at 09:55:08PM +0000, Reshad Rahman (rrahman) wrote:
    > Hi,
    > 
    > Thanks for the review, please see inline.
    > 
    > On 2019-05-07, 4:59 PM, "Benjamin Kaduk via Datatracker" <[e-mail]> wrote:
    > 
    >     Benjamin Kaduk has entered the following ballot position for
    >     draft-ietf-netconf-restconf-notif-13: Discuss
    >         
    >     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.
    > <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.
  <RR2> I'm ok with this text, will check with co-authors. 
  
    Feedback is welcome!
    
    >     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.)
    > <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.
    
    >     
    >     ----------------------------------------------------------------------
    >     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)?
    > <RR> There are no "long-lived RESTCONF sessions". Terminating (a) does not terminate the subscription.
    
    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.
    
    >     Section 2
    >     
    >     Please use the boilerplate from RFC 8174.
    > <RR> Ack, will do in next rev.
    >     
    >     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.
    > <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."?
    
    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.
    
    >     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?
    > <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> I will (try to) clarify the text.
    
    >     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.
    >     
    >     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?
    > <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.
<RR2> I will mention it explicitly.
    
    >     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.)
    > <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.
    
    >           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.
    > <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 :)
    
    >     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
    >     
    >        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.
    > <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.
<RR2> This text does seem clearer.
    
    >        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.
    > <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.
    
    Thanks.
    
    >        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.)
    > <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.
<RR2> Will do.

Regards,
Reshad.
    
    >     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.
    > <RR> I will add a note.
    
    Thanks!
    
    -Ben
    
    >     (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?
    > <RR> You are correct, will change to 10
    >     
    >     Similarly, subscription IDs are uint32, which IIUC gets encoded as a
    >     number.
    > <RR> Correct again.    
    >     
    > Regards,
    > Reshad.
    >