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

From: Benjamin Kaduk <[email-masked]>
Date: Fri, 1 Mar 2019 14:09:56 -0600

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

---

From: "Neil Jenkins" <[email-masked]>
Date: Sun, 03 Mar 2019 21:52:33 -0500

On Sat, 2 Mar 2019, at 07:10, Benjamin Kaduk wrote
> So this adversarial scenario just would need to change "create/update" to

Agreed, I'll make this change.

> language in describing it.

OK, how about this:

*It doesn't matter if some push events are dropped before they reach the client; next time it gets/sets any records of a changed type it will discover the data has changed and still sync all changes.*

> becomes needed in the future a spec update would likely be needed to

Sure, happy to do that.

> object. We don't want plaintext credentials floating around longer than

Which credentials are you referring to here? The push subscription doesn't contain any except I guess for the URL itself; I can note that this and the encryption keys MUST be securely erased from memory/storage immediately when the subscription is destroyed? If you're referring to the client's credentials, we're explicitly talking about when they've been expired or revoked, so are already useless.

Neil.

---

From: Benjamin Kaduk <[email-masked]>
Date: Sun, 3 Mar 2019 21:02:17 -0600

> *It doesn't matter if some push events are dropped before they reach the client; next time it gets/sets any records of a changed type it will discover the data has changed and still sync all change…

That works (modulo the nit of "the next time").

> > becomes needed in the future a spec update would likely be needed to

I've been informed in the meantime that the idea of the HTTP Encryption
coding folks, as specified in Section 2 of RFC 8188, is that you define a
new content-coding scheme (i.e., other than "aes128gcm") if you need a new
cipher.  So it probably wouldn't be too painful if we did need to slot a
new one of these in.

> Which credentials are you referring to here? The push subscription doesn't contain any except I guess for the URL itself; I can note that this and the encryption keys MUST be securely erased from m…

I may have been confused about whether this was JMAP Client/JMAP Server or
JMAP Server/push server interactions.  That is, I was thinking about the
JMAP server clearing out any keys or credentials it had for the second sort
of interaction.  So I guess that translates to "when the subscription
expires, wipe your copy of the client-generated encryption keys", right?

Thanks,

Benjamin

---

From: "Neil Jenkins" <[email-masked]>
Date: Sun, 03 Mar 2019 22:11:07 -0500

On Mon, 4 Mar 2019, at 14:02, Benjamin Kaduk wrote:
> of interaction.

OK, so yes this translates to clearing out the URL and any client-generated encryption keys. I have added:

*When a push subscription is destroyed, the server MUST securely erase the URL and encryption keys from memory and storage as soon as possible.*

Cheers,
Neil.

---

From: Benjamin Kaduk <[email-masked]>
Date: Sun, 3 Mar 2019 21:42:04 -0600

> *When a push subscription is destroyed, the server MUST securely erase the URL and encryption keys from memory and storage as soon as possible.*

Cool; thanks!

---

From: Benjamin Kaduk <[email-masked]>
Date: Fri, 8 Mar 2019 13:44:19 -0600

> Thanks for the detailed feedback Benjamin.

Forking the thread to get back to the comments; the -16 looks good w.r.t.
the discussion points.

> *Discussion points*
[...]
> 
> *Comments*
> 
> >  o "String[A]" - A JSON object where the keys are all "String"s, and
> >  the values are of type "A".
> > 
> > To avoid confusion about whether String is the only possible map key
> > type (vs., e.g., Id), maybe "A[B]" notation is more appropriate?
> 
> Sure, done.
> 
> > Section 1.2
> > 
> > Do we want to give any guidance about when it is (not) advisable to use
> > very short identifiers (even the 1-2 character case could be
> > noteworthy)? (I don't know what that guidance would be, so "no" is a
> > perfectly fine answer!)
> 
> Hmm, no. I don't think it really matters; the server can use whatever scheme it likes. (For example, might have monotonically increasing integers which you prefix with a letter for the type, leading to ids like "K1", "J6" etc. in the early cases; this is a perfectly reasonable scheme to use).
> 
> > W.r.t "NIL", that is the specifc 3-character identifier, right? Or do
> > we need to worry about having it embedded as part of a larger string?
> 
> Just the exact string "NIL".

Okay.  I guess the list would even be more parallel if the text was "Ids
that are the exact three characters "NIL" [...]", but this is your call.

> > (I don't see how prefixing every ID with an alphabetical character
> > avoids the "NIL" case, unless 'N' is disallowed from being that
> > character.)
> 
> Well, yes true, but this is general advice not a normative requirement, and if you might produce the letters "IL" as a suffix, then don't choose "N" as a prefix. Even then though, it should be fine unless these same ids are shared with another protocol where this string has special significance.
> 
> > Section 1.3
> > 
> > My math degree obliges me to note that, per trichotomy, zero is not a
> > positive integer.
> 
> I've renamed this to `UnsignedInt`.

Thanks.  (To be honest, I wasn't actually expecting this to change, so it
was a pleasant surprise.)

> > Section 1.6.1
> > 
> > nit: perhaps a "user object", or some broader rewrite like "in this
> > document, the keyword 'user' is used to refer to [...]" -- the current
> > text feels like it's dehumanizing, well, humans.
> 
> I have reworded this to:
> 
> *A user is a person accessing data via JMAP. A user has a set of permissions determining the data that they can see.*

Thanks.

> > Section 1.6.2
> > 
> > nit re. "All data belong to a single account": I assume this is "each datum
> > has a map to its respective account", not "the entire set of data in the
> > system is associated with a single privileged account", as that would
> > make the account distinction rather useless.
> 
> Yes. I have removed this sentence as redundant already based on previous feedback.
> 
> > Section 1.7
> > 
> > A "MUST" requirement for TLS would seem likely to disallow HTTP/3
> > (QUIC).
> 
> My understanding is that QUIC will also use TLS <https://datatracker.ietf.org/doc/draft-ietf-quic-tls/>.

The situation there is actually a bit complicated.  My understanding is
that the current plan is to use the TLS handshake messages, but transmit
them over the QUIC record layer, as a key-agreement mechanism.  The TLS
record-protection layer would not be used at all, and so I wouldn't really
characterize that as "using TLS".

> > Does "MUST confirm with the HTTP Authentication framework" preclude
> > other authentication schemes? The RFC 7235 framework has some warts,
> > and in many webapp settings there are plausible reasons to prefer
> > app-layer authentication schemes. I guess that JMAP as being an
> > API-driven model may have a more natural mapping to the 7235 framework,
> > and I do note the remarks in the shepherd writeup that an authentication
> > scheme was removed from a previous version of this document. So mostly
> > I'm just asking if we are intending to force the 7235 framework to be
> > the one and only authentication scheme for JMAP. (Who knows, maybe this
> > will drive adoption of new HTTP Authentication Schemes or prompt renewed
> > interest in their development?)
> 
> This has been raised by other reviewers too, and I think I'm going to just cut the line and the requirement. Implementors that want/need to use something other than 7235 will almost certainly do so regardless of what we write here.

That's fair.

> >  Clients MUST understand and be able to handle standard HTTP status
> >  codes appropriately.
> > 
> > (Is there a precise definition for "standard HTTP status codes"?)
> 
> No, I've cut this line as well. This was recommended text in an early draft of BCP 56bis <https://datatracker.ietf.org/doc/draft-ietf-httpbis-bcp56bis/>, but has since been cut.
> 
> >  An authenticated client can fetch the JMAP Session object with
> >  details about the data and capabilities the server can provide as
> > 
> > nit: maybe this is "a JAMP session object" with the indefinite article?
> 
> I used the definite article here because the client is authenticated, and therefore has access to one specific JMAP session object associated with those credentials.

Would that indicate "their"/"its", then?

> > Section 2
> > 
> >  o *username*: "String" The username associated with the given
> >  credentials.
> > 
> > This seems to implicitly require that all credentialed entities have
> > user accounts, denying the possibility for some shared-credential or
> > machine-credential models. Is this intended?
> 
> Hmm, fair point. I'll change the description to:
> 
> *The username associated with the given credentials, or the empty string if none.*
> 
> >  o *state*: "String" A string representing the state of this object
> >  on the server. If the value of any other property on the session
> >  object changes, this string will change. The current value is
> >  also returned on the API Response object (see section 3.3),
> >  allowing clients to quickly determine if the session information
> >  has changed (e.g. an account has been added or removed) and so
> >  they need to refetch the object.
> > 
> > As written this sounds like a serialized version of the full object
> > (without the 'state' field, of course, to avoid recursion), but I
> > suspect this is intended to be more like a generation number or hash or
> > serialized state, as a short lookup key. Greater clarity would be
> > welcome.
> 
> Yes, the intention is for this to be a short generation/modification number or hash. While this is preferred, it is not required to be a short string for functional operation. I hesitate to put further normative restrictions on it as this may make it harder for existing server implementations of other APIs with a similar concept but longer strings to implement a JMAP interface.
> 
> I'm happy to suggest this however; I'll change "A string representing…" to "A (preferably short) string representing…".

That looks good to me.

> > Section 3.2
> > 
> >  o *using*: "String[]" The set of capabilities the client wishes to
> >  use. The client MAY include capability identifiers even if the
> >  method calls it makes do not utilise those capabilities. The
> >  server advertises the set of specifications it supports in the
> >  JMAP Session object, as keys on the _capabilities_ property.
> > 
> > Are the "capability identifiers" the same as or different from the
> > "specifications [the server] supports"?
> 
> I have updated this to stick to "capability" as the terminology; a specification (document) may define more than one capability that can be supported independently, so the two are not interchangeable.
> 
> > Is the "client id" something that could also be called a "request ID"?
> > (I note that RFC 7252 calls a similar thing a "token" but has some text
> > wishing they called it a "request ID".)
> 
> Based on feedback from another reviewer I have renamed this "method call id". (A request in JMAP is a bundle of method calls so "request id" would not be appropriate.)

Sounds good.

> > The 'prefixed with a "#"' laguage seems like it has some potential for
> > confusion; is it better to write this like "the first character of the
> > creation ID MUST be octothorpe ('#')"?
> 
> The # is not part of the creation id. References to a creation id may be used in a context where an id is expected by prefixing the creation id with a #. Since this is not a valid character in an id, it is easy to determine that this must be a creation reference. I have added a reference at this point to the /set definition where this is explained in more detail.

That will help, though I'm not sure exactly how much.
Looking at this again, I think the biggest thing that confused me on my
first read is that I didn't internalize that creation ids and
ResultReferences are different things (since they both use the magic "#"
prefix), and I was trying to reconcile them into the same bucket.
So maybe when ResultReference is introduced there could be a note about how
"the ResultReference performs a similar role to that of the creation id, in
that it allows a chained method call to refer to information not available when
the request is generated.  However, they are different types and not
interchangable; the only commonality is the octothorpe used to indicate
them".  But, maybe I'm unusual for being confused about this and it's not
worth the extra text.

> > AFAICT having finished reading the doc, these can only be created by
> > explicit client action via a "create" array (or equivalent), where the
> > array keys are the "creation id"s since the client has to use some
> > handle before the server has assigned them. It should be possible to
> > add a very brief note to that effect here.
> 
> I have added cross-references to make this a bit clearer. There is now text here that says:
> 
> *As the server processes API requests, any time it successfully creates a new record it adds to this map the creation id (see the *create* argument to "/set" in section 5.3), with the server-assigned real id as the value.*
> 
> > Given the later usage in Section 3.3, I would consider splitting the
> > *Invocation* definition into a subsection.
> 
> OK
> 
> > Section 3.3
> > 
> > (editorial) If the second element of the Invocation tuple is literally
> > "arguments for the method", do we really have enough rope to make
> > response objects like this (unless we limit ourselves to conceptually
> > "in/out" arguments with no pure-output functionality)?
> 
> I think the document as a whole makes the syntax clear enough, but if you have a suggestion for better wording here I'm happy to consider it.

I don't have any wording suggestions, so this will probably be fine as-is.

> > Section 3.5
> > 
> > I expect that deployment will see differences of opinion as to which
> > checks fall under "syntactically valid", but it's unclear whether we
> > need to attempt to forestall such debate.
> 
> I would consider this to mean something that is valid JSON and when decoded matches the type signature of the Request object. I will clarify this in the spec, although I don't think small deviations in interpretation are likely to result in significant interoperability issues.

(I agree that it's unlikely for any interoperability issues to occur, so
I'm not very worried about this.)

> > Section 3.5.1
> > 
> >  o "urn:ietf:params:jmap:error:notRequest" The request parsed as JSON
> >  but did not match the structure of the Request object.
> > 
> > "the Request object" makes me think of "the thing the client stuck in
> > the field named "Request", which is probably not the intent. Perhaps
> > the key phrase is "type signature"?
> 
> Yes, that's clearer; I have changed this.
> 
> > Section 3.5.1.1
> > 
> > Since there is special handling for "urn:ietf:params:jmap:error:limit",
> > do we want an example to show that?
> 
> I'll add a second example.

Thanks.

> > Section 3.5.2
> > 
> >  With the exception of "serverPartialFail", the externally-visible
> >  state of the server MUST NOT have changed if an error is returned at
> >  the method level.
> > 
> > nit: You don't say where in the type signature "serverPartialFail" is
> > supposed to be in order to trigger the special handling.
> 
> Sorry, I don't understand this comment. serverPartialFail is a method-level error so it would be returned like any other method-level error:
> 
> [ "error", {
>   "type": "serverPartialFail"
> }, "call-id-1" ]

I think I misparsed what this was trying to say, namely that there is a
serverPartialFail element whose state is allowed to change if there is a
method-level error.  That is, of course, nonsense; the fact that the server
returns a serverPartialFail error code indicates that the "entire" server
state is indeterminate and the client must resync.  So any response to my
comment here would be along the lines of changing the text to "with the
exception of when the 'serverPartialFail' error is returned".

> >  Further general errors MAY be defined in future RFCs. Should a
> >  client receive an error type it does not understand, it MUST treat it
> >  the same as the "serverFail" type.
> > 
> > Does this imply that "serverPartialFail" is unique in its ability to
> > partially modify state?
> 
> General errors (that could be returned by any method) are expected to be defined rarely. A partial-fail where the server commits state changes but then fails part way is also expected to be very rare (because the client has to do a full resync to recover). 
> 
> If necessary, it's more likely a future specification could define an error that partially modifies state but is only returned in very specific circumstances and when the client has opted in to that capability, so it knows the client will be able to handle it.

Okay.

> > Section 3.6
> > 
> > (Same comment about '"prefixes with "#"' as above.)
> > 
> >  If an argument object contains the
> >  same argument name in normal and referenced form (e.g. "foo" and
> >  "#foo"), the method MUST return an "invalidArguments" error.
> > 
> > This "same argument name in normal and referenced form" applies even in
> > arbitrarily nested objects, right? It seems like this could potentially
> > be expensive and/or difficult to enforce.
> 
> Arguments are not nested. Only the top-level keys in the object are arguments. The value to an argument may be an object, but the keys in there are not arguments; they are properties of that object.

Thanks for clarifying; that makes sense now.

> > Section 5.1
> > 
> >  o *ids*: "Id[]|null" The ids of the Foo objects to return. If
> >  "null" then *all* records of the data type are returned, if this
> >  is supported for that data type.
> > 
> > Maybe note that the "maxObjectsInGet" capability limit might kick in
> > here for the "null" case?
> 
> OK, done.
> 
> >  o *properties*: "String[]|null" If supplied, only the properties
> >  listed in the array are returned for each Foo object. If "null",
> >  all properties of the object are returned. The id property of the
> >  object is *always* returned, even if not explicitly requested. If
> >  an invalid property is requested, the call MUST be rejected with
> >  an "invalidArguments" error.
> > 
> > Because we're constrained to a single type here, there's no way for only
> > some (but not all) of the objects to have any given property, right?
> 
> Correct.
> 
> > Section 5.3
> > 
> >  o *ifInState*: "String|null" This is a state string as returned by
> >  the _Foo/get_ method. If supplied, the string must match the
> > 
> > So, just to double-check, this applies to the state of all objects of
> > the type in question (for the account)?
> 
> Yes.
> 
> > It might be worth reiterating, since we frequently see this sort of check against the current state at
> > a per-object granularity, in other systems.
> 
> OK, will do.
> 
> > Do the "creation id"s in the "create" array include leading "#"?
> 
> No. A creation id is of type `Id` which does not allow this character. When a creation id is referenced from elsewhere, this is signified by prefixing the id with a #.
> 
> > Section 5.4
> > 
> >  o *destroyFromIfInState*: "String|null" This argument is passed on
> >  as the "ifInState" argument to the implicit _Foo/set_ call, if
> >  made at the end of this request.
> > 
> > This is "the Implicit _Foo/set_ call that effectuates the destruction of
> > the original", right?
> 
> Yes.

I think I was asking for a little more text here, basically to reiterate
that the Foo/set call is a deletion call.

> >  o *created*: "Id[Foo]|null" A map of the creation id to an object
> >  containing any properties of the copied Foo object that are set by
> >  the server (such as the _id_ in most object types). This argument
> >  is "null" if no Foo objects were successfully copied.
> > 
> > Maybe note that the id property will also likely differ from the one
> > that was passed in in the create array, since the ids in question are
> > from different accounts?
> 
> Sure, done.
> 
> > Section 5.6
> > 
> >  o *upToId*: "Id|null" The last (highest-index) id the client
> >  currently has cached from the query results. When there are a
> > 
> > Just to double-check: semantically this is an object identifier, but the
> > ordering we care about for the "up to" part is the ordering in the query
> > results?
> 
> Yes.
> 
> >  o *removed*: "Id[]" The _id_ for every foo that was in the query
> >  results in the old state and is not in the results in the new
> >  state. If the server cannot calculate this exactly, the server
> >  MAY return extra foos in addition that may have been in the old
> >  results but are not in the new results. If the sort and filter
> >  are both only on immutable properties and an _upToId_ is supplied
> >  and exists in the results, any ids that were removed but have a
> >  higher index than _upToId_ SHOULD be omitted. If the _filter_ or
> >  _sort_ includes a mutable property, the server MUST include all
> >  foos in the current results for which this property MAY have
> >  changed.
> > 
> > I'm having a hard time understanding this "MAY have changed" text
> > (which, for one, shouldn't be using 2119 language). Taking note that
> > this is the "removed" list, this text seems to be saying that I include
> > in the "removed" list any object that *is* in the current query results,
> > but which may have had a property change since the previous state. So
> > we end up having to do a "remove + add" for this sort of property
> > change, is that right?
> 
> Yes. If the item may have moved in the list, the client needs to remove and then re-add it at its current index to ensure its query results cache is correct. I will add a sentence to explain this.

Thanks!

> (I have changed the "MAY" to "may"; this was incorrect use of 2119 language, you're right.)
> 
> > We may need more detail on the "splices in" operation, with respect to
> > the indices in the "added" array reflecting the final state, and the
> > local cached indices needing to be updated on the fly during the
> > splicing operation in order to get things inserted in the proper places.
> 
> Right. I have tried to make this a bit clearer; it now reads:
> 
> *The result of this is that if the client has a cached sparse array of foo ids corresponding to the results in the old state:**
> *
> *
> *
> *    fooIds = [ "id1", "id2", null, null, "id3", "id4", null, null, null ]**
> *
> *
> *
> *then if it ***splices out*** all ids in the removed array that it has in its cached results:**
> *
> *
> *
> *    removed = [ "id2", "id31", ... ];
>     fooIds => [ "id1", null, null, "id3", "id4", null, null, null ]**
> *
> *
> *
> *and ***splices in*** (one-by-one in order, starting with the lowest index) all of the ids in the added array:**
> *
> *
> *
> *    added = [{ id: "id5", index: 0, ... }];
>     fooIds => [ "id5", "id1", null, null, "id3", "id4", null, null, null ]**
> *
> *
> *
> *and ***truncates*** or ***extends*** to the new total length, then the results will now be in the new state.**
> *
> *
> *
> *Note: splicing in adds the item at the given index, incrementing the index of all items previously at that or a higher index. Splicing out is the inverse, removing the item and decrementing the index of every item after it in the array.*

Thanks, I think that will help make things clear.

> > Section 5.8
> > 
> >  2. It must resolve back-references to previous method results that
> >  were processed on a different server. This is a relatively
> >  simple syntactic substitution, described in section 3.6.
> > 
> > Relatively simple, yes, but does require properly parsing the JSON
> > (right?).
> 
> Yes.

The security reviewer part of me wants the text to point out that this
puts the proxy's JSON parser squarely in the attack surface, so if it is
poorly implemented that's a risk for the system as a whole.  I want to say
that you could do (1) without a full JSON parser, so to some extent it is
just the backreferences that make this a full-fledged requirement on the
proxy.

> > Section 6.3
> > 
> > Why does Blob/copy use 'blobIds' instead of 'create' like generic /copy?
> 
> Because it's a different format (just an array of ids rather than a map of creation id to object). We considered it clearer if arguments with the same name were of the same type.

Good point; I think I missed that the first time around.

> >  o *fromAccountId*: "Id" The id of the account emails were copied
> >  from.
> > 
> >  o *accountId*: "Id" The id of the account emails were copied to.
> > 
> > I assme the "emails" are copy/paste bugs.
> 
> Yes, good spot. I've fixed this.
> 
> > Section 7.2
> > 
> >  o *deviceClientId*: "String" (immutable) An id that uniquely
> >  identifies the client + device it is running on. The purpose of
> >  this is to allow clients to identify which PushSubscription
> >  objects they created even if they lose their local state, so they
> >  can revoke or update them. This string MUST be different on
> >  different devices, and be different from other vendors. It SHOULD
> > 
> > What's the first vendor that's the basis for comparison?
> 
> Sorry, I'm not quite sure what you're asking here.

I think my fundamental confusion here is about where the device ID is
coming from -- is this supposed to be something like the serial number or
IMEI number from my phone, that is a fixed and permanently bound to a
specific device (and not changeable)?  I think we delved into related
topics for Alissa's Discuss, but not quite into this aspect of things.  If
I start talking about "vendors", is there some implication that the
identity of the current device's vendor is input to the
deviceClientId-generation process (whether explicitly or implicitly via a
manufacturer prefix in some other identifier)?  So the literal answer to my
question here is something like the "first vendor" is the manufacturer of
the device being identified, or the author of the app generating this
specific deviceClientId.

Even in the current text (-16), I wonder if "a device id" is quite enough,
as opposed to "a unique identifier associated with the device where the
JMAP client is running" or similar.

> >  be easy to re-generate, not depend on persisted state. A secure
> >  hash that includes both a device id and vendor id is one way this
> >  could be achieved.
> > 
> > Easy to re-generate by whom?
> 
> The client.
> 
> > How does the client get the deviceClientId value the first time?
> 
> It generates it from its vendor app id and a device id it gets from the OS.

E.g., we don't say "gets from the OS" anywhere in the -16.

> e.g. `sha256( "com.example.mail" . $device-id )`
> 
> (where I own example.com and `$device-id` is from the OS).
> 
> >  The POST request MUST have a content type of "application/json" and
> >  contain the UTF-8 JSON encoded object as the body. The request MUST
> > 
> > (editorial) Are we back to what the JMAP server sends to the notification URL?
> 
> Yes.
> 
> >  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.
> > 
> > How is the JMAP server expected to learn about credential expiry or
> > revocation?
> 
> That's implementation dependent.

I think when  I first read this I was confused about whether this was
credentials for [JMAP client to JMAP server] or [JMAP server to push
service].  But the PushSubscription object is a pure JMAP thing, so it's
the first case and the JMAP server does its own credential management, as
you note here.

Which is a long way of saying "I agree, and the current text is fine" :)

> >  When these credentials have their own expiry (i.e. it is a session
> >  with a timeout), the server SHOULD NOT set or bound the expiry time
> > 
> > (editorial) A session where?
> 
> If your authentication has the concept of a session that expires after a set amount of time, or after a set amount of idle time (rather than, say, just a BASIC username/password with no expiry time).

(seems like my same confusion as above, nothing to see here)

> > Section 7.2.1
> > 
> > I would suggest a section reference to "follows the common /get
> > semantics from Section 5.1, with the exceptions that [...]" to more
> > clearly incorporate by reference the existing text.
> 
> OK, I've added a reference.
> 
> > What nature of authorization checking is done for these get requests?
> 
> These are standard method calls that are sent like any other, and so authenticated like any other at the HTTP request level.

(The new text in the -15 addresses what I was getting at; thanks)

> > Section 7.2.3
> > 
> > Given that all of these times are going to be in the past for all
> > readers, do we want to say something about what time the client is
> > performing these operations at?
> 
> Yes, that's reasonable. I have done this (and fixed up the times in the examples to make sense relative to one another).

Thank you!

> > Section 8.1
> > 
> > Again, this (duplicated!) MUST for TLS prevents future HTTP/3 QUIC
> > usage.
> > 
> > Also, please reference RFC 7525.
> 
> I have added a reference to RFC 7525 and removed the duplication.
> 
> > Section 8.2
> > 
> > Please recommend against Basic. Also, the concept of what a "user's regular password" is seems a bit
> > underspecified.
> 
> OK. I've modified this to read:
> 
> *Use of the Basic authentication scheme is NOT RECOMMENDED. Services that choose to use this are strongly recommended to require generation of a unique "app password" via some external mechanism for each client they wish to connect.*
> 
> (I've just cut "user's regular password" since it's redundant.)

Beautiful!  (Maybe s/use this/use it/, but I'm sure the RFC Editor is on
top of it.)

> > Section 8.3
> > 
> > DNS SRV-based autodiscovery seems the only type of autodiscovery
> > available that is susceptible to the attack described here; you should
> > probably just state that explicitly.
> 
> OK, will do.
> 
> > Section 8.4
> > 
> > While true, 8529's security considerations are pretty sparse; we could
> > say more here about not overscanning, limiting string length, being
> > strict about tokenization, etc.
> 
> Do you have any suggested text?

As for any serialization format, parsers need to thoroughly check the
syntax of the supplied data.  JSON uses opening and closing tags for
several types and structures, and it is possible that the end of supplied
data will be reached when scanning for a matching closing tag; this is an
error condition and implementations need to stop scanning at the end of the
supplied data.

JSON also uses a string encoding with some escape sequences to encode
special characters within a string.  Care is needed when processing these
escape sequences to ensure that an escape sequence is fully formed before
the special processing is triggered, with special care taken when the
escape sequences appear adjacent to other (non-escaped) special characters
or the end of data (as in the previous paragraph).

If parsing JSON into a non-textual structured data format, implementations
may need to allocate storage to hold JSON string elements.  Since JSON
does not use explicit string lengths, the risk of denial of service due to
resource exhaustion is small, but implementations may still wish to place
limits on the size of allocations they are willing to make in any given
context, to avoid untrusted data causing excessive memory allocation.

> > Section 8.7
> > 
> >  and JMAP server the client MUST specify encryption keys when
> >  establishing the PushSubscription and ignore any push notification
> >  received that is not encrypted and signed with those keys.
> > 
> > There's no signing in RFC 8291; there is, however, a separate
> > authentication secret.
> 
> Right, I will update this text.
> 
> > Section 8.8
> > 
> > I would be surprised if the propsects for traffic analysis were limited
> > to just push. Even regular accesses may still be susceptible to traffic
> > analysis.
> 
> No doubt something could be gleaned. I will make this slightly more generic, although push notifications still have the clearer information leak.

Sure (the new text looks fine).

> > Section 9.4.3
> > 
> > You probably should document that recourse for non-response after 30
> > days is to request action from the IESG.
> 
> OK.

Thanks for all the updates!

I've changed my position to "No Objection" in the datatracker.

-Benjamin

P.S. I appreciate the separate "description" column for the error registry
in the -16, but the formatting of the current table is basically
unreadable.  I had to go find
https://github.com/jmapio/jmap/commit/3f4b7f83c99596a553bd81324c092516a9babbcf#diff-3004124a0560c91bfc79eb996bd81433
to get something useful to look at.

---

From: "Neil Jenkins" <[email-masked]>
Date: Tue, 12 Mar 2019 22:14:53 -0400

> Would that indicate "their"/"its", then?

I'll make it *the *user's* JMAP session object.*

> them". But, maybe I'm unusual for being confused about this and it's not

I don't think it hurts to clarify; I'll add this in.

> that the Foo/set call is a deletion call.

OK, I'll clarify this.

> JMAP client is running" or similar.

Yes, this should be clearer. I have rewritten this to be:

*This string MUST be different on different devices, and be different from apps from other vendors. It SHOULD be easy to re-generate, not depend on persisted state. **It is RECOMMENDED to use a secure hash of a string that contains:*
*
*
 1. *A unique identifier associated with the device where the JMAP client is running, normally supplied by the device's operating system.
*
 2. *A custom vendor/app id, including a domain controlled by the vendor of the JMAP client.
*
*
*
*To protect the privacy of the user, the deviceClientId id MUST NOT contain an unobfuscated device id.*

> context, to avoid untrusted data causing excessive memory allocation.

Great, thanks, I'll add that in.

> to get something useful to look at.

Yes, I'm hopeful the RFC editors may help with this; I'm not sure what the best approach is. (Just as an FYI, I also publish the latest version of the spec in a more readable HTML format at https://jmap.io/spec-core.html and https://jmap.io/spec-mail.html.)

I'll post a -17 when submissions reopen and hopefully this can progress through to join the mail spec in the final editing stage.

Cheers,
Neil.

---

From: Benjamin Kaduk <[email-masked]>
Date: Tue, 12 Mar 2019 21:26:36 -0500

> *To protect the privacy of the user, the deviceClientId id MUST NOT contain an unobfuscated device id.*

Thanks, this is a big help.

> I'll post a -17 when submissions reopen and hopefully this can progress through to join the mail spec in the final editing stage.

Alexey should be able to approve a manual posting by the secretariat during
the blackout; the usual reason for denying draft submission (so that
everyone can read drafts in preparation for meeting sessions) doesn't
really apply for drafts in this stage.

Thanks again for all the updates!

-Benjamin