Chris, Thanks for wading through these. Incorporating these will certainly remove any issues I had. As far as the style suggestions goe, that’s fine the GENART, IESG, and RFC editor will also ask for their pound of flesh ;)
Spt
> On Feb 8, 2021, at 18:19, Christian Hopps <cho...@chopps.org> wrote:
>
>
>
>> 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
