Early Review of draft-ietf-idr-entropy-label-06
review-ietf-idr-entropy-label-06-secdir-early-hardaker-2023-08-08-00

I have reviewed this document as part of the security directorate's
ongoing effort to review all IETF documents being processed by the
IESG. These comments were written primarily for the benefit of the
security area directors. Document editors and WG chairs should treat
these comments just like any other last call comments.

The summary of the review is: almost ready

Note: this is an early review, per request

In general, the document reads fairly well but could use some
improvements still.

General notes:
- Many acronyms are not expanded on first use like they generally
  should be except for the rare ones that likely everyone understands
  (BGP).  Though MPLS and LSR, for example, are probably in the
  grey-area for those I'd suggest expanding them.  I found it humerus,
  actually, to find the expansion for NLRI on the 7th page (section 3)
  even though it had been used a bunch before that.

- There are more than 5 authors/editors on the draft, where 5 is the
  current recommended maximum.

- In general I think the introduction could use a bit more clarity
  about the motivation and background of the document.  Maybe hint at
  the example in section 2.2 about the purpose of needing this new
  option, etc, or even moving the example into the intro and referring
  back to it.

- Appendix A seems important enough to warrant a pointer to it from
  somewhere else in the document (eg, section 3).  I found it
  surprising to be at the end but I hadn't heard of it yet.

Security specific thoughts:
- the security considerations sections rightly discusses privacy but
  then indicates that leaks will only happen when a router doesn't
  implement the specification, which is going to be very common in the
  beginning.  Thus, it seems that strong advice in the top of the
  document to operators that indicates RCAs should only be set up when
  all border routers that might propagate them also implement the RCA
  specification.  And in the security considerations or somewhere it
  should state that "implementations MUST use default configuration
  settings with RCA disabled".

Specific issues:
- Section 1: "the RCA identifies itself with the next hop of the
  originator".  I'm not sure "identifies" is the right term there.
  Maybe "the RCA is associated with..." or...

- The second to last paragraph in section 1 ("An RCA carried...")
  could use a bit of clarity/rewriting to cover all the potential
  cases and how to handle them.  One particular concern that is almost
  security-section worthy, but I didn't finish thinking through (but
  you're the experts) is what to do when new routes arrive that don't
  have a RCA where a previous route did.  I don't think this case is
  clearly covered on what should be done in this case.  The reason for
  the potential security concern is whether future route
  advertisements can affect or rely on previous ones with the RCA was
  attached.

- 2.1 If you want to ensure maximum interoperability (always a goal,
  IMHO), I would delete the MAY for handling things in any order.  I'd
  [IMHO] be very prescriptive and either require everything to be in
  order and routers should drop any RCA that doesn't match this
  requirement, or simply always process in order (or, I suppose, force
  routers to order them after receipt but then you still run into
  ordering of the same-code RCAs).  Doing something other than strict
  processing requirements means different routers may behave
  differently.  Similarly, in 2.4 there is a malformed-is-dropped and
  processing continues.  This may or may not be safe and it might be
  better to not process the entire RCA if any section of it can't be
  understood.

- 2.2 "In effect, this means that a given capability TLV will only be
  propagated along a path where it is suported by every BGP next-hop
  along the path" -- this might be true when constructing new RCAs,
  but isn't true when a router doesn't support RCA at all and passes
  it along as a binary blob.  In fact, this is stated two paragraphs
  later but could be better tied together / explained so it's clear
  which type of router (supporting or not) passes on RCAs/values.

Nits:
- The sentence in the introduction in ()s that states the name is
  somewhat regrettable could probably be removed.  It doesn't really add
  to the document and simply saying "note that this specification should
  not be confused with the BGP capabilities document [RFC5492]"

- 2.1: "In turn, each Capability is a triple".  Two comments:

  1. Here and likely other places, you may want to use "Router
  Capability Attribute" or "RCA" instead of "Capability", since
  earlier you made the point of ensuring you're not talking about BGP
  capabilities -- thus it makes sense to use consistently
  different/unique terminology throughout.

  2. I'd probably change "triple" to "Tag/Length/Value set" or
  something to be super clear, otherwise TLV is never really spelled
  out.

- 2.1 the capability length is supposed to be the length of *just the
  value* but in the ()s after "triple" it's listed as "Capability
  Length" which could confuse readers that it should include the
  code/length length header too (IE, an extra 16).

- 2.1 "... with different Capability Value and either ... Length." --
  I'd put "and either the same or different Capability Length" into
  ()s as it's less important in the uniqueness requirement than the
  value, which deserves greater emphasis (how nit is this comment, I
  know).

- 2.2 "If S is originating R" ... "MUST set the header" -- you might
  describe when this should be done and when it shouldn't.   Certainly
  if the intention was to include an RCA, then it's a MUST.  But it
  may be that one *shouldn't* be included at times (eg, when the
  trying to avoid leaking the upstream N, like discussed later)

- section 3 could use a short sentence simply helping with the context
  switch between describing RCAs generically and now switching to the
  only document-defined code-point.  Something like "ELCv3 is a
  capability code defined in this document to advertise the next
  hop..."

- 3.2: "MUST NOT include the ELCv3 capability except" - it might be
  easier to break this into two parts to avoid confusion.  maybe "SHOULD
  include the ELCv3 capability when it knows that the egress of
  the associated LSP L is EL-capable, otherwise it MUST NOT include
  the ELCv3 capability."

- 3.3: the last paragraph of 3.3 could also be broken into two
  sentences -- it's rather long and hard to read as is



-- 
Wes Hardaker
USC/ISI