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

From: Alvaro Retana <[email-masked]>
Date: Thu, 6 Sep 2018 18:47:43 +0200

On August 31, 2018 at 5:13:20 PM, Les Ginsberg (ginsberg) (
[email-masked]) wrote:

Les:

Hi!

I am not an author of this draft – but there are a couple of points that I
want to respond to in my role as Designated Expert for the IS-IS IANA
Codepoint Registries.

...

From: Lsr <[email-masked]> On Behalf Of Alvaro Retana
Sent: Friday, August 31, 2018 12:27 PM
To: [email-masked]
Cc: [email-masked]; Christian Hopps <[email-masked]>; [email-masked]
Subject: [Lsr] AD Review of draft-ietf-isis-reverse-metric-11

...

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

[Les:] The sub-TLV used matches the content of
https://www.iana.org/assignments/isis-tlv-codepoints/isis-tlv-codepoints.xhtml#isis-tlv-codepoints-22-23-25-141-222-223
 sub-TLV 18.

However, the definition in this registry cannot be used to define the
sub-TLV when sent in the Reverse Metric TLV unless we were willing to say
that the above registry also applies to the Reverse-Metric TLV. As
Designated Expert I would strongly object to such a suggestion as it is
clear that no other sub-TLV in this registry is applicable to the Reverse
Metric TLV.

That explanation works for me.

...

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.

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

[Les:] I strongly disagree. The permitted PDUs columns in the IANA registry
are intended to be normative. And the normative behavior (derived from ISO
10589 specification) is:

“TLVs received in a non-permitted PDU MUST be ignored.”

First of all, the IANA registries are not Normative, regardless of the
intention.  IANA basically coordinates the assignment of protocol
parameters [rfc2860] [rfc8126].  Note that guidance from IANA itself about
what should not be in IANA Considerations (and therefore not in the
registries) includes "Descriptions of how registered values should/should
not be used”. [1]

My point with this document is that in the section that describe the use of
the new TLV (§2, in this case) is not specific about which PDUs can contain
the TLV.  The text above just says "TLV may be present in any IS-IS Hello
PDU”.  I’m specifically looking for a sentence that says (something to the
effect of): “This TLV is only permitted in an IIH.”   Then the behavior
from 10589 kicks in because we clearly know which are the non-permitted
PDUs.

[1] https://www.iana.org/help/protocol-registration

(There is one exception to that related to LSP purges – but that is
explicitly stated in https://tools.ietf.org/html/rfc6233 )

You and I have discussed this privately. If you feel that this behavior is
insufficiently specified then let’s please fix it by adding normative
language to
https://www.iana.org/assignments/isis-tlv-codepoints/isis-tlv-codepoints.xhtml
 .

The registry is authoritative about the assignments, but it is not
Normative as to the behavior of (in this case) the TLVs that use those
assignments.

Requiring such specification in individual RFCs would mean that we would
have to generate bis versions of any existing RFC that defines a TLV
codepoint.

Requiring an RFC to document the specific behavior of an extension is not a
new requirement.

If we deem it necessary, I volunteer to do what is necessary to get the
registry updated appropriately.

Instead of trying to describe behavior in the registry, or generating bis
versions, an easier move might be to Update the existing RFCs with a single
document.  To be clear, none of that is a requirement to move this document
forward…which is my immediate concern.

Thanks!

Alvaro.

---

From: "Naiming Shen (naiming)" <[email-masked]>
Date: Tue, 25 Sep 2018 01:35:04 +0000

Hi Alvaro,

Sorry for the late response.

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.

Many thanks.
- Naiming

On Aug 31, 2018, at 12:27 PM, Alvaro Retana <[email-masked]<mailto:[email-masked]>> wrote:

Dear authors:

I just finished reading this document.  Thanks for the work on it!

I have several concerns (please see detailed comments below), and think the
draft still needs work.  Among other things, my main concerns include:

- the lack of an explicit definition of the reverse metric; examples and
explanations illustrate, but there is no single overview definition that
clearly (and early) talks about the offset...

- the new TLV is underspecified and there are error conditions that should
be addressed

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

Thanks!

Alvaro.

[Line numbers came from the idnits output.]

...
10                   IS-IS Routing with Reverse Metric
11                   draft-ietf-isis-reverse-metric-11

...
169 2.  IS-IS Reverse Metric TLV

[minor] This section would read better if you first introduced the TLV
(what is it for...), presented the figure and finally described the
fields.  For example, as written, the Flags field is first mentioned, then
shown in the figure, then the length is mentioned again under it, then a
new figure shows the Flags, and finally (after first talking about the
Metric) the specific flags are defined.  It is disjoint and feels messy.

<NS> Good point, will re-organize that.

171   The Reverse Metric TLV is composed of a 1 octet field of Flags, a 3

[nit] Strictly speaking, the TLV is composed of a Type, Length, etc..

<NS> Ok. Use the “The Value part of the … TLV”.

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.

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.

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.

<NS> OK.

<NS> Added other PDU MUST ignore it.

[major] What should happen if this TLV is included in a different PDU?

<NS> MUST be ignored, added in the text.

[major] Under what conditions may the receiver *not* ignore all the Reverse
Metric TLVs?  If not ignoring them, which one should it use?  IOW, why is
it a "SHOULD" and not a "MUST”?

<NS> Changed to MUST from SHOULD.

[major] What should the receiver do in response to the "error condition"?
Should it just ignore the TLVs?  Should it ignore the whole IIH?  Should it
restart the adjacency?  Nothing?  Something else?  Maybe log the error…

<NS> It says to ignore the TLV. “error condition” is a operational management
terminology. removed the “error condition” wording. it’s up to the
implementation to log or put in debug messages, etc.

185       0                   1                   2                   3
186       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
187       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
188       |      Type     |     Length    |    Flags      |     Metric
189       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
190             Metric  (Continue)        | sub-TLV Len   |Optional sub-TLV
191       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

193                            Reverse Metric TLV

[nit] Please add a Figure number.

<NS> Ok.

195      TYPE: TBD (be replaced by the value that IANA allocates)
196      LENGTH: variable (5 - 255 octets)
197      VALUE:

199         Flags (1 octet)
200         Metric (3 octets)
201         sub-TLV length (1 octet)
202         sub-TLV data (0 - 250 octets)

204          0 1 2 3 4 5 6 7
205         +-+-+-+-+-+-+-+-+
206         |  Reserved |U|W|
207         +-+-+-+-+-+-+-+-+

[major] Can the other Flag bits be assigned in the future?  If so, then
they should be marked as Unassigned and a registry should be created.

<NS> I saw Chris’s email on this issue, I assume we have done this way often.

209                              Figure 1: Flags

211   The Metric field contains a 24-bit unsigned integer.  This value is a
212   metric offset that a neighbor SHOULD add to the existing, configured
213   "default metric" for the IS-IS link.  Refer to "Elements of
214   Procedure", in Section 3 for details on how an IS-IS router should
215   process the Metric field in a Reverse Metric TLV.

[nit] I think that the term "default metric" may cause confusion to the
casual reader, specially because it sometimes used inside quotation marks.
I know this comes from 10589.  It may be a good idea to either include a
reference to 10589 when it is first mentioned, or to distinguish it somehow
(Default Metric -- with caps), or both.  At minimum, please be consistent
about the use of quotations marks.

<NS> Good point. I agree and will normalize them to be consistent.

...
240   The Reverse Metric TLV can include sub-TLVs when an IS-IS router
241   wishes to signal to its neighbor to raise its Traffic Engineering
242   (TE) Metric over the link.  In this document, only the "Traffic
243   Engineering Default Metric" sub-TLV [RFC5305], sub-TLV Type 18, is
244   defined and MAY be included in the Reverse Metric TLV, because that
245   is a similar 'reverse metric' operation to be used in TE

[nit] The "Traffic Engineering Default Metric" sub-TLV is not defined in
this document.  I think you mean that this document only defines the use of
that sub-TLV with the Reverse Metric TLV. See my suggestion in the next
item.

[minor] The Normative language feels a little clunky to me...I think you
may want to change it's location: (suggestion) s/The Reverse Metric TLV can
include sub-TLVs...sub-TLV Type 18, is defined and MAY be included in the
Reverse Metric TLV.../The Reverse Metric TLV MAY include sub-TLVs...sub-TLV
Type 18, is defined to be included in the Reverse Metric TLV…

<NS> Ok, will change to make this clear.

246   computations.  Upon receiving this TE METRIC sub-TLV in a Reverse
247   Metric TLV, a node SHOULD add the received TE metric offset value to
248   its existing, configured TE default metric within its Extended IS
249   Reachability TLV.  Use of other sub-TLVs is outside the scope of this
250   document.  The "sub-TLV Len" value MUST be set to zero when an IS-IS
251   router does not have TE sub-TLVs that it wishes to send to its IS-IS
252   neighbor.

[minor] Please be consistent with the names used.  For example, the Traffic
Engineering Default Metric sub-TLV (from rfc5305) is mentioned by that
name, but also by "TE METRIC sub-TLV" and "TE Default Metric sub-TLV".
This last one may be close, but given that rfc5305 doesn't even use that
name, at least expanding TE would help.

<NS> Ok, will use the consistent longer version.

254 3.  Elements of Procedure

256 3.1.  Processing Changes to Default Metric

258   The Metric field, in the Reverse Metric TLV, is a "reverse offset
259   metric" that will either be in the range of 0 - 63 when a "narrow"
260   IS-IS metric is used (IS Neighbors TLV, Pseudonode LSP) [RFC1195] or
261   in the range of 0 - (2^24 - 2) when a "wide" Traffic Engineering
262   metric value is used, (Extended IS Reachability TLV) [RFC5305]

[minor] It seems to me like this description might fit better where there
field is being described, in §2.

<NS> Ok, will move over.

263   [RFC5817].  It is important to use the same IS-IS metric mode on both
264   ends of the link.  On the receiving side of the 'reverse-metric' TLV,

[minor] Why is that reference to rfc5817 there?  It seems superfluous to me

<NS> I think the RFC5305 defines the operation of 2^^24-1 for “Default Metric”,
but it didn’t say anything on the TE default metric of that value. RFC5817
has the value specified.

[nit] I'm sure you mean the "same metric type" (not just the same metric) on both sides of the link...

[major] What happens if the metric type is not the same on both ends?  Should there be some Normative language there?

<NS> It’s an operational error in the first place, it’s not just reverse metric will be wrong.
I’m adding “, and in the entire IS-IS area or level”.

265       the accumulated value of configured metric and the reverse-metric
266       needs to be limited to 63 in "narrow" metric mode and to (2^24 - 2)
267       in "wide" metric mode.  This applies to both the default metric of
268       Extended IS Reachability TLV and the TE default-metric sub-TLV in LSP

[major] So...when offsetting the Default Metric the local router shouldn't go above 63 when using narrow metics.  Assuming narrow metrics...  Would receiving a reverse metric > 63 be an indication what the sender may not be using narrow metrics, or that at least something went wrong?  Should anything be done about that?

<NS> I think we don’t support this mix type mode, not for IS-IS in general.
But your example is only happens in one case of mixed model. I don’t
think inside reverse metric mechanism, we will try to define the ‘detection’ of that.

269       or Pseudonode LSP for the "wide" metric mode case.  If the "U" bit is
270       present in the flags, the accumulated metric value is to be limited
271       to (2^24 - 1) for both the normal link metric and TE metric in IS-IS
272       "wide" metric mode.

...
297     3.3.  Multi-Access LAN Procedures

299       On a Multi-Access LAN, only the DIS SHOULD act upon information
300       contained in a received Reverse Metric TLV.  All non-DIS nodes MUST
301       silently ignore a received Reverse Metric TLV.  The decision process
302       of the routers on the LAN MUST follow the procedure in section
303       7.2.8.2 of [ISO10589], and use the "Two-way connectivity check"
304       during the topology and route calculation.

[minor] This last sentence (about 10589), while not wrong, seems unnecessary.  There's nothing special about the processing of the reverse metric in LANs that makes calling out the 2-way connectivity check necessary.  Is there?  Is there a reason it is not called out for p2p links?  Should a general statement about only applying the metric after the 2-way check is performed be put somewhere?  Am I missing something?

<NS> Originally, there is no such a mention of that. Due to the mailing list had
a comment a while ago that on the LAN, only a subset of nodes supporting
this ‘reverse-metric’. The comments saying the result of SPF would be
inconsistent. And Les and I pointed out that, the “Two-way connectivity check”
had to be followed, and it would be consistent. I’m happy to remove to that
reference of 10589 here if we think it’s ok.

306       The Reverse Metric TE sub-TLV also applies to the DIS.  If a DIS is
307       configured to apply TE over a link and it receives TE metric sub-TLV
308       in a Reverse Metric TLV, it should update the TE Default Metric sub-
309       TLV value of the corresponding Extended IS Reachability TLV or insert
310       a new one if not present.

312       In the case of multi-access LANs, the "W" Flags bit is used to signal
313       from a non-DIS to the DIS whether to change the metric and,
314       optionally Traffic Engineering parameters for all nodes in the
315       Pseudonode LSP or solely the node on the LAN originating the Reverse
316       Metric TLV.

318       A non-DIS node, e.g., Router B, attached to a multi-access LAN will
319       send the DIS a Reverse Metric TLV with the W bit clear when Router B
320       wishes the DIS to add the Metric value to the default metric
321       contained in the Pseudonode LSP specific to just Router B.  Other
322       non-DIS nodes, e.g., Routers C and D, may simultaneously send a
323       Reverse Metric TLV with the W bit clear to request the DIS to add
324       their own Metric value to their default metric contained in the
325       Pseudonode LSP.  When the DIS receives a properly formatted Reverse
326       Metric TLV with the W bit clear, the DIS MUST only add the default
327       metric contained in its Pseudonode LSP for the specific neighbor that
328       sent the corresponding Reverse Metric TLV.

330       As long as at least one IS-IS node on the LAN sending the signal to
331       DIS with the W bit set, the DIS would add the metric value in the
332       Reverse Metric TLV to all neighbor adjacencies in the Pseudonode LSP,
333       regardless if some of the nodes on the LAN advertise the Reverse
334       Metric TLV without the W bit set.  The DIS MUST use the reverse

[major] This first sentence clearly says that the metric from a node with W set is used in all adjacencies, regardless of what other nodes (not setting the W bit) might say...which contradicts the paragraph right before it.

<NS> if you refer to the line 325 sentence above, then it says with the W bit “clear”.
since the whole paragraph is about no one is setting the W bit. but the last
sentence of that paragraph does not need to be there.

335       metric of the highest source MAC address Non-DIS advertising the
336       Reverse Metric TLV with the W bit set.  The DIS MUST use the metric
337       value towards the nodes which explicitly advertise the Reverse Metric
338       TLV.

[major] I don't understand what the last sentence is specifying.  All the nodes using the new TLV are explicitly advertising it…

<NS> Will remove this sentence, not sure why it was there.

340       Local provisioning on the DIS to adjust the default metric(s)
341       contained in the Pseudonode LSP MUST take precedence over received
342       Reverse Metric TLVs.  For instance, local policy on the DIS may be
343       provisioned to ignore the W bit signaling on a LAN.

[major] Should an operator be able to configure an adjusted metric to be used only if the Reverse Metric TLV is not received?  In other words, that case wouldn't be able to conform to the MUST.

<NS> Will add the ‘non-dynamic’ wording into that.

[major] The MUST above is in conflict with the text in §3.6 which says that "it is RECOMMENDED that implementations provide a capability to disable any changes".  (1) RECOMMENDED is the same as SHOULD, (2) disabling the change is equivalent to the local configuration taking precedence.

<NS> Ok, for Pnode, it’s a good idea to statically configure a ‘reverse-metric’; but
in general, a node can benefit from configuration of “ignoring” any ‘reverse-metric’
dynamic signaling (not to ignore local configuration of course).

Will add the words in the RECOMMENDED that sentence to highlight the
ignoring the hello sent ‘reverse-metric’ by policy.

345       Multi-Topology IS-IS [RFC5120] specifies there is no change to
346       construction of the Pseudonode LSP, regardless of the Multi-Topology
347       capabilities of a multi-access LAN.  If any MT capable node on the
348       LAN advertises the Reverse Metric TLV to the DIS, the DIS should
349       update, as appropriate, the default metric contained in the
350       Pseudonode LSP.  If the DIS updates the default metric in and floods
351       a new Pseudonode LSP, those default metric values will be applied to
352       all topologies during Multi-Topology SPF calculations.

354     3.4.  Point-To-Point Link Procedures

356       On a point-to-point link, there is already a "configured" IS-IS
357       interface metric to be applied over the link towards the IS-IS
358       neighbor.

[minor] Why is "configured" in quotation marks?  Is that different than configured (without quotations)?  Is this "configured" metric the same as the configured "default metric" mentioned in §2?

<NS> yes. removing quotes.

360       When IS-IS receives the IIH PDU with the "Reverse Metric" on a point-
361       to-point link and if the local policy allows the supporting of
362       "Reverse Metric", it MUST add the metric value in "reverse metric"
363       TLV according to the rules described in Section 3.1 and Section 3.2.

[major] Is there a reason for local policy only being mentioned in the p2p case?  I'm sure an operator may want to also not allow (using local policy) the use of the reverse metric in a LAN.  Should this indication that local policy may override any on-the-wire signaling be moved to a more general place in the document?

<NS> I think the document is talking about “allow/not-allow”.
ok. it’s a good idea to move this wording to a generic place. it is
along the same change of that “RECOMMENDED” comment above.

[nit] Note that except for this (I think general) point about local policy, this section just points back to §3.1 and 3.2..  Do we even need this section at all??

<NS> Right. the p2p section is removed.

365     3.5.  LDP/IGP Synchronization on LANs

367       As described in [RFC6138] when a new IS-IS node joins a broadcast
368       network, it is unnecessary and sometimes even harmful for all IS-IS
369       nodes on the LAN to advertise maximum link metric.  [RFC6138]
370       proposes a solution to have the new node not advertise its adjacency
371       towards the pseudo-node when it is not in a "cut-edge" position.

373       With the introduction of Reverse Metric in this document, a simpler
374       alternative solution to the above mentioned problem can be used.  The
375       Reverse Metric allows the new node on the LAN to advertise its
376       inbound metric value to be the maximum and this puts the link of this
377       new node in the last resort position without impacting the other IS-
378       IS nodes on the same LAN.

380       Specifically, when IS-IS adjacencies are being established by the new
381       node on the LAN, besides setting the maximum link metric value (2^24
382       - 2) on the interface of the LAN for LDP IGP synchronization as
383       described in [RFC5443], it SHOULD advertise the maximum metric offset
384       value in the Reverse Metric TLV in its IIH PDU sent on the LAN.  It
385       SHOULD continue this advertisement until it completes all the LDP
386       label binding exchanges with all the neighbors over this LAN, either
387       by receiving the LDP End-of-LIB [RFC5919] for all the sessions or by
388       exceeding the provisioned timeout value for the node LDP/IGP
389       synchronization.

[major] Should this document be marked as Updating rfc6138?  If not, why not?

<NS> I think they are both valid, RFC6138 is also valid although this reverse-metric
mechanism is simpler.

[major] I believe that the Normative specification ("...as described in [RFC5443], it SHOULD...") makes rfc5443 a Normative reference.

<NS> will move over.

391     3.6.  Operational Guidelines

393       A router MUST advertise a Reverse Metric TLV toward a neighbor only
394       for the period during which it wants a neighbor to temporarily update
395       its IS-IS metric or TE parameters towards it.

[major] The TLV is in the IIH.  Should it be in every message?  Would stopping mean just not advertising?  What if the sender doesn't want to wait until the next scheduled IIH?

<NS> This “period” wording is not the IIH interval, but the maintenance window period.
will add words to that.

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

413       It is RECOMMENDED that implementations provide a capability to
414       disable any changes to a node's individual interface default metric
415       or Traffic Engineering parameters based upon receiving a properly
416       formatted Reverse Metric TLVs.

[major] The text above is in conflict with the text in §3.3 which says that "local provisioning...MUST take precedence".  (1) RECOMMENDED is the same as SHOULD, (2) disabling the change is equivalent to the local configuration taking precedence.

<NS> As mentioned above, changed.

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.

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

429     5.  IANA Considerations

431       This document requests that IANA allocate from the IS-IS TLV
432       Codepoints Registry a new TLV, referred to as the "Reverse Metric"
433       TLV, possibly from the "Unassigned" range of 244-250, with the
434       following attributes: IIH = y, LSP = n, SNP = n, Purge = n.

[nit] IANA completed the early allocation, please update.

<NS> will add.

---

From: Alvaro Retana <[email-masked]>
Date: Wed, 26 Sep 2018 13:10:48 -0700

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

...

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.

...

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

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

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.

...

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.

:-(

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

---

From: "Naiming Shen (naiming)" <[email-masked]>
Date: Thu, 27 Sep 2018 22:36:26 +0000

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 <[email-masked]<mailto:[email-masked]>> wrote:

On September 24, 2018 at 9:35:06 PM, Naiming Shen (naiming) ([email-masked]<mailto:[email-masked]>) wrote:

Naiming:

Hi!

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.

...

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

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.

<NS> OK.

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.

...

I see…

<NS> ok. thanks.

...

:-(

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.

<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

---

From: Alvaro Retana <[email-masked]>
Date: Fri, 28 Sep 2018 14:45:04 -0700

On September 27, 2018 at 6:36:29 PM, Naiming Shen (naiming) (
[email-masked]) wrote:

Naiming:

Hi!

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

Thanks!

Alvaro.

...

Yes.

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.

...

<NS> OK.

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.

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

...

...

:-(

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

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

NEW>

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

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Fri, 28 Sep 2018 21:50:57 +0000

Alvaro, Naiming,

Changes sound good. See one inline.

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

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

Naiming:

Hi!

Thanks!

Alvaro.

...

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.

...

<NS> OK.

The text now reads:

   Metric TLVs.

s/MAY be optionally/MAY

You mean:

            s/MAY be optionally/May be

Thanks,

Acee

...

...

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

NEW>

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

---

From: Uma Chunduri <[email-masked]>
Date: Fri, 28 Sep 2018 23:19:52 +0000

Hi Naiming & Co-authors

Thanks for excellent work. Though, I reviewed the early versions of this draft could not follow all latest discussions/updates.

But just want to chime-in on part of the text posted here and the subsequent AD’s comment.

In-line [Uma]:
--
Uma C.

From: Lsr [mailto:[email-masked]] On Behalf Of Alvaro Retana
Sent: Friday, September 28, 2018 2:45 PM
To: Naiming Shen (naiming) <[email-masked]>
Cc: [email-masked]; Les Ginsberg (ginsberg) <[email-masked]>; [email-masked]; Christian Hopps <[email-masked]>; [email-masked]
Subject: Re: [Lsr] AD Review of draft-ietf-isis-reverse-metric-11

Naiming:

Hi!

Thanks!

Alvaro.

...

OLD>

NEW>

...

183   condition..

<NS> OK.

The text now reads:

   Metric TLVs.

s/MAY be optionally/MAY

...

...

[Uma]:  1. It’s good to refer IS-IS auth drafts 5304 and 5310 and analysis of both of these for  various deployment/operational security consideration with the karp document 7645
                 2. Also would suggest to remove “particularly on multi-access LANs” at the end. As metric change on (either side) can potentially impact routing decisions of the entire network.

NEW>