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

[Jordan Head <[e-mail]>]

Hi Alvaro,

My comments inline as jhead>>

Thanks


From: Tony Przygienda [e-mail]<mailto:[e-mail]>
Date: Wednesday, February 22, 2023 at 3:05 PM
To: Alvaro Retana [e-mail]<mailto:[e-mail]>
Cc: Jordan Head [e-mail]<mailto:[e-mail]>, [e-mail]<mailto:[e-mail]> [e-mail]<mailto:[e-mail]>, [e-mail]<mailto:[e-mail]> [e-mail]<mailto:[e-mail]>, [e-mail]<mailto:[e-mail]> [e-mail]<mailto:[e-mail]>, [e-mail]<mailto:[e-mail]> [e-mail]<mailto:[e-mail]>
Subject: Re: [Rift] AD Review of draft-ietf-rift-rift-16 (4.2.3.3)


[External Email. Be cautious of content]


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.

    jhead>> See next comment.

[] "close to optimal global flood reduction"

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

    jhead>> Between the comparisons to other protocols and the "marketing" component, I've removed the first paragraph of the Flooding section.

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

    jhead>> Removed.

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`

    jhead>> Fixed.

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

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

    jhead>> I've added pseudo code for TIE ordering in a new figure named "TIE Ordering", it should make things much clearer. It is normative, and I will refer to it as such as part of some of your other comments. I pasted it below, hopefully e-mail doesn't screw up the indentation, but I should have -17 published by the time you read this:

    for each TIEPacket:
        TIEHeader = TIEPacket.TIEHeader
        TIEElement = TIEPacket.TIEElement

        seq_nr = TIEHeader.seq_nr

        TIEID = TIEHeader.TIEID
        direction = TIEID.direction

        # System ID
        originator = TIEID.originator

        # TIETypeType
        tietype = TIEID.tietype
        tie_nr = TIEID.tie_nr

        if X.direction > Y.direction:
            return X.direction
        else if X.direction < Y.direction:
            return Y.direction
        else if X.originator > Y.originator:
            return X.originator
        else if X.originator < Y.originator:
            return Y.originator
        else:
            if X.tietype == Y.tietype:
                if X.tie_nr == Y.tie_nr:
                    if X.seq_nr == Y.seq_nr:
                        X.lifetime_left = X.remaining_lifetime - time since TIE was received
                        Y.lifetime_left = Y.remaining_lifetime - time since TIE was received

                        if absolute_value_of(X.lifetime_left - Y.lifetime_left) <= common.lifetime_diff2ignore:
                            return equal

                        else:
                            return TIE with largest lifetime_left
                    else:
                        return X.seq_nr compared to Y.seq_nr
                else:
                    return X.tie_nr compared to Y.tie_nr
            else:
                return X.TIEType compared to Y.TIEType

[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 = "")

    jhead>> Fixed


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.


    jhead>> Reworked, see following comments.

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

    jhead>> Removed the bits about comparison.

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

    jhead>> Removed the bits about being used in comparison, practically speaking they're not applicable here. Our new TIE Ordering reflects this.

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

    jhead>> Reworked this a bit by hinting toward the schema and being a more specific about the synchronization aspect:

        The TIEHEader can also carry an `origination_time` schema element (for fabrics that utilize precision timing) which contains the absolute timestamp of when the TIE was generated and an `origination_lifetime` to indicate the original lifetime when the TIE was generated. If carried, they can be used for debugging or security purposes (e.g. to prevent lifetime modification attacks).


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

    jhead>> The new TIE Ordering bits we state:

         "lifetime_left" = "remaining_lifetime" - time since the TIE was received.

    The exact proceudure to decrement is an implementation specific detail.

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.

    jhead>> Removed.

[minor] s/lifetime/remaining lifetime

    jhead>> Fixed.

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

    jhead>> Fixed.

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.

    jhead>> Fixed. I'll look for more inconsistencies and address them.

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

    jhead>> Most of Appendix C's items are already in the schema. I've added the couple that weren't there and removed Appendix C per Tony's comment.

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.


Juniper Business Use Only