search

trusteddomainproject / OpenDKIM

Other
97 stars 52 forks source link

Documentation suggestions for OversignHeaders/SignHeaders/OmitHeaders #131

Open raforg opened 3 years ago

raforg commented 3 years ago

The opendkim.conf(5) manpage states in the "OversignHeaders" section:

Note that listing a field name here and not listing it in the
SignHeaders list is likely to generate invalid signatures.

That means that you require the user to know if a header to be oversigned is in SignHeaders or not.

However, the manpage does not clearly state which headers are in SignHeaders by default.

The manpage states in the "SignHeaders" section:

By default, those fields listed in the DKIM specification as
"SHOULD" be signed (RFC6376, Section 5.4) will be signed by
the filter.

But RFC6376 doesn't list any headers that "SHOULD" be signed. In section 5.4.1, there is a list of "Common examples" including one "REQUIRED" (From). There are no SHOULDs there.

Is that list of common examples what was intended? Exactly that list? i.e.

From Reply-To Subject Date To Cc
Resent-Date Resent-From Resent-To Resent-Cc
In-Reply-To References
List-Id List-Help List-Unsubscribe List-Subscribe
List-Post List-Owner List-Archive

If so, please state it clearly in the manpage (and fix the reference to "SHOULD").

If a user wants to oversign a header, there's too much effort required in order to find out if they need to define SignHeader as well, and what goes in it.

It should be possible to just uncomment a line containing the default definition, and add to it.

Friends don't make friends read RFCs. Actually, that's not right. Friends do make friends read RFCs. :-)

Also, is it possible to add to the default SignHeaders value rather than completely redefine it? Such a method is documented in the manpage (only) for OmitHeaders, but not for SignHeaders or OversignHeaders. i.e. I would like to think that this would work:

SignHeaders *,+Content-Type
OversignHeaders From,Content-Type

But there's nothing in the manpage (or the sample opendkim.conf) to tell me that that is supported. Is it? If so, it should be documented that it works.

Maybe I have to do this instead:

SignHeaders From,Reply-To,Subject,Date,To,Cc,Resent-Date,Resent-From,Resent-To,Resent-Cc,In-Reply-To,References,List-Id,List-Help,List-Unsubscribe,List-Subscribe,List-Post,List-Owner,List-Archive,Content-Type
OversignHeaders From,Content-Type

I hope that's not the case, but that's what the documentation leads me to expect.

What would be awesome would be to change the behaviour so that if a header is included in OversignHeaders, then it is automatically added to SignHeaders if it is not already there. That would result in less effort for the user. There would be no need for the user to change SignHeaders as a consequence of wanting to change OversignHeaders. But the documentation changes would still be worthwhile.

Also, while the sample opendkim.conf does state that OmitHeaders, SignHeaders and OversignHeaders are comma-separated, the manpage doesn't mention this. A user that only encounters the manpage (or encounters it first) is given no idea exactly how to supply header names for these parameters. It could have been commas, spaces, colons, or something else.

The default opendkim.conf in the debian package doesn't mention it either. I found it on the web before finding out that the sample is in /usr/share/doc/opendkim. So having it documented in the manpage would be great.

So, my suggestions are:

In the opendkim.conf(5) manpage:

In the sample opendkim.conf:

For bonus points:

I'd be happy to make the documentation/sample changes as a patch or pull request if you'd like. I'd need to know if the ,- and ,+ facility applies to SignHeaders and OversignHeaders.

thegushi commented 1 year ago

It LOOKS like these are supported as well -- I don't see anything different in the handling of one of these versus the other, but I'm waiting for additional confirmation. If you want to come up with a pull, please base it on the develop branch.

raforg commented 1 year ago

I've realised that the *,+hdr and *,-hdr notation would work for OversignHeaders (and SenderHeaders) but it's irrelevant for them since they have no default/base list to modify, so it's not worth documenting the fact. Only OmitHeaders and SignHeaders have a default/base list.

I'm preparing a pull request that makes the other doc/config changes described above, but I have a question about something that isn't clear to me from the documentation. Which is the correct syntax for adding/removing multiple headers from a default list?

Is it:

SignHeaders *,+header1,header2,...
SignHeaders *,-header1,header2,...

or:

SignHeaders *,+header1,+header2,...
SignHeaders *,-header1,-header2,...

If it's the latter, is this possible?:

SignHeaders *,+header1,-header2

I couldn't easily tell from the code, and I want to put commented-out examples in opendkim.conf.sample to make it clear.

Also, I don't think I should add the feature that any header added to OversignHeaders is automatically added to SignHeaders. It looks like dkimf_db_open() creates and populates a CSL (comma-separated list) database, but there's no easy way to add to an existing database. If it could modify an existing CSL database, I could call it once for OversignHeaders and then call it again for SignHeaders. Doing this would take a better understanding of the code and a better testing environment than I have. And the extra documentation should make it easy enough to get it right.

I didn't even realise that the directives can only be supplied once. It makes sense in general, of course, but I should probably add a mention of this fact in the paragraph describing the *,+hdr and *,-hdr notation. It would be natural to assume that they could be used multiple times.

rafork commented 1 year ago

Rather than wait for an answer to the above (we're all busy), I have just created a pull request that assumes that the *,+header1,-header2 notation works, and shows examples of it in the config sample. If that's wrong, I'll change the pull request.