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

["Naiming Shen (naiming)" <[e-mail]>]

Alvaro,

Thanks again. more inline in replies. The new diff from the last is attached.

On Sep 26, 2018, at 1:10 PM, Alvaro Retana <[e-mail]<mailto:[e-mail]>> wrote:

On September 24, 2018 at 9:35:06 PM, Naiming Shen (naiming) ([e-mail]<mailto:[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.


<NS> ok, modified. see diff.


...

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


<NS> ok, missed that.

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.


<NS> done.

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.


<NS> ok, removed:-) but it used to be “.. may be optional present in …”,
changed to ".. MAY be optional present in …”, the formally define into
Hello PDU.

...

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.

<NS> ok. thanks.


...
 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:


<NS> Make sense, but I think one of the main reason of this draft is to simplify
the operation of maintenance. If an operator (within the same local domain) not
only needs to raise a ‘reverse metric’ on the local device, but also to hunt down
all the other devices on the same LAN and to enable this feature one by one, that
would defeats the purpose of this important usage.

Even though this is sent over to neighbor to act, but most provider network those
devices are under the same admin domain.

 [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.

<NS> although I would certainly want this mechanism to be enabled by default,
but I don’t want to express this in this document since there may generate some
security concern.

added in the “operational consideration” section that:

It is RECOMMENDED that the network operators disable the capability
                        when this Reverse Metric feature or mechanism is not being used in
                        the network where in the case an IS-IS implementation has this
                        mechanism enabled by default.

to see if you are ok with that.

Regards,
- Naiming