[CFRG] Re: IRTF Chair review of draft-irtf-cfrg-opaque-16

[Kevin Lewi <[e-mail]>]

Hi Colin,

Thanks for providing the review! Comments and replies below:

> Please add a sentence to the end of the Introduction to say “This
document is not an IETF product and is not a standard” to meet the
requirements of RFC 5743.

Done, in https://github.com/cfrg/draft-irtf-cfrg-opaque/pull/460 .

> The Discussion Venues section is marked “to be removed before
publishing”. That’s appropriate, but the CFRG Chairs should add a link to
the GitHub page in the “Additional resources” part of
https://datatracker.ietf.org/doc/draft-irtf-cfrg-opaque/ so it can be found
after publication.

@Stanislav, can you help with this? Thanks in advance!

> Section 2.1: some of the APIs reference specific sections of RFC 9497,
some do not. To avoid confusion, would it make sense to consistently
reference RFC 9497 in cases when the APIs are defined there?

Good idea, this has been addressed in
https://github.com/cfrg/draft-irtf-cfrg-opaque/pull/463 .

> Section 2.2: “Expand(prk, info, L): Expand a pseudorandom key prk using
the optional string info…” – will it be clear to readers what to do if the
optional string is not provided?

As it turns out, this Expand interface as used in the protocol always has
the info string supplied. I edited the text and removed the word “optional”
to make this more clear in
https://github.com/cfrg/draft-irtf-cfrg-opaque/pull/464 .

> Section 2.3: the definition of Stretch(msg) doesn’t specify the size of
the output. Should it? Similar issues occur elsewhere, e.g., auth_tag in
section 4.1.1, so if this is important it may be worth checking that the
sizes are consistently specified.

The Stretch() function can have a variable-length output size, with the
only requirement being that it satisfies collision resistance (which
implies that output size cannot be too small). The way Stretch() is used in
the protocol is that its output is fed into a hash function, and so the
length of the Stretch() output does not matter for protocol correctness
purposes.

I was not able to find any other occurrences of functions which are missing
a definition in the size of their output. The auth_tag value is an output
of MAC(), which is specified to have an output size of Nm.

> Section 3.2: for clarity, I suggest to change “The registration flow is
shown below” to “The registration message flow is shown below and the
process is described in detail in Section 5”.

Addressed in https://github.com/cfrg/draft-irtf-cfrg-opaque/pull/464 ,
thanks for the suggestion!

> Section 4.1.2: I found the syntax Create Envelope envelope with
(envelope_nonce, auth_tag) confusing since it reads like pseudo code but is
in the middle of a more precise description. It wasn’t immediately obvious
to me that it was referring to previously defined types. Would it be
clearer to say envelope = Envelope(envelope_nonce, auth_tag) or similar?
(There is similar phrasing in a number of other places)

Fair enough. The syntax has now been adjusted to read:

envelope = Envelope {

  envelope_nonce,

  auth_tag

}

in all of the places where this occurs. I think this might be a bit better
suited rather than using the constructor notation that you suggested, so as
to more easily distinguish it as a creation of a struct (as opposed to a
subroutine call).

> Section 5: what happens to the results that are not sent to the peer? For
example, CreateRegistrationRequest() returns (request, blind) and it looks
like request is sent to the server, but what is done with blind? (Similar
issue with the ladder diagrams elsewhere in this section and in Section 6)

Thanks for the catch! The blind parameter is supposed to be supplied to the
subsequent FinalizeRegistrationRequest() call in that diagram, but it is
absent from the input parameters. This has been fixed in
https://github.com/cfrg/draft-irtf-cfrg-opaque/pull/465 .

For the other diagram in Section 6, it does follow a similar pattern of
requiring the parties to keep state in between protocol messages, but this
is captured with the ClientState and ServerState structs, which are
(purposefully) not represented in the diagram in order to simplify its
presentation.

> Finally, I note that the appendices contain a number of test vectors. Has
the latest version of the draft been verified against a reference
implementation to ensure these are correct?

Yes, the test vectors in the latest version of the draft have been verified
in the following implementations: https://github.com/cloudflare/opaque-ts,
and https://github.com/facebook/opaque-ke . There are other implementations
which might also be up-to-date with the latest version, but I have not
confirmed: https://github.com/stef/libopaque,
https://github.com/bytemare/opaque/ .

—

Once we have confirmation that these changes suffice, we can go ahead and
publish a new version of the draft with all of these changes incorporated.

Thanks,
Kevin

On Wed, Sep 11, 2024 at 9:42 AM Colin Perkins <[e-mail]> wrote:

> The CFRG chairs have requested that draft-irtf-cfrg-opaque-16 be published
> as an RFC on the IRTF stream. The IRTF publication process is described in
> RFC 5743, and comprises a review by the IRSG to ensure technical and
> editorial quality, followed by a check by the IESG to ensure the work does
> not conflict with IETF standards activities.
>
> As IRTF Chair, I perform an initial review of all drafts submitted for
> publication on the IRTF stream before sending them for detailed review by
> the IRSG. This note provides my review comments, for discussion.
>
> The document shepherd write-up states that the authors have made all
> necessary IPR disclosures, as described at https://irtf.org/policies/ipr.
>
> *Result:* Ready with nits
>
> *RFC 5743 compliance:* The draft does not follow the guidelines in RFC
> 5743
>
> *Comments:*
>
> Thank you for preparing this draft. Apologies for the delay in my review.
> Overall this looks good, and I thank the authors and others in the group
> for the effort they have spent developing and reviewing the work.
>
> I have two procedural details that should be resolved:
>
>    -
>
>    Please add a sentence to the end of the Introduction to say “This
>    document is not an IETF product and is not a standard” to meet the
>    requirements of RFC 5743.
>    -
>
>    The Discussion Venues section is marked “to be removed before
>    publishing”. That’s appropriate, but the CFRG Chairs should add a link to
>    the GitHub page in the “Additional resources” part of
>    https://datatracker.ietf.org/doc/draft-irtf-cfrg-opaque/ so it can be
>    found after publication.
>
> While reviewing the draft, I also noted areas where I thought
> clarifications might be helpful. I’m a generalist, and very much not a
> security expert, so none of these should be considered blocking comments –
> I’m happy to be told that the draft follows the usual conventions in the
> field and the description will be clear to the intended audience:
>
>    -
>
>    Section 2.1: some of the APIs reference specific sections of RFC 9497,
>    some do not. To avoid confusion, would it make sense to consistently
>    reference RFC 9497 in cases when the APIs are defined there?
>    -
>
>    Section 2.2: “Expand(prk, info, L): Expand a pseudorandom key prk
>    using the optional string info…” – will it be clear to readers what to do
>    if the optional string is not provided?
>    -
>
>    Section 2.3: the definition of Stretch(msg) doesn’t specify the size
>    of the output. Should it? Similar issues occur elsewhere, e.g.,
>    auth_tag in section 4.1.1, so if this is important it may be worth
>    checking that the sizes are consistently specified.
>    -
>
>    Section 3.2: for clarity, I suggest to change “The registration flow
>    is shown below” to “The registration message flow is shown below and the
>    process is described in detail in Section 5”.
>    -
>
>    Section 4.1.2: I found the syntax Create Envelope envelope with
>    (envelope_nonce, auth_tag) confusing since it reads like pseudo code
>    but is in the middle of a more precise description. It wasn’t immediately
>    obvious to me that it was referring to previously defined types. Would it
>    be clearer to say envelope = Envelope(envelope_nonce, auth_tag) or
>    similar? (There is similar phrasing in a number of other places)
>    -
>
>    Section 5: what happens to the results that are not sent to the peer?
>    For example, CreateRegistrationRequest() returns (request, blind) and
>    it looks like request is sent to the server, but what is done with
>    blind? (Similar issue with the ladder diagrams elsewhere in this
>    section and in Section 6)
>
> Finally, I note that the appendices contain a number of test vectors. Has
> the latest version of the draft been verified against a reference
> implementation to ensure these are correct?
>
> Regards,
> Colin
>