Telechat Review of draft-baeuerle-netnews-cancel-lock-06
review-baeuerle-netnews-cancel-lock-06-genart-telechat-kyzivat-2017-09-21-00

Request Review of draft-baeuerle-netnews-cancel-lock
Requested rev. no specific revision (document currently at 09)
Type Telechat Review
Team General Area Review Team (Gen-ART) (genart)
Deadline 2017-09-26
Requested 2017-09-13
Authors Michael Bäuerle
Draft last updated 2017-09-21
Completed reviews Genart Last Call review of -05 by Paul Kyzivat (diff)
Secdir Last Call review of -05 by David Mandelberg (diff)
Genart Telechat review of -06 by Paul Kyzivat (diff)
Secdir Telechat review of -06 by David Mandelberg (diff)
Opsdir Telechat review of -06 by Joel Jaeggli (diff)
Assignment Reviewer Paul Kyzivat
State Completed
Review review-baeuerle-netnews-cancel-lock-06-genart-telechat-kyzivat-2017-09-21
Reviewed rev. 06 (document currently at 09)
Review result On the Right Track
Review completed: 2017-09-21

Review
review-baeuerle-netnews-cancel-lock-06-genart-telechat-kyzivat-2017-09-21

I am the assigned Gen-ART reviewer for this draft. The General Area 
Review Team (Gen-ART) reviews all IETF documents being processed by the 
IESG for the IETF Chair. Please wait for direction from your document 
shepherd or AD before posting a new version of the draft. For more 
information, please see the FAQ at 
<​http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Document: draft-baeuerle-netnews-cancel-lock-06
Reviewer: Paul Kyzivat
Review Date: 2017-09-21
IETF LC End Date: 2017-06-28
IESG Telechat date: 2017-09-26

Summary:

This draft is on the right track but has open issues, described in the 
review.

General Comments:

I have not attempted to validate the security properties of this 
document - leaving that to a security review.

I have also not attempted to verify the operational suitability of this 
mechanism because I don't have the experience needed to do so.

Issues:

Major: 1
Minor: 0
Nits:  0

(1) MAJOR:

In Section 2, the ABNF syntax provided does not do what you want it to. 
You supply:

        fields =/ *( cancel-lock / cancel-key )

as an extension to the definition in RFC5536:

     fields          =/ *( approved /
                           archive /
                           control /
                           distribution /
                           expires /
                           followup-to /
                           injection-date /
                           injection-info /
                           lines /
                           newsgroups /
                           organization /
                           path /
                           summary /
                           supersedes /
                           user-agent /
                           xref )

and that in turn extends RFC5322:

     fields          =   *(trace
                           *optional-field /
                           *(resent-date /
                            resent-from /
                            resent-sender /
                            resent-to /
                            resent-cc /
                            resent-bcc /
                            resent-msg-id))
                         *(orig-date /
                         from /
                         sender /
                         reply-to /
                         to /
                         cc /
                         bcc /
                         message-id /
                         in-reply-to /
                         references /
                         subject /
                         comments /
                         keywords /
                         optional-field)

     message         =   (fields / obs-fields)
                         [CRLF body]

RFC5536 got this wrong, and the new draft continues the mistake. The 
problem is with the way things are grouped. Let me give a simpler example:

     foo = *("a" / "b") / *("c" / "d")

This means the following are valid: ab aabb bab cd ccdd dcd
But the following are not: abcd ac

The latter is what you want, for which the syntax would be:

     foo = *("a" / "b" / "c" / "d")

It isn't easy to do a valid syntax extension like this because of way 
the ABNF of RFC5322 is structured. However, after offline discussion, we 
realized that RFC5322 already has an extension point for new headers via 
the <optional-field> rule. Along with that, RFC3864 established a 
process for registering header fields with IANA.

So, my recommendation is that to fix this, remove from section 2 the 
extension of the <fields> rule:

       fields =/ *( cancel-lock / cancel-key )

Instead, simply rely on the text to specify the newly defined header 
fields, and the IANA registration to link them to RFC5322. This will 
probably require some minor tweaking of the text. I won't try to do the 
wordsmithing here.