[Rift] AD Review of draft-ietf-rift-rift-16 (4.2.3.3)

[Alvaro Retana <[e-mail]>]

Hi!

Just 4.2.3.3.


Alvaro.



2404	4.2.3.3.  Flooding

2406	   The mechanism used to distribute TIEs is the well-known (albeit
2407	   modified in several respects to take advantage of Fat Tree topology)
2408	   flooding mechanism used in link-state protocols.  Although flooding
2409	   is initially more demanding to implement it avoids many problems with
2410	   update style used in diffused computation by distance vector
2411	   protocols.  However, since flooding tends to present a significant
2412	   burden in large, densely meshed topologies (Fat Trees being
2413	   unfortunately such a topology) RIFT provides as solution a close to
2414	   optimal global flood reduction and load balancing optimization in
2415	   Section 4.2.3.9.

[] "well-known...flooding mechanism used in link-state protocols.
...avoids many problems with...distance vector protocols."

Please don't assume knowledge and remove references/comparisons
to/with other protocols.

[] "close to optimal global flood reduction"

I might have to read §4.2.3.9, but this sounds like marketing to me.



2417	   As described before, TIEs themselves are transported over UDP with
2418	   the ports indicated in the LIE exchanges and using the destination
2419	   address on which the LIE adjacency has been formed.  For unnumbered
2420	   IPv4 interfaces same considerations apply as in other link-state
2421	   routing protocols and are largely implementation dependent.

[] "For unnumbered IPv4 interfaces same considerations apply as in
other link-state routing protocols and are largely implementation
dependent."

Assumes knowledge of other protocols.  Besides that, the sentence
doesn't say much by pointing to considerations that are "largely
implementation dependent".



2423	   TIEs are uniquely identifed by `TIEID` schema element.  `TIEID` space
2424	   is a total order achieved by comparing the elements in sequence
2425	   defined in the element and comparing each value as an unsigned
2426	   integer of corresponding length.  They contain a `seq_nr` element to
2427	   distinguish newer versions of same TIE.  TIEIDs also carry
2428	   `origination_time` and `origination_lifetime`.  Field
2429	   `origination_time` contains the absolute timestamp when the TIE was
2430	   generated.  Field `origination_lifetime` carries lifetime when the
2431	   TIE was generated.  Those are normally disregarded during comparison
2432	   and carried purely for debugging/security purposes if present.  They
2433	   may be used for comparison of last resort to differentiate otherwise
2434	   equal ties and they can be used on fabrics with synchronized clock to
2435	   prevent lifetime modification attacks.

[nit] s/identifed by `TIEID`/identified by the `TIEID`


[major] "total order achieved by comparing the elements in sequence"

When comparing, which value is considered better/more recent?


[minor] "They contain a `seq_nr` element to distinguish newer versions
of same TIE.  TIEIDs also carry `origination_time` and
`origination_lifetime`."

TIEID doesn't contain any of that:

/** Unique ID of a TIE. */
struct TIEID {
    /** direction of TIE */
    1: required common.TieDirectionType    direction;
    /** indicates originator of the TIE */
    2: required common.SystemIDType        originator;
    /** type of the tie */
    3: required common.TIETypeType         tietype;
    /** number of the tie */
    4: required common.TIENrType           tie_nr;
} (python.immutable = "")


But the TIEHeader does:

/** Header of a TIE. */
struct TIEHeader {
    /** ID of the tie. */
    2: required TIEID                             tieid;
    /** Sequence number of the tie. */
    3: required common.SeqNrType                  seq_nr;

    /** Absolute timestamp when the TIE was generated. */
   10: optional common.IEEE802_1ASTimeStampType   origination_time;
   /** Original lifetime when the TIE was generated.  */
   12: optional common.LifeTimeInSecType          origination_lifetime;
}


[minor] The grammar could be better:

s/
   TIEIDs also carry `origination_time` and `origination_lifetime`.
   Field `origination_time` contains the absolute timestamp when the
   TIE was generated.  Field `origination_lifetime` carries lifetime
   when the TIE was generated.
/
   The TIEHEader can also carry an `origination_time` that contains
   the absolute timestamp of when the TIE was generated, and an
   `origination_lifetime` to indicate the original lifetime when the
   TIE was generated.


[major] "Those are normally disregarded during comparison..."

Does this refer to the text in the second sentence ("`TIEID` space is
a total order achieved by comparing the elements in sequence...") or
to some other comparison.  Also, this makes me think that you have
been referring to the TIEHeader (and not the TIEID) all along -- is
that correct?


[major] "normally disregarded during comparison...[but]...may be used
for comparison of last resort"

Are they used or not?  The use of "normally" doesn't help because it
immediately points are possible exceptions...

If used, how are they compared?  Which one is better/more recent?


[minor] "can be used on fabrics with synchronized clock"

§4.3.4 says that RIFT assumes that all nodes are sync'ed, so "fabrics
with synchronized clock" would mean all the time, right?


[major] "can be used...to prevent lifetime modification attacks"

"can be used" or "are used"?  It seems (from a quick read of §4.4.3),
that neither are used -- but that the remaining lifetime
(remaining_lifetime) is.  Is that correct?

Looking closer, I noticed that the origination_lifetime and the
remaining_lifetime come from the same variable:

   12: optional common.LifeTimeInSecType          origination_lifetime;
   ...
   2: required     common.LifeTimeInSecType        remaining_lifetime;

But I couldn't figure out where the LifeTimeInSecType is decremented
as there's no TIE FSM.  Where is that specified?


This is the related text in the Security Considerations:

5755	7.4.  Lifetime

5757	   Traditional IGP protocols are vulnerable to lifetime modification and
5758	   replay attacks that can be somewhat mitigated by using techniques
5759	   like [RFC7987].  RIFT removes this attack vector by protecting the
5760	   lifetime behind a signature computed over it and additional nonce
5761	   combination which makes even the replay attack window very small and
5762	   for practical purposes irrelevant since lifetime cannot be
5763	   artificially shortened by the attacker.

[major] "Traditional IGP protocols are vulnerable...somewhat mitigated
by using techniques like [RFC7987]."

Remove all mentions of other protocols!  Also, the reference is
irrelevant because it points to an IS-IS specification.


[minor] s/lifetime/remaining lifetime


[] s/which makes even the replay attack window very small and for
practical purposes irrelevant since lifetime cannot be artificially
shortened by the attacker./which results in the inability of an
attacker to artificially shorten the remaining lifetime.



2437	   Remaining lifetime counts down to 0 from origination lifetime.  TIEs
2438	   with lifetimes differing by less than `lifetime_diff2ignore` MUST be
2439	   considered EQUAL (if all other fields are equal).  This constant MUST
2440	   be larger than `purge_lifetime` to avoid retransmissions.

[] It would be very nice if the naming and its use was consistent.
For example, you mentioned the "origination_lifetime" above, but
"origination lifetime" here.  It makes it really hard to search and to
keep track of where things are specified.


[major] "This constant MUST be larger than `purge_lifetime` to avoid
retransmissions."

They are both *constants*, which intuitively mean that they have a set
value.  Note that neither is listed in Appendix C.1 (Configurable
Protocol Constants), should they be?



2442	   All valid TIE types are defined in `TIETypeType`. This enum indicates
2443	   what TIE type the TIE is carrying.  In case the value is not known to
2444	   the receiver, the TIE MUST be re-flooded.  This allows for future
2445	   extensions of the protocol within the same major schema with types
2446	   opaque to some nodes with some restrictions.

[] Note my comments before about consistency related to TIE Types.