Last Call Review of draft-ietf-json-text-sequence-11
review-ietf-json-text-sequence-11-genart-lc-black-2014-12-11-00
Request | Review of | draft-ietf-json-text-sequence |
---|---|---|
Requested revision | No specific revision (document currently at 13) | |
Type | Last Call Review | |
Team | General Area Review Team (Gen-ART) (genart) | |
Deadline | 2014-12-16 | |
Requested | 2014-12-11 | |
Authors | Nicolás Williams | |
I-D last updated | 2014-12-11 | |
Completed reviews |
Genart Last Call review of -09
by David L. Black
(diff)
Genart Last Call review of -11 by David L. Black (diff) Secdir Last Call review of -09 by Carl Wallace (diff) Opsdir Last Call review of -09 by David L. Black (diff) Opsdir Telechat review of -11 by David L. Black (diff) |
|
Assignment | Reviewer | David L. Black |
State | Completed | |
Request | Last Call review on draft-ietf-json-text-sequence by General Area Review Team (Gen-ART) Assigned | |
Reviewed revision | 11 (document currently at 13) | |
Result | Ready | |
Completed | 2014-12-11 |
review-ietf-json-text-sequence-11-genart-lc-black-2014-12-11-00
And the -11 version resolves everything else, plus picks up improved versions of the editorial clarifications; -11 is Ready for RFC publication. Many thanks for the timely responses. Thanks, --David > -----Original Message----- > From: Black, David > Sent: Wednesday, December 10, 2014 10:51 AM > To: nico at cryptonector.com; General Area Review Team (gen-art at ietf.org); ops- > dir at ietf.org > Cc: ietf at ietf.org; json at ietf.org; Black, David > Subject: Gen-ART and OPS-Dir review of draft-ietf-json-text-sequence-10 > > The -10 version of this draft resolves items [A]-[E] from the > Gen-ART and OPS-Dir review of -09, and the IESG is in the process of > resolving the (silly) idnits complaint about RFC 20 being a possible > downref. > > For item [D], a different approach was taken instead of modifying > the ABNF - the resulting new Section 2.4 is a definite improvement > to the draft, and is significantly clearer than the modified ABNF > would have been. Nicely done. > > Item [F] about the <angle-bracketed> text in the IANA Considerations > (Section 4) remains open - if the intent is to not deal with replacing > that text until after IESG approval, an RFC Editor Note to that effect > should be added to Section 4. > > I have an additional editorial concern - given all the discussion about > UTF-8, it would be good for the draft to make it clear early on > that JSON text sequences are UTF-8 only. Here are some suggested changes. > > Abstract: > > This document describes the JSON text sequence format and associated > media type, "application/json-seq". A JSON text sequence consists of > any number of JSON texts, each prefix by an Record Separator > (U+001E), and each ending with a newline character (U+000A). > > "any number of JSON texts" -> "any number of UTF-8 encoded JSON texts" > > It also looks like ASCII names for RS and LF are being mixed w/Unicode > codepoints in the second sentence in the abstract. I'm not sure that's > a good thing to do, especially as the body of the draft refers to RS and > LF as being ASCII. Here are a couple of changes that would remedy this: > > "an Record Separator (U+001E)" -> "an ASCII Record Separator (0x1E)" > "a newline character (U+000A)" -> "an ASCII newline character (0x0A)" > > Section 2 JSON Text Sequence Format: > > I suggest adding this sentence as a separate paragraph at the end of this > section (i.e., just before Section 2.1): > > JSON text sequences MUST use UTF-8 encoding; other encodings of JSON > (i.e., UTF-16 and UTF-32) MUST NOT be used. > > Aside from item [F], all of the above are editorial suggestions, but I > think making this clear early in the draft will help avoid potential > implementer confusion. > > Thanks, > --David > > > -----Original Message----- > > From: Black, David > > Sent: Friday, December 05, 2014 9:51 AM > > To: nico at cryptonector.com; General Area Review Team (gen-art at ietf.org); ops- > > dir at ietf.org > > Cc: ietf at ietf.org; json at ietf.org; Black, David > > Subject: Gen-ART and OPS-Dir review of draft-ietf-json-text-sequence-09 > > > > This is a combined Gen-ART and OPS-Dir review. Boilerplate for both follows > > ... > > > > 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. > > > > I have reviewed this document as part of the Operational directorate's > ongoing > > effort to review all IETF documents being processed by the IESG. These > > comments > > were written primarily for the benefit of the operational area directors. > > Document editors and WG chairs should treat these comments just like any > other > > last call comments. > > > > Document: draft-ietf-json-text-sequence-09 > > Reviewer: David Black > > Review Date: Dec 5, 2014 > > IETF LC End Date: Dec 5, 2014 > > IESG Telechat date: Dec 18, 2014 > > > > Summary: This draft is on the right track, but has open issues > > described in the review. > > > > This draft specifies a format that packs multiple JSON texts into a > > single string. The ASCII RS (0x1E) character is used to separate texts, > > and a linefeed is appended to each text to ensure that a complete text > > always ends with a whitespace character. > > > > All of the open issues are minor - the most important ones center on > > treatment of incomplete JSON texts - that appears to be an afterthought > > in this draft and needs more attention. I also found a couple of > > minor issues in the Security and IANA Considerations sections, both of > > which are almost nits. > > > > Major issues: None. > > > > Minor issues: > > > > [A] Section 2.1: > > > > If parsing of such an octet string as a JSON text fails, the parser > > SHOULD nonetheless continue parsing the remainder of the sequence. > > > > That's not quite right - there are two levels of parsing, JSON > > sequence parsing and JSON text parsing of each text in the sequence, > > both of which might be implemented in a single-pass parser. For such an > > implementation, the above sentence could be (mis-)read to imply that the > > JSON text parse should resume from the point at which it failed, which > > would be silly (although I've seen heroic PL/1 parsers do exactly that). > > Instead, the parse needs to skip ahead to the next RS, ignoring the rest > > of the JSON text that failed to parse. I suggest: > > > > If parsing of such an octet string as a JSON text fails, and the > > octet string is followed by an RS octet, the parser > > SHOULD nonetheless skip ahead to that RS octet and continue parsing > > the remainder of the sequence from there. > > > > That also covers the case where there is nothing more to parse after the > > JSON text that caused the parse failure. > > > > [B] Section 2.3: > > > > Is incremental parsing of a JSON text within a sequence allowed, or > > is the parser required to not produce any results until the parse of > > the entire text is successful? I'd expect that incremental parsing > > is ok (so results may be produced from a text that ultimately fails > > to parse), and I think that's worth stating. > > > > [C] Section 2.4: > > > > Parsers MUST check that any JSON texts that are a top-level number > > include JSON whitespace ("ws" ABNF rule from [RFC7159]) after the > > number, otherwise the JSON-text may have been truncated. > > > > That reference to the "ws" rule doesn't get the job done because that > > rule allows a match to no characters - it's of the form ws = *( ... ) > > where ... is the list of whitespace characters. What's needed here is > > a rule of the form vws = 1*( ...) to force there to be at least one > > whitespace character, but see the next issue for a better way to deal > > with this topic by pulling the appended LF into the sequence parse > > instead of the text parse. > > > > [D] I wonder whether the possibility of incomplete texts ought to be > > encoded into the parsing rules to directly catch JSON texts that must > > be incomplete because the last character is not LF, e.g.: > > > > JSON-sequence = *(1*RS (possible-JSON / truncated-JSON / empty-JSON)) > > RS = %x1E; "record separator" (RS), see RFC20 > > possible-JSON = 1*(not-RS) LF ; attempt to parse as UTF-8-encoded > > ; JSON text (see RFC7159) > > truncated-JSON = *(not-RS) not-LFRS); truncated, don't attempt > > ; to parse as JSON text > > empty-JSON = LF ; only the LF appended by the encoder, nothing to parse > > > > not-RS = %x00-1D / %x1F-FF; any octet other than RS > > not-LFRS = %x00-09/ %x1B-1D / %x1F-FF; any octet other than RS or LF > > > > Note that this won't detect all incomplete JSON texts, because LF is allowed > > within a JSON text (and this should be stated). > > > > [E] Section 3 - Security Considerations > > > > Incomplete and malformed JSON texts can be used to attack JSON parsers - > > that should be pointed out, as I don't see that in RFC 7159's security > > considerations and incomplete texts are a relevant consideration for > > this draft. > > > > [F] Section 4 - IANA Considerations > > > > Security considerations: See <this document, once published>, > > Section 3. > > > > Interoperability considerations: Described herein. > > > > Published specification: <this document, once published>. > > > > Applications that use this media type: <by publication time > > < https://stedolan.github.io/jq > is likely to support this format>. > > > > Replace all three instances of the angle bracketed text. The first two > > instances should be RFC references (e.g., RFC XXXX) w/a note to the RFC > > Editor to insert the number of the RFC when published. The third instance > > should be resolved now, or could have an RFC Editor note added indicating > > that the author will resolve that during Authors 48 hours. > > > > Nits/editorial comments: > > > > idnits didn't like the reference to RFC 20 for ASCII: > > > > ** Downref: Normative reference to an Unknown state RFC: RFC 20 > > > > RFC 5234 (ABNF) uses this, which looks like a better reference: > > > > [US-ASCII] American National Standards Institute, "Coded Character > > Set -- 7-bit American Standard Code for Information > > Interchange", ANSI X3.4, 1986. > > > > --- Selected RFC 5706 Appendix A Q&A for OPS-Dir review --- > > > > Most of these questions are n/a because this draft describes a format > > that will be used in other protocols to which RFC 5706's concerns would > apply. > > > > A.1.4 Have the Requirements on other protocols and functional > > components been discussed? > > > > The specification of the interaction of the JSON sequence parser with the > > JSON text parser is not as clear as it should be for incomplete or malformed > > JSON texts. See Minor Issues [A]-[E] above. > > > > A.1.8 Are there fault or threshold conditions that should be reported? > > > > Yes, incomplete JSON texts - this is covered in sections 2.3 and 2.4. > > > > Thanks, > > --David > > ---------------------------------------------------- > > David L. Black, Distinguished Engineer > > EMC Corporation, 176 South St., Hopkinton, MA 01748 > > +1 (508) 293-7953 FAX: +1 (508) 293-7786 > > david.black at emc.com Mobile: +1 (978) 394-7754 > > ----------------------------------------------------