[Resending with corrected review date] 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-05 Reviewer: Paul Kyzivat Review Date: 2017-06-23 IETF LC End Date: 2017-06-28 IESG Telechat date: TBD 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: 3 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") This isn't easy to fix because of way the ABNF of RFC5322 is structured. What you need is for RFC5322 to be restructured as follows: fields = *trace-prefix *normal-field trace-prefix = trace *optional-field / *resent *(resent-date / resent-from / resent-sender / resent-to / resent-cc / resent-bcc / resent-msg-id) normal-field = orig-date / from / sender / reply-to / to / cc / bcc / message-id / in-reply-to / references / subject / comments / keywords / optional-field (Note: I've focused on the normal-field part. Additional work is probably required on the trace-prefix to get proper extensibility.) Once that is done, then RFC5536 could be revised as follows: normal-field =/ approved / archive / control / distribution / expires / followup-to / injection-date / injection-info / lines / newsgroups / organization / path / summary / supersedes / user-agent / xref And your ABNF can then be: normal-field =/ cancel-lock / cancel-key Then you will have a syntax that reflects your intent. Unfortunately this is a big deal because it requires revisions to both RFC5322 and RFC5536. The revision to RFC5322 would be entirely backward compatible in that it will accept exactly the same inputs. The revision to RFC5536 is *not* backward compatible, but IIUC it is compatible with the *intent* and presumably what has been deployed. (Obviously implementations of RFC5536 have not been generated directly from the ABNF.) I'm surprised this hasn't ever been reported. Another way to handle this would be to revise RFC5322 to simply include the generic syntax, such as: fields = *trace-prefix *optional-field and then rely on the IANA Permanent Message Header Field Repository to define the allowable fields. (2) Minor: In Section 3.5, step 1 says to hash the key using the algorithm from its scheme. But IIUC the scheme describes the algorithm that has already been used when constructing the Cancel-Key header. IIUC the serving agent need not hash the provided key. Rather it only uses the scheme to select the locks to compare the hash against. (3) Minor: IMO the examples in Section 5 seem incomplete. They don't make clear what is being done by each participating entity, and at what stage. And because all the examples use sha256 they don't make it clear that the algorithm used to create the hashed key could be different from that used to construct the corresponding lock. (4) Minor: In Section 8.1/8.2 the syntax/semantics of the Owner/Change controller is unspecified. But IANA is charged with allowing/rejecting change requests only from the "same" owner. How is IANA expected to verify this? What if a change is required but the original owner is no longer available? E.g., the owner might be specified by email address. Later the mail server for that email address may no longer exist.