Re: [Lsr] AD Review of draft-ietf-isis-reverse-metric-11

[Alvaro Retana <[e-mail]>]

On September 24, 2018 at 9:35:06 PM, Naiming Shen (naiming) (
[e-mail]) wrote:

Naiming:

Hi!

Thank you for your detailed review on this draft, I’ll try to reply to the
comments
inline below, and the proposed document diff is attached at the end. Let me
know
if you would like to have other formats.

In general, the diffs look good to me.  But I do have some more comments
below.

Once we iron those out and you post an update I will start the IETF Last
Call.

Thanks!

Alvaro.


...

Note that I read -11, but -12 was published in the interim -- so I'm
putting this comment up here.  The only change in -12 is the addition (in
the IANA Considerations section) of "a new registry for sub-TLVs of the
Reverse Metric TLV".  Why do we need this new registry?  The description
(in §2) of the use of this sub-TLV already references rfc5305 (where a TE
Default Metric sub-TLV is already defined), so it seemed somewhat natural
to me to simply reuse that sub-TLV here.  From the discussion on the list,
I understand the intent to "future proof", even if no other applications
come to mind.  If that is the path we want to go, then we'll need a
complete description of the new sub-TLV as well. [Some of my comments
bellow assume the existing sub-TLV from rfc5305.]


<NS> With the email exchange from you and Les, I assume I keep the same
as in the version 12 on this sub-TLV registry.

Yes.

However, I need you to add a description of the sub-TLV in this document.
Right now there is a reference to rfc5305, but as Les explained, even
though these sub-TLVs look the same, they are really different.  When doing
so, the references to rfc5305 (for the sub-TLV) would not be needed.



...

172   octet field containing an IS-IS Metric Value, and a 1 octet Traffic
173   Engineering (TE) sub-TLV length field representing the length of a

[minor] Even though it is the only one used, the sub-TLV length field is
not the "Traffic Engineering (TE) sub-TLV length field”.


<NS> Ok, remove the TE sub-TLV wording.

The new text still has the same wording: s/1 octet Traffic Engineering (TE)
sub-TLV length field/1 octet sub-TLV length field


174   variable number of Extended Intermediate System (IS) Reachability
175   sub-TLVs.  If the "sub-TLV len" is non-zero, then the Value field
176   MUST also contain one or more Extended IS Reachability sub-TLVs.

[minor] I'm guessing that by "Extended IS Reachability sub-TLVs" you really
mean the sub-TLVs for the Extended IS Reachability TLV, right?  Please at
least put in a reference to rfc5305.


<NS> Will reference to the rfc5305.

Going back to Les’ explanation of the need for a new registry.  This
document only defined type 18, so in reality the other sub-TLVs from
rfc5305 shouldn’t be used.

Suggestion:

OLD> If the "sub-TLV len” is non-zero, then the Value field MUST also
contain one or more Extended IS Reachability sub-TLVs [RFC5305].

NEW> If the "sub-TLV len” is non-zero, then the Value field MUST also
contain one or more sub-TLVs.


178   The Reverse Metric TLV is optional.  The Reverse Metric TLV may be
179   present in any IS-IS Hello PDU.  A sender MUST only transmit a single
180   Reverse Metric TLV in a IS-IS Hello PDU.  If a received IS-IS Hello
181   PDU contains more than one Reverse Metric TLV, an implementation
182   SHOULD ignore all the Reverse Metric TLVs and treat it as an error
183   condition.

[nit] The first two sentences sound redundant to me.


<NS> OK.


[major] The text above is not specific about which PDUs can include the
Reverse Metric TLV.  The text does say that it is optional and that it may
be in an IIH once...but it doesn't say anything about other PDUs.  The IANA
Considerations section contains the attributes to be included in the
registry, but those are not Normative.


<NS> Added other PDU MUST ignore it.

I saw the new text.  Thanks for being explicit about the TLV being used in
an IIH.

Les also pointed to the following in ISO 10589: “TLVs received in a
non-permitted PDU MUST be ignored.”  That means that the last sentence in
the new text ("If an IS-IS node received of IS-IS Reverse Metric TLV  in
the PDU other than the IS-IS Hello PDU, this TLV MUST be ignored.”) is
redundant.


...

406       When the link TE metric is raised to (2^24 - 1) [RFC5817], either
due
407       to the reverse-metric mechanism or by explicit user configuration,
408       this SHOULD immediately trigger the CSPF re-calculation to move
the
409       TE traffic away from that link.  It is RECOMMENDED also that the
CSPF
410       does the immediate CSPF re-calculation when the TE metric is
raised
411       to (2^24 - 2) to be the last resort link.

[major] These Normative statements are ok, but just for the case where the
metric is raised because of the reverse metric, and not for the explicit
configuration case.  This document is specifying the behavior of the
reverse metric, not the general configuration response.  IOW, if someone
doesn't read this document, then there's no way for the general case to
apply to them -- and the general case (changed config) is not dependent on
implementing the reverse metric.


<NS> That’s a good point. We have been strugling on this point. This is
added due
to some request from the field, and those networks would like to see this
2^^24-1 and
2^^24-2 to be immediately notified and re-evaluate TE paths, instead of
fixed
long interval for re-evaluate. By at least specifying in this document, we
document
this behavior. Now if the CSPF module do immediate or not, does not break
anything.
But the providers will have a place to point to when they try to convince
the
implementation behavior from vendors.

We’ll remove this wording if you insist. Then someone can start a new
document
to specifying such a needed behavior.

I see…

Let’s leave it in — if someone else picks up on it, then let’s take it out.

...

 418     4.  Security Considerations

420       The enhancement in this document makes it possible for one IS-IS
421       router to manipulate the IS-IS default metric and, optionally,
422       Traffic Engineering parameters of adjacent IS-IS neighbors.
Although
423       IS-IS routers within a single Autonomous System nearly always are
424       under the control of a single administrative authority, it is
highly
425       RECOMMENDED that operators configure authentication of IS-IS PDUs
to
426       mitigate use of the Reverse Metric TLV as a potential attack
vector,
427       particularly on multi-access LANs.

[major] Authentication doesn't prevent a subverted router from using the
reverse metric.


<NS> Right. But authentication is all IS-IS has.

:-(


[minor] I would love to see more text on what the threat really is.  I
think that includes being able to divert traffic, sent traffic over less
preferred paths, etc. -- in short, this extension can have a significant
effect on routing decisions.


<NS> Right. But this is no different from any other IS-IS operations, such
as changing local interface metric. reverse-metric
is no worse than those. all of them can change the routing behavior in the
network.

True.  However, this is a new mechanism being defined in this document.  At
least recognizing the potential effect and that it “is no worse” is
important.

Note that the difference between, for example, locally changing the metric
and the reverse-metric is that the local change is like shooting yourself
on the foot.  The reverse-metric allows someone else to influence you.
That’s the reason form my comment below:

 [major] Is there anything (besides authenticating) that can be done to
mitigate the threat?  The text talks about local configuration having the
ability to override a reverse metric -- but it seems like the intent is for
the default to be "accept the reverse metric"..  Making the default be
"don't accept reverse metrics" (i.e. having to explicitly configure the
willingness to accept the new TLV) would help by only allowing the reverse
metric where the operator knows it wants it.  Was there any discussion
about this in the WG?


<NS> I don’t recall there is such a discussion. This document does not
specify which is the default
behavior. An implemetation can use either opt-in or opt-out, which does not
break the
inter-operability.

It would be great if the document took a stance on that.