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

["Neil Jenkins" <[e-mail]>]

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.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

OK.

Cheers,
Neil.