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

[Robert Raszuk <[e-mail]>]

Ok 3 octets to keep byte boundary sounds reasonable indeed.

> to keep the flow label matching functionality

On that point I have a mixed feelings.

On one hand original thought for adding this into flow-spec was based on
manual setting of flow-labels.

Now if that setting is recommended to be automated and random I am really
afraid that any attempt to retrieve flow label values and to signal them
with BGP FS may result in massive burden to BGP - at any given time there
can be millions of flows. Leave alone dynamic nature of those short lived
flows.

Then even if BGP can signal it, RRs can propagate it then the attempt to
install such number of ACLs into FS receiver's data plane will either fail
or the box will crash.

Cheers,
R.

On Sat, Nov 7, 2020 at 1:02 AM Erik Kline <[e-mail]> wrote:

> FWIW, I had assumed that the 1/2/4 byte formats were for encoding the
> non-zero parts of a flow label, i.e. flowlabels 1-255 could fit in 1 byte,
> 256-65535 in 2 bytes, and everything else up to 2^20-1 in 4 bytes.
>
> If that's true, it seemed reasonable to me.  Also reasonable, I would
> think, is to just specify, say, a fixed 3 byte unsigned int.
>
> I think it would be valuable to keep the flow label matching
> functionality, not least because it can be used to group into a flow the
> fragments where the upper layer header info is not present.
>
> On Fri, Nov 6, 2020 at 3:01 PM Robert Raszuk <[e-mail]> wrote:
>
>> Hi,
>>
>> For the record of part of this discussion regarding size of flowspec v6
>> flow label field I am perfectly ok to have it fixed size of 20 bit field.
>>
>> As a matter of fact if we assume that flow label allocation nature is to
>> be dynamic by IETF recommendation I am also perfectly fine to remove flow
>> label match from flow spec v6 all together.
>>
>> Kind regards,
>> Robert.
>>
>> On Wed, Nov 4, 2020 at 5:36 PM Benjamin Kaduk <[e-mail]> wrote:
>>
>>> 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.
>>> >
>>> >
>>>
>>>