Re: [IPsec] AD Review of draft-ietf-ipsecme-iptfs-12

[Christian Hopps <[e-mail]>]

Hi Roman,

Thanks for the review!

I've made some changes in the document to cover most of your suggestions and have a few comments/questions on the rest below.

Roman Danyliw <[e-mail]> writes:

> Hi!
>
> I performed an AD review of draft-ietf-ipsecme-iptfs-12.  Thank you for this work and the patience of the WG in getting it processed.
>
> I have a number of comments below, but the document is in good shape so please process them concurrently with the IETF LC feedback.
>
> ** Thank you for getting an early TSVART review and addressing that feedback given congestion control issues this work raises
>
> ** Section 1.  Editorial.
>
> OLD
> While directly
>    obscuring the data with encryption [RFC4303], the traffic pattern
>    itself exposes information due to variations in its shape and timing
>
> NEW
> While directly obscuring the data with encryption [RFC4303], the patterns in the message traffic may
> exposes information due to variations in its shape and timing.

Ok.

> ** Section 2.1.  Typo. s/the the/the/

Fixed.

> ** Section 2.4.2.1.
>
> Enabling circuit breakers is also a reason a user
>    may wish to enable congestion information reports ...
>
> Consider if s/a user may wish to/local policy may/ to generalize who is doing
> the tuning. There are a few other places were "user" is the noun tuning the
> stack.

If you feel strongly about this I will change it; however, I did consider it, and would prefer to leave them as "user". It's always a user doing the configuration at some point in the process, it's specifically talking about why one may wish to turn something on, and it's also a bit more "active voice" which I was taught is more engaging/readable.

> ** Section 3.
>
> Thus, to support
>    congestion control the receiver must have a paired SA back to the
>    sender
>
> Should the be a "MUST" (i.e., s/receiver must have/received MUST have)?

Changed.

> ** Section 3.
>
> If the SA back to the sender is a non-AGGFRAG_PAYLOAD
> enabled SA then an AGGFRAG_PAYLOAD empty payload (i.e., header only)
>    is used to convey the information.
>
> I'm missing something -- if the SA is not AGGFRAG_PAYLOAD-enabled, what how can it send anything AGGFRAG related?

In ESP the payload is given by the next header field. Sending an empty AGGFRAG_PAYLOAD (header only) in ESP doesn't require enabling AGGFRAG_PAYLOAD on the SA, you just set the ESP next-header value appropriately.

Enabling is not "allowing AGGFRAG_PAYLOADs in ESP", it's saying to the receiver, "I'm going to send you all my traffic encapsulated in AGGFRAG_PAYLOADs".

> ** Section 4.
>
> All IP-TFS specific configuration should be specified
>    at the unidirectional tunnel ingress (sending) side
>
> Should is used here. Not in the normative sense. What would be the alternative
> to specifying the unidirectional tunnels on the sending side?

The intent of the text was simply to highlight that receiver side configuration is not (should not be) required.

> ** Section 4.1.
>
> For non-congestion
>    controlled mode, the bandwidth SHOULD be configured.
>
> What would it mean for this not be configured?  That the end-point set a packet size and rate?

We have to be careful in the spec to allow for other uses (i.e., non-constant send rate use), that's the reason this isn't a MUST.

> ** Section 4.3. Is this a generic statement that how congestion control is done
> is a local configuration option, or is this a Boolean configuration of use
> congestion control or not (aka, Section 6.1.1 vs. 6.1.2.?)

It's saying that the host or server software (and it's configuration) decides what congestion control to use and whether to use it or not.  So all of the above. :)

> ** Section 5.1.
>
> If any
>    requirement flags are not understood or cannot be supported by the
>    receiver then the receiver SHOULD NOT enable use of AGGFRAG_PAYLOAD
>
> Is the WG sure that this shouldn't be MUST NOT?  What's the user case where you want to continue?

I believe so. It's allowing for (future) cases where a requirement flag is not supported, but AGGFRAG_PAYLOAD can still be made to work.

> ** Section 6.1.4.  Typo. s/it's USE_AGGFRAG/its USE_AGGFRAG/

Fixed.

> ** Section 7.1.  Why the reference to RFC7120?  Since this registry is going to be "Expert Review", this document doesn't apply.

Well RFC7120 says in the Introduction that "expert review" does not require a formal IETF action before IANA performs allocation. That's what the reference was for basically. Can remove if you think it's unneeded.

> ** Section 7.1.  To double check, there is no more guidance to provide to the expert reviewer?

It's pretty wide open what sub-type might be used in the future (could even be completely different payload encapsulations/uses etc), so we didn't see a reason to limit/guide the experts anymore here.

> ** Section 8.  Editorial.
>
> This document describes an aggregation and fragmentation mechanism
>    and it use to add TFC to IP traffic.  The use described is expected
>    to increase the security of the traffic being transported.
>
> The first sentence doesn't parse and I recommend more precision on the second. Perhaps:
>
> NEW
>
> This document describes an aggregation and fragmentation mechanism to
> efficiently implement TFC for IP traffic. This approach is expected to reduce
> the efficacy of traffic analysis on IPSec communication.

Ok.

> ** Section 8. Editorial. s/As noted in (Section 3.1)/As noted in Section 3.1,/

Fixed.

> ** Section 8.
>
> As noted previously in Section 2.4.2, for TFC to be fully maintained ...
>
> What does it mean to for TFC to be "fully maintained"?

Changed to "maintained".

Thanks,
Chris.

>
> Regards,
> Roman
>
> _______________________________________________
> IPsec mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/ipsec