Subject: WG last call review of draft-ietf-6man-segment-routing-header

From: "Adrian Farrel" <[email-masked]>
Date: Thu, 5 Apr 2018 21:11:01 +0100

All,

I have read draft-ietf-6man-segment-routing-header-11 and find myself
conflicted. I really want us to get a definition of the SRH published so that we
can get on with making and deploying SRv6 boxes, but I also want us to write a
specification that is consistent with IPv6 and contains no ambiguities (since
those conditions seem necessary for interoperable and robust implementations).
Additionally, there are a lot of nits that could do with hoovering up - a
language review by a native English speaker might help.

Sorry that I haven't cross-checked my comments against those already in the
issue tacker.

Cheers,
Adrian

===Major===

2.3
   It is assumed in this document that the SRH is added to the packet by
   its source, consistently with the source routing model defined in
   [RFC8200].

Who is assuming, and what would it mean if they were wrong?
As this is a spec, I think you should be more definitive. But you don't need
2119 language, you can just say:
   Implementations conformant to this specification add the SRH to the
   packet at the packet's source, consistent with the source routing
   model defined in [RFC8200].

---

3.
Could not find any explanation of how/when to set the P-flag and what to do when
I see it set.

---

3.
Could not find any explanation of how/when to set the O-flag and what to do when
I see it set.

---

3.
Could not find any explanation of how/when to set the A-flag and what to do when
I see it set. Furthermore, there is no definition of what an "important TLV" is.

---

3.
There is no discussion of the setting or treatment of the Tag field.

---

3.1 
The various subsections contain a lot of "MUST" language. I didn't find any
statement of how a node should behave if it discovers a violation of any of
these.

---

8200 says that "Each extension header should occur at most once." Note that this
lower case usage means that normal English language rules apply and "should" is
proscriptive (as in, "you should not eat poison."
The authors of 8200 may want to comments, but I think you have contradicted this
with your 3.2 "SHOULD only appear once in the packet." You need to say "MUST NOT
appear more than once."

---

===Minor===

2.3
It is not clear to me whether a legacy node that forwards a packet that has an
SRH and which is not addressed to the legacy node is in the SR domain by your
definition. You say:
   We define the concept of the Segment Routing Domain (SR Domain) as
   the set of nodes participating into the source based routing model.
Those legacy nodes clearly do not participate in the model since they are
SRH-unaware, but I suspect you want to consider them as part of the domain. Or
are you saying that all nodes in an SR domain must be SRH-aware? (That would be
consistent with 2.3.1 and 2.3.2.)

Actually, it is a bit odd that this document is defining the SR Domain. 

(BTW s/into/in/)

But section 5.2 seems to make it clear that you consider a node that forwards a
packet that contains an SRH, but that forwards based only on the DA and without
inspecting the SRH to be a "node involved in a segment routing network."

But note Section 6 which seems to take a slightly different view of what the
domain is.
   o  Added by an SR edge router when entering the segment routing
      domain or by the originating host itself.  The source host can
      even be outside the SR domain;

6.2.2 stretches the definition still further.

6.3.2 seems clear that a node outside an SR domain can generate a packet that
contains an SRH and send it to an SR domain. That seems to be yet another
definition.

---

2.3.1
   Typically, an IPv6 packet received at ingress (i.e.: from outside the
   SR domain), is classified according to network operator policies and
   such classification results into an outer header with an SRH applied
   to the incoming packet.
You need to also mention the other cases that makes this "typically" and not
"always".

Possibly, however, you mean that 2.3.1 is the typical case and that 2.3.2 is the
other case?

---

3.
Where possible, you should refer to 8200 for the definition of fields so as to
not cause any confusion about redefinition.
You can do this for "Next Header" and "Hdr Ext Len". You already do it for
"Segments Left".

---

3. and 7.
You would be well advised to create a registry and assignment rules for the
Flags field.

---

3.

   o  Last Entry: contains the index, in the Segment List, of the last
      element of the Segment List.

To be consistent with section 5.1, you need to explain whether the index is
zero-based or starts at 1.
Looking at 5.1, it seems that you intend to be zero-based. That is, if n=1, you
would set Last Entry to 0.

(There is some discussion in another thread about the Segments Left field. You
should also make it clear whether this is zero-based or not.)

---

3.1
   The "Length" field of the TLV is primarily used to skip the TLV while
   inspecting the SRH in case the node doesn't support or recognize the
   TLV codepoint.

There is a hint here, but you need to describe how a node behaves if it does not
support or recognize a TLV. Can it always safely skip over it? Does that apply
to all TLVs under all circumstances?

---

3.1.1

Given that the information in ingress node TLV is also in the SA, you need to
explain what this TLV is for and why it is present.

You say:
      Defines the node where the packet is
      expected to enter the SR domain.
The use of the future tense is very odd here as the SRH is not present until the
packet enters the SR domain.

You say:
      In the encapsulation case
      described in Section 2.3.1, this information corresponds to the SA
      of the encapsulating header.
Presumably, in the case for 2.3.2, this corresponds to the source. You should
say so.

---

3.1.1, 3.1.2, and 3.1.3

Notwithstanding that you have defined no flags, don't you want an IANA registry?

---

3.1.2
   o  Egress Node: 128 bits.  Defines the node where the packet is
      expected to exit the SR domain.  In the encapsulation case
      described in Section 2.3.1, this information corresponds to the
      last segment of the SRH in the encapsulating header.

Given that a segment is not always a routing instruction and does not always
correspond to a node or interface address , this is ambiguous. You could be
saying that the last SID (i.e., the first in the stack) must be a node address,
or you could be saying that this TLV can hold any SID.

Given that this information is in the SRH already, you might explain why this
TLV adds value.

---

3.1.3

   o  Opaque Container: 128 bits of opaque data not relevant for the
      routing layer.  Typically, this information is consumed by a non-
      routing component of the node receiving the packet (i.e.: the node
      in the DA).

Do you intend to mean that sometimes this information *is* consumed by a routing
component? 
If so, you are contradicting 3.1 that has
   It has to be noted
   that the information carried in the TLVs is not intended to be used
   by the routing layer.

---

3.1.4
   o  Length: 0 to 7
   o  Padding: from 0 to 7 octets of padding.  Padding bits have no
      semantic.
Unless I am mistaken, a padding TLV with length 6 SHOULD NOT be used.

---

3.1.4
   o  Padding TLV is optional and MAY only appear once in the SRH.
No, you mean "MUST NOT appear more than once"

---

3.1.6
This TLV is underspecified. 8300 defines the existence of Metadata and that is
presumably what you want to carry here. If that is the case, then you need to
give a little more structure because NSH metadata has a metadata type and then
some data which may be structured as TLVs (type 2) or may be a fixed number of
bytes (type 1). You should also define how this data is used.

Frankly, a better approach would be to strike the definition of this TLV from
this document and move it to draft-xuclad-spring-sr-service-chaining

---

3.2

      Only the router whose address is in the DA field of the packet
      header MUST inspect the SRH.

What does this mean?
- The DA-addressed router MUST inspect the SRH, but other routers MAY
- The DA-addressed router MUST inspect the SRH, and other routers MUST NOT

---

4.1 and 4.2
   6.  ELSE
   7.    drop the packet
No ICMP?

---

4.1
   o  By default, a local SID bound to the End function does not allow
      the decapsulation of an outer header.  As a consequence, an End
      SID cannot be the last SID of an SRH and cannot be the DA of a
      packet without SRH.

   o  If the decapsulation is desired, then another function must be
      bound to the SID (e.g., End.DX6 defined in
      [I-D.filsfils-spring-srv6-network-programming]).

"By default" implies that there is a way to allow a local SID bound to the End
function to cause decapsulation. What is it?

As currently written, no way of doing decapsulation is provided in this
document. Is there a mechanism (i.e., a function) you can define in this
document so that this spec is complete?

---

5.1

      The packet is sent out towards the packet's DA (the first
      segment).

      HMAC TLV may be set according to Section 6.

      If the segment list contains a single segment and there is no need
      for information in flag or TLV, then the SRH MAY be omitted.

For tidiness, you have an ordering problem with these instructions.

---

Not sure where it should be stated (perhaps 5.3?) how a node that receives a
packet addressed to it (i.e., the DA) that contains an SRH and that does not
understand or support the SRH must act. I presume you will want to lean on 8200
for this.

Oh, I found this in section 6. That's OK. Maybe a forward pointer?

---

6.1.5

Looks good, but isn't an additional mitigation that is often applied to not
allow packets containing extension headers to enter your operational domain?
That makes 2.3.2 hard (without IP-in-IP encapsulation). So you should probably
state explicitly that you need that filtering to be turned off, and that this
makes the other mitigations in this section more important.

---

6.2
If I understand the trust model in this section, you have a single key shared
among all SRH-aware routers in the system (domain). The usage you describe is
that each router has a trust peering with its neighbor and so does not need to
(although is allowed to) verify the HMAC. From this you conclude that only the
ingress router to the domain needs to check the HMAC. But surely it is the
ingress to the domain that creates the HMAC, so it would be a bit odd for it to
check it. Thus, by your own logic, no node on the path needs to check the HMAC.

Actually, I agree with your logic, but that makes me wonder why you have the
HMAC TLV and whether anyone will ever use it.

But the chain of trust model you rely on is flawed in the use case described in
2.3.2. In this case, the packet with the SRH is forwarded across a set of nodes
that are outside the control of the SR domain. Those nodes may be malign and
while such nodes could do all sorts of other evil things, tweaking the SRH is a
way to cause unpredictable and possibly hard-to-trace errors when the packet
re-enters the SR domain. In those cases the HMAC is highly valuable and the
downstream SR-capable node SHOULD use it to validate the packet.

---

I don't buy paragraphs 2 and 3 of 6.3.3.
Inspecting the SRH of a single packet is quite a different thing from causing a
traceroute to enter and traverse the SR domain. So while you correctly describe
how to mitigate against traceroute, you have not described how to stop SRH
inspection.

There are three points of note:
1. Packets originating outside the domain and carrying an SRH (if this is
allowed after working on the definitions of "domain") can be inspected more
easily.
2. There is nothing much that can be done to hide the SRH 
3. Knowledge of the segments and TLVs (when they are not just routing
instructions) may give away information about the purpose of the packet and so
expose the user's usage patterns.

Number 3 may be a real issue and any form of packet snooping can get at this. It
would appear that privacy is the cost of putting end-point information in a
routing header.

---

6.3.4 has "As an SR domain is usually a subset of one administrative domain")

But 6.1.2 said "nodes within the SR domain, which is within one single
administrative domain"

Figure 1 gives the impression (but does not say explicitly) that the only use
case is a single administrative domain (since Figure 2 is only an overlay)

Some clarity is needed.

---

While it is not mandatory, the inclusion of an RFC 5706 manageability
considerations section in this document would be highly helpful.

---

7.1 asks for an Expert Review registry. RFC 8126 says you need to also write
some guidance for the Designated Experts.

---

It is certainly not mandatory, but reworking section 8 to follow the format and
guidance of RFC 7942 would make the section much more valuable.

===Nits===

Don't refer to this document as a "draft" as that text will be broken in a
published RFC.

---

Section 1 of all documents published as RFCs must be the Introduction.

---

Some of the references to I-D.filsfils-spring-srv6-network-programming make it a
normative reference.

---

Per the RFC style guide, the 2119 boilerplate should be moved to its own section
in the document. You should also use the correct boilerplate that references RFC
8174.

---

Abstract (ditto Section 2)
"SR allows to enforce"
Allows who/what to enforce?

---

2.1 (ditto Section 3)
   This document defines a new type of Routing Header (originally
   defined in [RFC8200]) called the Segment Routing Header (SRH)
That's ambiguous since is appears to say the SRH was originally defined in 8200.
Suggest reordering the words a little.

---

The plural of SID is SIDs not SID's

---

2.2.
   A local SID of N could be an IPv6 address of a local interface of N
   but it does not have to.
It does not have to what?

---

Need to expand abbreviations on first use. I saw:
SID
OIF
DA
SA
HMAC

---

2.3
s/Segment Routing header/Segment Routing Header/

---

3.1

   It has to be noted
   that the information carried in the TLVs is not intended to be used
   by the routing layer.  Typically, TLVs carry information that is
   consumed by other components (e.g.: OAM) than the routing function.

The first sentence tells us that the TLVs are not for the routing layer. The
second sentence is, therefore, a little superfluous (except for the example).

---

You have multiple occurrences of "optional" at least some of which are meant to
be "OPTIONAL"

---

4.2
   o  The End.X function is required to express any traffic-engineering
      policy.
Is this saying that End.X MUST express any traffic-engineering policy, or that
the purpose of End.X is to express any traffic-engineering policy?

(Note that this is the only mention of TE policy in this document, so the text
is not helpful.)

---

5.1

   The mechanism through which a Segment List is derived is outside of
   the scope of this document.  As an example, the Segment List may be
   obtained through:

Tempting and understandable though it is to give a list of example mechanisms,
you have rightly said that it is out of scope and you should probably scratch
the examples.

---

5.1

      The DA of the packet is set with the value of the first segment.
      ).

Typo or missing text?

---

5.1

s/Last_Entry/Last Entry/

---

6.2.2

s/home set-up box/home set-top box/   ????

---

6.2.2

      Downstream routers may not inspect
      the HMAC field.

s/may not/MUST NOT/ ?

---

From: "Darren Dukes (ddukes)" <[email-masked]>
Date: Mon, 23 Apr 2018 20:47:43 +0000

Hi Adrian.  Thanks for your review of rev11.

The following issue numbers were open by you and are closed in revision revision 12. Please respond to this email acknowledging the closures so Bob can record it.
#57<https://trac.ietf.org/trac/6man/ticket/57>, #58<https://trac.ietf.org/trac/6man/ticket/58>, #59<https://trac.ietf.org/trac/6man/ticket/59>, #60<https://trac.ietf.org/trac/6man/ticket/60>, #61<https://trac.ietf.org/trac/6man/ticket/61>, #63<https://trac.ietf.org/trac/6man/ticket/63>,

#62<https://trac.ietf.org/trac/6man/ticket/62> is not closed yet, sending me specific examples would be useful.

#64<https://trac.ietf.org/trac/6man/ticket/64> Still open.  It contains your minor issues/Nits.  19 of which are fixed/resolvable in rev12, 6 remain open.  I’ll send a separate email on this.

Thanks!
 Darren

---

From: "Adrian Farrel" <[email-masked]>
Date: Mon, 23 Apr 2018 22:29:29 +0100

Thanks Darren,
 
I'll work through these as promptly as I can.
 
Adrian
 
From: Darren Dukes (ddukes) [mailto:[email-masked]] 
Sent: 23 April 2018 21:48
To: [email-masked]
Cc: [email-masked]; [email-masked]; [email-masked]
Subject: Re: WG last call review of draft-ietf-6man-segment-routing-header

The following issue numbers were open by you and are closed in revision revision 12. Please respond to this email acknowledging the closures so Bob can record it.
 <https://trac.ietf.org/trac/6man/ticket/57> #57,  <https://trac.ietf.org/trac/6man/ticket/58> #58,  <https://trac.ietf.org/trac/6man/ticket/59> #59,  <https://trac.ietf.org/trac/6man/ticket/60> #60,  <https://trac.ietf.org/trac/6man/ticket/61> #61,  <https://trac.ietf.org/trac/6man/ticket/63> #63, 
 
 <https://trac.ietf.org/trac/6man/ticket/62> #62 is not closed yet, sending me specific examples would be useful.
 
 <https://trac.ietf.org/trac/6man/ticket/64> #64 Still open.  It contains your minor issues/Nits.  19 of which are fixed/resolvable in rev12, 6 remain open.  I’ll send a separate email on this.
 
Thanks!
 Darren