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

[Martin Thomson <[e-mail]>]

Hi Ben,

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

On 16 August 2017 at 12:21, Ben Campbell <[e-mail]> wrote:
> 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.

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

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

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.

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

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.

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

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

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

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.

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

Normative is easier than thinking about rephrasing :)

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

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

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

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

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

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

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.