Subject: [Crypto-panel] review of draft-irtf-cfrg-hpke-05

From: Stephen Farrell <[email-masked]>
Date: Mon, 10 Aug 2020 18:22:21 +0100

Hiya,

I implemented draft -02 and will be doing -05 shortly.  I
didn't yet do that so haven't verified the test vectors.
Will follow up if any issues arise.

Overall this is ready to proceed IMO. If my entire review
were ignored all would still be ok.  I do have one thing I
think the RG might want to discuss, a few minor comments
and some very ignorable nits.

Cheers,
S.

The thing to ponder relates to the IANA considerations.
Why not add a "recommended" column a la TLS.  The RG can
hand over responsibility to some DEs appointed by the IESG
and call for the same setup as TLS.  (I.e. other than
the initial values recommended == yes requires IETF
standards track, otherwise spec required.) If we don't
do that then applications using HPKE will always each
need to say which suites are MUSTs, leading to IMO mostly
pointless variation and possibly worsening interop if
libraries implement disjoint sets of suites.

Minor comments:

- Is random() needed? it only seems to be used in the
context of "DeriveKeyPair(random(Nsk))"

- Do we really need to s/HPKE-05/RFCXXXX/ later? Why not
just change to "HPKE-first-rfc" once the RG are done with
the document? (There can be delays @ IRSG and subsequently
that I'd prefer not have to affect interop.)

- p7 & elsewhere: refers to "HPKE-05 " - that space seems
like a bad idea.  I missed it until I got to p27.  And
it's not consistently present and the text is ambiguous as
to whether the replacement ought be "RFCXXXX"
or "RFCXXXX "

- It strikes me as odd to not have any identifier for the
public key being used but to have one for the PSK being
used. If allowing the application to handle that for
public keys works, why won't that work for PSKs?

- DeriveKeyPair() for NIST curves requires the HPKE code
to know the order of the various NIST curves. That seems
like the kind of thing where bugs might arise that
wouldn't be noticed much. I also don't think many crypto
APIs would provide a ``get_order_as_integer(curve_id)``
function, so using the wrong value would seem not
unlikely. I'd say maybe add the values to use here
somewhere for each curve. If not in 7.1.2 then put in a
forward reference. Perhaps also say what can happen if the
wrong value for "order" is used.

- p29: I'm not sure why section 8.8 is useful here. I think
it'd be better deleted TBH - it might muddy the definition
of "signature" for some people and doesn't seem to add much
as-is.

nits:

- p4: PGP and fiveG don't depend on HPKE, whereas msl and
esni do. Maybe worth saying that.

- p5: I2OSP and OS2IP aren't expanded here - be no harm to
do so

- p6: Open as the opposite of Seal, doesn't seem like the
best choice - there are too many functions called open()
in too many contexts. Unseal would be better IMO.

- p5: DeriveKeyPair was added after discussion in github
(and maybe on the MLS list) but I don't recall any
disussion of that on the CFRG list at all. Was there any?
Adding this seems fine to me if MLS wants it and the
definition is also almost fine (see above) so this is just
a process nit.

- p5 & elsewhere: "fixed-length" is used in various places
where it's not quite true - the various lengths are fixed
only after you pick a ciphersuite - so people's code has
to support different sizes (if they support >1 suite)