Subject: [Webpush] Ben Campbell's Discuss on draft-ietf-webpush-vapid-03: (with DISCUSS and COMMENT)

From: Ben Campbell <[email-masked]>
Date: Tue, 15 Aug 2017 19:21:30 -0700

Ben Campbell has entered the following ballot position for
draft-ietf-webpush-vapid-03: Discuss

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-webpush-vapid/

----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

In section 2: "A push service MAY reject a request with a 403 (Forbidden) status
code [RFC7235] if the JWT signature or its claims are invalid."

This seems to leave the possibility of simply ignoring an invalid VAPID token
or signature. Assuming we are talking about push servers that support
VAPID in the first place, that seems dangerous. Wouldn't it be safer in
the general case to treat a request with an invalid VAPID token as at
least a bit fishy?

I don't mean to say that ignoring the token is never the right thing to
do. But the MAY seems week without some guidance on what other actions
might be reasonable under what circumstances.

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

Substantive:

I agree with Ekr's DISCUSS.

-3:
"This authentication scheme is intended for use by an application server
when using the Web Push protocol [RFC8030], but it could be used in
other contexts if applicable."

This guidance seems risky without some description of the properties a
given context should have for vapid to be appropriate to use. Please
consider elaborating, or removing the clause after the comma.

"Some implementations permit the same P-256 key to be used for signing
and key exchange.  An application server MUST select a different private
key for the key exchange [I-D.ietf-webpush-encryption] and signing the
authentication token."

I don't understand the intent here. It seems to say some implementations
do this, but MUST not. Does "implementations" refer to VAPID or
something else?

-4.1: "A push service MUST ignore the body of a request that uses a
different media type."

This seems to give VAPID a monopoly on adding
bodies. It precludes adding other media types in future webpush
extensions. Is that the intent?

- 4.2: "A push service MUST reject a message that omits mandatory
credentials with a 401 (Unauthorized) status code. "

This was already stated in section 2. Please avoid repeating normative
text, as it creates ambiguity about which text is authoritative, and can
complicate future updates.  This section seems the more detailed, so perhaps
section 2 could avoid the 2119 text. (But see my DISCUSS point on section 2.)

- 8.2:  [I-D.ietf-webpush-encryption] should be normative, since it is
referenced in a MUST level normative requirement in section 3.2. (If
that reference is not intended to be normative, then please rephrase the
referring sentence.)

Editorial:

- Abstract: Please mention the name of the mechanism in the abstract. It
might also be helpful to mention that there are cryptographic signatures
involved. (Same for section 1).

In general, the numerous mentions of "this design" tend to be ambiguous
about whether we talk of the design in this document rather than that of
webpush in general. Once could substitue "VAPID" for many instances.

- 1.0: "Additionally, this design also ..." "Additionally" and "also"
are redundant

- 1.2: Please use the boilerplate from RFC 8174.

- 4.1: "The user agent includes the public key of the application server when
requesting the creation of a push subscription."

I assume this means "A user agent that wishes to create a restricted
subscription...". I know the heading says "Creating a restricted
subscription", but in general the body text make sense even without the
heading.

"The public key is then added to the request to create a push
subscription.  The push subscription request is extended to include a
body.  The body of the request is a JSON object as described in
[RFC7159].  A "vapid" member is added to this JSON object, containing
the public key on the P-256 curve, encoded in the uncompressed form
[X9.62] and base64url encoded [RFC7515].  The media type of the body is
set to "application/webpush-options+json" (see Section 6.3 for
registration of this media type)."

The actor(s) are obscured by the heavy use of passive voice.

- 6.3: Don't we usually list the IESG as the change controller?

---

From: Martin Thomson <[email-masked]>
Date: Wed, 16 Aug 2017 16:09:31 +1000

Hi Ben,

I've collected the changes mentioned below into a PR here:
  https://github.com/webpush-wg/webpush-vapid/pull/44

> might be reasonable under what circumstances.

I think that "MAY" is entirely appropriate given the non-blocking
nature of the mechanism. A push service might look at the header
field, but not require the token. Maybe they just wanted to attach an
email address to a stream of messages (that's what we do).

> consider elaborating, or removing the clause after the comma.

I forget how that got there.  Unless someone screams, I've removed the
clause in your PR.  If it was critical somehow, there isn't anything
stopping someone finding a use for this and reusing it without the
clause.

> do this, but MUST not. Does "implementations" refer to VAPID or

In NSS and many elliptic curve libraries, you can create an EC key for
P-256.  You can then use that key for signing AND key exchange.  You
shouldn't though.

> extensions. Is that the intent?

No, changed to "can" instead.  The intent was to allow for extension,
not prohibit it.

> section 2 could avoid the 2119 text. (But see my DISCUSS point on section 2.)

This isn't included in Section 2.  Section 2 defines what a valid
token is.  This section defines behaviour in the case that a
subscription is limited to a particular application server key.  I
realize now that this might have been unclear, so I have reworded to:

"A push service MUST reject a message sent to a restricted push subscription if
that message includes no "vapid" authentication or invalid "vapid"
authentication.  A 401 (Unauthorized) status code might be used if the
authentication is absent; a 403 (Forbidden) status code might be used if
authentication is invalid."

This also allows me to clear up a minor gripe about the prescriptive
nature of the text regarding status codes.

> that reference is not intended to be normative, then please rephrase the

Normative is easier than thinking about rephrasing :)

> involved. (Same for section 1).

A legacy of the history of the document - it started as a survey of
alternatives and the abstract never got cleaned up fully.

> webpush in general. Once could substitue "VAPID" for many instances.

Corrected as I saw them.  Of the two instances of the word "design",
one was corrected already (see below) and the other I have corrected
to reference RFC 8030, which is the subject in both cases.

> - 1.0: "Additionally, this design also ..." "Additionally" and "also"

Hopefully fixed in the changes I made in response to ekr's review:
https://github.com/webpush-wg/webpush-vapid/pull/43/files

> The actor(s) are obscured by the heavy use of passive voice.

I made a couple of changes.

> - 6.3: Don't we usually list the IESG as the change controller?

I'm mostly cargo-culting here.  This choice didn't elicit comment when
I asked for media-type review.

For reference, the last one of these that went through the IESG came
back with very specific instructions, and a citation.  I can't find
that email, or the citation, but I think that it was RFC 6838, Section
3.1, which doesn't seem particularly crisp:

   The "owner" of a media type registered in the standards tree is
   assumed to be the standards-related organization itself.

I did find the documents that I remember: RFCs 8142 and 7946 use
"Internet Engineering Task Force" and "IETF" respectively.  It seems
like this isn't entirely consistent though.  I will accept the wisdom
of the IESG on this matter.

---

From: Alexey Melnikov <[email-masked]>
Date: Wed, 16 Aug 2017 09:54:24 +0100

> of the IESG on this matter.

IMHO, either IESG or IETF is fine. In both cases we know what you mean.

---

From: Ben Campbell <[email-masked]>
Date: Wed, 16 Aug 2017 10:52:42 -0500

> On Aug 16, 2017, at 1:09 AM, Martin Thomson <[email-masked]> wrote:
> 
> Hi Ben,
> 

That mostly looks good, modulo discussion below. I think you dropped the word “meaning” from the 8174 (was 2119) reference. Also, that RFC recommends specific boilerplate. (I’m not going to get wrapped around whether you use that specific boilerplate, as long as the meaning is clear.)
> email address to a stream of messages (that's what we do).

I think there’s a fundamental difference between not validating in the first place, and ignoring a failed validation. My point is not to say that ignoring it is the wrong thing to do, just that I think some more elaboration here is needed. Would it make sense to say something of the effect of “SHOULD NOT use the information from a token that has failed validation”?

[…]

> P-256.  You can then use that key for signing AND key exchange.  You

Would it make sense to say “Some library implementations…”?

[…]

> token is.

Oops, that was a cut and paste error. I meant to quote the following sentence: 

" A push service MAY reject a
   message that includes invalid credentials with a 403 (Forbidden)
   status code. "

>  This section defines behaviour in the case that a

I like that paragraph better, but does it conflict with the text in section 2, as discussed in my DISCUSS?

[…]

> of the IESG on this matter.

I’ll go with Alexey’s separate response. That is, never mind.

---

From: Martin Thomson <[email-masked]>
Date: Thu, 17 Aug 2017 11:01:02 +1000

> That mostly looks good, modulo discussion below. I think you dropped the word “meaning” from the 8174 (was 2119) reference. Also, that RFC recommends specific boilerplate. (I’m not going to get wra…

The master branch has the boilerplate.  I have updated the PR, which
no longer includes a change.

> I think there’s a fundamental difference between not validating in the first place, and ignoring a failed validation. My point is not to say that ignoring it is the wrong thing to do, just that I t…

That's easy.  MUST NOT even.  I added:  "A push service MUST NOT use
information from an invalid token."

> Would it make sense to say “Some library implementations…”?

The PR says "Some elliptic curve implementations ".  I think that's better.

>    message that includes invalid credentials with a 403 (Forbidden)

I'll respond below.

> I like that paragraph better, but does it conflict with the text in section 2, as discussed in my DISCUSS?

The key point here is that the rejection mandated in Section 4.2 is
specific to *restricted* subscriptions.  The text in Section 2 (above)
is a more general statement.  I don't think that the two are in
conflict.

> I’ll go with Alexey’s separate response. That is, never mind.

:)

---

From: Ben Campbell <[email-masked]>
Date: Wed, 16 Aug 2017 20:04:22 -0500

> On Aug 16, 2017, at 8:01 PM, Martin Thomson <[email-masked]> wrote:
> 
> information from an invalid token.”
> 

WFM. I will clear.

> The PR says "Some elliptic curve implementations ".  I think that's better.

Also WFM.

> is a more general statement.  I don't think that the two are in

Okay.

[…]