[Pals] AD review of draft-ietf-pwe3-iccp-stp

["Adrian Farrel" <[e-mail]>]

Hello authors,

I have done my usual AD review of your draft. The purpose of the review
is to ensure that the document is ready to go forward for IETF last call
and IESG evaluation, and to catch any issues before those stages in the
process.

My review has uncovered a number of issues that I would like you to
address before I advance the document. You may make the changes and
post a new revision, or you can discuss the issues with me and tell me
I am wrong (very happy if you do that! :-)

Additionally, if you can find someone who is interested in this work who
can perform an English language review for you that would be really
helpful. If not, the RFC Editor will try to fix up the English, but
there is a risk that they will break the technical meaning when they
process the text.

I have asked the PALS working group for a volunteer, and Ignas Bagdonas
([e-mail]) has offered to help.  Please contact him
direct. But you might also know someone who could help with this.

I suggest that you wait for this review and then combine it with the
fixes to the comments I have noted below. In the meantime, I will mark
the I-D as "revised I-D needed".

Thanks for the work,

Adrian

====

idnits notes some problems with the references.

 == Missing Reference: 'RFC5226' is mentioned on line 981, but not defined

 == Unused Reference: 'RFC6310' is defined on line 1032, but no explicit
    reference was found in the text

 == Outdated reference: draft-ietf-pwe3-iccp has been published as RFC 7275

---

The document shepherd noted:

> Also, as I was writing this up, I noticed in several places that there
> was a consistent typo that transposed "RFC 7275" as "RFC 7257".
>
> There is one additional typo I noticed during final review, for
> consistency with RFC 7275, the term "Redundant Group (RG)" should be
> "Redundancy Group (RG)". This comes up twice in the draft. Again, this
> can be fixed by the RFC Editor.

Please fix these issues now.

---

Your IANA considerations section needs some work.

You need to say:

   The IANA maintains a top-level registry called "Pseudowire Name
   Spaces (PWE3)".  It has a sub-registry called "ICC RG Parameter
   Types".

   IANA is requested to make 13 allocations from this registry as
   shown below.  IANA is requested to allocate the codepoints in a
   sequential block starting from the next available value in the 
   range marked for assignment by IETF review 0x2000-0x2FFF).  All
   assignments should reference this document.

      Parameter Type Description
      -------------- ---------------------------------
      TBD1           STP Connect TLV
      TBD2           STP Disconnect TLV
      TBD3           STP System Config TLV
      TBD4           STP Region Name TLV
      TBD5           STP Revision Level TLV
      TBD6           STP Instance Priority TLV
      TBD7           STP Configuration Digest TLV
      TBD8           STP Topology Changed Instances TLV
      TBD9           STP STP CIST Root Time TLV
      TBD10          STP MSTI Root Time TLV
      TBD11          STP Synchronization Request TLV
      TBD12          STP Synchronization Data TLV
      TBD13          STP Disconnect Cause TLV

Now you will need to go through the document and replace the specific
numbers you have used with the "TBD" indicators in this table.

---

3.3.4 has two fields that need more explanation...

      - Pri

        The Instance Priority

How is this field interpreted? Does a higher or lower value indicate a
higher priority?

      - InstanceID

        The instance identification number of the MSTI.

Where does this value come from and how is it encoded (presumably in line
format).

---

In 3.4.1

      - InstanceID List

        The list of the instances whose topology is changed as indicated
        by the Topology Change Notification (TCN) Messages as specified
        in [802.1q] Section 13.14.

Assuming this is a list of InstanceID values as found in 3.3.4 you need
to explain how this list is formed.

Does a list with two entries look like
    0                   1                   2
    0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |      InstanceID#1     |      InstanceID#2     |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Or like...

    0                   1                   2                   3
    0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   | InstanceID#1          | rsvd  | InstanceID#2          | rsvd  |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Or like...

    0                   1                   2                   3
    0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   | rsvd  | InstanceID#1          | rsvd  | InstanceID#2          |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

If the first case, how do you set the length field for the TLV?

---

In 3.4.2, I think you can say the exact vale of Length.

---

Also in 3.4.2 you have four fields conveying time values. You need to
tell us what units they are in.

---

Section 3.4.3 defines a TLV with a strange name.  The "STP MSTI Root
Time" has no concept of "time" carried in it. Perhaps the remaining
hops refers to the "Time To Live"? It would be nice if the text
explained this.

---

Also in 3.4.3 I think you can say the exact value of Length.

---

Also in 3.4.3 there is an InstanceID using 16 bits, but it appears
(from 3.3.4) that InstanceIDs only use 12 bits.

---

Section 3.5

      - Request Type

        14-bits specifying the request type, encoded as follows:

           0x00   Request Configuration Data
           0x01   Request State Data
           0x3FFF Request All Data

Curiously, this is a 16-bit field. Can you help us to understand where
the 14 bits of information are placed in the field.

---

Section 3.6

      - Flags

        2 octets, response flags encoded as follows:

           0x00 Synchronization Data Start
           0x01 Synchronization Data End

Are you saying you cannot set both "Data Start" and "Data End" on the
same message? If so, then what you have defined is one flag.

    0                   1                   2                   3
    0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |U|F|   Type=0x004B             |    Length                     |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |     Request Number            |     Flags                   |S|
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

   The S-flag occupies the least significant bit of the Flags field and
   is interpreted as follows:

           S = 0 : Synchronization Data Start
           S = 1 : Synchronization Data End

But if you allow start and end on the same message you need two flags.

    0                   1                   2                   3
    0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |U|F|   Type=0x004B             |    Length                     |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |     Request Number            |     Flags                 |E|S|
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

   The S-flag is interpreted as follows:

           S = 1 : Synchronization Data Start

   The E-flag is interpreted as follows:

           E = 1 : Synchronization Data End

   If is not valid for both flags to be clear at the same time.

Please decide which you mean.

Now, also please tell me about the other Flags. Will they ever be
defined?
- If "yes", then I think you need a new IANA registry, and you should
  point to the new subsection of Section 6 that you will need to write.
- If "no", then I think you should mark the field as Reserved and not
  Flags.

---

While the ICC security considerations apply here, I think Section 5 is
a little thin. It seems to me that using this approach, an attack on ICC
(running in the provider's network) can break not only the ability to
deliver traffic across the provider's network, but the ability to route
traffic within the customer's network. That is, careful attack on ICC
can break STP within the customer network.

Perhaps I am wrong, or perhaps you need to discuss this.