Re: [openpgp] Review of draft-ietf-openpgp-crypto-refresh-07

[Daniel Kahn Gillmor <[e-mail]>]

On Fri 2022-12-16 16:33:22 +0000, Hammell, Jonathan F wrote:
> As promised at IETF 115, I've reviewed draft-ietf-openpgp-crypto-refresh-07

Thanks very much for this review, Jonathan, and sorry for the delay in
responding.  some of your comments have already been addressed, and i've
tried to make concrete changes that address the bulk of what remains in
https://gitlab.com/openpgp-wg/rfc4880bis/-/merge_requests/270 (this also
incorporates a lot of what looks to me like solid, non-controversial
editorial work already done by Daniel Huigens and Falko Strenzke.

Rather than giving each of your notes a line-by-line response, i'm just
going to cite the things that the above MR *doesn't* cover with
something reasonably close to what you recommended.

> Perhaps it should also be recommended that a sender encrypt to their
> own public key to allow decryption of replies without having to lookup
> the sent PKESK packet.

I'm not sure i fully understand this, but i think encrypt-to-self
guidance is generally probably domain-specific (that is, it has more to
do with, say, how the mail user agent works than OpenPGP itself).
There is some related text in
https://datatracker.ietf.org/doc/draft-ietf-lamps-e2e-mail-guidance that
is not OpenPGP specific.  Maybe this guidance belongs there?

> - Sections 5.1.3 and 5.1.4 say "The value 'm' in the above formula is
> the plaintext value described above...", but "plaintext" is not
> mentioned above.  I assume it is "The resulting octet string" from the
> last sentence of 5.1.2, but the linking of this could be more clear.

I think this has been fixed in Daniel Huigens'
"no-v3-pkesk-cfrg-padding" branch, part of outstanding MR !267.

> - In the paragraph following the list in Section 5.3.2 (v5 SKESK), the
> first sentence is very long and it is not clear what it is describing.
> I suggest "A key-encryption key is derived using HKDF (see [RFC5869])
> with SHA256 has the hash algorithm.  The Initial Keying Material (IKM)
> for HKDF is the resulting key derived from S2K.  No salt is used.  The
> info parameter for HKDF is comprised of the Packet Tag in new format
> encoding (bits 7 and 6 set, bits 5-0 carry the packet tag), the packet
> version, and the cipher-algo and AEAD-mode used to encrypt the key
> material. [New paragraph]"  A diagram would help here.  See also the
> similar description in 5.5.3.

I've made clarifying changes to these sections that roughly match these
suggestions in the commit "Adjust descriptions of HKDF use", but i have
not tried to supply a diagram.  If anyone wants to take a crack at a
diagram, i'd be happy to review.

> - Section 11.1.4, paragraph 2 has multiple interpretations for the
> "MUST" requirement.  Either the primary MUST always be an algorithm
> capable of making signatures in order to create self-signatures, or if
> one wishes to create self-signatures, then the primary key MUST be
> capable of making signatures.

Resolving this would mean picking an understanding of this text.  I
think primary keys should only ever be sign-only, but this seems like it
might be contentious, so i've split it out into a distinct MR:

  https://gitlab.com/openpgp-wg/rfc4880bis/-/merge_requests/269

> - Section 3.3 introduces Key IDs and says that they cannot be assumed
> to be unique, but it would be helpful to at least mention fingerprints
> as the better option for a unique value. I suggest renaming 3.3 as
> "Key IDs and Fingerprints" and then replace the last sentence with "A
> fingerprint is cryptographically unique per key, but may be calculated
> differently according to the version of the key.  See Section 5.5.4
> for more details on Key IDs and fingerprints."

I think there's some dispute about whether it's safe to say
"cryptographically unique" when we know that (for example) there are
SHA-1 collisions, so i've gone with a different wording here.  Please
take a look at the commit f14509a8575efd9fa5ac48febe58b54deda9269c
("update Key ID note to refer to Fingerprints as well")

> - In Section 3.7.1.4 (Argon2): s/kibibytes/kilobytes

Are you sure?
https://www.rfc-editor.org/rfc/rfc9106.html#section-3.1-2.5 says
kibibytes.  Changing it to kilobytes here would cause a mismatch. I did
not make this change.

> - Section 4.2 does not make it clear that Bit 6 of the Packet Tag is
> used to identify Legacy packet formats.  It says Bit 6 is "Always one
> (except for Legacy packet format)", but it would be helpful in the
> following paragraph to start with "A Packet Tag with a zero in Bit 6
> indicates a Legacy packet format."  Or add this sentence to the
> beginning of 4.2.2.

ugh, this terminology is a mess.  The section says that the first octet
is the "packet tag", but then it also says that some subset of the bits
in the first octet are *also* the "packet tag".  I've tried to clean
this up, including a variation on your suggestion in git commit
ea88d35f7956b30cdb9f7d10e36c3116976a416c ("Clarify terminology between
"packet tag" and "first octet" of packet")

> - The last paragraph of Section 4.2.1.4 (Partial Body Lengths) lists
> data packet types, and maybe should be revised to include "encrypted
> and integrity protected" data packets, unless it is not intended for
> that packet type as per the final sentence.

I'm really not a fan of the partial body length packets -- i think they
cause implementation confusion and i don't know how many implementations
can handle them (see
https://gitlab.com/sequoia-pgp/openpgp-interoperability-test-suite/-/issues/29).
I have not tried to clean this text up, because i'm kind of concerned
about encouraging anyone to generate more packets of this type.

> - In Section 5.5.3, the items listed as "[Optional]" seem like they
> are actually conditional.  The list in Section 7 is more clear for
> describing conditional items.

I think i agree with you -- "conditional" means you don't get to choose,
but circumstances will dictate whether it appears or not; "optional"
sounds more like "you get to decide whether you want to include it." --
but just replacing "[Optional]" with "[Conditional]" seems confusing to
me too, because the variant form of "conditional" (i.e. what happens
when the predicate is not met) might be "instead of A, use B", whereas
the variant form of "optional" is pretty clearly "instead of A, use
nothing".  So I'm not sure how to fix this concretely.

Section 7 (the cleartext signing framework) is a much shorter list, and
it doesn't use any explicit label like "[Optional]".  we could remove
the label in the earlier section, but i think that makes the text even
less comprehensible.  Suggestions for concrete improvement welcome!

> - Section 5.5.5.6, bullet 3 says it is a "variable-length field", but
> the sub-items are each one octet.

This field is "variable length" because it is a versioned field.  That
is, in the current (and only known) version (1), it is fixed-length.
but a future version could have a different length.  I believe the idea
of an explicit KDF is probably on its way out, and certainly we don't
know how to increment this version in a reliable, interoperable way.  So
i've left this alone for now, but i do think that killing off this
field, or at least committing explicitly to not incrementing it, would
be a good thing (similar to how we've abandoned the idea of incrementing
the MDC approach).

I've opened an issue to track this concern, but i'm not convinced it'll
get into the crypto-refresh if there's anything contentious here:

  https://gitlab.com/openpgp-wg/rfc4880bis/-/issues/161

> - In Section 5.2.3.12 (Preferred AEAD Ciphersuites), indicate this subpacket must be in the cryptographically-protected field of a signature packet to avoid this subpacket being removed to encourage use of default algorithms.  
>
> - Section 5.2.3.17 (Revocable): If a signature is marked as non-revocable in an unhashed subpacket, what is the expected behavour if that signature is revoked?  Perhaps this subpacket should be recommended to be in the cryptographically-protected field.
>
> - Section 5.2.3.32 (Issuer Fingerprint): This subpacket should also be recommended to be cryptographically-protected.

I'm not sure what to do with these recommendations, or how much guidance
the draft needs to give about whether subpackets should be hashed or
not.  I mean, in general, the rule is probably "rely on the hashed
subpacket before you rely on the unhashed one" and "if you need
cryptographic assurance, don't look at unhashed subpackets", but i'm
sure there are exceptions to each of these rules.

I think this is probably best addressed separately, as noted in
https://gitlab.com/openpgp-wg/rfc4880bis/-/issues/96

Regards,

        --dkg