[manet] Fwd: Re: AD Review of draft-ietf-manet-dlep-pause-extension-04

[Lou Berger <[e-mail]>]

apparently I made someone mad ;-)  See below for original message



-------- Forwarded Message --------
Subject: 	Re: [manet] AD Review of draft-ietf-manet-dlep-pause-extension-04
Date: 	Thu, 07 Mar 2019 18:45:48 -0800
From: 	[e-mail]
To: 	[e-mail]



Message rejected by filter rule match




-------- Forwarded Message --------
Subject:     Re: [manet] AD Review of 
draft-ietf-manet-dlep-pause-extension-04
Date:     Thu, 7 Mar 2019 21:45:37 -0500
From:     Lou Berger <[e-mail]>
To:     Alvaro Retana <[e-mail]>, 
[e-mail] 
<[e-mail]>
CC:     [e-mail] <[e-mail]>, Rick Taylor 
<[e-mail]>, Mobile Ad-hoc Networks Working Group 
<[e-mail]>



Hi Alvaro/Rick,

      Same apologies as the last message, see below.

Also changes described in this message are posted at:

https://github.com/louberger/dlep-extensions/commit/ab094b42b9d41f394ad002cef6058090ccdd9ce8


and a formatted version at:

https://xml2rfc.tools.ietf.org/cgi-bin/xml2rfc.cgi?url=https://raw.githubusercontent.com/louberger/dlep-extensions/master/pause/draft-ietf-manet-dlep-pause-extension.xml


On 2/12/2019 4:12 PM, Alvaro Retana wrote:
 > Ping!
 >
 > Where are we with this document?  I haven’t seen related discussion to
 > Rick’s point either.
 >
 > Alvaro.
 >
 > On November 17, 2018 at 5:13:26 AM, Rick Taylor (
 > [e-mail]) wrote:
 >
 > (Sorry for the top post… damn outlook)
 >
 >
 >
 > Hi Authors,
 >
 >
 >
 > If I remember correctly – and there has been a bit of delay with this
 > draft, with the CCAMP/not-CCAMP move – the intention of the authors 
was to
 > make the pause extension simple and self-contained, avoiding getting 
caught
 > up in the DiffServ Aware/Credit Window/Sub-TLV draft reference
 > reorganization that has happened.
 >
 >
 >
 > If this is still the intention, then I agree with Alvaro that trying to
 > define generic Sub-TLVs in this document is unnecessary, and should be
 > removed.
 >
The  Queue Parameters Data Item needs a list of objects semantic -- 
having a consistent way of doing this facilitates reusable code.  I 
don't see any downside of the current definition.


 >
 > However, given the DiffServ Aware/CW drafts have almost caught up 
with the
 > Pause extension, do the authors/WG want to rethink their intention?

I think the current approach of having a DI-specific sub-DI format that 
happens to be common works just fine.  It's also cleaner that having a 
single sub-DI registry and track which sub-DIs are valid for which DIs.

David has even shown that this works fine in code:

https://github.com/mit-ll/LL-DLEP/commit/283b2f7618aeee1fcec606fdc7b4f659f300caaf

so unless there's a strong reason to go back on the conclusions we came 
up with (in the WG) before, I don't think we should revisit the current 
sub-DI approach documented in the different drafts.

 >
 >
 > Cheers,
 >
 >
 >
 > Rick
 >
 >
 >
 > *From:* manet [mailto:[e-mail]] *On Behalf Of *Alvaro 
Retana
 > *Sent:* 16 November 2018 22:00
 > *To:*[e-mail]
 > *Cc:*[e-mail]; Mobile Ad-hoc Networks Working Group
 > *Subject:* [manet] AD Review of draft-ietf-manet-dlep-pause-extension-04
 >
 >
 >
 > Dear authors:
 >
 >
 >
 > I just finished reviewing this document.  I have several comments (see
 > below) and a couple of significant issues, which I'm mentioning in 
the next
 > 2 bullets.  I will wait for these to be addressed before starting the 
IETF
 > LC.
 >
 >
 >
 > (1) This is a comment for the WG (not just the authors):
 >
 >
 >
 > This is the first draft that specifies a Sub Data Item. Given that 
rfc8175
 > says that the Value in a Data Item "contains data specific to a 
particular
 > Data Item", I don't think it is necessary to define a generic DLEP 
Sub Data
 > Item..  However, not all future extensions may follow the same model.  Is
 > there a need/interest in normatively specifying the characteristics of a
 > Sub Data Item?

I believe we went back and forth on this as a WG (although I'll have to 
dig to find the discussion)  and agreed to not do this. At one time we 
discussed having a common sub-DI tyoe vale space for all DIs, I believe 
we agreed on having a common TLV format that is unique to each DI that 
wants to use a sub-DI.  This translates to DI unique sub-DI type 
namespace and allowing for future DIs to choose if they use the common 
sub-DI format or some other sub-di format.


 >
 >
 >
 > (2) Security Considerations:  The extension in itself doesn't change the
 > security characteristics of DLEP, I agree with that. However, I think 
that
 > the functionality does -- please see specific comments below.
 >
 >
 >
 > Thanks!
 >
 >
 >
 > Alvaro.
 >
 >
 >
 >
 >
 > [Line numbers from idnits.]
 >
 >
 >
 > ....
 >
 > 70       1.  Introduction
 >
 > .....
 >
 > 78         The base DLEP specification does not include any data 
plane flow
 >
 > 79         control capability.  Various flow control methods are 
possible,
 > e.g.,
 >
 > 80         see [I-D.ietf-manet-credit-window].  The extension defined in
 > this
 >
 > ....
 >
 >
 >
 > [minor] A reference to something other than I-D.ietf-manet-credit-window
 > (expired, etc.) may be more appropriate.
 >
done.


 >
 >
 >
 > ....
 >
 > 103     2.  Extension Usage and Identification
 >
 >
 >
 > 105       The use of the Control Plane Based Pause Extension SHOULD be
 >
 > 106       configurable.  To indicate that the Control Plane Based Pause
 >
 > 107       Extension is to be used, an implementation MUST include the
 > Control
 >
 > 108       Plane Based Pause Extension Type Value in the Extensions 
Supported
 >
 > 109       Data Item.  The Extensions Supported Data Item is sent and
 > processed
 >
 > 110       according to [RFC8175].
 >
 >
 >
 > [minor] "The use of the Control Plane Based Pause Extension SHOULD be
 > configurable."  Is there a default recommended configuration?

I think this is implementation specific, e.g., if more than one FC 
mechanisms is implemented the vendor may choose which is preferred.


 >
 >
 >
 >
 > ....
 >
 > 201     3.1.1.  Queue Parameter Sub Data Item
 >
 > ....
 >
 > 234       Sub Data Item Type:
 >
 >
 >
 > 236          A 16-bit unsigned integer that indicates the type and
 >
 > 237          corresponding format of the Sub Data Item's Value 
field.  Sub
 > Data
 >
 > 238          Item Types are scoped within the Data Item in which they are
 >
 > 239          carried, i.e., the Sub Data Item Type field MUST be used
 > together
 >
 > 240          with the Data Item Type to identify the format of the 
Sub Data
 >
 > 241          Item.  This field MUST be set to one (1) for the Queue
 > Parameter
 >
 > 242          Sub Data Item.
 >
 >
 >
 > [minor] "...the Sub Data Item Type field MUST be used together with the
 > Data Item Type to identify the format of the Sub Data Item." It sounds as
 > if this text wants to specify the behavior for *any* Sub Data Item Type.
 > Please reword to be specific in to this document.  [Also, see my note at
 > the top.]
sure.
 >
 >
 > [major] Please define a registry for the Sub Data Item Types to be used
 > with the Queue Parameters Data Item.
 >
okay.


 >
 > 244       Length:  Variable
 >
 >
 >
 > 246          Copying [RFC8175], Length is the number of octets in the sub
 > data

 > 247          item, excluding the Type and Length fields.
 >
 >
 >
 > [minor] Copying?  As far as I can tell, rfc8175 doesn't define any 
sub data
 > items.
 >
sure. Dropped Copying [RFC8175]
 >
 > 249       Queue Index:
 >
 >
 >
 > 251          An 8-bit field indicating the queue index of the queue
 > parameter
 >
 > 252          represented in the sub data item.  Only the first 
instance a a
 >
 > 253          particular Queue Index value is meaningful. Subsequent sub
 > data
 >
 > 254          items containing the same Queue Index values, if 
present, MAY
 > be
 >
 > 255          logged via a management interface and MUST otherwise be
 > ignored.
 >
 >
 >
 > [nit] s/a a/of a
 >
Thanks!
 >
 > [minor] Pause and Restart reserve the Queue Index of 255 to indicate "all
 > traffic", but that value is not reserved here.  Should it?
 >
I think this is covered by Num Queues = 1 and thereby no Queue Parameter 
Sub Data Item carried in the DI.

 >
 > ...
 > 269  DS Field Qn:
 >
 > 271     The data item contains a sequence of 8 bit DS Fields.  The
 > 272     position in the sequence identifies the associated queue index.
 > 273     The number of DS Fields present should equal the sum of all Num
 > 274     DSCPs field values.
 >
 > [major] "should equal", or "MUST equal"?  If the number of DS Fields
 > doesn't add up, should the sub data item be considered invalid?  I
 > assume that would invalidate the whole data item.
s/should equal/MUST
 >
 >
 > ...
 > 2863.2. Pause
 > ...
 > 293  A modem may indicate that traffic is to be suppressed on a device
 > 294  wide or destination specific basis.  An example of when a modem 
might
 > 295  use device wide indications is when output queues are shared across
 > 296  all destinations, and destination specific might be used when per
 > 297  destination queuing is used.  To indicate that suppression 
applies to
 > 298  all destinations, a modem MAY send the Pause Data Item in a Session
 > 299  Update Message.  To indicate that suppression applies to a 
particular
 > 300  destination a modem MAY send the Pause Data Item in a Destination
 > 301  Update Message.
 >
 > [major] I think that the two MAYs above are out of place.  I
 > understand that sending the Pause Data Item in the corresponding
 > Update Message is optional, but the sentences say that "To indicate
 > that suppression applies...", which means that if the Pause Data Item
 > is not sent, then there is no indication...so sending it is not optional.
 >
okay s/MAY/MUST * 2
 >
 > ...
 > 312  A router which receives the Pause Data Item MUST cease sending the
 > 313  identified traffic to the modem.  This may of course translate into
 > 314  the router's queues exceeding their own thresholds.  If a received
 > 315  Pause Data Item contains a Queue Index value other than 0, 255, or a
 > 316  queue index established by a Session Initialization or Session 
Update
 > 317  Message, the router MUST terminate the session with a Status Data
 > 318  Item indicating Invalid Data.
 >
 > [major] Terminating the session seems drastic to me given that the
 > wrong index relates to a single destination and the action has an
 > impact over all.

This is how DLEP handles errors in general.  I'm not sure that else 
should be done. Do you have an suggested alternative?


 >
 >
 > ...
 > 3473.3. Restart
 > ...
 > 354  The sending of this data item parallels the Pause Data Item, see the
 > 355  previous section, and follows the same rules.  This includes that to
 > 356  indicate that transmission can resume to all destinations, a modem
 > 357  MAY send the Restart Data Item in a Session Update Message. It also
 > 358  includes that to indicate that transmission can resume to a
 > 359  particular destination a modem MAY send the Pause Restart Item in a
 > 360  Destination Update Message.  Finally, the same rules apply to queue
 > 361  indexes.
 >
 > [major] Same comment as above about the MAYs.
 >
same response: s/MAY/MUST * 2
 > 363  A router which receives the Restart Data Item SHOULD resume
 > 364  transmission of the identified traffic to the modem.
 >
 > [minor] Should the Restart always follow a Pause?  I think that is the
 > intent.  The question is whether it MUST happen that way?  As is, the
 > text leaves the possibility open for a modem to send a Restart without
 > having sent a Pause.  I think that is ok, just checking...
 >
 >
I think leaving as is is more "liberal in what you accept"


 > ...
 > 3844. Security Considerations
 >
 > 386  The extension introduces a new mechanism for flow control between a
 > 387  router and modem using the DLEP protocol.  The extension does not
 > 388  inherently introduce any additional threats above those 
documented in
 > 389  [RFC8175].  The approach taken to Security in that document applies
 > 390  equally when running the extension defined in this document.
 >
 > [major] The extension gives the modem the ability to stop the traffic
 > sent by a router..  A rogue or compromised modem could stop traffic
 > sent by the attached router.  Protecting the modem is out of scope,
 > but I think the threat should at least be pointed out.
 >
A rogue modem can just drop all the traffic too, is this really 
substantive new vulnerability -  but sure, I can add a sentence.
 > rfc8175 already mentions the risk that "an attacker might pretend to
 > be a DLEP participant...by injection of DLEP Messages once a session
 > has been established", which is related to this case. However, I
 > think that the point still needs to be called out specifically because
 > of the new functionality introduced.
 >
 >
Added


<t>
    Note that this extension does allow a compromised or impersonating
    modem to suppress transmission by the router, but this is not a
    substantively different threat by such a compromised modem simply
    dropping all traffic destined to, or sent by a router. <xref
    target="RFC8175"/> defines the use of TLS to protect against the
    impersonating attacker.
</t>

 > ...
 > 466Appendix A.  Acknowledgments
 >
 > 468  The sub data item format was inspired by Rick Taylor's "Data Item
 > 469  Containers".
 >
 > [nit] Is this a draft?  Reference?

I think just an idea/ discussion.

Thanks again for the comments -- once we resolve these I'll push a new 
version.  (I'll also push one before the monday cutoff no matter what.)

Lou