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

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

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 have added this statement in this document.

    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?

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

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