Subject: [RTG-DIR] RTG Dir QA review of draft-ietf-nvo3-gue

From: "Adrian Farrel" <[email-masked]>
Date: Mon, 20 Jun 2016 15:52:30 +0100

Hi, 

I've been randomly selected from the Routing Directorate to perform a
QA review of this document. The philosophy behind QA reviews can be
found at https://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDirDocQa

In short, the purpose of the review is to uncover issues or fixing or
wider discussion earlier in the process than sometimes happens with
RTG Dir reviews around or even after WG last call. It is not my 
intention to be overly critical or harsh n my review, but I have tried
to raise everything I could think of - my intention is to allow you to
be able to say "Yes, that was looked at, discussed, and agreed."

Please do follow up with questions or discussion, but don't feel that
you have to convince me of things - you need to convince the WG.

Cheers,
Adrian

---

Being forced to read this document, I'm afraid I was required to enter a
third-party IPR disclosure because of the IPR already disclosed against
draft-ietf-mpls-in-udp that became RFC 7510. This should show up on the
NVO3 mailing list.

---

It seems to me that there is some careful coordination needed with other
work on encapsulation of transport or network protocols in UDP. This
idea clearly has value in NVO3, but I should have thought it sat better
in the TSVWG. I hope the NVO3 chairs have discussed this with the TSVWG
chairs to ensure that there is no friction. This is particularly 
important because it will be important to recognise that only one of
this draft and draft-manner-tsvwg-gut is likely to make it to RFC.

You seem to have correctly addressed the three issues that have most 
worried the TSVWG (checksum, congestion and security), so that is all 
good, but I would recommend getting the TSVWG involved for a full and 
detailed review now and for each future revision of the document. In
fact, I would have tended towards making this a TSVWG document, but so
long as the chairs, the ADs, and the WGs are happy, that should be fine.

---

Overall, this work is a good idea and needed. When we did MPLS-in-UDP
there was a background proposal to generalise and only burn one port
number for al UDP encapsulations. This achieves that end.

However, I think this proposal may be too general and too extensible.
Future-proof is good, but there seem to be a lot of bells and whistles
defined here that have no specific use proposed, and no indication that
a future use might ever be defined. I think it is one thing that it
should be possible to extend a protocol, and another that it defines
multiple fields and extension mechanisms that might never be used.

I comment on some of these mechanisms in my notes below.

---

In section 3.1, please add a forward pointer to section 6 instead of
"below"

      o Source port (inner flow identifier): This should be set to a
        value that represents the encapsulated flow. The properties of
        the inner flow identifier are described below.

Probably add a forward pointer each time "inner flow identifier" is
mentioned.
           
---

In 3.1 when describing the contents of the Proto/ctype field it would 
be helpful to b crystal clear of which set of IP protocol numbers you
are using. Maybe a reference to
http://www.iana.org/assignments/protocol-numbers/protocol-numbers.xhtml

---

Maybe add forward pointers to 3.2, 3.3, and 3.4 from 3.1.

Similarly 3.5 from 3.2.

---

I wonder whether you have possibly overdone the future-proofing since
you have defined no uses for flags nor any possible fields or extension
fields. The mechanism you have defined could actually be added later if
needed (although it is possible to believe this to not happen for a 
number of years) by assigning a flag (the E flag as now), defining a new
field to contain extension flags, and proceeding as described.

There is nothing wrong with what you have written, but it does seem to
complicate the base protocol with a very long-term extensibility
horizon.

---

3.3 has 

   New flags should be
   allocated from high to low order bit contiguously.

I am pretty sure you want s/should be/are to be/
Also, this text needs to be in the IANA section as well otherwise IANA 
will not know that they are constrained.

---

3.3

   Flags may be paired together to allow different lengths for an
   optional field. For example, if two flag bits are paired, a field may
   possibly be three different lengths. Regardless of how flag bits may
   be paired, the lengths and offsets of optional fields corresponding
   to a set of flags must be well defined.

This works, of course, but aren't you again being too flexible and too
clever? For a field that might have two lengths, you are not saving any
bits. Why not simply allow a field with two possible lengths to simply
be defined as two different fields?

---

3.3

   Flags (or paired flags) are idempotent such that new flags should not

Is that s/should not/do not/ ?

---

3.4                                                                  

   An encapsulator and decapsulator MUST agree on the meaning
   of private data before using it.

How? Using an OID? Using a control message? 

---

I am not enthusiastic about allowing "private data" in the packet 
header. I can see its use for specific functions that you have called
out (security and identifiers), but even then I am not too comfortable.
Actually, wouldn't security and identifiers by standard fields rather
than private data?

Recall that these UDP packets will be exchanged by many implementations
and the ideal is that every speaker should be able to understand every
packet. Also recall that things that might be used as covert channels 
are best avoided.

---

Don't you need an IANA registry to track control messages?

---

Version 0x01 of GUE is very "clever". I wonder whether it is really
necessary.

In any case, you should discuss it in the Introduction with an
explanation of what it is, and some motivation.

---

5.4 has

   The decapsulator validates packets, including fields of the GUE
   header. If a packet is acceptable, the UDP and GUE headers are
   removed and the packet is resubmitted for IP protocol processing or
   control message processing if it is a control message.

...but, of course, the contents of the GUE packet that is not a control
packet may be any protocol as indicated by the version number and proto
fields. Passing anything other than IP for IP protocol processing might
be considered a mistake :-)

---

In 5.4

   ...otherwise malformed
   header, it must drop the packet and may log the event.

That is better as MUST and MAY according to other usage in the draft.
You might do well to check all uses of must/should/may to check whether
they could/should be in upper case.

When saying "may log the event" it is traditional (and probably good) to
also give advice about thresholds and risks of log-swamping when under
attack.

Although "otherwise malformed" might cover it, I think you should call
out "unknown or unsupported payload protocol".

---

In 5.5

   It
   may encapsulate a GUE packet in another GUE packet, for instance to
   implement a network tunnel.

Doesn't that require a protocol number to be assigned for GUE?

---

In 5.6

   A middle box may interpret some flags and optional fields of the GUE
   header for classification purposes, but is not required to understand
   all flags and fields in GUE packets. 

I think you mean s/all/any of the/

---

5.6.1 has

   The source port set in the UDP
   header must be the destination port the peer would set for replies.

But 3.1 has

      o Source port (inner flow identifier): This should be set to a
        value that represents the encapsulated flow. The properties of
        the inner flow identifier are described below.

      o Destination port: The GUE assigned port number, 6080.

You can't achieve both and it would seem that the only way GUE can be 
"symmetrical" is to use the same source port in both directions.

---

The text in 5.6.2
   This method
   is problematic since ports numbers do not have global meaning
   ([RFC7605]) and a packet which is not GUE but destined to the same
   port number could be misinterpreted.
...sent me scurrying to 7605. I think the point is not that the port
   number does not have global meaning, but that "It is important to
   recognize that any interpretation of port numbers -- except at the
   endpoints -- may be incorrect, because port numbers are meaningful
   only at the endpoints," and "Ultimately, port numbers indicate 
   services only to the endpoints, and any intermediate device that
   assigns meaning to a value can be incorrect."

Maybe similar enough, but I think that the intent of 7605 is to say 
that a service may be run over many different port numbers so you can't
guarantee to find all instances of the service by looking for the port
number. I don't think the intent of 7605 is to say that something in the
network seen using port 6080 might not be GUE.

However, if you proceed with this you'll need to:
- resurrect draft-herbert-udp-magic-numbers
- make it  normative reference
- explain where the GUE magic number comes from

I think you would do well to reduce 5.6.2 to just an observation on
middlebox behavior, and remove all reference to draft-herbert-udp-magic-
numbers.

---

I suspect that discussing NAT as you do in 5.7 will not make you very
popular. It is true that NAT exists, and it is worth observing what 
would happen if a GUE packet went through a NAT. But I am not sure that
this is a problem to be solved in this document.

Indeed, since you don't actually solve it but only make suggestions 
about how it might be solved, I suggest reducing the text and saying 
that another document could be written in the future to describe NAT-
traversal for GUE packets.

BTW, where you say...
   connection semantics must be applied to a
   GUE tunnel as described above
... I think you are probably referring to section 5.6.2. You should be
explicit if that is the case, but consider my comments about 5.6.2.

---

5.8.2 has

    The GUE header checksum (in version 0x0) provides a UDP-lite
    [RFC3828] type of checksum capability as an optional field of the
    GUE header.

This is confusing! At first read we might assume you mean the checksum
field in the GUE header as shown in 3.1, but I think you are actually
calling that the UDP checksum. Reading between the lines, you are 
describing an optional field called the GUE Checksum that could be
included in the GUE header (if the corresponding flag is set). You need
to:

- fix the broken reference ([GUECSUM] or [REMCSUM]?)
- make the reference normative
- consider simply moving the description to this document.

---

5.9
Pay attention to the current discussion on the softwire and nvo3 lists.

---

6.2
      o An encapsulator may occasionally change the inner flow
        identifier used for an inner flow per its discretion (for
        security, route selection, etc). Changing the value should
        happen no more than once every thirty seconds.

I assume the limitation is because statistical load balancing will not
work if there is too much variance in hashable fields. However, 30 
seconds may be a very large number of packets. Is there any science
behind that value?

---

6.2
      o Decapsulators, or any networking devices, should not attempt any
        interpretation of the inner flow identifier, nor should they
        attempt to reproduce any hash calculation. They may use the
        value to match further receive packets for steering decisions,
        but cannot assume that the hash uniquely or permanently
        identifies a flow.

I agree with "should not attempt". But then you give an example of 
applying (limited) interpretation.

If the source port can change (even only once every 30 seconds) then 
what does it mean to "match further packets"? After all, between 29.9
and 30.1 seconds is only a short window, but during such time, any
such matching for steering would be invalid.

---

Hooray for section 7!
Could you please point to it from the Introduction because it will 
significantly help the reader.

Probably you should add 7510 to the long list of references.

---

Section 7 says
      o GUE permits encapsulation of arbitrary IP protocols, which
        includes layer 2 3, and 4 protocols. This potentially allows
        nearly all traffic within a data center to be normalized to be
        either TCP or UDP on the wire.
How so "normalized to TCP"? That doesn't seem to be mentioned anywhere
in this document, and so either needs a reference or to be dropped.

---

I'm not convinced that punting security to a separate document is the
best idea, and it will make progressing this document hard unless you
can get that other document adopted and well advanced.

---

Could you please get IANA to update the reference for port 6080 to
point to this document. And could you please update section 9 so that
when this document is published as an RFC IANA will update the registry
to point to the RFC.
                                                           
---

The IANA section will need some more work

   IANA is requested to create a "GUE flag-fields" registry to allocate
   flags and optional fields for the primary GUE header flags and
   extension flags. This shall be a registry of bit assignments for
   flags, length of optional fields for corresponding flags, and
   descriptive strings. There are sixteen bits for primary GUE header
   flags (bit number 0-15) where bit 15 is reserved as the extension
   flag in this document. There are thirty-two bits for extension flags.

I think you need to separate out the new registries rather than try to
put all of the stuff into one registry which wold then have an 
ambiguous name.

You should also;
- set out what you want IANA to track in tabular form so that they can
  reproduce it in the registry without any confusion
- pre-populate the registries with any values you have defined (such as
  the E flag)
- describe the allocation policies for each registry

---

I think a good number of your references are normative. These include...
[GUESEC]
[UDPMAG]
[REMCSUM] or [GUECSUM] if it exists

---

It is always helpful to state at the top of an Appendix "This appendix
is informational and does not constitute a normative part of this
document."

Yeah! I'm always grumpy when I'm asked to do that, but it does actually
help get it past the IESG, and it might even help the reader.

---

From: Tom Herbert <[email-masked]>
Date: Mon, 27 Jun 2016 16:06:37 -0700

Hi Adrian,

Thanks for the detailed and insightful comments! Some replies in line

> draft-ietf-mpls-in-udp that became RFC 7510. This should show up on the
We saw that. Note that this applies to several other UDP encapsulation
protocols.

> this draft and draft-manner-tsvwg-gut is likely to make it to RFC.
Much of the content around TSV issues (e.g. zero UDP6 checksum and
congestion control) is based on that in MPLS/UDP and GRE/UDP.

> multiple fields and extension mechanisms that might never be used.
Yes, two of the authors on this draft are also authors for GRE/UDP.
GRE/UDP has received excellent review in TSVWG and we would like to
have similar review of GUE.

>         the inner flow identifier are described below.
Okay.

> Probably add a forward pointer each time "inner flow identifier" is
Okay.

> http://www.iana.org/assignments/protocol-numbers/protocol-numbers.xhtml
Okay. Offhand, all protocol numbers except for IPv6 extension headers
are normally allowed. Protocol #59 is allowed even with IPv4.

> Maybe add forward pointers to 3.2, 3.3, and 3.4 from 3.1.
Okay.

> field to contain extension flags, and proceeding as described.
Extensions have been defined in several other drafts. We are
consolidating a core set into one draft (fragmentation, security,
payload transform, checksum, and remote checksum offload). An
additional option for network virtualization is defined in
draft-hy-gue-4-secure-transport.

Agree that it is unnecessary to define E-bit now, with above
extensions we've currently proposed allocating seven bits so we still
have eight free (assuming E-bit). I think we should just mention this
as the mechanism when we need to extend the number of flag bits.

> will not know that they are constrained.
Okay.

> be defined as two different fields?
Then we have to process it as two different fields. We currently have
defined security flags to be two bits allowing three possible sizes
(64, 128, or 256bits). Also this allows for creating multibit fields
in the flag bits themselves, similar to how Recur is defined in GRE.

> Is that s/should not/do not/ ?

I think "must not" is appropriate.

> How? Using an OID? Using a control message?
It's implementation specific. For many deployments, like ours at
Facebook, configuration is sufficient for such things. GUE does not
require control messages for normal protocol operation, they could
defined (some C-bit=0 messages) but that is out of scope for this
document. The key point is that agreement and meaning of private data
is matter between the two communicating endpoints. GUE does not
preclude the development of a commonly used definitions for private
data, say for instance a set of TLVs like Geneve might define, but we
shouldn't need to declare that in the base protocol.

> Actually, wouldn't security and identifiers by standard fields rather
Yes, security and identifiers are being defined as well known
extensions. The reason for having a private data section is to allow
implementations or sites to extend the protocol for their own
purposes. They may or may not intend standardize. What we don't want
to happen is that people randomly reserved bits for their own purposes
so that in the future we we need to define a standard use for them we
can't because there is some significant deployment already using them.
This is especially a concern for an nvo3 protocol which is more of DC
protocol then protocol used on the Internet so we anticipate more
"customization".

> Don't you need an IANA registry to track control messages?
Yes.

> Version 0x01 of GUE is very "clever". I wonder whether it is really
We think it's clever too :-). We didn't initially design GUE with the intent
of doing this. This request came up on int-area list. Motivation is
that IPv4 and IPv6 are probably most common payloads, this save 4
bytes (as a form of GUE hdr compression), and this obviates the need
for define IP-in-UDP as a separate protocol with separate port
numbers.

> explanation of what it is, and some motivation.
Okay.

> be considered a mistake :-)
Okay.

> they could/should be in upper case.
Okay.

> also give advice about thresholds and risks of log-swamping when under
Do you know if there is an RFC to reference?

> out "unknown or unsupported payload protocol".
Okay.

> Doesn't that require a protocol number to be assigned for GUE?
Technically, no, This could look like IP|UDP|GUE|UDP|GUE. Will clarify.

> I think you mean s/all/any of the/
Yes.

> "symmetrical" is to use the same source port in both directions.
3.1 text is a "should" for this reason. Will clarify that symmetric
port number for NAT is an exception to source port assignment.

> network seen using port 6080 might not be GUE.
Actually, I think that is exactly what it means.

> - explain where the GUE magic number comes from
Yes, there has been more discussion related to the UDP service
identification problem on the SPUD lists. We borrowed the idea of the
magic-number from draft-hildebrand-spud-prototype, but I have become
less enthralled with it over time. I'm inclined to remove the concept
of it from GUE draft.

> this is a problem to be solved in this document.
We can take the section. We now have Transport-over-UDP (
draft-herbert-transports-over-udp) which would cover that in depth.

> - consider simply moving the description to this document.
This is described in the core extensions draft so it can be taken out.

> Pay attention to the current discussion on the softwire and nvo3 lists.
Okay. We should also reference the GUE fragmentation option.

> seconds may be a very large number of packets. Is there any science
No :-) Just that thrashing does not seem to be a great idea. The Linux
implementation already does this, after N retransmissions of a TCP
connection we choose new flow identifiers (src port for UDP, IPv6 flow
label, etc.) in hopes of finding a better path.

> applying (limited) interpretation.
It's the semantics of "interpretation". Will try to clarify.

> such matching for steering would be invalid.
No. If a devices choose to try to match groups of packets in some
stateful way it can only do this as soft state. It cannot assume any
persistence the tuple it matches. The 30 second rule is only a should,
a node can change the src port for packet if it desires (I suspect
there may be extreme case where security concerns might warrant that).

> significantly help the reader.
Okay.

> Probably you should add 7510 to the long list of references.
Okay.

> in this document, and so either needs a reference or to be dropped.
It's a reference the fact that network HW tend to optimize for UDP and
TCP, so the consequence is that networks would only use UDP or TCP.

> can get that other document adopted and well advanced.
The separate item is an option for stronger security. It is not
required for deployment. We can elaborate on security issues for the
base GUE draft (like use IPsec if you're paranoid, etc).

> when this document is published as an RFC IANA will update the registry
Okay.

> - describe the allocation policies for each registry
Okay.

> is informational and does not constitute a normative part of this

Okay.

> help get it past the IESG, and it might even help the reader.

Awesome review and very helpful!

Thanks,
Tom

---

From: "Adrian Farrel" <[email-masked]>
Date: Tue, 28 Jun 2016 10:09:52 +0100

Hi Tom,

Thanks for engaging.

Some snipping...

> We saw that. Note that this applies to several other UDP encapsulation

You might (or might not) want to enter third-party disclosures yourself against those other I-Ds which I am fortunate enough to have not been involved with.

> are normally allowed. Protocol #59 is allowed even with IPv4.

I think you will have this covered, but to be clear, I didn't mean what subset of protocols, I meant which set of numbers used to identify protocols (e.g., not Ethertypes).

> draft-hy-gue-4-secure-transport.

OK. Maybe some text like...
This document does not define any extensions, but a number of extensions are defined in [reference].

And then add an Informative reference.

> in the flag bits themselves, similar to how Recur is defined in GRE.

Hmmm, well, OK.
If you've thought this through.

> shouldn't need to declare that in the base protocol.

OK. Got it.
Maybe a few words like...

   An encapsulator and decapsulator MUST agree on the meaning
   of private data before using it. The mechanism to achieve this
   agreement is outside the scope of this document but could 
   include implementation-defined behavior, coordinated
   configuration, in-band communication using GUE control
   messages, and out-of-band messages.

> protocol then protocol used on the Internet so we anticipate more

Yeah, I get the use case. I just don't like it :-)
It's a hard line to draw. There's payload and there's overhead. I don't see that there is a way here to make people do the right thing - perhaps we shouldn't worry about that?

An option is to define a new payload protocol and put it all in there. It's marginal.

Anyway, I still think you might consider the use of an OID to help prevent surprises while processing what is otherwise unstructured/unknown data.

> Do you know if there is an RFC to reference?

Hmm, I don't have a reference to this being required or advised.
I do have an example off the top of my head...
RFC 7274 section 3.1.1

The thinking here is that logging usually uses more resources than a data plane message, so it is possible to swamp a system through a relatively easy attack. Thus rate limiting on logs is a good precaution. Or event counts rather than logs, etc.

> Technically, no, This could look like IP|UDP|GUE|UDP|GUE. Will clarify.

Aha! Thanks.

> port number for NAT is an exception to source port assignment.

Thanks

> less enthralled with it over time. I'm inclined to remove the concept

Wfm.

> label, etc.) in hopes of finding a better path.

OK, maybe say "desirable to avoid thrashing or flapping the value".

Possibly say "should be configurable" and "a default value of 30 secs is recommended"

> TCP, so the consequence is that networks would only use UDP or TCP.

Ah, OK.
So GUE allows normalization to UDP and that, in turn, allows a DC hardware to be normalized to TCP or UDP.

> base GUE draft (like use IPsec if you're paranoid, etc).

Thanks, that would be good.
That means this document would describe "base security" (which should be "good enough") with a pointer out to security document for "advanced security".

> Awesome review and very helpful!

You're welcome.
I expect I am now your "designated RTG reviewer" for this document for the rest of its life, so we will probably run into each other again during the lifecycle. I will try to retain cache :-)

Best,
Adrian
--
Support an author and your imagination.
Tales from the Wood - Eighteen new fairy tales.
http://www.feedaread.com/books/Tales-from-the-Wood-9781786100924.aspx
http://www.amazon.co.uk/Tales-Wood-Adrian-Farrel/dp/1786100924
Or buy from me direct.

---

From: Tom Herbert <[email-masked]>
Date: Tue, 28 Jun 2016 10:10:07 -0700

> Anyway, I still think you might consider the use of an OID to help prevent surprises while processing what is otherwise unstructured/unknown data.
If we go this route I would suggest we add a "TLVs present" bit that
would indicate that the private data region contains well known TLVs.
The TLV format and name space could be based on those defined in
Geneve. With this Geneve protocol would then effectively be a subset
of GUE functionality, and conceptually is a way to unify two of the
three nvo3 proposed protocols.

Tom