Re: [trill] Routing directorate review of draft-ietf-trill-channel-tunnel

[Donald Eastlake <[e-mail]>]

On Thu, Apr 28, 2016 at 9:26 AM, Jonathan Hardwick
<[e-mail]> wrote:
> Hello,
>
> I have been selected as the Routing Directorate reviewer for this
> draft. The Routing Directorate seeks to review all routing or
> routing-related drafts as they pass through IETF last call and IESG
> review, and sometimes on special request. The purpose of the review
> is to provide assistance to the Routing ADs. For more information
> about the Routing Directorate, please see
> http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir
>
> Although these comments are primarily for the use of the Routing
> ADs, it would be helpful if you could consider them along with any
> other IETF Last Call comments that you receive, and strive to
> resolve them through discussion or by updating the draft.
>
> Best regards
> Jon
> ===
>
> Document: draft-ietf-trill-channel-tunnel
> Reviewer: Jon Hardwick
> Review Date: 28 April 2016
> Intended Status: Standards Track
>
>
> Summary
>
> I have some concerns about this document and recommend that the
> Routing ADs discuss these issues further with the authors.
>
>
> Comments
>
> The draft is overall well written and the specification is quite
> easy to understand,

Thanks.

>                      but I found some of the terminology and
> rationale to be confusing.  I would prefer to see this clarified
> before the document is published as RFC.  Note that this is the
> first TRILL document I’ve reviewed, so my context comes largely from
> mailing list searches and the shepherd’s report.
>
>
> Major Comments
>
> The motivations for this draft are quite obscure from the
> perspective of the outsider J which makes it hard for me to evaluate
> the proposed mechanism.
>
> I think the problems that the draft solves should be more clearly
> spelled out.  From the introduction:
>
>    This document updates [RFC7178] and specifies extensions to RBridge
>    Channel that provide two additional facilities as follows:
>
>       (1) A standard method to tunnel a variety of payload types by
>           encapsulating them in an RBridge Channel message.
>
>       (2) A method to provide security facilities for RBridge Channel
>           messages.
>
> I think that number (1) requires more explanation because the
> RBridge channel already provides a standard method for a variety of
> payload types to be transmitted without needing the current draft.
> What tunneling capability is this draft adding?

Good point.

The RBridge Channel facility does provide a "protocol number" which
is, in essence, the "type" of its payload. However, there are three
limitations of RBridge Channel: (1) No security; (2) No way to
leverage the many existing defined Ethertypes as a payload type; and
(3) RBridge Channel can only send typed messages either (3a) between
RBridges in a campus and (3b) between end stations and RBridges on the
same link. Earlier versions of this draft included mechanisms
extensions in area 3, for example, for sending RBridge Channel
messages between end stations and RBridges not on the same link;
however, this added significant complexity and there appears to be no
current need for such extensions so they were dropped, leaving only
extensions in areas 1 and 2.

How about the following change on additional facility 1 in the draft:

OLD
      (1) A standard method to tunnel a variety of payload types by
          encapsulating them in an RBridge Channel message.
NEW
      (1) A standard method to tunnel payloads whose type may be
          indicated by Ethertype through encapsulation in RBridge
 Channel messages.

> A significant amount of text in the draft discusses number (2),
> which secures the channel payload, presumably to cover cases where
> the payload has no in-built security mechanism.  This appears to be
> the major purpose of the draft.  The draft achieves number (2) by
> adding a security shim header between the RBridge channel header and
> the payload.  One consideration in doing this is to remain backwards
> compatible with RFC 7178, and it looks like the working group has
> decided to achieve backwards compatibility by defining a new RBridge
> channel protocol type called “channel tunnel” – where this
> effectively means the RBridge channel payload contains an additional
> security shim which in turn contains an identifier that determines
> the real payload protocol type.
>
> I find the term “channel tunnel” misleading, as the draft does not
> appear to add any additional tunnelling capability above and beyond
> the tunnelling that can already be done using RFC 7178.  The draft
> actually describes an RBridge channel with enhanced security, so a
> term like “secure channel” would make more sense to me than “channel
> tunnel”.

OK, I understand why you think that term is misleading. While it seems
quite reasonable to called the added fields a "shim", note that the
facility currently called "Channel Tunnel" is quite closely integrated
with the existing RFC 7178 RBridge Channel facility. For example,
there is only one error reporting mechanism. Errors in the "Channel
Tunnel" facility added by this draft are reported as if they were
errors in the RBridge Channel messages to which the "shim" was added.

I don't actually like your suggestion of "secure channel" as a new
name.  How about re-naming the facility being added by this draft as
the "RBridge Channel Header Extension"?

I believe that RFC 7783 and only that RFC that references this draft
using the term "Channel Tunnel" but this is a very minor informational
passing reference. There are drafts in the publication requested state
that reference this draft using the term "Channel Tunnel" but it seems
that it would be relatively straightforward to change the name to
"RBridge Channel Header Extension" or some other new name in those
drafts and even easier to change it in drafts still under the control
of the TRILL WG.

> Minor Comments
>
> Section 3.1 – “Any particular use of the Null Payload should specify
> what VLAN or priority should be used when relevant.” – is unclear
> and no context for this statement is given.  Should be used by what
> and for what purpose?

OK. How about:

   Any particular use of the Null Payload should specify what VLAN or
   FGL and what priority should be used in the inner data label of the
   RBridge Channel message (or in an outer VLAN tag for the native
   RBridge Channel message case) when those values are relevant.

> Section 4.3 feels like a corollary to section 4.5 and so may be
> better placed as a subsection of 4.5.

The method of deriving keying material given in Section 4.3 is also
used in DTLS security as mentioned in Section 4.6 so I think it should
remain a separate section.

> Section 4.6 “The PType indicates the nature of the
> application_data.” - is potentially open to misinterpretation.  At
> face value it sounds like you are leaking some potentially sensitive
> information about the “nature” of the encrypted payload.  I think
> all you are actually saying is that it indicates whether the payload
> is an Ethertype, an Ethernet frame etc.  Suggest instead “In this
> case, the PType value in the RBridge Channel Tunnel Protocol
> Specific Data applies to the decrypted application_data.”

OK.

> Section 5.2 “with a payload type (PType) indicating a nested RBridge
> Channel message” – strictly all the PType can indicate is that the
> payload is Ethertyped; on its own it cannot indicate a nested
> RBridge Chanel message.  Suggest “and it contains a nested RBridge
> Chanel message”.

OK.

> Section 6.2
>
> “Section xxx of [RFC 7178]” should be “Section 3.2 of [RFC 7178]”.

Right. Sorry about that.

> Don’t you also need a new IANA registry for the “Rbridge Channel
> Error Subcodes” listed in table 5.2?

My opinion is that, for the first document in which you specify a
field and some values, it is a judgment call whether you should
create an IANA registry or not.  If you expect multiple groups to
start requesting values to multiple purposes, then creating a registry
from the start is the way to go. On the other hand, if a field is
internal to a particular protocol and you don't expect any new field
values to be assigned until there is a significant extension of that
protocol, I don't see any problem in deferring the registry creation
to the second document. This is the second document assigning values
for RBridge Channel Error Codes so it creates a registry for them. It
does not create a registry for SubERR field values.

> Nits
>
> Section 3.2
>
> “as describe in” -> “as described in”

OK.

> Section 4
>
> “not to met” -> “not to meet”

OK.

> 2nd paragraph – this sentence is quite long and hard to parse.

You're right. Looking at the sentence, it seems fairly easy to
simplify and split into two sentences. How about the following
replacement:

   The Channel Tunnel DTLS based security specified in Section 4.6
   below is intended for pairwise (known unicast) use. That is, the
   case where the M bit in the TRILL Header is zero and any
   Outer.MacDA is individually addressed.

> Section 4.2 & Section 5.1
>
> “As show in” - > “As shown in”

OK.

> Section 4.3
>
> “The use Derived Material” -> “The use of the Derived Material”

OK.

> Does Derived Material really need to be capitalized in this section?

Well, it is capitalized in the equation. Seems to me reasonable to
capitalize in both cases to indicate that a specific type of Derived
Material is being talked about.

> Section 4.5
>
> “can reasonable be” -> “can reasonably be”

OK.

> Section 4.6
>
> “minimum MTU Sz” -> “minimum MTU size”

Sz is a standard TRILL symbol widely used in TRILL documents and
defined in Section 1.1 of this draft. I would prefer to make the
following change in Section 4.6: "the TRILL campus wide minimum MTU
Sz" -> "Sz".

> “Actual application_data sent with Channel Tunnel” -> “Actual
> application_data sent within the Channel Tunnel”

OK.

> Why do you say “application_data” not “application data”?

"application_data" is the name of the field type in DTLS.

> Appendix Z should presumably be removed prior to IETF last call.

While I'm not sure how much help it would be to IESG members or IETF
participants in reviewing the draft, I don't see any reason for
Appendix Z to be removed until RFC publication. But at least a
notation asked the RFC Editor to delete it should be added.

Thanks,
Donald
=============================
 Donald E. Eastlake 3rd   [phone] (cell)
 155 Beaver Street, Milford, MA 01757 USA
 [e-mail]