[ietf-dkim] editorials and nits

[Eric Allman]

> #1 saying "proof" and "non-repudiation" in the abstract is a
> mistake and potentially misleading. Rephase to use less difficult
> terms, e.g.  talk about signatures being evidence rather than proof.

Given the sensitivity over the exact wording of the abstract, I'm not 
willing to make this change without discussion of some proposed 
wording.

> #2 1.1, 1st set of bullets. The biggest difference between dkim and
> s/mime or pgp signatures IMO is that with dkim we do not expect
> that signature failure will lead to message (delivery) failure,
> whereas with s/mime or pgp we do. I'd mention this in a bullet.

There seems to be a difference of opinion on this, so I'll leave it 
for now.

> #3 1.1, 2nd set of bullets. dkim *does* require a ttp - the DNS.
> Better to say that dkim requires no *new* ttp.

I don't see DNS as a "third party" in the same sense as a CA for 
certs.  Yes, DNS has to work, but it isn't a third party (unless you 
want to count the root servers, I suppose).  By this logic, we should 
also include the multiple third parties that run the routers and all 
the rest of the infrastructure.

> #4 1.1, last para. Seems a bit early to introduce selectors here -
> was there a reason (that still applies)? If not, then maybe this
> can be deleted now.

I don't see it as necessary.  Deleted.

> #4 3.2, 1st sentence. "multiple key signing/verfication algorithms"
> is not a good phrase, suggest changing to "multiple digital
> signature algorithms" instead.

I hope you mean 3.3, otherwise you may be reading a very out of date 
draft.  Changed.

> #5 3.3.1 and 3.3.1, phraseology is still a bit odd. Suggest
> changing from: "That hash is then signed by the signer using the
> RSA algorithm (defined in PKCS#1 version 1.5 [RFC3447]; in
> particular see section 5.2) with an exponent of 65537 as the
> crypt-alg and the signer's private key.  The hash MUST NOT be
> truncated or converted into any form other than the native binary
> form before being signed." ...to... "The signature is calculated
> using the RSA algorithm with a fixed public exponent of 65537 - if
> a different public exponent is required, then a new DKIM signing
> algorithm must be defined."

Adding the specific reference was specifically requested by another 
reviewer --- in fact, I think your proposal changes it back to almost 
exactly what was objected to before.  I think this requires some 
discussion.

> #6 3.3.3 suggest s/"do not understand"/"cannot verify"/

I changed this to "do not implement".  "Cannot verify" is too vague: 
there are lots of reasons a verifier might not be able to verify a 
signature.

> #7 3.3.4 says "RSA keys of at least 1024 bits" and similar. The 1024
> refers to the modulus length so that would be more correctly stated
> as "RSA keys with modulus at leat 1024 bits long". Similar fixes
> could be done with other parts of the same section.

I would like some discussion on this.  This might be more technically 
accurate for a cryto expert, but I know that if I as a non-crypto guy 
read this, I would immediately ask myself "now, is that the same as 
the key length or something else altogether?"

> #8 3.3.4 would be a little clearer if it said that "Verifier
> security policies may use the length..."

Agreed.

> #9 3.4 s/result in an authentication failure/result in signature
> verification failure/ since dkim isn't authenticating, strictly
> speaking.

It seems pretty pedantic, but OK, changed.

> #10 3.4.5, 1st sentence: s/and verified/and to be verified/
> or just drop the last two words.

I just dropped them.

> #11 3.4.5, end of 1st informative note: s/ignore the tag/ignore
> content after the indicated length/ Reason - if the ignore the tag
> then they won't verify the signature.

Actually, in our early discussion over this we actually did mean that 
the verifier can simply ignore the tag, and yes, it won't verify. 
Some people deemed that to be a feature, not a bug.

> #12 3.4.5, 3rd para: s/body length count is made after/body
> length count is calculated after/

Done.

> #13 3.4.5, 2nd informative note: I think this just repeats stuff in
> the 1st informative note and could be deleted.

The point of this note was to provide a clear rationale, which 
doesn't really seem to be fully done in the first (implementation) 
note.  I moved it up before the first note and deleted the first 
sentence of the implementation note, so it now reads:

        INFORMATIVE RATIONALE: This capability is provided because it
        is very common for mailing lists to add trailers to messages
        (e.g., instructions how to get off the list). Until those
        messages are also signed, the body length count is a useful
        tool for the verifier since it may as a matter of policy
        accept messages having valid signatures with extraneous data.

        INFORMATIVE IMPLEMENTATION NOTE:  Using body length limits
        enables an attack in which an attacker modifies a message to
        include content that solely benefits the attacker. It is
        possible for the appended content to completely replace the
        original content in the end recipient's eyes and to defeat
        duplicate message detection algorithms. To avoid this attack,
        signers should be wary of using this tag, and verifiers might
        wish to ignore the tag or remove text that appears after the
        specified content length, perhaps based on other criteria.

Does this address your comment adequately?

> #14 3.4.5, last informative note: again I think this could be
> deleted.

I deleted this and we'll see if anyone screams.  There are so many 
notes in that section because the "l= is evil" faction wanted to make 
it very clear that you can get yourself into a heap of trouble by 
using it inappropriately.

> #15 3.5 "q=" the text says "dns/txt" but the ABNF contains "txt/dns"
> and the text description isn't clear (enough) that "dns" is allowed
> and means "dns/txt".

Whoops, the ABNF was wrong.  And actually "dns" by itself is /not/ 
allowed; note the sentence reading "The only option defined for the 
"dns" query type is "txt", which MUST be included."

> #16 3.5, example at end. This example doesn't contain a "bh=", it'd
> be better if it did since that's REQUIRED.

Right you are.  It was also missing a semicolon after the z= value.

> #17 3.6.1 talks about "the response" which is dns specific, and I
> thought we didn't want to be here, so s/response/record/ maybe?

Works for me.

> #18 3.6.2.1 and elsewhere, I see ""_domainkey"" and other instances
> of double-double quotes. Can't see why those are there.

Someone else reported the same thing and I couldn't find it --- but 
this time I did (use of spanx with style=verb causes quoting in plain 
text).  It should be fixed now.

> #19, 3.7. This talks about two hashes all the time, which is
> correct, but a bit misleading since any sensible API will include
> both hashing and private key application inside a "sign()"
> function. The end result is that the dkim programmer calls "hash()"
> once and either "sign()" or "verify()" once. Suggest adding a
> sentence along these lines to clear that up. If someone ever
> calculated "sign(hash())" then they'd be wrong and this text sort of
> suggests that. Similar comment about the pseudo-code at the end
> where I'd rather see something like:
>
>        body-hash = hash( canon_body )
>        signature = sign( canon_header || DKIM-SIG )

I disagree.  This section has nothing to do with what an API would 
look like, it's about what algorithms have to be applied and when. 
For example, I can easily see your change being interpreted to mean 
that you don't hash the header at all.

> #20, 3.7, 4th last para. This sort-of threw me a bit since it seems
> like an MUA instruction. Is it? If so, then maybe move to the
> appendix/overview? This is the one starting "When calculating the
> hash on messages..."

No, this is not an MUA function --- it is perfectly reasonable for a 
gateway to convert an 8-bit message to a 7-bit form long after the 
MUA is done with it, and dot-stuffing definitely has nothing 
whatsoever to do with the MUA.

> #21, 3.7, last para. "sans" is nice, but would it confuse
> non-franglish speakers?

Actually it's an English word, derived from Latin, as in "sans 
serif".  It is also a French word, and came into Middle English from 
Old French.  But if other English speakers want me to change it, Ita 
Sit (Latin for "so be it", although we more commonly use the word 
"amen", although that has another connotation).

> #22, 5.2, last para. Change "when the selector containing the
> corresponding public key is expected to be removed before the
> verifier...", to, "when the public key in the relevant selector is
> expected to be revoked before the verifier..." since we're not
> recommending removing the selector but rather zeroing out the key.

Is that what we are recommending?  I thought that omitting the key 
was a way of saying "we're sorry, but we had to pull that key 
unexpectedly."  In fact, the usual way of dropping a key would be to 
drop the record entirely.  Otherwise you would have ever-growing name 
spaces.

> #23, 5.5, last 2 paras about "l=". I think this has all already
> been said, so it could be removed from here.

I would consider removing the informative note, but I think it is 
completely reasonable to include all the steps in the section on 
"Compute the Message Hash and Signature", even if they do duplicate 
some other text.

> #24, Section 6, 1st sentence says "may expire a public key"
> s/expire/revoke/ since dkim, (properly), doesn't do expiry, only
> revocation.

See my comments on #22.

> #25, 6.1, 1st note, "other clues" is a bit opaque - include a
> reference to somewhere where the reader can find those clues or
> maybe reword.

I changed this to read "INFORMATIVE IMPLEMENTATION NOTE: Verifiers 
might use the order as a clue to signing order in the absence of any 
other information. However, other clues as to the semantics of 
multiple signatures (such as correlating the signing host with 
Received headers) may also be considered."

> #26, 6.1 "; this is local policy..." that "this" is a maybe a tad
> ambiguous, suggest replacing with "; such treatment is a matter
> for local policy..."

Done.

> #27, 6.1.3, list item #1 says "create a canonicalised copy" which
> isn't necessary - all you need to is feed the right bits through
> the hashing, so an entire copy need never be created. Suggest
> rewording to say "re-create the canonicalized octets" or something.
> (Or, add an informative note along the above lines.)

This is a description of an algorithm, not an implementation.  I'll 
put in an implementation note, but frankly getting this wording 
sufficiently unambiguous has proven challenging, and I'm reluctant to 
wade in again without a really good reason.  I will change the intro 
to read "... consists of actions semantically equivalent to the 
following steps."  I can also add something in the Implementer's Note 
if you feel it needs more.

> #28, 6.2. The reference to ID-AUTH-RES might be better off being
> removed for process reasons.

Even as an example?  It's informational, not normative.

> #29, 8.1.1. Missing the example.

Right, thanks.  I added a paragraph reading:

        For example, if an attacker can append information to a
        text/html body part, they may be able to exploit a bug in
        some MUAs that continue to read after a </html> marker, and
        thus display HTML text on top of already displayed text.  If
        a message has a multipart/alternative body part, they might
        be able to add a new body part that is preferred by the
        displaying MTA.

> #30, 8.2. This section mainly applies to MUA signing, which is not
> the main use-case, and so the threat is less serious. Suggest
> saying that  at the start of the section along with a sentence
> like: "Protection of private keys on servers can be easily achieved
> though the use of  specialised crytographic hardware."

Added.

> #31, A.2, the example has no "bh=". Needs recalculating. And would
> be better if the example inlcuded real signatures and the
> corresponding public key record too.

I did get a (correct) bh= value included, but haven't recomputed the 
signature (it's getting late today; I'll try to get to that tomorrow).

> #32, A.3, the authentication results header should not, IMO be
> included here.

I think it is very important to make it clear how one might convey 
results to higher levels.  Otherwise all of A.3 doesn't have any 
reasonable example.  It doesn't have to be Authentication-Results, 
just something.

> #33, Appendix B. I expected to see a bit here about the type of
> setup we have at IETF meetings, where the IETF's MTA signs and the
> verifier has to handle the From being nothing like the signer
> identity.

Sorry, I don't see any signatures coming from the IETF MTA.  Can you 
be more specific?

> #34, Appendix C. I think that this can be deleted. Others may
> disagree. In any case, the text says 768 and the command line 1024,
> no  password handling is shown and the last part could confuse since
> that signature is not usable for DKIM. So if this isn't deleted,
> then a bunch of changes are needed.

I fixed the discrepancy, but I don't think it should be deleted. 
It's non-normative and will help people understand how to use the 
standard, which I think is A Good Thing.

I'm not sure what you mean about password handling.  I just ran 
through these instructions, and there are no passwords requested.

> Total Nits
> ----------
>
> #1 When the [[]] text disappears there'll be nothing between
> "introduction" and "overview" - suggest getting rid of 1.1 and just
> having the text be the introduction.

Works for me.

> #2 1.1, 2nd set of bullets. The last point here would be better in
> the 1st set of bullets.

Makes sense.  Done.

> #3 2.3, last sentence. Does the single abnf definition here offer
> value? If so, then why not one for FWS too? Suggest deleting this.

I think the ABNF does have value.  We don't include it for FWS 
because that's imported from another document (RFC2822); the ABNF is 
there.

> #4 3.5, 3rd para: is "the null string" well enough defined? Maybe
> "an empty string" would be better?

Done.

> #5 3.5, last para before tags says: "Defined tags are described
> below." which is sort of obvious. Suggest deleting it.

Done (but why do I have the creepy feeling someone is going to 
object?).

> #6 3.6.1 "k=" says that the public key is in the "p=" value, but its
> actually the modulus.

I guess I'm confused.  If this isn't the public key, what is?

> #7 3.6, 3rd last para s/string of characters/string of octets/

3.7 perhaps?  I've made that change.

Thanks for your comments.

eric