Re: [Jmap] Benjamin Kaduk's Discuss on draft-ietf-jmap-core-14: (with DISCUSS and COMMENT)

[Benjamin Kaduk <[e-mail]>]

Just replying on the discussion points for now, as I want to also be
reviewing the jmap-mail document today...

Most of these look good, and thank you for the updates!

On Thu, Feb 28, 2019 at 01:14:44AM -0500, Neil Jenkins wrote:
> Thanks for the detailed feedback Benjamin.
> 
> *Discussion points*
> 
> > This document (twice) has a MUST requirement for HTTP over TLS, which
> > seems to exclude any future usage of HTTP/3 over QUIC. (It's also
> > probably not needed to repeat the normative requirement in two places; I
> > noted both in the COMMENT section.)
> 
> OK, I'm happy to change this to say https:// scheme instead. I've changed the text to just say:
> 
> *JMAP uses HTTP [@!RFC7230] to expose API, Push, Upload and Download resources. All HTTP requests MUST use the https:// scheme ([@!RFC2818] HTTP over TLS).*
> 
> I have moved the minimum TLS requirements to the security considerations, and removed the HTTP reference there, so the normative requirements are no longer duplicated.
> 
> > Section 1.6.2 asserts that "all data belong to a single account". And
> > yet, we seem to have PushSubscription objects in Sections 7.2.1 and
> > 7.2.2 that disclaim any relationship to an account, which seems
> > internally inconsistent. 
> 
> Yes, this was inconsistent; PushSubscription objects are special as they are tied to authentication credentials rather than an account. I have removed just this sentence, since I think the document is clear enough without it.
> 
> > It's also unclear to me from the text in the
> > latter sections what mechanism is used to determine whether a given
> > request is authorized to see a given PushSubscription object. Is it
> > supposed to be based on the authentication credentials to the API
> > service directly, or a user abstraction, or something else?
> 
> The authentication credentials. You're right this was not clear; I have added explicit statement of this to the document.
> 
> > Section 5.3
> > 
> >  Some records may hold references to other records (foreign keys).
> >  That reference may be set (via create or update) in the same request
> >  as the referenced record is created. To do this, the client refers
> >  to the new record using its creation id prefixed with a "#". The
> >  order of the method calls in the request by the client MUST be such
> >  that the record being referenced is created in the same or an earlier
> >  call. The server thus never has to look ahead. Instead, while
> > 
> > I think this means you need to specify what order the server does the
> > create, update, and destroy lists in -- that is, that all creates are
> > done before all updates, etc..
> 
> This is only true for objects with references to the same type (e.g. Mailbox). I have added this sentence to the end of that paragraph:
> 
> *In the case of records with references to the same type, the server MUST order the creates and updates within a single method call so that creates happen before their creation ids are referenced by another create/update in the same call.*
> 
> There is no need to specify more constraints than this for ordering as it would not be externally visible; servers should have the flexibility to optimise as they see fit.

I agree that this sort of ordering is the relevant part and we don't
actually need to specify an absolute order of operations.  Though I guess
if some client was silly enough to both create and delete the same item in
a single request, the delete would need to be referred to by the creation
ID, which would provide the needed dependency to force the ordering.  So
this adversarial scenario just would need to change "create/update" to
"create/update/delete" to be covered.

> > Section 5.5
> > 
> > The Unicode Collation Algorithm (<http://www.unicode.org/reports/tr10/>
> > is not listed in the IANA collation registry for internet application
> > protocols; since the session object limits the collationAlgorithms to
> > those in the registry, at present, it is not permitted to use that
> > collation algorithm. It would seem that this document should stimulate
> > the registration of that collation algorithm in some fashion (whether by
> > explicitly doing so in its IANA Considerations or otherwise).
> 
> There is no requirement in the spec for the default algorithm to be one of those listed in the collationAlgorithms capability; the server can do whatever it wants in the default case. We will certainly look at registering UCA, but such a registration doesn't belong in this document and will not materially change this document so should not block publication.

Rereading, I see that the example of Unicode Collation is just as one that
meets the requirements needed for a (server-dependent) default algorithm,
as you note.  So, sorry for misreading that (and I would have cleared
anyway with just a declaration of intent to register).

> > Section 7.1
> > 
> >  o *changed*: "Id[TypeState]" A map of _account id_ to an object
> >  encoding the state of data types that have changed for that
> >  account since the last StateChange object was pushed, for each of
> > 
> > I don't see how these semantics provide the properties stated in Section
> > 7, "[i]t doesn't matter if some push events are dropped before they
> > reach the client; it will still get all changes next time it syncs." In
> > particular, if the client misses a state change for the CalendarEvent
> > type, and then no other changes that affect CalendarEvents occur, the
> > client will remain unaware of the changes to CalendarEvents, even if
> > other push notifications for other types come in. Am I misunderstanding
> > one or more of the behavior or stated guarantees?
> 
> It's stating that on the next resync, whether that be due to a future push for the same type, or the client making any /get or /set for that type and seeing the different state string returned, will result in the client coming fully up to date. Losing the push does not mean there is data the client will now no longer see.

Perhaps there is room for a little more clarity on what "it syncs" means
(i.e., full sync or just scoped to a specific data type).  To be clear,
this is a fine design; I just want to be sure that we have clarity of
language in describing it.

> > Section 7.2
> > 
> >  As a push subscription causes the JMAP server to make a number of
> >  requests to a previously unknown endpoint, it can be used as a vector
> >  for launching a denial of service attack. To prevent this, when a
> >  subscription is created the JMAP server immediately sends a
> >  PushVerification object to that URL (see section 7.2.2). The JMAP
> >  server MUST NOT make any further requests to the URL until the client
> >  receives the push and updates the subscription with the correct
> >  verification code.
> > 
> > I think the JMAP server also needs to rate-limit even the initial
> > PushVerification generation, per-user(?), in order to close the DoS
> > and annoyance-vector risks.
> 
> Yes, for annoyance mitigation there should be some rate limits here. I don't think we need to be specific on how it is rate-limited; that's up to the server. I'll add a mention of this to the security considerations.
> 
> > 
> >  o *keys*: "Object|null" (immutable) Client-generated encryption
> >  keys. If supplied the server MUST use them as specified in
> >  [RFC8291] to encrypt all data sent to the push subscription. The
> >  object MUST have the following properties:
> > 
> >  * *p256dh*: the P-256 ECDH Diffie-Hellman public key as described
> >  in [RFC8291], encoded in URL-safe Base64 representation as
> > 
> > What's the crypto agility story for these web push encryption keys?
> > (See BCP 201.)
> 
> There isn't one, because as far as I can see RFC8291 <https://tools.ietf.org/html/rfc8291> doesn't have one and that's what this is supporting. What do you suggest?

The pragmatic thing to do would be to note in the security considerations
that there is no algorithm agility for Web Push Encryption, and if agility
becomes needed in the future a spec update would likely be needed to
provide it.

> > Also, when these expirations fire (e.g., for Basic Authentication
> > credentials), we need a normative requirement to actually destroy the
> > private credentials; there's a lot going on here so maybe I missed it,
> > but I don't think I saw one.
> 
> I think we already have this. The spec says:
> 
> *The push subscription is tied to the credentials used to authenticate the API request that created it. Should these credentials expire or be revoked, the push subscription MUST be destroyed by the JMAP server.*
> 
> Or were you referring to something else?

I was thinking that you need to clear out the memory/disk storage that hold
the credentials (e.g., password), as well as destroying the subscription
object.  We don't want plaintext credentials floating around longer than
needed.

-Benjamin