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

["Acee Lindem (acee)" <[e-mail]>]

Alvaro, Naiming,

Changes sound good. See one inline.

From: Alvaro Retana <[e-mail]>
Date: Friday, September 28, 2018 at 5:45 PM
To: "Naiming Shen (naiming)" <[e-mail]>
Cc: "Les Ginsberg (ginsberg)" <[e-mail]>, "[e-mail]" <[e-mail]>, "[e-mail]" <[e-mail]>, "[e-mail]" <[e-mail]>, Christian Hopps <[e-mail]>
Subject: Re: AD Review of draft-ietf-isis-reverse-metric-11
Resent-From: <[e-mail]>
Resent-To: Yingzhen Qu <[e-mail]>, Christian Hopps <[e-mail]>, Acee Lindem <[e-mail]>
Resent-Date: Friday, September 28, 2018 at 5:45 PM

On September 27, 2018 at 6:36:29 PM, Naiming Shen (naiming) ([e-mail]<mailto:[e-mail]>) wrote:

Naiming:

Hi!

Some answers below.  Please take a look and publish an update.

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.

The first MUST (below) sounds as if there must always be one of these sub-TLVs.  However, if other sub-TLVs are defined in the future, and this one is not needed, the MUST would make it mandatory.

OLD>

"This sub-TLV is optional, and MUST appear once at most in the Reverse Metric TLV, otherwise the entire Reverse Metric TLV MUST be ignore on the receive."

NEW>

This sub-TLV is optional, if it appears more than once then the entire Reverse Metric TLV MUST be ignored.



...


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.

The text now reads:

   The Reverse Metric TLV MAY be optionally present in any IS-IS Hello

   PDU. A sender MUST only transmit a single Reverse Metric TLV in a

   IS-IS Hello PDU. If a received IS-IS Hello PDU contains more than

   one Reverse Metric TLV, an implementation MUST ignore all the Reverse

   Metric TLVs.



MAY and optional are the same thing, so the first sentence is redundant.

s/MAY be optionally/MAY

You mean:

            s/MAY be optionally/May be

Thanks,

Acee





...


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

Yeah…that’s the ongoing discussion with the Sec ADs/SecDir: what security guarantees are provided by the “same admin domain”?

Even in the same domain, a router could be subverted and cause problems.  Yes, if that happens then maybe the operators have bigger problems — which probably means that the “same domain” assumptions are not met.

In any case, we can wait for the SecDir/Sec AD to bring up the point before adding text...



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

NEW>

If an implementation enables this mechanism by default, it is RECOMMENDED that it be disabled by the operators when not explicitly using it.