Subject: Re: [openpgp] The checksum may appear

From: Paul Wouters <[email-masked]>
Date: Wed, 28 Apr 2021 23:58:19 -0400 (EDT)

On Thu, 25 Mar 2021, Ángel wrote:

It is not clear to me what the WG would like to do here on the
"optional checksum" item.

Ángel proposed something, Werner agreed, but then Ángel wasn't sure
anymore. It would be good to get more and/or clarified views on this
issue.

Paul

> On 2021-03-19 at 07:54 +0100, Werner Koch wrote:
>> > -The checksum with its leading equal sign MAY appear on the first line
>> > after the base64 encoded data.
>> > +If present, the checksum with its leading equal sign SHALL appear on
>> > the next line after the base64 encoded data.
>> 
>> Adding "optional" and making the CRC a SHOULD create indeed clarifies
>> the intention of the RFC.  Thus I am in favor of this change.
>
> Note I wasn't placing the later. I only stated that you can only place
> the checksum at the end.
> I was planning to treat this as a feature request and add a phrase with
> such SHOULD, since I agree it's a good idea, but turns out I can't come
> out with a better rationale than “it's cheap enough it makes sense to
> do it even if not giving much value”.
>
> What is the goal of the armor CRC?
> The only good use case I can think of is when a human has been
> involved, such as when restoring a key from paper.
>
> On other scenarios, the CRC would either be too weak (e.g. in presence
> of an active attacker) or protecting from an error that would already
> have been handled at lower layers.
> [1] and [2] suggest it was added to avoid modem line noise altering the
> messages (which nowadays should be discarded at e.g. TCP).
>
> Without a compelling use case, I don't think it should be a SHOULD.
> And finally, we should at least mention why it was once considered
> useful.
>
>
> The original change is available in git mode at
> https://gitlab.com/Angel-Gonzalez/rfc4880bis/-/tree/checksum-may-appear
>
>
> Best regards
>
> 1- https://mailarchive.ietf.org/arch/msg/openpgp/3K6tSdebEjQw8K1z1pZkXxyvu-k/
> 2- https://mailarchive.ietf.org/arch/msg/openpgp/2FmAqP-nJkV08E1qQ4YNO2xR2Pc/
>
>
> _______________________________________________
> openpgp mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/openpgp

---

From: Paul Wouters <[email-masked]>
Date: Thu, 29 Apr 2021 00:01:08 -0400 (EDT)

On Sat, 20 Mar 2021, Ángel wrote:

> Subject: Re: [openpgp] [RFC4880bis PATCH] Clarify CRC-24 C example
>     implementation

This issue was also left with multiple competing texts, and so I would
like the WG to come up with 1 change for me to apply :)

Paul

> On 2021-03-19 at 13:24 -0400, Daniel Kahn Gillmor wrote:
>>
>> fwiw, i'm fine with either of these.  I note that the change i'd
>> originally proposed renamed the constant to CRC24_GENERATOR, to align
>> it with the term in the text ("poly" doesn't appear anywhere in the
>> text, but "generator" does).
>
> Right. I began from Hal's mail [1] Particularly the second option is
> his code just adding the comment. Thus I didn't want to add another
> change with the constant rename (something I'm fine with).
>
>
> Another potential point is if 0xffffff should have been changed to
> 0xffffffUL, as we are also changing the type to unsigned, but it looks
> redundant.
>
>
>> That said, we are pretty clearly in bike-shedding territory here.
>>
>> I have heard no one advocate for leaving the text unchanged, and i have
>> heard no one advocate for just silently amending the code to drop bit 25
>> from the constant.
>
> Indeed. Even if discussing which wording might be (slightly) better,
> let me explicitly state my support, in that I think all proposed
> changes on this thread represent an improvement over the existing text.
>
> Best regards
>
>
> 1-
> https://mailarchive.ietf.org/arch/msg/openpgp/UMLreIiKtKzXEnPT5ZbcDDjRQX0/
>
>

---

From: Daniel Kahn Gillmor <[email-masked]>
Date: Thu, 29 Apr 2021 12:15:52 -0400

> like the WG to come up with 1 change for me to apply :)

The version that you've committed to gitlab's main branch looks good to
me:

------
## An Implementation of the CRC-24 in "C"

    #define CRC24_INIT 0xB704CEL
    #define CRC24_GENERATOR 0x864CFBL

    typedef unsigned long crc24;
    crc24 crc_octets(unsigned char *octets, size_t len)
    {
        crc24 crc = CRC24_INIT;
        int i;
        while (len--) {
            crc ^= (*octets++) << 16;
            for (i = 0; i < 8; i++) {
                crc <<= 1;
                if (crc & 0x1000000) {
                    crc &= 0xffffff; /* Clear bit 25 to avoid overflow */
                    crc ^= CRC24_GENERATOR;
                }
            }
        }
        return crc & 0xFFFFFFL;
    }
------

It's a little bit weird that one constant is written as 0xFFFFFFL and
the other is written as 0xffffff -- maybe we want to align the
orthography (and should 0x1000000 also have an L suffix)?  But even
without those little tweaks, this is strictly better than the earlier
version.

If anyone has any objections to this, please speak up!

   --dkg