Re: [Tsv-art] [IPsec] Tsvart early review of draft-ietf-ipsecme-iptfs-03

Christian Hopps <chopps@chopps.org> Sun, 20 December 2020 03:47 UTC

Return-Path: <chopps@chopps.org>
X-Original-To: tsv-art@ietfa.amsl.com
Delivered-To: tsv-art@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id CB8663A097E; Sat, 19 Dec 2020 19:47:54 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.896
X-Spam-Level:
X-Spam-Status: No, score=-1.896 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, HTML_MESSAGE=0.001, SPF_HELO_NONE=0.001, SPF_NONE=0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 0iwppsIRLKIg; Sat, 19 Dec 2020 19:47:53 -0800 (PST)
Received: from smtp.chopps.org (smtp.chopps.org [54.88.81.56]) by ietfa.amsl.com (Postfix) with ESMTP id E922D3A0978; Sat, 19 Dec 2020 19:47:52 -0800 (PST)
Received: from [192.168.2.5] (047-050-069-038.biz.spectrum.com [47.50.69.38]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by smtp.chopps.org (Postfix) with ESMTPSA id 1F0676020D; Sun, 20 Dec 2020 03:47:52 +0000 (UTC)
From: Christian Hopps <chopps@chopps.org>
Message-Id: <FA8AEFD6-7B94-4A41-9715-25C19CAEF25B@chopps.org>
Content-Type: multipart/signed; boundary="Apple-Mail=_3F57C823-97C3-4EC7-B63F-89A425C1EF5F"; protocol="application/pgp-signature"; micalg="pgp-sha512"
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\))
Date: Sat, 19 Dec 2020 22:47:51 -0500
In-Reply-To: <C35712E0-553A-4D1A-9254-E30FD2ACD7D0@strayalpha.com>
Cc: Christian Hopps <chopps@chopps.org>, ipsec@ietf.org, tsv-art <tsv-art@ietf.org>, draft-ietf-ipsecme-iptfs.all@ietf.org
To: Joseph Touch <touch@strayalpha.com>
References: <160706317241.25013.15326204319913211090@ietfa.amsl.com> <559B100B-4BC2-4E95-AFF8-95BBAE0BDAC8@chopps.org> <724B2213-3B5E-4057-8343-4EE039034417@strayalpha.com> <1B7AE37A-1A16-47C5-95A3-FD131A1F0E20@chopps.org> <C35712E0-553A-4D1A-9254-E30FD2ACD7D0@strayalpha.com>
X-Mailer: Apple Mail (2.3608.120.23.2.4)
Archived-At: <https://mailarchive.ietf.org/arch/msg/tsv-art/IQPihrEe1YJgaK1OUQ9syotVdo0>
Subject: Re: [Tsv-art] [IPsec] Tsvart early review of draft-ietf-ipsecme-iptfs-03
X-BeenThere: tsv-art@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Transport Area Review Team <tsv-art.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/tsv-art>, <mailto:tsv-art-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/tsv-art/>
List-Post: <mailto:tsv-art@ietf.org>
List-Help: <mailto:tsv-art-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/tsv-art>, <mailto:tsv-art-request@ietf.org?subject=subscribe>
X-List-Received-Date: Sun, 20 Dec 2020 03:47:55 -0000


> On Dec 19, 2020, at 7:41 PM, Joseph Touch <touch@strayalpha.com> wrote:
> 
> 
> 
>> On Dec 19, 2020, at 1:17 AM, Christian Hopps <chopps@chopps.org <mailto:chopps@chopps.org>> wrote:
>> 
>> Changes are underway. One comments inline.
>> 
>>> On Dec 4, 2020, at 11:42 AM, Joseph Touch <touch@strayalpha.com <mailto:touch@strayalpha.com>> wrote:
>>> 
>>> Hi, Christian,
>> 
>> [...]
>> 
>>>>> There is no clear utility in having the blockoffset point past the end of the
>>>>> current packet. It serves – at best – as only a partially useful check on the
>>>>> next packet. I.e., if the two blockoffsets disagree, presumably a packet is
>>>>> lost – but if they agree, it cannot be concluded that a packet is not lost. It
>>>>> is sufficient that it points to the end of the tunnel packet.
>>>> 
>>>> No harm either though, right? Having implemented this, I can tell you that it does help detect bugs in ones code while in development. :)
>>> 
>>> It’s useful as a check, but it’s important to explain what the check means.
>>> 
>>> There are four cases (BO = blockoffsets, seq = ESP sequence number)
>>> 
>>>                               ESP - no gap         ESP - gap
>>> ---------------------------------------------------------------------
>>> BO - aligned          OK                         BO-FP-err
>>> BO - misaligned    SN-FP-err              Typ-err
>>> 
>>> Typ-err is the typical error when a packet is lost, because that should result in both an ESP seqno gap and blockoffset misalignment.
>>> 
>>> SN-FP-err is when there’s no ESP seq no gap but the blockoffsets are misaligned. This should only ever happen if the seqno rolls around and you missed it, which you should have some other mechanism to prevent (i.e., drop old packets when they’re half the sequence space old - which is easy to predict because you know the tunnel packet generation rate).
>>> 
>>> BO-FP-err is when there’s an ESP seq no gap but the blockoffsets are aligned. This is just a false positive (thus “FP” in my notation). S
>>> 
>>> In any of the error cases, you do not reassemble.
>>> 
>>> So checking blockoffset alignment only helps you know whether your sequence number check and timeout discard is working correctly.
>>> 
>>> Protocol implementation correctness should be checked by test vectors, not during normal code operation IMO. At best, it’s unnecessary complexity. At worst, it gives you a false sense of wanting to reassemble when you shouldn’t.
>> 
>> Conversely, test-vectors are harder to write w/o this and must be external, whereas this is a simple sanity check that can be added inline (perhaps conditionally) to code which has been shown to catch bugs. This will help implementations be more robust.
> 
> Test vectors run only during testing. By putting this test in regular operation, you’re adding overhead to check that the code is working every time it runs.

The test is already performed to determine if the fragment is complete or not, regardless of whether you use the real value or some marker value.

> It’s your call; I was just pointing out that this is very inefficient.
> 
>> Regarding complexity, having implemented this I can say there really is no complexity :)
> 
> If the LOC > 0, then it’s not “no complexity”.

There really is no complexity added.

copylen = min(inner_remain, outer_remain)
ohdr->block_offset = inner_remain; /* Only Diff */
memcpy(outer_buf, inner_buf, copylen);
if (inner_remain > copylen) more to go...

vs.

copylen = min(inner_remain, outer_remain)
ohdr->block_offset = copylen; /* Only Diff */
memcpy(outer_buf, inner_buf, copylen);
if (inner_remain > copylen) more to go...

Except the former allows for creating more robust implementations, and also differentiates the case where a fragment just fits or not for the receiver. The latter ends up stripping away useful information. Both ways point the block offset past the end of payload. I see the loss for stripping the information here, I don't see the gain.

Thanks,
Chris.

> 
>> You set the block offset to the remaining inner packet length to be sent (or 0 if no packet fragmentation is in progress). This value is sitting right there in the code so you can either plug it in or use some marker value (you suggest the length to the end of the payload). On receive the code is identical: does it exceed the length of the payload or not.
> 
> Agreed, it’s simple - but it’s nonzero effort and code. Again, at run-time. For a check that basically keeps saying the code is working, rather than checking an actual packet error.
> 
> Again, recommending inefficient code is up to you.
> 
> Joe