[alto] Benjamin Kaduk's No Objection on draft-ietf-alto-incr-update-sse-20: (with COMMENT)

Benjamin Kaduk via Datatracker <noreply@ietf.org> Thu, 12 March 2020 00:02 UTC

Return-Path: <noreply@ietf.org>
X-Original-To: alto@ietf.org
Delivered-To: alto@ietfa.amsl.com
Received: from ietfa.amsl.com (localhost [IPv6:::1]) by ietfa.amsl.com (Postfix) with ESMTP id 9BBE83A080E; Wed, 11 Mar 2020 17:02:45 -0700 (PDT)
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
From: Benjamin Kaduk via Datatracker <noreply@ietf.org>
To: "The IESG" <iesg@ietf.org>
Cc: draft-ietf-alto-incr-update-sse@ietf.org, alto-chairs@ietf.org, alto@ietf.org, Vijay Gurbani <vijay.gurbani@gmail.com>, vkg@bell-labs.com
X-Test-IDTracker: no
X-IETF-IDTracker: 6.120.0
Auto-Submitted: auto-generated
Precedence: bulk
Reply-To: Benjamin Kaduk <kaduk@mit.edu>
Message-ID: <158397136560.19639.8260312978650035895@ietfa.amsl.com>
Date: Wed, 11 Mar 2020 17:02:45 -0700
Archived-At: <https://mailarchive.ietf.org/arch/msg/alto/44nyS3d-X_Z5RpeghRERFZt5R3A>
Subject: [alto] Benjamin Kaduk's No Objection on draft-ietf-alto-incr-update-sse-20: (with COMMENT)
X-BeenThere: alto@ietf.org
X-Mailman-Version: 2.1.29
List-Id: "Application-Layer Traffic Optimization \(alto\) WG mailing list" <alto.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/alto>, <mailto:alto-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/alto/>
List-Post: <mailto:alto@ietf.org>
List-Help: <mailto:alto-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/alto>, <mailto:alto-request@ietf.org?subject=subscribe>
X-List-Received-Date: Thu, 12 Mar 2020 00:02:46 -0000

Benjamin Kaduk has entered the following ballot position for
draft-ietf-alto-incr-update-sse-20: No Objection

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-alto-incr-update-sse/



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

Thanks for your response to the genart reviewer; I'm happy to see that
change staged for the next version.

It's a bit interesting to see that we present JSON merge patch and JSON
patch in the reverse order in which they were defined (RFC 7396/7386 vs.
6902).

It's very surprising to me that we replicate the algorithm for JSON
merge patch and SSE but just refer to RFC 6902 for the JSON patch
algorithm.

What happens if either party closes the update stream without the proper
clean-up message(s)?

Section 1

   considerations by both ALTO servers and clients; Section 13 discusses
   a design feature that is not supported; Section 10 discusses security
   issues; The last two sections review the requirements for future ALTO
   services to use SSE and IANA considerations, respectively.

I think this last remark perhaps should not have survived a section
reordering to put the not-supported design feature (section 13) last.

Section 2

   Stream Control Server: An stream control server providing the stream
   control service.

Perhaps we could avoid defining a "stream control server" as a "stream
control server" that does some things.

   Control Update Message: A control update message is a message in an
   update stream for the update stream server to notify the ALTO client
   of related control information of the update stream.  The first
   message of an update stream is a control update message and provides
   the URI using which the ALTO client can send stream control requests
   to the stream control server.  Additional control update messages in
   an update stream allow the update stream server to notify the ALTO
   client of status changes (e.g., the server will no longer send
   updates for an information resource).

The way this is written makes me want to double-check that I have the
flow/terminology right: there's an update stream of update messages from
server to client, that can include data updates but also can include
control update messages.  Control update messages give the client
information about the stream control service, potentially distinct from
the (regular) update service, and the client sends requests on the
stream control service, that are expected to result in changes to what
the server sends through the (regular) update service?  Would it ever
happen that a server sends a message on the control service connection, or
that a control update message is present on the control service?

Section 3.1

Is it worth noting an explanation of why the HTTP PATCH method is
unsuitable for ALTO SSE (presumably because it goes client-to-server and
we require the opposite)?

Section 3.1.1

   one JSON value into another.  Specifically, JSON merge patch treats
   the two JSON values as trees of nested JSON objects (dictionaries of

I suggest clarifying "the two JSON values" (e.g., "the old and new JSON
values").

Section 3.3

   Despite the two features of HTTP/2, this design chooses an HTTP/1.x
   based design for the simplicity of HTTP/1.x.  An HTTP/2 based design

I suggest to say "HTTP/1.x-compatible design" rather than "HTTP/1.x
based design" -- the SSE API is part of HTML5, and as such is usable
with both those versions of HTTP (and future versions).

Section 4.1

   An update stream can provide updates to both GET-mode resources, such
   as ALTO network and cost maps, and POST-mode resources, such as ALTO
   endpoint property service.  Also, to avoid creating too many update
   streams, this design allows an ALTO client to use one update stream
   to receive updates to multiple requests.  In particular, the client
   may request to receive updates for the same resource but with
   different parameters for a POST-mode resource.  The updates for each

This implies by omission that update-stream consolidation is not
possible for GET-mode resources or different POST-mode resources; I'd
suggest to reword to clarify that this is in addition to being able to
consolidate updates for multiple resources into a single stream.

   The objective of an update stream is to continuously update the ALTO
   client on data value changes given by the client's requests.  This

nit: "data value changes given by the client's requests" almost reads
like it's the client's requests that are producing the data value
changes, rather than the client's requests indicating which data values
to report changes to.

Section 4.2

   The ALTO client can then use the URI to ask the update stream server
   to (1) send data update messages for additional resources, (2) stop

nit(?): if the client is talking to the stream control server, is it
really asking the *update stream* server to do anything?

Section 5.3

   control-uri:  the URI providing stream control for this update stream
        (see Section 7).  The server MUST send a control update message
        with an URI as the first event in an update stream.  If the URI

It seems like it's worth mentioning whether/when the server can send a
control-uri at other times.


(I agree with Alexey about describing the 'description' as
"human-readable", which it sounds like is going to happen.)

Section 6.3

   If this update stream can provide data update messages with
   incremental changes for a resource, the "incremental-change-media-
   types" field has an entry for that resource-id, and the value is the
   media-type of the incremental change.  Normally this will be

Should this be "media-type or types", since a comma-separated list is
allowed?

   When choosing the media-type to encode incremental changes for a
   resource, the update stream server SHOULD consider the limitations of
   the encoding.  For example, when a JSON merge patch specifies that
   the value of a field is null, its semantics is that the field is
   removed from the target, and hence the field is no longer defined
   (i.e., undefined); see the MergePatch algorithm in Section 3.1.1 on
   how null value is processed.  This, however, may not be the intended
   result for the resource, when null and undefined have different
   semantics for the resource.  In such a case, the update stream server
   SHOULD choose JSON patch over JSON merge patch.

I don't see how this is SHOULD (vs. MUST).  Won't things break if you
don't?

Section 6.5

   add: specifies the resources (and the parameters for the resources)
        for which the ALTO client wants updates.  We say that the add-
        request creates a substream.  The ALTO client MUST assign a
        unique substream-id (Section 5.2) for each entry, and uses those
        substream-ids as the keys in the "add" field.

Unique within what scope?  (Globally unique could prove challenging...)

   incremental-changes:  the ALTO client specifies whether it is willing
        to receive incremental changes from the update stream server for
        this substream.  If the "incremental-changes" field is "true",
        the update stream server MAY send incremental changes for this
        substream (assuming the update stream server supports
        incremental changes for that resource).  In this case, the

Since MAY is not an obligation, I think the parenthetical is not
strictly speaking needed.

        changes" to "false".  An alternative design of incremental-
        changes control is a more fine-grained control, by allowing a
        client to select a subset of incremental methods from the set
        announced in the server's capabilities.  But this adds
        complexity to the server, which is more likely to be the
        bottleneck.  Note that the ALTO client cannot suppress full

I suggest rewording to be more clear that this alternative design is not
part of this specification (and in fact was rejected from
consideration?).

        replacement.  When the ALTO client sets "incremental-changes" to
        "false", the update stream server MUST send a full replacement
        instead of an incremental change to the ALTO client.  The update
        stream server MAY wait until more changes are available, and
        send a single full replacement with those changes.  Thus an ALTO
        client which declines to accept incremental changes may not get
        updates as quickly as an ALTO client which does.

Do we have any requirement that incremental changes be sent "as quickly
as possible" or can they be delayed as well?

Section 6.7.1

   o  The first event MUST be a control update message with the URI of
      the update stream control service Section 7 for this update
      stream.

I thought it was allowed to send a null control-uri in this message.

   o  As soon as possible after the ALTO client initiates the
      connection, the update stream server MUST send a full replacement
      for each resource-id requested with a version tag.  In this case
      the update stream server MAY omit the initial full replacement for
      that resource, if the "tag" field the ALTO client provided for
      that resource-id matches the tag of the update stream's current

This second sentence contradicts the MUST in the first one; please
rephrase to avoid the internal inconsistency.  (Also, please clarify
what "this case" is, since we haven't mentioned a specific one by that
point in the text.)

   o  If this update stream provides update for resource-ids and R0 and
      R1, and if R1 depends on R0, then the update stream server MUST

nit: s/update/updates/

Section 6.7.2

   A update stream server MAY offer several different update stream
   resources that provide updates to the same underlying resource (that
   is, a resource-id may appear in the "uses" field of more than one
   update stream resource).  In this case, those update stream resources
   MUST return the same update data.

The phrase "update data" is used only once in this document, and in
light of the previous discussion about sending the "same updates" but
potentially different patch events, it seems that greater specificity is
called for in what is intended here.

Section 6.7.3

   JSON Patch and Merge Patch provide the incremental encoding benefit
   but can be applied to only a single JSON object.  If an update stream
   service (1) supports a resource providing a multipart media type and
   (2) specifies an incremental media type for the resource in its
   capabilities, the server MUST (1) use substream-id.content-id in its
   `event` field, (2) include the content-id in the multipart message,
   and (3) the content identified by the content-id must be a single
   JSON object.

Which message is "the multipart message"?  Is it on the update stream?

Section 7

   An stream control service allows an ALTO client to remove resources

nit: s/An/A/

Section 7.1

I agree with Alexey to reference RFC 3986 here.

   It is expected that the update stream server will assign a unique
   stream id to each update stream instance and will embed that id in
   the associated stream control URI.  However, the exact mechanism is
   left to the update stream server.  ALTO clients MUST NOT attempt to
   deduce a stream id from the control URI.

Is this stream ID used for anything outside of the update stream
server's internal processing?

   To prevent an attacker from forging a stream control URI and sending
   bogus requests to disrupt other update streams, stream control URIs
   SHOULD contain sufficient random redundancy to make it difficult to
   guess valid URIs.

I think this needs to be a MUST.
It is probably also worth referencing
https://www.w3.org/TR/capability-urls/ .

Section 7.5

   An stream control service accepts the same input media type and input

nit: s/An/A/

Section 7.6

   o  If any substream-id in the "remove" field was not added in a prior
      request, the error code of the error message MUST be
      E_INVALID_FIELD_VALUE; the "field" field SHOULD be "remove" and
      the "value" field SHOULD be the array of the invalid substream-

nit: s/the array/an array/; we are defining this array for the first
time.

      id in the same request.  However, it is legal to remove a
      substream-id twice.

I suggest making this requirement to track
previously-used-but-now-closed substream-ids more explicit.  (It also
comes uo for the next bullet point.)

   o  If any substream-id in the "add" field has been used before in
      this stream, the error code of the error message MUST be
      E_INVALID_FIELD_VALUE, the "field" field SHOULD be "add" and the
      "value" field SHOULD be the array of invalid substream-ids.

nit: s/the array of/an array of the/ (same reasoning as before)

Section 8

[Just noting that I, as well, did not perform a detailed check of the
examples.]

Section 8.4

   If the ALTO client needs the "bandwidth" property for additional
   endpoints, the ALTO client can send an "add" request on the stream
   control URI:

The "add" request also includes a priv:ietf-load request that we don't
mention here.

Section 9.3

   An update stream server can avoid such issues by offering update
   streams only for filtered cost maps which do not allow constraint
   tests.

Maybe say something about "having to handle such complicated behavior"
instead of "such issues" (which are not particularly clearly indicated)?

Section 10

I greatly appreciate the way these security considerations are framed,
fiting into the context of the base protocol and then with the
additional risks discussed separately; thank you!


Is there anything interesting to say about this mechanism making it
easier to use an updated resource with a stale version of a resource
used by that first resource?

With the possibility for the update server and stream control server to
be different entities, there is a risk of information skew or
communications breakdown between them, as well as invalid or spoofed
messages claiming to be on that private communications path.  It would
be worth a brief discussion of what sort of problems can arise from that
separation of roles.

Just to check: SSE guarantees that we get events in order, so there are
no reordering considerations within a stream.  IIRC we did have some
discussion about synchronization of the same updates appearing on
multiple streams and of dependent updates within a single stream, which
should take care of the main points in this space.

I was also not able to read very closely the SSE spec, but it sounded
like perhaps the client-side implementation was allowed (or required?)
to introduce linefeed characters into the data buffer for the event in
question, e.g., to reflect the line breaks in the actual transferred
encoding.  Do we need to say anything about the sender behavior to avoid
putting line breaks in places that would have semantic consequences for
the ALTO updates?

Section 10.1

   To avoid these attacks on the update stream server, the server SHOULD
   choose to limit the number of active streams and reject new requests
   when that threshold is reached.  An update stream server SHOULD also

This transfers the attack from the update server to the honst clients
trying to use it (as noted in the following paragraph); it may be
possible to use somewhat more clever logic involving IP reputation,
rate-limiting, and compartmentalizing the overall threshold into smaller
thresholds that apply to subsets of potential clients.  (Client
authentication is also a potential mitigation for some of these attacks,
as is mentioned in the following paragraph.)  None of this is
necessarily incompatible with the current "SHOULD choose to limit"
guideline, of course, though we may want to give a bit more sense of the
nuances involved.

Section 10.2

   Also, under overloading, the client may no longer be able to detect
   whether an information is still fresh or has become stale.  In such a
   case, the client should be careful in how it uses the information to
   avoid stability or efficiency issues.

Is it possible to get into a state where stale ALTO data causes a client
to behave in a way worse for the network than not using any ALTO data at
all?

Section 10.3

I thought https was a MUST for ALTO, which would provide the needed
confidentiality even in the absence of mutual authentication.

Section 11

   At the low level encoding level, new line in SSE has its own
   semantics.  Hence, this design requires that resource encoding does
   not include new lines that can confuse with SSE encoding.  In
   particular, the data update message MUST NOT include "event: " or
   "data: " at a new line as part of data message.

It may also be worth forbidding these not at a new line, since a modular
server implementation might apply an arbitrary line-breaking policy.

Section 13

I suggest adding a little more introduction at the start to give the
background that the following description is a protocol design and
considerations that could have been used, but was rejected.