Subject: [Cfrg] 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)

---

From: "Christopher Wood" <[email-masked]>
Date: Sat, 15 Aug 2020 07:00:59 -0700

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

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

> 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

> context of "DeriveKeyPair(random(Nsk))"

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

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

> to whether the replacement ought be "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

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

> 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

> of "signature" for some people and doesn't seem to add much

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

---

From: Stephen Farrell <[email-masked]>
Date: Sat, 15 Aug 2020 15:23:13 +0100

Hiya,

Just a bit more on one thing below...

On 15/08/2020 15:00, Christopher Wood wrote:
>> - 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.

Sure, I get the "why" and am fine with that. The only
concern is that when the RG are finished and then hit
"publication requested" it might take a while for the
RFC number to be known (e.g. a few months). This draft,
plus the esni and svcb drafts are likely to form a small
cluster so that might well extend the delays some. If MLS
were added to that cluster that might add more too.

If OTOH, we ask the RFC editor to replace with a string
we already know then I think that should reduce the
need for breaking interop after the cluster-induced delay.
(It's true that for this to be useful, the same trick
might need to be more widely adopted, but starting with
hpke seems fine as others in the putative cluster would
depend on this and not the other way about.)

Cheers,
S.

---

From: "Manger, James" <[email-masked]>
Date: Mon, 17 Aug 2020 01:59:32 +0000

HTTP/2 Version Identification https://tools.ietf.org/html/draft-ietf-httpbis-http2-17#section-3.1 took a different approach to this issue.
It defined the final string "h2"; said only implementations of the final, published RFC can use that; earlier implementations MUST NOT use that; they can use "h2-<draft-number>", eg "h2-17"; a note to the rfc-editor asked for this text to be remove in the final RFC.

That avoids the risk of delays in the publication pipeline; but increases the risk that an implementation of an incompatible early draft ignores the text and uses the final id. It is probably a better compromise.

--
James Manger

-----Original Message-----
From: Cfrg <[email-masked]> On Behalf Of Stephen Farrell
Sent: Sunday, 16 August 2020 12:23 AM
To: Christopher Wood <[email-masked]>; [email-masked]
Subject: Re: [Cfrg] review of draft-irtf-cfrg-hpke-05

[External Email] This email was sent from outside the organisation – be cautious, particularly with links and attachments.

Hiya,

On 15/08/2020 15:00, Christopher Wood wrote:
>> - 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.)
>

Sure, I get the "why" and am fine with that. The only concern is that when the RG are finished and then hit "publication requested" it might take a while for the RFC number to be known (e.g. a few months). This draft, plus the esni and svcb drafts are likely to form a small cluster so that might well extend the delays some. If MLS were added to that cluster that might add more too.

If OTOH, we ask the RFC editor to replace with a string we already know then I think that should reduce the need for breaking interop after the cluster-induced delay.
(It's true that for this to be useful, the same trick might need to be more widely adopted, but starting with hpke seems fine as others in the putative cluster would depend on this and not the other way about.)

Cheers,
S.

---

From: Ilari Liusvaara <[email-masked]>
Date: Mon, 17 Aug 2020 11:37:19 +0300

On Mon, Aug 17, 2020 at 01:59:32AM +0000, Manger, James wrote:
> HTTP/2 Version Identification https://tools.ietf.org/html/draft-ietf-httpbis-http2-17#section-3.1
> took a different approach to this issue. It defined the final string
> "h2"; said only implementations of the final, published RFC can use
> that; earlier implementations MUST NOT use that; they can use
> "h2-<draft-number>", eg "h2-17"; a note to the rfc-editor asked for
> this text to be remove in the final RFC.

And on what other things did, Websockets (RFC 6455) had version number
that was the draft number of last incompatible change. And it happened
that the final version number in the RFC (13), was not the same as the
final draft number (17). The preceeding version to that was 8.

ACME (RFC 8555) would have used sequential version numbers for
incompatible method versions, starting from -01, but turns out that no
such changes happened for any published method (one method got version
-02, but was latter scrapped entierely).

-Ilari