Subject: Re: [apps-discuss] draft-kucherawy-rfc5451bis

From: Dave Cridland <[email-masked]>
Date: Wed, 3 Apr 2013 11:18:33 +0100

Murray,

On Fri, Mar 22, 2013 at 7:21 PM, Murray S. Kucherawy <[email-masked]>
wrote:
>
> Hi Dave, thanks for the review.   Comments below.
>

Thanks for the new versions (so many it confused me greatly trying to
review the diffs - slow down!).

>
> On Fri, Mar 15, 2013 at 4:34 AM, Dave Cridland <[email-masked]> wrote:
>>
>> For the most part, this document looks fine and solid.
>
>
> Hooray!
>
>>
>>
>> Two items concern me in a major way; one further item has me mildly
nervous.
>>
>> First, the minor one:
>>
>> The header includes definition of a version ABNF production optionally
following the authentication server's identity. It took me some time to
find the note of how to treat this; I didn't see any examples with it
present either.
>
>
> I've updated one of the examples to include a version.
>

This satisfies my comment, FTR.

>>
>>
>> Second, the first major one:
>>
>> The examples tend to imply, for the most part, a canonical form to the
header, which places CFWS is only particular places. In particular, all the
examples are of the form auth-srv-id "; " method "=" result, with some
optional properties and/or reason, with one single exception (which
includes a comment in lieu of a reason). It's not immediately apparent to a
comparatively novice reader that the following header field is valid:
>>
>> Authentication-Results: foo.example.net (My mailserver) / (I am number)
1 (You see) ; dkim (Because I like it) /2 (Two yay) = (wait for it) pass ;
policy (And a dot can go here) . (Like that) fruit (which surprised me) =
(because I was not expecting it) banana
>>
>>
>> I appreciate it's always nicer to only write pretty examples, but given
the specification, providing a nasty one is probably needed, lest people
expect to take a whitespace delimited token and split it on '='. For some
reason I particularly wasn't expecting the CFWS around the dot. (Maybe
that's because I'm out of practise with header parsing).
>>
>> In any case, highlighting this with an example seems appropriate and
useful.
>
>
>
> Copied to the draft, with some adjustments to make it compliant and fit
the width limit.

Likewise, this satisfies my comment.

>>
>>
>> Thirdly, one more major concern:
>>
>> You'll note that I included a version for the method in my example
above. The definition for this, from the comments in the ABNF, suggests
that this relates to the version of RFC 5451 and successors used, but this
seems unlikely - I'm also at a loss as to what a receiving implementation
should do with such a method, since the handling of version seems to be
specified only for the version number after the auth-srv-id.
>>
>> I have a vague suspicion that the correct thing to do here may be to
deprecate the method version entirely, but I might be missing something -
still, it doesn't appear to relate to the method, but the header field.
>>
>>
>
> The method version refers to the method itself, which is specified
elsewhere; the authserv-id version refers to this draft and thus the syntax
of this header field.  The idea is that if you find a version after an
authserv-id that you can't handle, you stop trying to parse right away
because what follows might not be what you expect.  In terms of a method
version, the parser should ignore a method result if the version of the
version is not supported in case the semantics of the result have a
different meaning.  If, for example, DKIM v2 (were such a thing to exist)
yielded a "pass" for different reasons than v1 did, a consumer of this
field might not want the altered semantics to apply.  This is a way to
indicate this and let the consumer decide.
>
> Is your reply to this "You should say that?"  :-)
>

Yes... And I think your §2.4 "Version Tokens" (new in -03) is good here.

However, you have in -04's ABNF (with much elided):

     authres-header = "Authentication-Results:" [CFWS] authserv-id
              [ [CFWS] "/" [CFWS] version ]
              ( [CFWS] ";" [CFWS] "none" / 1*resinfo ) [CFWS] CRLF
            ; the special case of "none" is used to indicate that no
            ; message authentication is performed

     version = 1*DIGIT [CFWS]
             ; indicates which version of this specification is in use;
             ; this specification is version "1", and the absence of a
             ; version implies this version of the specification

     method = Keyword [ [CFWS] "/" [CFWS] version ]
            ; a method indicates which method's result is
            ; represented by "result", and is one of the methods
            ; explicitly defined as valid in this document
            ; or is an extension method as defined below

The conflation of the two different versions into one is (I find)
confusing, though §2.4 goes a long way to correcting that - perhaps far
enough, but I remain uneasy.

I think your choices are:

a) Rewrite the comment below the version production to point the reader to
§2.4

b) Introduce two different productions, one for "authres-version" and one
for "method-version", such that:

     authres-header = "Authentication-Results:" [CFWS] authserv-id
              [ [CFWS] "/" [CFWS] authres-version ]
              ( [CFWS] ";" [CFWS] "none" / 1*resinfo ) [CFWS] CRLF
            ; the special case of "none" is used to indicate that no
            ; message authentication is performed

     authres-version = version [CFWS]
             ; indicates which version of this specification is in use;
             ; this specification is version "1", and the absence of a
             ; version implies this version of the specification

     method = Keyword [ [CFWS] "/" [CFWS] method-version ]
            ; a method indicates which method's result is
            ; represented by "result", and is one of the methods
            ; explicitly defined as valid in this document
            ; or is an extension method as defined below

     method-version = version [CFWS]
             ; indicates which version of the method specification is in
use;
             ; those those methods defined in this specification are
version "1", and the absence
             ; of a method-version implies this specification

     version = 1*DIGIT
             ; Version numbers are a simple string of digits; point
versions are not allowed.

c) Or, of course, both.

d) Ignore me.

Using (b) gives you the option of using those terms in §2.4 (and elsewhere
as needed) to clarify which version you mean you mean, too.

Obviously I'd rather you didn't do (d), but I'll accept it if you do. :-)

Dave.

---

From: "Murray S. Kucherawy" <[email-masked]>
Date: Wed, 3 Apr 2013 11:58:50 -0700

Submitting draft-ietf-appsawg-rfc5451bis now, with changes in response to
your feedback as follows:

On Wed, Apr 3, 2013 at 3:18 AM, Dave Cridland <[email-masked]> wrote:

> [...]
> [...]

> Obviously I'd rather you didn't do (d), but I'll accept it if you do. :-)

(b) will appear in the first Working Group version, which I'm about to
post.  Thanks for the suggestion.

-MSK