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

[Alvaro Retana <[e-mail]>]

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

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.

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