> On Feb 8, 2021, at 6:44 AM, Valery Smyslov <smyslov.i...@gmail.com> wrote:
> 
> Hi,
> 
> I think that in its current form the draft is too focused on a single 
> application for
> the Aggregation and Fragmentation mode - IP-TFS. From architectural
> point of view I'd like to see the draft first defining the mechanism itself
> and then describing possible applications for it, focusing on IP-TFS
> as the primary application.
> 
> We discussed this with Christian off the list in length and came to
> a good compromise regarding naming of new entities defined in the draft.
> After re-reading the draft I still think that its structure of the document 
> should be
> changed to better decouple mechanism from its applications.

Hi Valery,

Thanks for your continued reviews and suggestions.

I agree with what Lou with regards to it going too far to recast/redirect this 
work any further. I did do a round of changes based on our agreement to help 
with future uses, and while it's nice that this work could lead to these uses, 
those should be documented in another document at this point. The focus of this 
work has and should continue to be traffic flow security.

I've incorporated your other suggestions from below.

> 5. Section 2.1:
> 
>   This is accomplished using a new Encapsulating Security Payload (ESP,
>   [RFC4303]) type which is identified by the number AGGFRAG_PAYLOAD
>   (Section 6.1).
> 
> RFC 4303 doesn't define what is "Encapsulating Security Payload type ".
> You should clarify this in more detail and also mention that
> the AGGFRAG_PAYLOAD number must be in the ESP "Next Header" field.

s/type which is identified by the number/Next Header field value/

> 7. Section 2.2.1:
> 
> s/ Figure 2: Layout of IP-TFS data block/ Figure 2: Layout of Data Block

Updated.

> 8. Section 2.2.2:
> 
>   Only
>   when there is no data to encapsulated is end padding required, and
>   then an explicit "Pad Data Block" would be used to identify the
>   padding.
> 
> Nit: isn't better:
> 
> s/no data to encapsulated/no data to encapsulate

Fixed

> 9. Section 2.2.3:
> 
>   When using the AGGFRAG_PAYLOAD in conjunction with replay detection,
>   the window size for both MAY be reduced to share the smaller of the
>   two window sizes.  This is b/c packets outside of the smaller window
>   but inside the larger would still be dropped by the mechanism with
>   the smaller window size.
> 
> I wonder why MAY is used here. It should be MUST instead.
> As you explained there is no point for the sizes to be different.

They remain different mechanisms and the user may wish to have them treated 
differently (e.g., logging replayed packets).

> And please s/b\/c/because

Fixed.

> 10. Section 2.2.3:
> 
>   Finally, as sequence numbers are reset when switching SAs (e.g., when
>   re-keying a child SA), an implementation SHOULD NOT send initial
>   fragments of an inner packet using one SA and subsequent fragments in
>   a different SA.
> 
> Two issues here - first why SHOULD NOT and not MUST NOT?
> In general you cannot reliably reassemble packet if it is fragmented over
> several SAs, so it will be dropped. Why do you allow this?

Changed to MUST NOT.

> Then, IPsec architecture allows several parallel ESP SAs
> to co-exist with the intention that kernel may use any of these SAs to send 
> packets
> (e.g. for improving performance, see draft-pwouters-multi-sa-performance).
> I think you should mention that in this case a care must be taken not to
> fragment outgoing packets over several parallel SAs. I.e. if a packet get 
> fragmented,
> all its fragments must be sent over single ESP SA.

Covered by the switch to MUST NOT.

> 11. Section 2.2.4:
> 
> IKEv2 always creates ESP SAs in pairs, so you don't need to send empty 
> AGGFRAG_PAYLOAD payloads
> over non-AGGFRAG enabled SAs. I understand your intention to use this mode in 
> non-IKEv2
> environments, but I think that in case of IPsec you don't need to send
> AGGFRAG_PAYLOAD payload over non- AGGFRAG enabled SAs.

SAs are a concept that extend beyond IKEv2 so I think that the text is OK here.

> I think some text should be added that this section is only applicable
> for non-IKEv2 use case.

Add: "Currently this situation is only applicable in non-IKEv2 use cases."

> 12. Section 2.3:
> 
>   Empty AGGFRAG_PAYLOAD payloads (Section 2.2.4) are used to
>   transmit congestion control information on non-IP-TFS enabled SAs, so
>   intermixing is allowed in this specific case.
> 
> As I said before it is not needed in case of IKEv2, so I'd rather to
> stress that this is only allowed if AGGFREAG SAs are not being created in 
> pairs
> (non-IKEv2 use case).

Covered with above change.

> 13. Section 2.3:
> 
>   While it's possible to
>   envision making the algorithm work in the presence of sequence number
>   skips in the AGGFRAG_PAYLOAD payload stream, the added complexity is
>   not deemed worthwhile.
> 
> I have trouble understanding applicability of this text to the section
> describing Exclusive SA Use. Shouldn't it be in 2..2.3?

It's just referring to what happens (sequence number skips) if one intermixes 
which we are disallowing.

> 14. Section 2.4.2:
> 
> This section is concerned with IP-TFS use of AGGFREG mode.
> First, I think it must be mentioned in the text.
> Then, the text lacks discussion on how to deal with congestion when ESP over 
> TCP is used (RFC 8229).
>> From my understanding it is TCP that will take care of congestion in this 
>> case.
> If both mechanisms are in use then I suspect that it may affect performance
> (like TCP in TCP), because in this case congestion information (RTT etc.)
> will be highly inaccurate.
> 
> More general note - using ESP over TCP may also add some other implications
> on using AGGFRAG (e.g. header values mapping, ECN, fixed packet size etc.),
> I think it's worth to add some text. Is IP-TFS use of AGGFRAG ever make sense
> when ESP is sent over TCP?

I'll add this to the previous (non-congestion control mode) section.

"Non-congestion control mode is also appropriate if ESP over TCP is in
use [RFC8229]."

> 
> 15. Section 3:
> 
>   In order to obtain these values the receiver sends
>   congestion control information on it's SA back to the sender.
> 
> s/it's/its

Fixed.

> 
> 16. Section 5.1:
> 
>   The USE_AGGFRAG notification MUST NOT be sent, and MUST be ignored,
>   during a CREATE_CHILD_SA rekeying exchange as it is not allowed to
>   change use of the AGGFRAG_PAYLOAD payload type during rekeying.  A
>   new child SA due to re-keying inherits the use of AGGFRAG_PAYLOAD
>   from the re-keyed child SA.
> 
> In IKEv2 re-keying of Child SA is semantically almost equal to creating a new 
> Child SA
> with the only exception that the SA which is being rekeyed is indicated.
> However, all the properties of newly created substitute SA are negotiated as 
> if it is not a rekey
> (transforms, mode of operation). I don't see any reason why to break this 
> with AGGFRAG.
> I prefer to re-negotiate AGGFRAG in this case just to follow
> current IKEv2 practice. This will simplify implementations.
> And I see no problem if after AGGFRAG SA is replaced with normal one
> and visa versa. Am I missing something?

I think you have a good point on simplicity. I've removed that paragraph and 
removed "non-rekeying" in the text in the previous paragraph.

> 17. Section 5.1:
> 
>   The USE_AGGFRAG notification contains a 1 octet payload of flags that
>   specify any requirements from the sender of the message.  If any
>   requirement flags are not understood or cannot be supported by the
>   receiver then the receiver should not enable use of AGGFRAG_PAYLOAD
>   payload type (either by not responding with the USE_AGGFRAG
>   notification, or in the case of the initiator, by deleting the child
>   SA if the now established non-AGGFRAG_PAYLOAD using SA is
>   unacceptable).
> 
> It is not clear for me from this text whether these flags are negotiated
> or independently announced by peers. In other words - are they
> independent in both direction or they must be the same?

They are not negotiated as they are "requirements from the sender of the 
message" and the receiver either understands and accepts them or doesn't use 
AGGFRAG_PAYLOADs.

I've changed
  "that specify any requirements from the sender"

to

 "that specify requirements from the sender"

perhaps that makes it more clear?

> 18. Section 5.1:
> 
> Add some text that USE_AGGFRAG cannot be combined with USE_TRANSPORT_MODE.
> 
> General note: I think some words should be added somewhere (Section 2?)
> that AGGFRAG implies encapsulation whole IP packets, so it cannot be
> used for Transport mode ESP SA.

I'll add this to 5.1:

"As the use of AGGFRAG_PAYLOAD payloads is currently only defined for
non-transport mode tunnels, the USE_AGGFRAG notification MUST NOT be
combined with USE_TRANSPORT notification."

> 19. Section 6.1:
> 
>   An IP-TFS payload is identified by the ESP payload type
>   AGGFRAG_PAYLOAD which has the value 0x5.
> 
> Where this value has come from? Isn't it registered with IANA?
> We had a long discussion about this in the WG, current
> text looks like the value was squatted...

The discussion lead to the understanding that the ESP next-header field is not 
actually required to be IP protocol number, and people don't generically use it 
that way so we are free to choose whatever non-used value we would like. We 
chose 0x5 simply b/c that *was* an IP protocol number that is allocated but 
will not used; however, this wasn't required -- it's just trying to be smart 
about picking a value. :)

I don't think we need to go into this in the document though -- it's enough 
that its a valid value to use.

> Another issue: what is "ESP payload type"?
> RFC 4303 doesn't have this term. You should first
> introduce it.

I've changed payload type to "ESP Next Header value" in the document.

> 20. Section 6.1.4:
> 
>   0:
>      6 bits - reserved, MUST be zero on send, unless defined by later
>      specifications.
> 
> Add a sentence that these bits MUST be ignored on receipt.

They must not be ignored. If they are set and they and not understood then 
AGGFRAG mode will not be enabled as indicated in section 5.1

> 21. IANA Considerations.
> 
> I wonder why newly defined registry "AGGFRAG_PAYLOAD Sub-Type Registry "
> has an allocation policy "Standards Action". From my understanding
> this is too restrictive. Why "RFC Required" or "Expert Review" is 
> insufficient?
> Out of 256 possible values only 2 are defined and I don't expect
> a lot of allocation requests for this registry, so why do you need such
> a restrictive policy?

You're the second person to suggest this so I've changed it to "Expert Review". 
:)

> 
> Another issue - I would have mark value 0 as "Reserved" (just in case
> a special value is needed in future), but it's a matter of taste.

0 is a very useful value b/c a typical non-congestion control header can simply 
be zero'd efficiently. I've definitely used this fact in our implementation.

> 
> 22. Security Considerations.
> 
> Add a text that correct functionality of the AGGFRAG mode
> requires restoring packets order on the receiver. Since this
> is done by utilizing ESP Sequence Number field, the ESP
> header must be authenticated, and thus the ESP SA MUST
> be created with authentication other than NONE
> or with AEAD cipher.

I think this falls under the non-IPTFS use case. I'd like to leave it out as it 
would be confusing.

> Probably it's worth to mention this somewhere in Section 2.2.3...
> 
> 23. Appendix C.1.1:
> 
>   The overhead of IP-TFS is 40 bytes per outer packet.
> 
> I failed to understand where 40 bytes came from. From my understanding the 
> overhead
> of AGGFRAG is either 4 bytes or 24 bytes depending on mode used (comparing 
> with usual ESP).
> Am I missing something?

See final comment below..


> s/bytes/octets

Fixed.

> 
> 24. Appendix C.1.2:
> 
>   The overhead per inner packet for constant-send-rate padded ESP
>   (i.e., traditional IPsec TFC) is 36 octets plus any padding, unless
>   fragmentation is required.
> 
> Again, I failed to understand where 36 octets came from.
> In general ESP overhead depends on transforms in use (which
> influence IV and ICV sizes), so you should mention under
> which condition this overhead was computed.

These were done a couple years ago when we first published this, and for some 
reason I had the ESP overhead (shared by both IPTFS and regular IPsec) plugged 
into the table formulas as 16.

I'll rerun the tables using AES-GCM-256 values. New value IPsec/ESP overhead 
would be 54 (20 (IP) + 8+2 (ESPH/F) + 8 (IV) + (16 ICV))

The results will still highlight the same things, but thanks for catching this! 
:)

Thanks,
Chris.

> 
> Regards,
> Valery.
> 
> 
> 
> _______________________________________________
> IPsec mailing list
> IPsec@ietf.org
> https://www.ietf.org/mailman/listinfo/ipsec
> 

Attachment: signature.asc
Description: Message signed with OpenPGP

_______________________________________________
IPsec mailing list
IPsec@ietf.org
https://www.ietf.org/mailman/listinfo/ipsec

Reply via email to