Re: [Gen-art] genART review of draft-ietf-cdni-logging-15

["Francois Le Faucheur (flefauch)" <[e-mail]>]

Hello Martin,

Thanks for your thorough review. Please see proposed resolution or reponse below:

On 12 Feb 2015, at 02:24, Martin Thomson <[e-mail]<mailto:[e-mail]>> wrote:

And because I hit send too soon, here's more...

I think that this is ready.  I have a couple of major concerns:

Major General 1.

However, I would like to see more consideration given to forward
compatibility.  There are a lot of normative statements regarding what
MUST be included in the file, but no actions described for a receiver.
That means that it is hard to establish expectations regarding file
format evolution.

Good point.

I have added this at the end of S3.3:

NEW:
“
An uCDN-side implementation of the CDNI Logging interface MUST reject a CDNI Logging File that does not comply with the occurences specified above for each and every directive. For example, an uCDN-side implementation of the CDNI Logging interface receiving a CDNI Logging file with zero occurence of the Version directive, or with two occurences of the Integrity-hash, SHOULD reject this CDNI Logging File.
"


And also edited the end of S3.4 as per the following:

OLD:
"
 An uCDN-side implementation of the CDNI Logging interface MUST be
   able to accept CDNI Logging Files with CDNI Logging Records of
   Record-Type "cdni_http_request_v1" containing any CDNI Logging Field
   defined in Section 3.4.1 as long as the CDNI Logging Record and the
   CDNI Logging File are compliant with the present document.
“
NEW:
“
 An uCDN-side implementation of the CDNI Logging interface MUST be
   able to accept CDNI Logging Files with CDNI Logging Records of
   Record-Type "cdni_http_request_v1" containing any CDNI Logging Field
   defined in Section 3.4.1 as long as the CDNI Logging Record and the
   CDNI Logging File are compliant with the present document.

In case, an uCDN-side implementation of the CDNI Logging interface receives a CDNI Logging File with HTTP Request Logging Records that do not contain field values for exactly the set of field names actually listed in the preceding “Fields” directive, the implementation SHOULD reject those HTTP Request Logging Records, and SHOULD accept the other HTTP Request Logging Records .
"


If the file is rejected when the version isn't
"CDNI/1.0", then that makes changes to that field impossible (and the
specification of ABNF for it somewhat pointless).

Right, the expectation is not that the file be rejected if the version is not "CDNI/1.0”.

I have incorporated the following change:

OLD:
"
The value MUST be "CDNI/1.0" for the version specified
         in the present document.
“
NEW:
“
The entity transmitting a CDNI Logging File as per the present document MUST set the value to "CDNI/1.0”. In the future, other versions of CDNI Logging File might be specified; those would use a value different to "CDNI/1.0” allowing the entity receiving the CDNI Logging File to identify the corresponding version.
"



Major Specific 2.

  An implementation of the CDNI Logging interface MUST support the
  TLS_DHE_RSA_WITH_AES_128_GCM_SHA256 cipher suite ( [RFC5288]).

Please don't do that.  There are mandatory to implement cipher suites
in all the specs you cite and you don't need anything special here.  I
have no problem with recommending PFS suites, but you might be better
served there by citing the uta TLS usage draft:
https://tools.ietf.org/html/draft-ietf-uta-tls-bcp

Absolutely. We were not aware of the TLS BCP which is why we tried to make some minimum recommendations, but we’d much rather leverage teh UTA work.

Here’s the edit that has been incorporated:
OLD:
“
   An implementation of the CDNI Logging interface MUST support the
   TLS_DHE_RSA_WITH_AES_128_GCM_SHA256 cipher suite ( [RFC5288]).  An
   implementation of the CDNI Logging interface SHOULD prefer cipher
   suites which support perfect forward secrecy over cipher suites that
   don't.
"
NEW:
"
 The general TLS usage guidance in [I-D.ietf-uta-tls-bcp] SHOULD be
   followed.
“




S 3.4.1
  o  cs(<HTTP-header-name>):
HTTP header fields, esp. in HTTP/2 can contain HTAB, even if that
isn't permitted by the grammar.  You probably want to describe how
that is handled.

Since the value of the HTTP header is encoded as a “quoted string” (QSTRING), is it not OK to have a HTAB within the HTTP header value as long as it is properly bracketed by the Quotes?



Bottom of P33 (won't adding the established-origin directive
invalidate the MD5?
What do you expect to happen there?  (I see two
choices, it would help if you could even *hint* at what you would
prefer implementations to do).

Yes, adding the established-origin directive invalidate the MD5. So it needs to be recalculated after adding an established-origin directive.

The text specifying the Integrity Hash already covers that :
"
         If the entity
         receiving a non-corrupted CDNI Logging File adds an
         Established-Origin directive, it MUST then recompute and update
         the Integrity-Hash directive so it also protects the added
         Established-Origin directive.
“

Do you see that we need to say something more?



Nit: HTTP/2 not HTTP/2.0

Fixed.


Question: There is no mention made of range requests, for which this
might be well suited, particularly if files get large.

The document says that "client-side implementation of the CDNI Logging interface MAY pull, at its convenience, a CDNI Logging File “ using standard HTTP. Since range request is just a standard HTTP mechanism, the clien-side may use it if it sees fit.
Do you feel it is worth specifically calling out that range requests can be used?
No problem in doing that if you feel it is useful.


I note the privacy considerations are pretty reasonable.

Great.

 I'd be much
happier if IP anonymization were not optional though.

Hmm.
As discussed in response to another related comment, I see value in leaving the choice.
For example, exchanging full IP address can help with some use cases such as fault tracking across the interconnected CDNs.
Also, logging full IP address is the standard practise today in a single CDN environment, so CDN operators are likely to expect that from an interconnected CDN , unless there are reasons to anonymize in a particuylar environment.

I’ll address your other batch of comment separately.

Francois



On 12 February 2015 at 12:02, Martin Thomson <[e-mail]<mailto:[e-mail]>> wrote:
I am the assigned Gen-ART reviewer for this draft. For background on
Gen-ART, please see the FAQ at

<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Please resolve these comments along with any other Last Call comments
you may receive.

Document: draft-ietf-cdni-logging-15
Reviewer: Martin Thomson
Review Date: 2015-02-12
IETF LC End Date: 2015-02-18
IESG Telechat date: (if known)

Summary:

Major issues:

Minor issues:

S3.1 needs to define the basic atom that is used for the logging file.
It appears as though the atom is an octet.

S3.2 uses the wildcard ABNF rule (i.e., <text....>) too aggressively.
RECLINE (a potentially confusing name) could be more strictly defined.

   <CDNI Logging File> = 1*<DIRGROUP RECGROUP>

... is not valid and could be:

   CDNI-LOG-FILE = 1*(DIRGROUP / RECGROUP)

That implies that an empty file is not valid.  I'd have thought that *
rather than 1* would be better.

S3.3 again uses wildcard rules too aggressively.  DIRNAME (not the
unix command) should be defined more explicitly, even if it is just to
specify what characters are allowed.  Use text to further constrain
it.  As it stands, a generic processor cannot know that the name
doesn't permit the inclusion of (for instance) ":" or CRLF.  A common
method is to define something like this as:

   DIRNAME = Version-directive / UUID-directive / ... / extension-directive

More commonly, go up a level and define directive using the choice.
That way, each directive can define a name and value together using
ABNF.  e.g.

   Version-directive = "Version" ":" HTAB "CDNI" "/" 1*DIGIT "." 1*DIGIT

I wouldn't be so prescriptive about the version format.  Unless there
are specific semantics you want to extract from each digit.

What is a receiver expected to do if the version *isn't* CDNI/1.0 ?
Discard the entire file?  That doesn't seem like a good way to ensure
forward compatibility.

UUID: this a Universally Unique IDentifier (UUID)
        from the UUID Uniform Resource Name (URN) namespace specified
        in [RFC4122]) for the CDNI Logging File.

I don't know how to apply this.  Is it the UUID portion of the URN or a URN?

Please don't use MUST here: If the
        two values are equal, then the received CDNI Logging File MUST
        be considered non-corrupted.

MD5 collisions are easy to manufacture.  I'd also drop the MUST from
the following sentence.

S3.4: more wildcards

S3.4.1: I wouldn't worry about HTTP/2 being special here.  All that
speculation is painful, especially since HTTP/2 will likely be
published before this document.  Better to just cite it and avoid
getting caught up in the details.




Nits/editorial comments:

There are lots of uses of lowercase "may".

Page 6 has a few instances of missing hyphens in dCDN3/dCDN2 etc...
S6.2.1: chunck

S3.1 the DATE/TIME rules can/should reference RFC 3339

S3.3 using ":" HTAB as a separator makes it hard to construct these
files manually.

S3.4 use of "c:" in combination with the unordered list is confusing
when the prefix is actually "c-".  Maybe use the string "c-" quotes
and all to denote the prefix.

S3.4.1 citing RFC 7230 is sufficient for HTTP/1.1; and probably for
HTTP in general at this moment.