> On Feb 3, 2021, at 10:38 PM, Sean Turner <s...@sn3rd.com> wrote: > > Hi! I mostly just have nits, but there are a couple of questions interspersed > below.
Hi Sean,
Thanks so much for this very thorough review!
I have made the vast majority of the suggested changes with only a few style
exceptions (and incorporated Paul's input in his follow-on email).
I will post a new version soon with these changes (noted below) as well as
addressing some from Valery as well.
> s1, para 1: s/While one may directly obscure the data through the use
> of/While directly obscuring the data with
Fixed
> s1, para 1: s/it’s/its
>
Fixed
> s1, para 3: s/IP-TFS/IP-TFS (IP Traffic Flow Security)
Fixed
>
> s1, last para: s/IP-TFS provides for dealing with network congestion/IP-TFS
> addresses network congestion
"Additionally, IP-TFS provides for operating fairly within congested networks"
> s1: You mention full TFC. Is it partial TFC if you use a non-constant
> send-rate? if so that might be good to qualify in the 3rd paragraph with
> something like:
> OLD:
>
> A non-
> constant send-rate is allowed, but the confidentiality properties of
> its use are outside the scope of this document.
>
> NEW:
>
> A non-
> constant send-rate is allowed to support partial TFC, but the
> confidentiality properties of its use are outside the scope of
> this document.
There are other ideas being studied to achieve full TFC w/o use of constant
send-rate (e.g., using some sort of randomizing), I'm fairly sure that this
just reduces to a lower logical fixed send rate that utilizes burstiness. In
any case since I am aware of there being research being done in this area I
think leaving the text as is makes sense (i.e., I don't want to claim anything
not fixed-rate must be partial TFC).
>
> s1.1, para 1: Use updated terminology para from 8174 ("BCP 14” is missing):
>
> The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL
> NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED",
> "MAY", and "OPTIONAL" in this document are to be interpreted as
> described in BCP 14 [RFC2119] [RFC8174] when, and only when, they
> appear in all capitals, as shown here.
Fixed.
>
> s2, para 1:
>
> is the "(SA)" needed? ... tunnel (SA) ...
> s/it’s/its
Fixed.
> s2, 2nd para: I tripped over this para a couple of times. Does this get the
> same point across?
>
> OLD:
>
> The primary input to the tunnel algorithm is the requested bandwidth
> used by the tunnel. Two values are then required to provide for this
> bandwidth, the fixed size of the encapsulating packets, and rate at
> which to send them.
>
> NEW:
>
> The primary input to the tunnel algorithm is the requested
> bandwidth for the tunnel. Two values needed to determine
> the bandwidth are the fixed size of the encapsulating
> packets and the rate at which to send them.
Changed to:
The primary input to the tunnel algorithm is the requested bandwidth
to be used by the tunnel. Two values are then required to provide for
this bandwidth use, the fixed size of the encapsulating packets, and
rate at which to send them.
> s2, 3rd para: s/or could be/or be
Fixed
> s2, 4th para: s/requested tunnel used bandwidth/
> requested tunnel bandwidth
Changed to:
"requested bandwidth to be used"
and "The packet send rate is the requested bandwidth to be used divided by the
size of the encapsulating packet."
> s1, last para to make it match the rest of the sentence: s/The egress of the
> IP-TFS/The egress (receiving) side of the IP-TFS
Fixed.
>
> s2.1, 1st para: s/In order to maximize bandwidth IP-TFS/
> In order to maximize bandwidth, IP-TFS
Fixed
>
> s2.1, 3rd para: Does this say the same thing:
>
> OLD:
>
> This is accomplished using a new Encapsulating Security Payload (ESP,
> [RFC4303]) type which is identified by the number AGGFRAG_PAYLOAD
> (Section 6.1).
>
> NEW:
>
> IP-TFS uses a new Encapsulating Security Payload (ESP, [RFC4303])
> type identified by the number assigned to AGGFRAG_PAYLOAD
> (Section 6.1).
I think I prefer the original text, if that's OK, as it is saying how it does
the paragraph above vs. just making another statement (i.e., I think it ties
the text together better).
> s2.2: I had a really hard time parsing this sentence:
>
> The AGGFRAG_PAYLOAD payload content defined in this document is
> comprised of a 4 or 24 octet header followed by either a partial, a
> full or multiple partial or full data blocks.
>
> There are three options: partial, full or multiple partial, or full data?
> Maybe it’s just missing a comma: s/multiple partial or/multiple partial or,
Changed to:
The AGGFRAG_PAYLOAD payload content defined in this document is
comprised of a 4 or 24 octet header followed by either a partial
datablock, a full datablock, or multiple partial or full datablocks.
>
> s2.2.1: Should we be specific about the IPv4 and IPv6 length’s field name:
>
> OLD:
>
> Likewise, the length of the data block is extracted from the
> encapsulated IPv4 or IPv6 packet's length field.
>
> NEW:
>
> Likewise, the length of the data block is extracted from the
> encapsulated IPv4’s Total Length or IPv6’s Payload Length fields.
Fixed
>
> s2.2: s/It’s/It is
> s2.2.3: s/to be able to reassemble/to reassemble
> s2.2.3: s/This possible interleaving/This interleaving
> s2.2.3: s/sender to always be able to send a/sender to always send a
> s2.2.3, 4th para: s/Finally, we note/Finally, note
> s2.2.3, 5th para: s/As the amount of reordering that may be present is hard
> to predict the window/As the amount of reordering that may be present is hard
> to predict, the window
Fixed.
>
> 2.2.3, 5th para: I am all about not littering I-Ds with 2119-language, but
> here it looks like you are burying the MUST in the parenthetical. BTW - I
> think the sentence stands on its own without the "i.e.” and it could safely
> be deleted. Would this rewording work:
>
> Gaps in the sequence numbers will not work for this document,
> therefore ESP sequence number stream MUST increase monotonically
> by 1 for each subsequent packet.
Fixed
>
> s2.2.3, penultimate para: s/b/c/because
Fixed
> s2.2.3, last para: Should the I-D state what happens if the implementation
> does send initial fragments of an inner packet using one SA and subsequent
> fragments in a different SA. I.e., motivate the SHOULD NOT.
>
> s2.2.3, last para: implementation here refers to sender right so maybe:
> senders SHOULD NOT
Simplified to "senders MUST NOT"... :)
> s2.2.3.1: Implementations here refers to senders so maybe:
> s/An implementation/Senders
> s/Implementation implementing/Senders implementing ;)
Fixed
> s2.2.4: s/In order to support/To support
Fixed
> s2.2.5, 1st para:
> s/(by design!)/(by design) ;)
> s/although an implementation/although a sender
> s/An implementation SHOULD/A sender SHOULD
Fixed
> s2.2.5, 2nd para:
> s/an implementation/a sender
> s/([RFC3168])/[RFC3168]
Fixed
> s2.2.6, 1st para: s/([RFC0791])/[RFC0791]
Fixed, and update other single refs to not use parens.
> s2.2.6, 2nd para: 2119 the should?: s/should be/SHOULD be. Is there a reason
> you would want to handle the errors differently? If not then would the
> following also be true (i.e., replace "should be" with "are":
>
> Any errors (e.g., ICMP errors arriving back at the tunnel ingress due
> to tunnel traffic) are handled the same as with non IP-TFS
> IPsec tunnels.
Fixed
> s2.2.7, 1st para: s/am implementation/a sender
Fixed
>
> s2.2.7, 2nd para: s/([RFC4301])/[RFC4301]
Fixed
> s2.3: Does this work?
>
> OLD:
>
> It is not the intention of this specification to allow
> for mixed use of an AGGFRAG_PAYLOAD enabled SA.
>
> NEW:
>
> This document does not specify mixed use of an
> AGGFRAG_PAYLOAD enabled SA.
Fixed
> s2.4.1, 1st para: s/In the non-congestion controlled mode IP_TFS /In the
> non-congestion controlled mode, IP-TFS
Fixed
> s2.4.1, 2nd para:
> Should the should be SHOULD?
> s/In this case packet/In this case, packet
> There is a reference to a user. Is it really a user?
I think it's a should (non-normative), as this is guidance on the right way to
deploy IPTFS, but not how to implement it.
> s2.4.2, 2nd para:
> s/the ingress sends/the ingress side sends
> Maybe just swap this around?
>
> OLD:
>
> An example of an implementation of the [RFC5348] algorithm which
> matches the requirements of IP-TFS (i.e., designed for fixed-size
> packet and send rate varied based on congestion) is documented in
> [RFC4342].
>
> NEW:
>
> [RFC4342] provides an example of the [RFC5348] algorithm which
> matches the requirements of IP-TFS (i.e., designed for fixed-size
> packet and send rate varied based on congestion.
Fixed.
> s2.4.2, 3rd para: s/In particular these/In particular, these
Fixed
> s2.4.2, 4th para:
> Should the must be MUST?
> s/The lack of receiving this information/Not receiving this information
Yes, and fixed.
> s2.4.2, 4th and 5th paras: s/it’s/its
Fixed
> s2.4.2, 6th para: Does this work?
>
> OLD:
>
> When an implementation is choosing a congestion control algorithm (or
> a selection of algorithms) one should remember that IP-TFS is not
> providing for reliable delivery of IP traffic, and so per packet ACKs
> are not required and are not provided.
>
> NEW:
>
> When choosing a congestion control algorithm (or
> a selection of algorithms) note that IP-TFS is not
> providing for reliable delivery of IP traffic, and so per packet ACKs
> are not required and are not provided.
Fixed
> s2.4.2, last para: s/It’s/It is
Fixed
> s3, 1st para:
> s/and also be able to approximate/and also to approximate
> s/([RFC5348])/[RFC5348]
> s/In order to obtain these values the/
> In order to obtain these values, the
> s/Thus in order, to support/Thus, to
Fixed
> s/is used to convey/conveys
This is a style one I think that I'd like to leave be, I think it better
highlights that one needs to send these empty payloads in this case.
> s3, 1st and 3rd para: s/it’s/its
Fixed.
>
> s3, 3rd para: Nits complained about this so I am assuming it’s MUST NOT here
> - s/MUST not/MUST NOT
Fixed
>
> s3.1, 1st para: s/egress endpoint/egress (receiving) side
Fixed
> s3.1, last para: s/For this reason ECN/For this reason, ECN/
Fixed
> s4: s/should be able to be specified/should be specified
Fixed
> s4.1: s/For non-congestion controlled mode the/For non-congestion controlled
> mode, the
Fixed
> s4.1: Does this work:
>
> OLD:
>
> For congestion controlled mode one can configure the
> bandwidth or have no configuration and let congestion
> control discover the maximum bandwidth available.
>
> NEW:
>
> For congestion controlled mode, the bandwidth can be
> configured or the congestion controller discovers
> the maximum bandwidth available.
Changed to:
"For congestion controlled mode, the bandwidth can be configured or the
congestion control algorithm discovers and uses the maximum bandwidth
available."
>
> s5.1, 2nd para: s/is used to enable use of/enables the
Fixed
> s5.1, 3rd para:
> s/To request using the/To request use of the
> s/then response/then the response
Fixed
> s5.1, penultimate para: Should the should not be SHOULD NOT?
Fixed
> s6.1: s/8 bit/8-bit
> s6.1: s/specification/document
Fixed
> s6.1.1: s/"DataBlocks” data/“DataBlocks" ?
I'm going to leave this as the ``"DataBlocks" data'' as it is referring to that
area of the packet data. DataBlocks may be spread across multiple packets or
there may be multiple DataBlocks in a single packet.
> s6.1.1: s/16 bit/16-bit
> s6.1.2: s/7 bit/7-bit
> s6.1.2: s/1 bit/1-bit
> s6.1.2 x3: s/32 bit/32-bit
> s6.1.2: s/22 bit/22-bit
> s6.1.2 x2: s/21 bit/21-bit
> s6.1.3: s/4 bit/4-bit
> s6.1.3.1/s6.1.3.2/s6.1.3.3: s/4 bit/4-bit
> s6.1.3.1/s6.1.3.2: s/16 bit/16-bit
> s6.1.3.3: s/extends/Extends
Fixed
> s6.1.4: s/As discussed in Section 5.1 a/As discussed in Section 5.1, a
Fixed
> s6.1.4, D bullet (make it match C): s/Don't Fragment bit, if/Don't Fragment
> bit. If
Fixed
> s7: I may have missed this in earlier discussions but why is the registration
> policy Standards Action? I thought that most of the registries were trending
> more towards Expert Review.
No particular reason other than it was the conservative choice. :)
> s8, 1st para: s/Traffic Flow Confidentiality/TFC
Fixed
> s8:
>
> You warned in s1 about using a non-constant send-rate, but shouldn’t that be
> echoed here?
As mentioned previously it's not necessarily less secure to use a non-constant
send rate (research being done by others here), we are only claiming use of
constant send rate is more secure.
> Likewise maybe repeat the ECN covert channel statement.
Repeated.
>
> s9.2: r/[draft-iab-wire-image]/[RFC8546]
Fixed.
> Appendix B: YMMV here but since this is an Appendix and you’re repeating the
> current practice s/SHOULD/should
Fixed.
Thanks again for the very thorough review!
Thanks,
Chris.
>
> Cheers,
> spt
>
>> On Jan 24, 2021, at 20:55, Tero Kivinen <kivi...@iki.fi> wrote:
>>
>> This is the start of 3 week WGLC on the document, ending 2021-02-15.
>> Please submit your comments to the list, also send a note if you have
>> reviewed the document, so we can see how many people are interested in
>> getting this out.
>> --
>> kivi...@iki.fi
>>
>> _______________________________________________
>> IPsec mailing list
>> IPsec@ietf.org
>> https://www.ietf.org/mailman/listinfo/ipsec
>
> _______________________________________________
> IPsec mailing list
> IPsec@ietf.org
> https://www.ietf.org/mailman/listinfo/ipsec
signature.asc
Description: Message signed with OpenPGP
_______________________________________________ IPsec mailing list IPsec@ietf.org https://www.ietf.org/mailman/listinfo/ipsec
