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

[Benjamin Kaduk <[e-mail]>]

Benjamin Kaduk has entered the following ballot position for
draft-ietf-jmap-core-14: Discuss

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)


Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.


The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-jmap-core/



----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

There's a lot here, and I was not reading in the best of environments,
so it's quite possible that I am just confused or missed something while
reading.  On the whole, this document is well-written and gives me a
good picture of how things work.  That said, there are still some places
where it looks like we may need to have some discussions...

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

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

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

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

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?

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.

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

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.


----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

I also have a big pile of mostly editorial comments.

Section 1.1

"Type signatures" sounds a lot like a "JSON Schema".  But we can work
with what we have, no doubt :)

   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?

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

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?

(I don't see how prefixing every ID with an alphabetical character
avoids the "NIL" case, unless 'N' is disallowed from being that
character.)

Section 1.3

My math degree obliges me to note that, per trichotomy, zero is not a
positive integer.

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.

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.

Section 1.7

A "MUST" requirement for TLS would seem likely to disallow HTTP/3
(QUIC).

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

   Clients MUST understand and be able to handle standard HTTP status
   codes appropriately.

(Is there a precise definition for "standard HTTP status codes"?)

   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?

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?

   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.
(Same comment for all other "state" fields in the document.)
In particular, the /changes endpoints seem to place fairly strict bounds
on the generation/tracking of *state*.

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

Was there much discussion of short Strings as method names vs. URIs?

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

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 ('#')"?
I'm also still a bit unclear at this point about how the client tells
the server which entry in the createdIds map to update when which action
completes, but hopefully there is more later.
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.

Given the later usage in Section 3.3, I would consider splitting the
*Invocation* definition into a subsection.

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

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.

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

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?

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.

   "invalidArguments": One of the arguments is of the wrong type or
   otherwise invalid, or a required argument is missing.  A
   "description" property MAY be present to help debug with an
   explanation of what the problem was.  This is a non-localised string,
   and is not intended to be shown directly to end users.

It seems almost certain that it *will* be shown directly to end users,
intent or not.  But disavowing localization is probably sufficient for
now.
(et seq.)

   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?

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.

Section 4

I'm trying to consider whether there is any sort of canonicalization or
un-escaping behavior that a server might perform on method inputs so
that the response would be almost but not exactly the same as the
request.  (Such behavior could perhaps be used for fingerprinting an
implementation.)  I can't think of any, though.

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?

   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?

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

Do the "creation id"s in the "create" array include leading "#"?

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?

   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?

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?

   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?

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.

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

Section 6.3

Why does Blob/copy use 'blobIds' instead of 'create' like generic /copy?

   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.

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?

      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?
How does the client get the deviceClientId value the first time?

   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?

   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?

   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?

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.

What nature of authorization checking is done for these get requests?

Section 7.2.2

(Same comment about invariants.)

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?

Section 8.1

Again, this (duplicated!) MUST for TLS prevents future  HTTP/3 QUIC
usage.

Also, please reference RFC 7525.

Section 8.2

Please recommend against Basic.  SCRAM is probably a tolerable default
suggestion; some of the other options that have nicer-looking crypto
properties are also not widely deployed, IIUC.

Also, the concept of what a "user's regular password" is seems a bit
underspecified.

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.

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.

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.

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.

Section 9.4.3

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