Re: [Lsr] AD Review of draft-ietf-isis-te-app-09

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

Alvaro -

Thanx for the continued review.
V11 of the draft has been posted to address your additional comments.

More inline.

> -----Original Message-----
> From: Alvaro Retana <[e-mail]>
> Sent: Tuesday, February 25, 2020 4:42 AM
> To: Les Ginsberg (ginsberg) <[e-mail]>; draft-ietf-isis-te-
> [e-mail]
> Cc: Acee Lindem (acee) <[e-mail]>; [e-mail]; [e-mail]
> Subject: RE: AD Review of draft-ietf-isis-te-app-09
> 
> On February 7, 2020 at 1:50:58 AM, Les Ginsberg wrote:
> 
> Les:
> 
> Hi!
> 
> > We have published V10 of the draft addressing your comments.
> 
> It looks like your copy of my review was truncated (not sure why).
> The archive has the complete version, and I pasted the remaining
> comments at the end of this message.
> 
> https://mailarchive.ietf.org/arch/msg/lsr/Q39ilCqsDo0WIhayZ57RTqHEHCQ/
> 
> 
> > Note that the authors of the IS-IS and OSPF drafts are "synchronizing" as
> > there is a lot of similarity in your comments regarding the two drafts. We
> > have decided to close all issues with you for the IS-IS draft first - then
> > hopefully the equivalent changes can be made for the OSPF draft can be
> made
> > and resolved more quickly.
> 
> Makes sense!
> 
> 
> > Detailed responses inline.
> 
> I have some responses myself; most of them not-major.  I've indicated
> the ones that I consider major -- it would be ideal if others in the
> WG chimed in with any opinions.
> 
> 
> Thanks!
> 
> Alvaro.
> 
> 
> ...
> > > (B) I was able to identify one significant functional difference which
> > > warrants a discussion of the reason for it and maybe the pros/cons:
> > >
> > > §4.1 talks about the behavior when "both SABM Length and UDABM
> Length
> > > are zero". This document makes the use of the attributes optional while
> the
> > > OSPF draft mandates their use (§3).
> > >
> > [Les:] Both drafts use the word "MAY" in the respective sections entitled
> > "Use of Zero Length Application Identifier Bit Masks".
> >
> > "MAY" - or perhaps "may" - makes sense to me because in the absence of
> SABM
> > bits there is no way to know if a given application will have any reason to
> > use the attributes advertised. The text is intended to say use by any
> > application is "permitted". I have changed the text in this way.
> 
> I'm fine with the changes you made.
> 
> This is the text from the OSPF draft that I was referring to:
> 
>    When neither the Standard Application Bits nor the User Defined
>    Application bits are set (i.e., both SABM Length and UDABM Length are
>    0) in the ASLA sub-TLV, then the link attributes included in it MUST
>    be considered as being applicable to all applications.
> 
> From your explanation, I see the intent, but the use of "MUST" gives
> the impression that there's no choice.
> 
[Les:] The OSPF text will be changed to be consistent with what is in the IS-IS draft.

> 
> 
> ...
> > > 177 3. Legacy Advertisements
> ...
> > > [minor] Please add references to where all the values in this section
> > > are defined...if the same as the references above.
> > >
> > [Les:] Done.
> 
> Sorry, I meant the RFCs where the sub-TLVs are defined, not the registries.

[Les:] Well, the IANA registry identifies the Reference RFCs - and has the advantage that it gets updated should that ever change. So I would suggest providing a link to the registry is actually better than identifying the RFC for each code point. :-)
?? 

> 
> 
> 
> ...
> > > 231 4.1. Application Identifier Bit Mask
> > >
> > > 233 Identification of the set of applications associated with link
> > > 234 attribute advertisements utilizes two bit masks. One bit mask is for
> > > 235 standard applications where the definition of each bit is defined in
> > > 236 a new IANA controlled registry. A second bit mask is for non-
> > > 237 standard User Defined Applications(UDAs).
> ...
> > > [minor] "second bit mask is for non-standard User Defined
> > > Applications" The explicit point is made here that the UDAs are used
> > > for *non-standard* applications. Given that these bits are for *user
> > > defined* applications, the user can make the bits mean anything they
> > > want (including SRTE, LFA, etc.), right? If so, then highlighting
> > > *non-standard* seems not to be needed. s/for non-standard User
> > > Defined Applications/for User Defined Applications
> > >
> > [Les:] In theory it is possible for a User Defined Application to perform the
> > same functions as a standard application (such as SRTE). However, if it did
> > so it would not be "Standard SRTE" and the bit could NOT be considered as
> the
> > same as "standard SRTE".
> 
> I didn't mean just the same function, but the same application.  Let
> me try again.
> 
> Can an operator do the following:  define a bit (call it X) in the
> UDABM to mean SRTE (*the* SRTE)?  IOW, for the routers operating in
> the network (which all would be aware of the X-bit), when the X-bit is
> set it indicates that SRTE will use the information.  This operation
> would result in exactly the same thing as if the S-bit was set.
> 
> Why would anyone want to do this?  For security reasons (for example):
> by not using the defined bit, anyone eavesdropping on the LSPs would
> not know which application is enabled.
> 
> In any case, this is just a nit/minor comment...

[Les:] (I am repeating myself but...)
UDAs can do whatever they like - and by definition are out of scope of the draft.
The notion of using a UDA to implement an SDA as a means of security seems very unwise to me. The point of providing standard code points is to insure interoperability.

> 
> 
> ...
> > > [major] In a mixed network, where some routers support this
> > > specification, but other don't, it seems that setting the L-flag would
> > > be useful as all applications would "fall back" to the legacy
> > > advertisements. However, not setting the L-flag seems to have the
> > > potential for the routers to use different information resulting in
> > > (at best) inconsistent decisions if the values are not the same.
> > >
> > [Les:] The revised Deployment considerations section now addresses this.
> 
> [nit] In the new §6.3.3 (Interoperability with Legacy Routers) it
> might be good to mention the setting of the L-flag.  For example,
> extend Step 1, "Receiving routers continue to use legacy
> advertisements" to include a mention of the L-flag being set.
> 
[Les:] This isn't a nit - it is a misunderstanding.
We aren't using L-bit here at all. 
We are accommodating the coexistence of legacy/non-legacy routers.

Legacy routers only send/receive legacy advertisements. They don't understand
the new advertisements - let alone the L-bit.

To migrate to all new routers - at which point legacy advertisements can be deprecated - you must follow the three steps described. The transition between steps is controlled by some implementation specific knob.

Please reread and let me know if it is clear.

> 
> 
> ...
> > > [major] "Undefined bits MUST be transmitted as 0 and MUST be ignored
> > > on receipt." What if the sender and the receiver support a different
> > > set of applications? For example, suppose that the sender supports a
> > > new application identified by the N-bit, and sets only that bit; the
> > > receiver treats the N-bit as undefined and ignores it. The result is
> > > that, for the receiver, there seems to be no indication of an
> > > application. Is "no application" a valid indication, or should at
> > > least one bit always be set?
> > >
> > [Les:] Just like unknown TLVs, unknown bits MUST be ignored.
> > I have added text.
> 
> Yes, that is ok.
> 
> [major] But you didn't answer my question: Is "no application" a valid
> indication, or should at least one bit always be set?  I guess that
> the receiver wouldn't see anything it understands...  Would this be
> equivalent to zero-length masks?
> 
[Les:] There are three cases:

1)You have application specific advertisements - which you send with the appropriate bit(s) set in the mask.

2)You have application specific advertisements but want all applications to use
them. You send with zero-length bit mask.

3)You have no advertisements.

> 
> 
> > > [minor] Following on the previous comment... It seems that a
> > > requirement is for the routers/nodes in the domain to agree on the
> > > meaning of the bits: support the same Standard Applications and
> > > interpret the User Defined bit mask the same way. Please add this
> > > type of information to the Deployment Considerations Section.
> > >
> > [Les:] Standard Applications by definition have bits which are assigned
> > by IANA. There is therefore no need to say routers have to agree
> > on their meaning.
> > User Defined Applications are - by definition - outside the scope of this
> > specification. They can do whatever they want.
> 
> Let me try again...
> 
> What issues can arise from the routers supporting a different set of
> applications/bits?  I can imagine several ways for this to occur, for
> example: routers that have not been upgraded to support the latest
> assignments...bad configuration practices there the UDAs are not
> consistently instantiated in the whole network...
> 
> IOW, if a new application is implemented in the network, but not all
> the routers are aware of the corresponding bit (because they're
> running old software or were not configured correctly, for example),
> then it is possible that the network will not operate as expected.
> 
[Les:] How a router indicates that it supports a given application is outside the
scope of this specification.
For example, https://tools.ietf.org/html/draft-ietf-lsr-flex-algo-06#section-10  describes how this is done for flex-algo.

> 
> 
> ...
> > > 337 4.2. Application Specific Link Attributes sub-TLV
> ...
> > > 361 Multiple Application Specific Link Attribute sub-TLVs for the same
> > > 362 link MAY be advertised. When multiple sub-TLVs for the same link are
> > > 363 advertised, they SHOULD advertise non-conflicting application/
> > > 364 attribute pairs. A conflict exists when the same application is
> > > 365 associated with two different values of the same link attribute for a
> > > 366 given link. In cases where conflicting values for the same
> > > 367 application/attribute/link are advertised all the conflicting values
> > > 368 MUST be ignored.
> > >
> > > 370 For a given application, the setting of the L-flag MUST be the same
> > > 371 in all sub-TLVs for a given link. In cases where this constraint is
> > > 372 violated, the L-flag MUST be considered set for this application.
> > >
> > > [major] Tying these two paragraphs together... If one of the sub-TLVs
> > > has the L-flag set, then all the sub-TLVs that include at least one
> > > common application MUST be considered as having the L-flag set as
> > > well. This seems to become an attack vector: a rogue IS can set the
> > > L-flag (and/or set application bits) in all/some of the
> > > sub-TLVs...which may result in no attribute information (if the
> > > sub-sub-TLVs were the only source of information and are ignored) for
> > > a link...
> > >
> > [Les:] The text says:
> >
> > "Link attribute sub-sub-TLVs for the corresponding link attributes MUST
> NOT
> > be advertised for the set of applications specified in the Standard/User
> > Application Identifier Bit Masks and all such advertisements MUST be
> ignored
> > on receipt."
> >
> > There is no reason to send multiple sub-TLVs w L-bit set on a given link
> > because by definition such a sub-TLV MUST NOT have any sub-sub-TLVs.
> >
> > Authentication (as always) is our protection against attacks.
> 
> [major] No, not against an IS that has been taken over: an attacker
> has access to it, its configuration, etc., it is rogue.
> Authentication only proves that the IS is the IS it says it is, not
> that it is not sending malicious information.
> 
> To be clear, I'm not asking for the rogue node problem to be solved in
> this document.  Just that the risks are recognized.
> 
> I'm mentioning this point about rogue nodes because that is the new
> focus of the SEC area.  I really don't want to go forward with
> Security Considerations consisting of a single line...but if you
> prefer to discuss with the SecDir/SEC AD, I'm fine with it. :-)
> 
[Les:] Security discussions always seem to be a bit of a tug-of-war.
What you seem to be saying is that if an attacker gets "the keys", it
can fill a TLV with misinformation. I totally agree. But how are the new
TLVs defined in this document any different in this regard than existing
TLVs?

An attacker today - given it has the keys - could advertise bogus neighbors or bogus metrics or...

??

> 
> 
> 
> ...
> > > [major] "...and IANA is requested to assign the legacy sub-TLV
> > > identifer to the corresponding sub-sub-TLV." (s/identifer/identifier)
> > > Is this assignment something that should also be done going forward
> > > (for new "legacy sub-TLVs"), or just for the initial values in the
> > > registry? The new registry has a registration policy of "Expert
> > > Review", please include any instructions for the DEs-to-be in the IANA
> > > Considerations section...and maybe a (Normative) pointer to rfcxxx for
> > > completeness.
> > >
> > [Les:] I have added text in the IANA section.
> 
> [minor] Please remove the remaining IANA-instructions-related text
> from this section.
> 
[Les:] Not sure what text you want removed. ??

> 
> ...
> > > 463 5.1. Use of Legacy Advertisements
> ...
> > > [] I think that the text in this section can be generalized to any
> > > application, not just ones defined in this document. s/The Standard
> > > Applications defined in this document MAY continue/The Standard
> > > Applications may continue
> > >
> > [Les:] The existing applications have the special property that they are
> > already deployed using legacy advertisements. This cannot be said for
> > any as yet to be defined applications. You can insist in the documents
> > that define new applications that they discuss whether legacy
> advertisements
> > can/cannot be used, but this document cannot speak to new applications
> > without being clairvoyant.
> 
> I still think that the text can be generalized...but I won't insist on
> it since no one else in the WG is.
> 
> [major] I do however want to contrast the last sentence above:
> 
>    You can insist in the documents that define new applications that they
>    discuss whether legacy advertisements can/cannot be used...
> 
> ...with the first sentence of the last paragraph:
> 
>    New applications which future documents define to make use of the
>    advertisements defined in this document MUST NOT make use of legacy
>    advertisements.
> 
> If "future documents...MUST NOT make use of legacy advertisements",
> then no one can't ask them to justify their use.  It's like trying to
> be clairvoyant...
> 
> I would be ok if s/MUST NOT/SHOULD NOT, which (given the rest of the
> paragraph) puts the burden on new documents to justify using legacy
> advertisements.
> 
[Les:] You are correct that I misspoke in my email reply to your review - but I believe the document itself is unambiguous in this regard. New applications MUST NOT use legacy. This simplifies things for new deployments and I'd like to stick to the current text.

> 
> ...
> > > 612 Migrating RSVP-TE away from legacy advertisements could result in
> > > 613 some implementation simplification as it allows the removal of code
> > > 614 which encodes/decodes the legacy advertisements. Whether this is
> > > 615 seen as desirable is something for the marketplace to determine
> 
> Your reply ends here...but my comments kept going. ;-)  I'm not sure
> of the reason, but I've seen long messages be truncated.  The archive
> has the full review:
> https://mailarchive.ietf.org/arch/msg/lsr/Q39ilCqsDo0WIhayZ57RTqHEHCQ/
> 
[Les:] Yes - sorry. I noticed this in the OSPF spec review because it was severely truncated, but missed it in the IS-IS review because the truncated portion was much smaller than the non-truncated.

My selective survey shows this happens with some frequency to other authors for reviews you post,
but only for emails sent to the authors. The email to members of the WG list alias seem to be unaffected. What that tells us about root cause I am not sure.

> You added rfc8402 as a reference for SRTE, but the text also says
> (§2): "...SRTE which is defined in
> [I-D.ietf-spring-segment-routing-policy]."  Pick one.
> 
[Les:] Fixed

> 
> I am pasting below the remaining comments (based on the -09 text):
> 
> 
> 612	   Migrating RSVP-TE away from legacy advertisements could result in
> 613	   some implementation simplification as it allows the removal of code
> 614	   which encodes/decodes the legacy advertisements.  Whether this
> is
> 615	   seen as desirable is something for the marketplace to determine.
> 
> [?] "allows the removal of code which encodes/decodes the legacy
> advertisements"  If the sub-sub-TLVs used are the same as the legacy
> ones, wouldn't the code be pretty much the same??  Either way,
> considerations which may be implementation-dependent seems out of
> place is a section talking about deployment.
> 
[Les:] I have removed this text.

> 
> 617	8.  IANA Considerations
> 
> [minor] For clarity, consider using sub-sections below.
> 
[Les:] Done

> 619	   This document defines a new sub-TLV for TLVs 22, 23, 25, 141, 222,
> 620	   and 223.
> 
> [major] Please explicitly include the name of the registry.

[Les:] Done

> 
> 622	    Type  Description             22   23   25  141  222  223
> 623	    ----  ---------------------  ---- ---- ---- ---- ---- ----
> 624	     16   Application Specific     y    y  y(s)   y    y    y
> 625	           Link Attributes
> 
> 627	   This document defines one new TLV:
> 
> [major] Explicitly mention the name of the registry.

[Les:] Done

> 
> 629	    Type  Description             IIH LSP SNP Purge
> 630	    ----  ---------------------   --- --- --- -----
> 631	     238  Application Specific     n   y   n    n
> 632	           SRLG
> 
> 634	   This document requests a new IANA registry be created to control
> the
> 635	   assignment of sub-sub-TLV codepoints for the Application Specific
> 636	   Link Attributes sub-TLV.  The suggested name of the new registry is
> 637	   "sub-sub-TLV code points for application specific link attributes".
> 638	   The registration procedure is "Expert Review" as defined in
> 639	   [RFC8126].  The following assignments are made by this document:
> 
> [major] Explicitly indicate where the new sub-registries should be
> created.  In this case in the "IS-IS TLV Codepoints" registry.
> 
[Les:] Done

> [minor] Adding a reference to rfc7370 would be useful.  Make it Normative.
> 
[Les:] I added this - but at the beginning of the IANA section.

> [major] If the legacy specifications are extended, is it expected that
> new sub-sub-TLVs will also be created?  If so, there need to be
> explicit instructions to the DEs in the legacy registries.
> 
[Les:] Since the draft specifies that new applications MUST NOT use legacy sub-TLVs, the case you are concerned with cannot arise.

> 
> ...
> 663	   This document requests a new IANA registry be created, under the
> 664	   category of "Interior Gateway Protocol (IGP) Parameters", to
> control
> 665	   the assignment of Application Identifier Bits.  The suggested name
> of
> 666	   the new registry is "Link Attribute Applications".  The registration
> 667	   policy for this registry is "Standards Action" ([RFC8126] and
> 668	   [RFC7120]).  The following assignments are made by this document:
> 
> [minor] If we're setting up a applications, why not make it generic:
> "IGP Applications", for example...??
> 
[Les:] Because this is defining bit positions which only apply to the SABM/UDABM fields in the new ASLA sub-TLV.

> [minor] s/"Standards Action" ([RFC8126] and [RFC7120])./"Standards
> Action" [RFC8126].     Standards Action is only defined in rfc8126;
> the reference to rfc7120 is not needed.
> 
[Les:] I copied this from RFC8665. Are you saying that document is incorrect?
Happy to do whatever is most correct.

> 670	    Bit #   Name
> 671	   ---------------------------------------------------------
> 672	     0      RSVP-TE (R-bit)
> 673	     1      Segment Routing Traffic Engineering (S-bit)
> 674	     2      Loop Free Alternate (F-bit)
> 
> [major]  Add a line to cover the rest of the range and call it "Unassigned".
> 
[Les:] Done

> 676	   This document requests a new IANA registry be created to control
> the
> 677	   assignment of sub-TLV types for the application specific SRLG TLV.
> 678	   The suggested name of the new registry is "Sub-TLVs for TLV 238".
> 679	   The registration procedure is "Expert Review" as defined in
> 680	   [RFC8126].  The following assignments are made by this document:
> 
> [major] Indicate where this registry should be created.
> 
[Les:] Done

> [minor] Adding a reference to rfc7370 would be useful. Make it Normative.
> 
[Les:] See earlier reply.

> 682	    Value    Description
> 683	    ---------------------------------------------------------
> 684	     0-3     Unassigned
> 685	      4      Link Local/Remote Identifiers (see [RFC5307])
> 686	      5      Unassigned
> 687	      6      IPv4 interface address (see [RFC5305])
> 688	      7      Unassigned
> 689	      8      IPv4 neighbor address (see [RFC5305])
> 690	     9-11    Unassigned
> 691	     12      IPv6 Interface Address (see [RFC6119])
> 692	     13      IPv6 Neighbor Address (see [RFC6119])
> 693	    14-255   Unassigned
> 
> [major] Do you want the reference to the assignments to be this
> document, the RFC where the encoding is defined, or both?  I would
> suggest both.  Please add a column to reflect that: e.g.,
> "[RFC5307] [This Document]"...and take out the "(see ...)" text.
> 
[Les:] I added another column for "Encoding Reference"

> 695	9.  Security Considerations
> 
> 697	   Security concerns for IS-IS are addressed in [ISO10589, [RFC5304],
> 698	   and [RFC5310].
> 
> [major] ISO10589 is not in the References section.  It should be a
> Normative reference.
> 
> [major] rfc5304/rfc5310 only talk about authentication.  What about
> vulnerabilities specifically introduced by the new functionality?
> This document doesn't define new TE-related attributes, just a new way
> to advertise them...perhaps include something like this:
> 
>    This document defines a new way to advertise already defined TE
> Attributes.
>    As these TLVs are not used for SPF computation or normal routing, the
>    extensions specified here have no direct effect on IP routing.  Tampering
>    with the information defined in this document may have an effect on
>    applications using it, including potential sub-optimal Traffic Engineering.
>    This is not new...see [RFC5305], [RFC5307], [RFC6119], and [RFC8570].
> 
[Les:] I adapted your suggested text.

> 
> And then there are possible vulnerabilities introduced by this
> document.  I put some comments above about the ability of a rogue
> router to render the data unusable by setting, for example the L-flag,
> or sending conflicting values.  At best, this action could result in
> sub-optimal paths (by taking some links out of consideration)...
> Consider adding text related to that.

[Les:] I have a harder time with this suggestion and so did not incorporate it. See my replies above.

   Les