Thanks for the review!
On Thu, Apr 26, 2012 at 2:40 PM, Ted Hardie <[email protected]> wrote:
> Please resolve these comments along with any other Last Call
> comments you may receive. Please wait for direction from your
> document shepherd or AD before posting a new version of the draft.
>
> Document: draft-ietf-alto-protocol-11.txt
> Title: ALTO Protocol
> Reviewer: Ted Hardie
> Review Date: April 26, 2012
>
> Summary: Despite the length of this review, the document is
> fundamentally ready for publication on the standards track. There
> are sections which need clarification or re-consideration, but
> they are not fundamental to the protocol's operation.
>
> Major:
>
> Section 5.1 needs clarification. While it clearly introduces the
> types for the cost type and cost mode parameters, it buries the type
> for the cost itself within the description of the ordinal cost mode:
>
> 5.1.2.2. Cost Mode: ordinal
>
>
> This Cost Mode is indicated by the string 'ordinal'. This mode
> indicates that the costs values to a set of Destination Network
> Locations from a particular Source Network Location are a ranking,
> with lower values indicating a higher preference. The values are
> non-negative integers.
>
> This makes it unclear whether Costs of the numerical Cost Type are
> similarly restricted to non-negative integers. Based on other
> descriptions of ordinal comparison (e.g. q values in content
> negotiation), I also presume that the clients are not expected to
> infer anything about the weights based on the integers chosen for the
> ordinal type. That is: 3 PIDS with costs of 2, 4, 6 should be treated
> the same if the assigned values are 3, 17, 18, because the order is
> the same. Additional text to highlight this (or provide the
> appropriate treatment) would be useful. The section would also
> benefit from an example of a numerical operation other than
> normalization.
Ack - this can certainly be clarified. Your understanding for ordinal
costs is correct. Numerical costs can be any non-negative
floating-point number (representable in JSON, of course).
> In Section 7.2, the document describes the request/response traffic as
> based on "standard HTTP"; the security considerations sections appears
> to recommend TLS for integrity protection in 11.3 (this property is
> not mentioned in 7.2.5, which lists TLS as MAY for authentication and
> encryption). This should be reconciled; if 11.3 is not recommending
> TLS or any object-based security, more detail is probably needed on
> the consequences of an attacker controlling this data.
Agree. Integrity protection and authentication are probably the
crucial ones, with encryption valuable in some deployment scenarios.
We can make these consistent.
Detail on what happens when an attacker controls the data would be
good. I thought we had that text in one of the documents, but the
reference escapes me at the moment. We'll at least add an explicit
pointer to where this is discussed in more detail.
>
> Minor:
>
> General comment: there is a great deal of material in this document
> that is either tutorial in nature or which defends a particular set of
> design choices (e.g. section 6.1). While this material is not
> inherently bad, it is interleaved into the specification in ways that
> make it difficult to extract the concrete protocol specification. If
> there were sections that the WG felt could be moved to an appendix
> summarizing the benefits of the ALTO approach and its choices, the
> spec might get significantly tighter and easier to read.
That sounds reasonable. I'll hang on for others in the WG to respond
if there are other thoughts for which text should be moved and where.
At the very least, moving 6.1 earlier on in the document sounds reasonable.
On the flip side, another possibility would be to add a forward
pointer from the introduction to the core spec (for those who only
care about the spec pieces).
>
> In Section 4.1, the concept of a PID as an identifier is introduced,
> but without a pointer to the definition of the identifier type. There
> is a forward reference to section 5, but this describes cost maps, not
> the PID identifier itself. Reading (or grepping) forward provides a
> pointer to PIDName, but without a succinct relationship defined. I
> suggest adapting the description in 7.7.1.1.5 for this purpose:
>
> "A PID is a US-ASCII string of type PIDName (see section 7.5.1) and
> its associated set of endpoint addresses."
>
> I believe this could be inserted between the first two sentences of
> the first paragraph in section 4.1
Sounds good to me.
>
> In Section 4.2.1, the document says:
>
> A RECOMMENDED way to satisfy this property is to define a PID that
> contains the 0.0.0.0/0 prefix for IPv4 or ::/0 (for IPv6).
>
> This assumes that the cost map provided includes all IP addresses.
> While this may be true for some use cases, there are other use cases
> where it may not be true, and the text here should probably be
> modified to handle those cases. Something like
>
> "A RECOMMENDED way to satisfy this property is to define a PID with
> the shortest enclosing prefix of the addresses provided in the map.
> For a map with full IPv4 reachability, this would mean including the
> 0.0.0.0/ prefix in a PID; for full IPv6 reachability, this would be
> the ::/0 prefix"
>
> might suit the case.
Sounds good.
>
> In section 7.2.2, the document says:
>
> "Note that it is possible for an ALTO Server to employ caching for the
> response to a POST request. This can be accomplished by returning an
> HTTP 303 status code ("See Other") indicating to the ALTO Client that
> the resulting Cost Map is available via a GET request to an alternate
> URL (which may be cached)."
>
> The phrase "employ caching" is ambiguous here, as the results of the
> initial POST are not cacheable even in the case of a 303; only
> the results of the later GET request are cachable. Since cachability
> is a major reason cited for the re-use of HTTP, some additional text
> on the use cache control directives ("public" and "Max-age" seem
> particularly important in this context) might also be useful.
(discussed on a different thread where we seemed to reach a conclusion)
>
> In 7.4.2, the document describes the error resource, and notes this:
>
> reason A (free-form) human-readable explanation of the particular
> error
>
> If I understand JASONString correctly, no language tag is supplied,
> so human readability will be highly variable, Fixing that, rather
> than relying on a local mapping of the underlying code, is hard
> work. Dropping this optional aspect of the error resource may
> be something for the WG to consider; this pushes localization
> of the error code description to the endpoint.
That seems reasonable. The intent was to help with debugging (and not
display to a user), so I'd either be okay with removing it or keeping
it and not worrying about the language. Is there standard guidance to
apply here?
> The endpoint property resource seems to be specified in too many
> sections for easy comprehension. Section 7.7.3.1 describes it, but
> points to 7.7.3.1.4 (the related capabilities) which in turn points to
> 3.1.3, which says:
>
> This service allows ALTO Clients to look up properties for individual
> endpoints. An example endpoint property is its Network Location (its
> grouping defined by the ALTO Server) or connectivity type (e.g.,
> ADSL, Cable, or FTTH).
>
> It is only in Section 10.3, with the creation of the registry, that
> it is clear that these are registered tokens, rather than free text.
> I suggest that this be clarified early, with a forward pointer to
> the actual registry.
It looks like the reference to 3.1.3 was wrong. It should have been a
direct pointer to 10.3. Thanks for catching that. We can make it
clear when pointing forward to 10.3 that these are identifiers from a
registry.
> In section 9.3, the document notes that the ALTO client SHOULD use
> STUN to determine a public IP for a source address. However,
> the description of the Endpoint Cost Service contains this text:
>
> If the list of Source
> Endpoints is empty (or not included), the ALTO Server MUST
> interpret it as if it contained the Endpoint Address corresponding
> to the client IP address from the incoming connection
>
> It would seem a valid recommendation here would be to propose that the
> CLIENT omit the Source Endpoint where it currently recommends the use
> of STUN. I infer that this is not recommended because the Endpoint
> Address service may be provided within a NAT boundary shared with the
> CLIENT, thus making the source address of the packet different from
> that which would be seen externally (resolving this would require the
> ALTO service to have general knowledge of the NAT bindings). But the
> opposite case is not handled. What happens when the destination
> address is within the NAT boundary of the client? Won't the external
> address determined by STUN will have a similar mismatch to the true
> origin address for packets destined to the internal destination?
Yes - I see the disconnect. I'd like to avoid solving the larger
deployment considerations under NATs (the deployment consideratoins
document should be handling that) in this document and instead focus
on the messaging details. Perhaps it would be best to change the
statement w.r.t. STUN to instead say MAY. This document can certainly
give a concise statement of the problems you identified there.
How does that sound?
> In 9.4, it may be useful to note that there are private ASNs which
> are not unique, and that a single looking glass may therefore give
> an incomplete view of the full mapping of IP addresses to ASNs. In
> general, this section seems to point to future work, and I suggest
> that it more clearly say that a standardized method for this mapping
> is left to future work.
Agree - we'll add text there to convey that.
>
>
> In section 10.4, it may be useful to advise the Expert Reviewer
> on how to handle registrations which differ only as to case;
> for example, would PRIV: or EXP: be permitted in a registration?
>
> Nits:
>
> The Abstract says:
>
> These maps provide simplified,
> yet enough information of a network for applications to effectively
> utilize.
>
> This seems to be missing a noun or to require restructuring. Possibly:
>
> "These maps provide simplified information, but it is enough for
> applications to effectively use."?
>
> "Additional services are built on top the maps." should be "on top of
> the maps".
>
> In the Introduction, "see below" could be shifted to a more precise
> forward reference to a specific section.
>
> In section 2.2, the document says:
>
> In particular, an ALTO Server defines network
> Endpoints (and aggregations thereof) and generic costs amongst them,
> both from the network region's own perspective.
>
> I personally found this hard to parse. Some re-wording may be useful.
> (Possibly eliminating the ", both"?)
Ack. We'll clean these up.
>
> In Figure 1, the top most enclosing box is labelled "ISP". While that is
> certainly a common use case, there seem to be others. Given the
> preceding text, would it be more general to use the term "Network
> Region"?
Agree. "Network Region" sounds good.
>
> In section 3, the document says:
>
> "Note that functionality offered in different
> services are not totally non-overlapping (e.g., the Map Service and
> Map Filtering Service)."
>
> Would it be equivalent to say "functionality offered in different
> services may overlap"?
Agree. Fewer double-negatives would be better :)
>
> Section 3 is titled "Protocol Structure", but the subsections are about
> specific services, and the base text doesn't really describe what
> most IETF readers will understand as a description of the structure of
> the protocol (that seems to be in Section 7). A different title (maybe
> "Service framework"?) would seem to be a better fit.
Agree. I like "Service Framework"
> I believe Section 4 might read more naturally if the second paragraph
> were moved up to be first.
Agree.
> In 7.5.1, the encoding of PID Name is given in relation to a permitted
> US-ASCII code range, except for the "." separator. Providing the
> related code point would be useful. Several other sections use
> "alphanumeric" as a limitation without giving the code point ranges;
> in those cases, the actual code point ranges would be useful.
Ack.
> The Content-Length in some examples (e.g. in 7.6.3) are listed
> as [TODO]; this should be fixed.
Yeah - these haven't changed in a few revisions, so it would be
reasonable to resolve those TODO's now :)
> In section 9.2, the document says "one path my have a NAT64 middlebox)",
> where my should be "may".
>
> In Section 10.1, it is common for the change controller to be the IESG,
> rather than the individual editors of the draft.
Ack.
Thanks again!
Rich
_______________________________________________
alto mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/alto
