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

["Les Ginsberg (ginsberg)" <[e-mail]>]

Alvaro –

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.

Please see inline.


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

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


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.

In general, the sub-TLV space is private to each TLV codepoint. We have made specific exceptions for “TLVs 22, 23, 25, 141, 222, and 223” and “TLVs 135, 235, 236, and 237” because in those cases we have a family of TLVs which provide similar functionality and it is expected that most of the sub-TLVs will be common to the family of TLVs. But this is quite deliberately the exception – not the rule. Please see https://tools.ietf.org/html/rfc7370

The use of the same sub-TLV code point (18) for the TE Metric under the Reverse Metric TLV as is used in TLVs 22 etc. is a sensible convenience for implementations, but it is certainly not required and should not be used to argue that the definition under TLV 22 etc. is applicable to Reverse Metric TLV.

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.

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

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

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.

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

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

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

Please let me know how you wish to proceed.
Thanx.

   Les


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

[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"?

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

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.

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.

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.


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

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.

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.

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.

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

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?

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?

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.

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

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.

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

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?

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?

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

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?

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

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?


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

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.

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.

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

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

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.