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

[Alvaro Retana <[e-mail]>]

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.



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



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


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



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



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



...
> > 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. :-)




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


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


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

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.


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.


617	8.  IANA Considerations

[minor] For clarity, consider using sub-sections below.

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.

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.

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.

[minor] Adding a reference to rfc7370 would be useful.  Make it Normative.

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


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

[minor] s/"Standards Action" ([RFC8126] and [RFC7120])./"Standards
Action" [RFC8126].     Standards Action is only defined in rfc8126;
the reference to rfc7120 is not needed.

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

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.

[minor] Adding a reference to rfc7370 would be useful. Make it Normative.

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.

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


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.