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

[Jonathan Hammell <[e-mail]>]

Hi DKG,

On Sat, Mar 25, 2023 at 2:10 AM Daniel Kahn Gillmor
<[e-mail]> wrote:
>
> 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.

I appreciate all the effort that has gone into this.

> 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?

I understand your point that a general decision whether to
encrypt-to-self is probably domain-specific.

The reason I suggested it here was that I was thinking of a minor
efficiency gain where the MUA might use the the KDF shortcut described
in the third paragraph of 5.13.2 by caching the encrypted session key
packet for faster decryption when a user is browsing through a thread
of encrypted messages.  However, that is very implementation specific,
so I agree you can ignore this.

> > - 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.

No, I checked MR !267 (and MR !270 and -08) and it doesn't fix this
editorial comment.  The issue is that 5.1.3 and 5.1.4 refer to "the
plaintext value described above", but there is no mention of
"plaintext" in earlier text.

> > - 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.

I'm okay with the textual change in the commit.

> > - 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

Okay.  I see Justus's comment on the MR about certifications, but I
think that would be more confusing for the reader (e.g. I had to go
back a re-read more about certifications).  If it is agreed to go with
this interpretation, I think it should be kept to the algorithm type.
I don't have a strong opinion between "an algorithm capable of making
signatures" or "a signing algorithm".

> > - 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")

Okay, I understand your concern that readers might not be aware of the
subtlety of "cryptographically unique" or be able to assess that.

I looked at the commit and, in that vein, I suggest removing
"cryptographically" to say "A fingerprint is more likely to be unique
than a key ID, but is less  convenient for a human to reference (see
14.6).  The fingerprint and key ID..."

> > - 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.

Sorry, my mistake.  I wasn't familiar with that term.

> > - 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")

Yes, your suggestion in the commit is better.  Thanks.

> > - 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.

Yes, it seems like an implementation headache.  Feel free to ignore to
avoid further proliferation.

> > - 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!

You could remove the "[Optional]" labels and put these items in an
indented sublist preceded by "- Conditionally included fields:".

> > - 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

Okay.  Understood.

> > - 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

Okay.  Thanks.


Regards,
Jonathan

On Sat, Mar 25, 2023 at 2:10 AM Daniel Kahn Gillmor
<[e-mail]> wrote:
>
> 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
> _______________________________________________
> openpgp mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/openpgp