Re: [Cfrg] review of draft-irtf-cfrg-hpke-05

[Christopher Wood <[e-mail]>]

Thanks for the feedback, Stephen! Please see inline below.

On Mon, Aug 10, 2020, at 10:22 AM, Stephen Farrell wrote:
> 
> 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.

Excellent. Suggestions for improving the test vector format are also welcome, should you find it problematic when testing. 

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

Interesting suggestion. I filed this issue to track it:

   https://github.com/cfrg/draft-irtf-cfrg-hpke/issues/145

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

It's just a utility function, so it seems appropriate to define along with the others.

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

This was done to ensure each draft version (going forward) is incompatible. We probably should have done this from the start.

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

Hmm, the replacement text says, 'replace "HPKE-05" with "RFCXXXX"', which should yield the correct result when piped through sed. That said, the extra space probably isn't needed. We can remove it:

   https://github.com/cfrg/draft-irtf-cfrg-hpke/issues/143

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

Well, the identifier for the public key is typically the public key. This doesn't 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.

That's a good suggestion. We can just list the orders for each of these curves:

   https://github.com/cfrg/draft-irtf-cfrg-hpke/issues/141

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

That's reasonable. I filed an issue to discuss removing it:

   https://github.com/cfrg/draft-irtf-cfrg-hpke/issues/142

> nits:

<snip>

I filed this issue to track the nits:

  https://github.com/cfrg/draft-irtf-cfrg-hpke/issues/144

Thanks again!

Best,
Chris