Subject: [Webpush] AD Evaluation: draft-ietf-webpush-encryption

From: Adam Roach <[email-masked]>
Date: Mon, 10 Jul 2017 18:04:40 -0500

Webpush Encryption author --

This is my AD review for draft-ietf-webpush-encryption-08.

This document is ready for IETF last call. I have a handful of comments, 
mostly editorial, that you should treat the same as any other last call 
comments.

The notational conventions section is cute; however, I believe that it 
would benefit from being brought in line with the more conventional 2119 
boilerplate.

The final paragraph of section 2.1 uses the word "any" in a rather 
sweeping fashion, implying that the algorithms, key sizes, and 
consequent strengths for providing authentication, integrity, and 
confidentiality are immaterial. I would suggest qualifying this more 
carefully.

The summary in section 3.4 is greatly appreciated. I'm a bit confused 
about the "salt = random(16)" as something that appears in the "for 
both" section. As written, this appears to indicate that each side 
generates their own salt as input to the PRK, which I doubt works 
(unless there's some black magic at play here that I don't understand). 
Appendix A would appear to indicate that the Application Server 
generates this value, and provides it to the "receiver" (user agent?). 
This is all made somewhat more confusing by the fact that 3.3 defines 
"salt" (which is the HKDF salt rather than the PRK generation salt) to 
be equal to the authentication secret, generated by the client. Needless 
to say, it's pretty easy to get lost in all the salting -- I would 
suggest some notational distinction between them. Also, please make it 
clear in Section 3.4 who generates the salt for the PRK.

Section 4: s/the some of the length/the sum of the length/

Section 5: "base64url" needs a definition -- I would suggest citing RFC 
4648, section 5.

The final ciphertext in the appendix appears to contain a spurious space.

Please expand the following acronyms upon first use; see 
https://www.rfc-editor.org/materials/abbrev.expansion.txt for guidance.

  - UA - User Agent
  - HMAC - Hashed Message Authentication Code
  - PRK - Unknown
  - CEK - content-encryption key

/a

---

From: Martin Thomson <[email-masked]>
Date: Tue, 11 Jul 2017 10:48:03 +1000

Thanks Adam,

Should you care: https://github.com/webpush-wg/webpush-encryption/pull/15

I can generate a new version and submit it at a time of your choosing
if these changes are acceptable.

On 11 July 2017 at 09:04, Adam Roach <[email-masked]> wrote:
> The final paragraph of section 2.1 uses the word "any" in a rather sweeping
> fashion, implying that the algorithms, key sizes, and consequent strengths
> for providing authentication, integrity, and confidentiality are immaterial.
> I would suggest qualifying this more carefully.

I've changed this to "An authenticated communication mechanism that
provides adequate confidentiality and integrity protection, such as
HTTPS [...]"

> The summary in section 3.4 is greatly appreciated. I'm a bit confused about
> the "salt = random(16)"

That's an error.  I have moved this to the right place.

> Section 4: s/the some of the length/the sum of the length/

Not in my copy; I think that Matt fixed this for me already.

> Section 5: "base64url" needs a definition -- I would suggest citing RFC

How did I miss that...

> The final ciphertext in the appendix appears to contain a spurious space.

I broke these into 32 character chunks so that line wrapping wouldn't
hurt too much, that's all.  I'll explain that better.