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

Martin Thomson <martin.thomson@gmail.com> Wed, 16 August 2017 06:09 UTC

Return-Path: <martin.thomson@gmail.com>
X-Original-To: webpush@ietfa.amsl.com
Delivered-To: webpush@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id D990A124BAC; Tue, 15 Aug 2017 23:09:35 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.7
X-Spam-Level:
X-Spam-Status: No, score=-2.7 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_LOW=-0.7, SPF_PASS=-0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id H-EO_bYWcaKY; Tue, 15 Aug 2017 23:09:33 -0700 (PDT)
Received: from mail-io0-x22a.google.com (mail-io0-x22a.google.com [IPv6:2607:f8b0:4001:c06::22a]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 3B0BA1241F5; Tue, 15 Aug 2017 23:09:33 -0700 (PDT)
Received: by mail-io0-x22a.google.com with SMTP id g71so10026035ioe.5; Tue, 15 Aug 2017 23:09:33 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=yRbq9DqQhgGPeh4tvDtNjgNUGurIfmAmnuzQBiSm0Ro=; b=gPrUiJxmIyBrydA71qhGYzBsKvQwB2CXgraUGlVy1dFd/d0GlTcB2v+7DkqCkKi65S LJxqPEVO6H8gaBpfH4yAnxhtk02wC91WlVNTSryF7F3h8+K7XDY9+lvdik7mDHrSYJWh eZUuPoFKLiItHWJdm0zsEpnEV2sSHpCj3Pp0hd77nwyqHeuwDpalm8FCyKHFZoi0xtKz N4w5GBMBL2mfHl3V57VK3iI3s0OveK4J3XMyoDECDKS+lRHxZ742mVRcv9MBtyXpf69C 37EpWSagkAqoqeP0XCjyzY/qHbR+dYvZGbr7cQuXfa3w46SceimA0WxaHt8mtiGlLIdK eZCg==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=yRbq9DqQhgGPeh4tvDtNjgNUGurIfmAmnuzQBiSm0Ro=; b=E/uxEm0q6g8XqinnWL+p/l2xUvoz325LxApA8CflpVzqSkHHAmm3hdeST7jF424JGv bSEVg2zQ9Co09ZFfIBMt8AbAFoTV8RU6yF0wSnsI/gY/rMJZHkyk+FUWdavCSWcCJsft dm5ga3adtWV8hqMN/PyekNkx7K62aj9LXjCWw5edGs83ihnZCstTmQUrwfou3gz86zeq bLN1TYLeM4uDJ77aJtl5QxfWXndmYcAhRVY1h+od8hFIx700fOzPmGjkoio1cuWCmcRH qxDsGIk7mMDIMS0NR2dYv+Kj9241LTf9vlGWB9d1eXVbfvQjM434a289itLd4lKmgDj4 oRvQ==
X-Gm-Message-State: AHYfb5hiNtL1pC+0BOl8jtb/mD7Maw/rU5PjoaCO8C/Q5qOiGGhRLUiV ool/SSgHlf37+uEYBfd1TZjbHBLBRw==
X-Received: by 10.107.134.196 with SMTP id q65mr485526ioi.193.1502863772364; Tue, 15 Aug 2017 23:09:32 -0700 (PDT)
MIME-Version: 1.0
Received: by 10.107.164.42 with HTTP; Tue, 15 Aug 2017 23:09:31 -0700 (PDT)
In-Reply-To: <150285009047.12577.1184580798882485308.idtracker@ietfa.amsl.com>
References: <150285009047.12577.1184580798882485308.idtracker@ietfa.amsl.com>
From: Martin Thomson <martin.thomson@gmail.com>
Date: Wed, 16 Aug 2017 16:09:31 +1000
Message-ID: <CABkgnnU_QXMuVZMNycW=yj9-kU=v+Ooo5HV0v9p84zcYzdKA_Q@mail.gmail.com>
To: Ben Campbell <ben@nostrum.com>
Cc: The IESG <iesg@ietf.org>, draft-ietf-webpush-vapid <draft-ietf-webpush-vapid@ietf.org>, Phil Sorber <sorber@apache.org>, webpush-chairs@ietf.org, "webpush@ietf.org" <webpush@ietf.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://mailarchive.ietf.org/arch/msg/webpush/2qsIZet7Icn7e6j3ESzk6Oio_Bc>
Subject: Re: [Webpush] Ben Campbell's Discuss on draft-ietf-webpush-vapid-03: (with DISCUSS and COMMENT)
X-BeenThere: webpush@ietf.org
X-Mailman-Version: 2.1.22
Precedence: list
List-Id: Discussion of potential IETF work on a web push protocol <webpush.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/webpush>, <mailto:webpush-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/webpush/>
List-Post: <mailto:webpush@ietf.org>
List-Help: <mailto:webpush-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/webpush>, <mailto:webpush-request@ietf.org?subject=subscribe>
X-List-Received-Date: Wed, 16 Aug 2017 06:09:36 -0000

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 <ben@nostrum.com> 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.