Subject: [manet] Tsvart last call review of draft-ietf-manet-dlep-pause-extension-06

From: Bob Briscoe via Datatracker <[email-masked]>
Date: Thu, 04 Apr 2019 16:56:32 -0700

Reviewer: Bob Briscoe
Review result: On the Right Track

This document has been reviewed as part of the transport area review team's
ongoing effort to review key IETF documents. These comments were written
primarily for the transport area directors, but are copied to the document's
authors and WG to allow them to address any issues raised and also to the IETF
discussion list for information.

When done at the time of IETF Last Call, the authors should consider this
review as part of the last-call comments they receive. Please always CC
[email-masked] if you reply to or forward this review.

==1. Introduction==

It would be useful to describe the use-case where the modem does not implement
active queue management but the router does, so the modem can use flow control 
to push the queue back into the router, where it can be more intelligently
controlled.

===Scope within Intro===
Please extend the single sentence about scope (end of 2nd para of Intro) to
explain that pause control only applies to data in the router to modem
direction.

Please also mention that the scope of pause-based flow control is limited to
one hop and to a point-to-point link between router and modem, not multipoint.
The modem does not track the source of the data in its queue, so it cannot
focus pause messages onto particular sending stations on a multipoint link, or
onto particular ingress ports of the router.

Why is the scope limited to DLEP radio links? I would have thought this
protocol is generally useful between a and modem and router.

==3. Extension Data Items==

Pls define the direction of the messages:
s/The xxx Data Item is used by a modem to.../
 /A modem sends the xxx Data Item to its peer router to.../

===Unsafe design?===
Is it not good practice to make the data plane robust to unexpected control
plane failures (e.g. key expiry, incorrect or mis-timed change of address,
etc.) and vice versa? So, would it not be more robust for the router to
time-out a pause if no restart appears? Also, if the last message received
before shutting down or suspending was a pause, should the router restart or
resume in the paused state? What if the router enters a power-saving state
after it is paused and misses a restart message?

==Queue size in bytes for informational purposes?==
Why? This strikes me as like one of those Government forms you have to fill in
with an ill-formed question that is mandatory to answer, even though sometimes
there is no answer, and you cannot proceed until you've answered, even though
the answer is not needed anyway. For instance, if there is a shared physical
buffer, a size cannot be straightforwardly given for each logical buffer. So,
if buffer size info is not used by the protocol, do not include it in the
protocol.

On the other hand, how is the threshold at which the modem sends a pause
configured. Is that out of scope? If so, where is it specified? Wherever this
is specified, it should be possible to specify the threshold in time units
(queue delay at the current service rate of the queue), not just in bytes. This
is important for queues in a hierarchy where the service rate varies, e.g.
dependent on traffic in another queue with priority over it. Or simply where
the modem can operate at different rates.

==3.3 Restart==

  "A router which receives the Restart Data Item SHOULD resume
   transmission of the identified traffic to the modem."

Why only SHOULD? Under what conditions would it not?

==4. Security Considerations==

  "The extension does not inherently
   introduce any additional vulnerabilities above those documented in
   [RFC8175]."

Er, no... What about the ability for an off-path attacker to stop the router
sending data!?

The last part about TLS needs to be worded differently. Because of the above
additional vulnerability, the router MUST verify that all 3 types of message
are authenticated by the modem. This requires client authentication mode of
TLS, which is not mentioned in RFC8175, so it needs to be mentioned here.  Or
is the TLS session set up by the router (so the modem is the authenticated
server)? Also this raises the question of key management, if every modem has to
be authenticated by its router.

   "but this is not a
   substantively different attack by such a compromised modem simply
   dropping all traffic destined to, or sent by a router."

Er, no... The modem does not need to be compromised for a 3rd party to send
spoof messages purporting to be from the modem.

==Nits==

1. Intro
"DLEP peers are comprised of a modem and a router" is incorrect English for
what you mean (it means that each peer consists of a modem and a router).
Better to do away with this sentence completely, and alter to the previous
sentence to "It provides the exchange of link related control information
between a modem and its DLEP peer router."

3.1
s/with Queue Index/
 /with a Queue Index/

Scale:
s/An 4-bit/
 /A 4-bit/

In general, I think the term "queue size" is wrongly being used where "buffer
size" should be used (the queue size is the varying size of the queue within
the buffer at any one time).

Also, pls consistently use the term 'paused' not 'suppressed', which has the
potentially ambiguous meaning of 'discarded'.

Delete gratuitous 'is':
  "The motivating use case [is] for this
   data item is when a modem's internal queue length exceeds a
   particular threshold."

CURRENT:
  "e.g., when there
   a non queue related congestion points within a modem, but such are
   not explicitly described in this document."
SUGGESTED:
  "e.g., when there
   are non queue related congestion points within a modem. Such use-cases are
   not explicitly described in this document."

---

From: Lou Berger <[email-masked]>
Date: Wed, 10 Apr 2019 20:17:51 -0400

Bob,

Thanks for the comments - see below for responses.

> to push the queue back into the router, where it can be more intelligently

I guess I'll have to talk to the Shepherd on this one to see how much 
he/the WG would want to add on this at this point.  Perhaps giving a 
reference to the earlier PPPOE (rfc5578) would suffice, what do you think?

> onto particular ingress ports of the router.

Done.

> protocol is generally useful between a and modem and router.

Because this is a DLEP extension.  Other mechanisms exist for other 
technologies, e.g., RFC5578 or Ethernet PAUSE/PFC.

>   /A modem sends the xxx Data Item to its peer router to.../
okay (paraphrase a bit)
> after it is paused and misses a restart message?

I generally agree that a control plane fault should not result in a data 
plane loss -- in some environments, I'd say this is a must.  This said, 
your comment goes to a design principle in DLEP RFC8175 where control 
plane error result in session resets and data plane impacts.  I think 
changing this basic behavior of DLEP is beyond the scope of this 
extension.  I think a general modification of base DLEP to support such 
would be a fine idea.

> if buffer size info is not used by the protocol, do not include it in the
It is largely for reporting to a user/operator for "informational 
purposes".  If an implementation chooses, it can put in zero or 
infinity.  In most cases I understand this will be a straightforward 
value that can be reported to the router and its operators.
> the modem can operate at different rates.

I think a service rate / queue delay property would be very interesting 
- but this didn't come up in the WG, so I don't think it is appropriate 
to add it here.  There is also nothing preventing such information being 
added in a future extension.

> Why only SHOULD? Under what conditions would it not?

if it has no data to send. I don't object changing this to a MUST if you 
think important.

> Er, no... What about the ability for an off-path attacker to stop the router

the same attacker can subvert the dlep session an cause a session reset 
and take down all traffic.  So how is this case different?

> spoof messages purporting to be from the modem.

Fair point - how about:

   Note that this extension does allow a compromised or impersonating
   modem to suppress transmission by the router.  Similar attacks are
   generally possible base DLEP, for example an impersonating modem may
   cause a session reset or a compromised modem simply can
   drop all traffic destined to, or sent by a router.  <xref
   target="RFC8175"/> defines the use of TLS to protect against the
   impersonating attacker.

> between a modem and its DLEP peer router."

sure.

>   /with a Queue Index/

okay

>   /A 4-bit/
okay
> potentially ambiguous meaning of 'discarded'.

will clarify the intent.

>     data item is when a modem's internal queue length exceeds a
yes,
>     not explicitly described in this document."
Done!

Thank you for the comments.

Lou

PS the working document has been updated, if interested see 
https://github.com/louberger/dlep-extensions/tree/master/pause

> _______________________________________________
> manet mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/manet
>

---

From: Justin Dean <[email-masked]>
Date: Wed, 10 Apr 2019 22:25:00 -0400

A few comments inline marked JD, Justin Dean WG chair.

> reference to the earlier PPPOE (rfc5578) would suffice, what do you think?
JD will talk with Lou and respond in separate email.

> extension.  I think a general modification of base DLEP to support such

JD there may be more robust and clever solutions here that don't break the
base specification. Absence of an acknowledgement is not the same as an
error, syntax or otherwise, which causes a reset. Allowing for a periodic
restart message if data does not resume may be sufficient.

> JD I actually think this should be a MAY but with added text explaining
why data may not be sent to the modem. No data, better external links,
other.

---

From: Bob Briscoe <[email-masked]>
Date: Thu, 11 Apr 2019 23:25:09 +0100

Justin, Lou,

Replying to both at once, inline...

On 11/04/2019 03:25, Justin Dean wrote:
> On Wed, Apr 10, 2019, 8:41 PM Lou Berger <[email-masked] 
> <mailto:[email-masked]>> wrote:
>
>     Bob,
>
>     > [email-masked] <mailto:[email-masked]> if you reply to or
>     forward this review.
>     >
>     > ==1. Introduction==
>     >

[BB] What I was looking for was some indication of valid and/or invalid 
use cases. I couldn't find anything about what problem this extension 
solves.

>     technologies, e.g., RFC5578 or Ethernet PAUSE/PFC.
[BB] OK.

> JD there may be more robust and clever solutions here that don't break 
> the base specification. Absence of an acknowledgement is not the same 
> as an error, syntax or otherwise, which causes a reset. Allowing for a 
> periodic restart message if data does not resume may be sufficient.
[BB] I agree that it should be possible to make pause in itself robust.

My point about control plane errors doesn't have to be taken as an 
architectural point. I was merely saying that if restart fails for some 
reason, you don't want data to be paused for ever.

One way to do that is simply to have a globally known constant timeout 
on a pause. Then, if a modem wants the router to pause longer than this 
timeout, it has to repeat the pause before the (globally known) timeout. 
This just ensures that the protocol fails safe.

Design is up to you - you don't have to use my suggestion.

>     value that can be reported to the router and its operators.
[BB] I suggest then that the draft says "A value of zero means unknown 
or unavailable."

>     > configured. Is that out of scope? If so, where is it specified? 
>
[BB] I think this part of the question has been overlooked.

>     Wherever this

[BB] "Didn't come up in the WG" is surely not a valid excuse - that's 
why the IETF does out-of-area reviews, isn't it? Measuring queues in 
time units has become common practice since about 2010, at least in 
transport circles, in order to better handle variable rate links.

>     >     transmission of the identified traffic to the modem.."
>     >
> JD I actually think this should be a MAY but with added text 
> explaining why data may not be sent to the modem. No data, better 
> external links, other.
[BB] All I am highlighting here is that it's best practice to accompany 
a 'SHOULD' with an explanation of the cases where the recommendation 
doesn't apply. And if there are none, then the draft ought to say 
'MUST', which makes for simpler code - fewer exceptions to handle.

Even if there's no data at the moment, restart surely still means resume 
transmission (whenever there is data).

>     and take down all traffic.  So how is this case different?
[BB] Subverting the dlep session is not an additional vulnerability 
introduced by this extension.
Whereas an unauthenticated pause message is an additional vulnerability 
introduced by this extension.

I'm just pointing out that the sentence isn't true.

>        target="RFC8175"/> defines the use of TLS to protect against the
That's better.

But, as I said, I didn't see anything in RFC8175 about using TLS with 
client authentication. That just might mean I didn't find it.

>     https://github.com/louberger/dlep-extensions/tree/master/pause
Lou, Thx

Bob

>     > [email-masked] <mailto:[email-masked]>
> Tsv-art mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/tsv-art

-- 
________________________________________________________________
Bob Briscoe                               http://bobbriscoe.net/