Last Call Review of draft-baeuerle-netnews-cancel-lock-05
review-baeuerle-netnews-cancel-lock-05-genart-lc-kyzivat-2017-06-23-01

• Status
• Email expansions
• History

• Versions
• 00
• 01

Review
review-baeuerle-netnews-cancel-lock-05-genart-lc-kyzivat-2017-06-23

[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.