Re: [Idr] Benjamin Kaduk's Discuss on draft-ietf-idr-flow-spec-v6-19: (with DISCUSS and COMMENT)

[Benjamin Kaduk <[e-mail]>]

Hi Christoph,

On Wed, Nov 04, 2020 at 12:41:13PM +0100, Christoph Loibl wrote:
> Hi Benjamin,
> 
> Thanks for your review of the document. I have edited the document and I think cleared your discuss with the changes (please see my comments inline). 

Also inline, though I will not specifically comment on the (many) bits that
look good as-is.

> Since I have a few more changes to apply the new version is not yet online (however I attached the modified version to this message). Since the draft submission is closed now I will ask Alvaro to publish the changed document today.
> 
> Cheers  Christoph
> 
> 
> > ## DISCUSS:
> > A fairly minor point, but I think that allowing Type 13 (flow label)
> > component values to be encoded as 2-byte quantities encourages the
> > selection of non-random flow label values, and thus violates the
> > guidance from RFC 6437 that these values "should be chosen such that
> > their bits exhibit a high degree of variability" and that "third parties
> > should be unlikely to be able to guess the next value that a source of
> > flow labels will choose." While having the short 1-byte encoding for a
> > flow label of 0 might be reasonable, a 2-byte label can represent at
> > most 16 bits of the 20-bit identifier space, discouraging the use of the
> > high 4 bits, when such bits of unpredictability are scarce already.
> > Let's discuss how big an issue this is and what might be done to
> > mitigate it.
> 
> The Type-13 component is not the flow label itself. It is only the filter that allows to match on that particular field in the IPv6 header of the packets. If the flow-label encoded in the packet (unlikely, but possible) can be encoded in 8,16-bits one could encode the match filter in a shorter form. However, this may be very unlikely. This specification does not choose those flow labels, but allows to match on flow labels in packets (usually generated by other systems).   

I understand that this specification is describing patterns to match flow
labels and not the flow labels themselves, yes.  However, this document is
saying that, *if* the flow labels themselves have a certain structure, then
the flow specification (from this document) to match the flow labels has a
more compact encoding.  This, in turn, sets up an incentive for the party
assigning flow labels to conform to the specific structure of flow label
values, in order to take advantage of the more compact encoding.  I believe
that using any kind of substructure when assigning flow label values is
contrary to the guidance of RFC 6437 (and, relately, harms the overall
security of the internet), and thus am supplying resistance to a mechanism
that incentivizes using such internal structure in flow label values.
The discussion that I would like to have here relates to the magnitude of
incentive in question (to use specific substructure when generating flow
label values), which in turn is related to how much benefit there is from
shaving two bytes off the flow specification encoding.  I do not believe
that your comment above is trying to make such an assessment on the
magnitude of the incentive in question or the value of having the slightly
more compact encoding available.

> > Please also confirm that we are providing all the information required of
> > us by RFC 5701 and 5575bis (see comments on Section 6.1); I am not sure
> > whether I am reading the references correctly in these regards.
> 
> We added the definition of the Local Administrator Field in the Extended community to be aligned with the IPv4 rt community:
> 
> ADDED:
>        The Local Administrator sub-field contains a number from a numbering
>           space that is administered by the organization to which the IP
>           address carried in the Global Administrator sub-field has been
>           assigned by an appropriate authority. 
> 
> > 
> > There seems to be an error in the sample code (flow_rule_cmp_v6()): the
> > snippet
> > if comp_a.offset < comp_b.offset:
> > return A_HAS_PRECEDENCE
> > if comp_a.offset < comp_b.offset:
> > return B_HAS_PRECEDENCE
> > duplicates the condition, whereas the condition should be swapped for
> > correct operation.
> 
> This in fact is a problem in the code that has been corrected, and a test has been added.

I am happy to hear that a test has been added in addition to fixing the
bug.

> > ## COMMENT:
> > Thanks for this straightforward and easy-to-read document! Just a few
> > minor comments.
> > 
> > Section 3.3
> > 
> > This component uses the Numeric Operator (numeric_op) described in
> > [I-D.ietf-idr-rfc5575bis] Section 4.2.1.1. Type 3 component values
> > SHOULD be encoded as single octet (numeric_op len=00).
> > 
> > Why only SHOULD? (Likewise for Section 3.4, 3.5, etc.)
> > 
> 
> This is because we want these types to be aligned with the IPv4 types as much as possible in order to allow for reusable code.

Ah, of course.  I should have checked that before commenting; sorry.

> > Section 3.7
> > 
> > Contains a list of {numeric_op, value} pairs that are used to match
> > the 20-bit Flow Label IPv6 header field ([RFC8200] Section 3).
> > 
> > [It seems that RFC 8200 §3 just points to RFC 8200 §6, which itself
> > mostly points to RFC 6437. I don't know if it is useful to
> > short-circuit some of those references.]
>  
> Flow Spec does not care about what and how this Flow Label is actually encoded. It matches against this particular 20-bit field in the header. I think it is better to keep this reference.
> 
> > Section 3.8.1
> > 
> > The following example demonstrates the prefix encoding for: "packets
> > from :🔢5678:9A00:0/64-104 to 2001:DB8::/32 and upper-layer-
> > 
> > Where is the "/64-104" notation introduced? I did not see it in RFC
> > 4291. (It also appears in §3.8.2, though the latter uses "/65-104"
> > to demonstrate the operation of padding.)
> 
> Appended this to the Type-1 compontent section:
> 
>    Note: This Flow Specification component can be represented by the
>    notation ipv6address/length if offset is 0, or ipv6address/offset-
>    length.  The ipv6address in this notation is the textual IPv6
>    representation of the pattern shifted to the right by the number of
>    offset bits.  See also Section 3.8.

Thanks!

> > 
> > Section 4
> > 
> > The definition for the order of traffic filtering rules from
> > [I-D.ietf-idr-rfc5575bis] Section 5.1 is reused with new
> > consideration for the IPv6 prefix offset. As long as the offsets are
> > equal, the comparison is the same, retaining longest-prefix-match
> > semantics. If the offsets are not equal, the lowest offset has
> > precedence, as this flow matches the most significant bit.
> > 
> > I will note just to confirm my understanding that this procedure seems
> > to give higher precedence to, e.g., 1234::/1-4 than to
> > :🔢5678:9a00/81-128 even though the latter is inspecting more bits
> > of the address/prefix. (To be clear, some choice has to be made and I
> > have no reason to prefer a different one over this one, I am just
> > exploring the consequences of this choice.)
> 
> Yes this is the case. In general the lower offset generates a lower number of consecutive IPv6 address blocks that would match (offset=0 matches IP addresses only from a single prefix, offset=1 matches 2 blocks, offset=2 4, ...), this can be extended to offset=127 which matches every second IPv6 address. 
>  
> > It is again (as per 5575bis) surprising that we allow for a "not-equal"
> > comparison of differently encoded (e.g., flow label) values that have
> > the same semantic meaning, but I expect the same response (and no
> > document change) as when I made the comment the first time.
> 
> While we add IPv6 functionality into Flow-Spec (rfc5575bis) changing any of the fundamental behaviour is out of the scope of this document. 
> 
> > Section 5
> > 
> > The validation procedure is the same as specified in
> > [I-D.ietf-idr-rfc5575bis] Section 6 with the exception that item a)
> > of the validation procedure should now read as follows:
> > 
> > Are there also any AFI/SAFI differences from 5575bis to consider in
> > terms of "routes received over"/etc.?
> > 
> 
> Flow-Spec V6 uses a different AFI/SAFI but this is already covered in the rfc5575bis text when it comes to validation:
> 
>    The validation process described below validates Flow Specifications
>    against unicast routes received over the same AFI but the associated
>    unicast routing information SAFI:

Thank you for the extra explanation; I suspected that there was something
like this that I was missing.

> 
> > Section 6.1
> > 
> > This extended community uses the same encoding as the IPv6 address
> > specific Route Target extended community [RFC5701] Section 2 with the
> > high-order octet of the Type always set to 0x80 and the Sub-Type
> > always TBD.
> > 
> > RFC 5701 suggests that since we are allocating the "TBD" sub-type value,
> > we should also state what the semantics of the "local administrator"
> > 2-octet field are.
> > 
> 
> This has been edited into the document. See also the DISCUSS section.
> 
> > Interferes with: All BGP Flow Specification redirect Traffic
> > Filtering Actions (with itself and those specified in
> > [I-D.ietf-idr-rfc5575bis] Section 7.4).
> > 
> > I think we are supposed to also state what action to take when
> > encountering interfering actions.
> 
> This is already in rfc5575bis Section 7.7, and also applies to this document.
> 
> > 
> > Section 7
> > 
> > It was a little surprising to me that the inability to match (e.g.) TCP
> > flags or Port values on non-initial IP fragments was not reiterated in
> > the security considerations of 5575bis. I don't think it would make
> > sense to mention that just in this document's security considerations,
> > though -- if we are to change anything it should be in 5575bis. But I
> > guess we had that conversation already, at
> > https://mailarchive.ietf.org/arch/msg/idr/zvpLPjllvKWFuyraZUVz8lB8Wz0/
> > and thus no change is expected.
> 
> While IPv4 and IPv6 fragmentation serves the same pupose the encoding is different so we needed to redefine thi section. FS is working stateless on the packets and can only match on what is in the packet.

Exactly.  I am proposing that this restriction (working stateless and
cannot match [application-layer headers] things not in the packet) is
mentioned again in the security considerations section.  I believe that
this limitation can be described in a way that is agnostic to IPv4 vs IPv6,
and thus would be better placed in 5575bis than in this document.

> I cannot access the url from above.

Huh, interesting.  Perhaps if you log in with your datatracker credentials?
I had specifically tried to find one in the idr archive, since the iesg
archive is definitely non-public.
The referred-to message is "Re: [Idr] Benjamin Kaduk's Discuss on
draft-ietf-idr-rfc5575bis-22: (with DISCUSS and COMMENT)" from  Fri, 24
April 2020 02:12 UTC, Message-ID: <[e-mail]>.

Thanks again,

Ben

> > 
> > Section 8.1.2
> > 
> > (side note) it seems a little weird to have the IPv4 version be the one
> > that gets the unqualified (e.g., "Destination Prefix") name.
> 
> This is because we are not changing names of rfc5575bis to avoid confusion.
> 
>