On Sat, Mar 30, 2013 at 10:29 AM, Jeremy Harris <jgh@???> wrote:
>> I have finished coding up my first draft of DMARC support into Exim
>> I welcome anybody to look over the code, to test it, etc.
>
> acl.c
> dmarc_has_been_checked variable
> type should be bool.
Fixed.
> is the operation safe for multiple mails per smtp conn? (also ~enable_forensic et.al.).
It should be, but thinking about it, I do not think I have tested
that. On the TODO list.
> case ACLC_DMARC_STATUS (also ACLC_DKIM_STATUS
> (style nit) space after comma for function args.
It looks weird when I put spaces in, but I'll do it if it's considered
harsh on the eyes.
> dmarc.c
> general
> style - function return type on separate line, zero first-level indent
> of code within function, to match the rest of exim coding.
I asked (I think it was) Phil about it a long while back, and he said
(my recollection and paraphasing) "if you edit someone else's file,
follow whatever convention they used, but if you own/create the file,
use whatever works best for you." It's trivial to change if that
position is not popular.
> dmarc_init()
> style - function opening { on new line.
Fixed.
> Why ifdef EXPERIMENTAL_SPF ? Doesn't the whole facility depend on it?
Yeah, it really is kinda useless without it. I have modified the
dmarc.c so that it will #error with a complaint about missing SPF if
it's not present, and it will #error with a complaint about missing
DKIM if it's not present.
> Should the error cases shortcircuit further processing?
The code checks at various points for the variable dmarc_abort, which
is set if there is a problem detected. Were there points you think I
omitted that check and it needs to be added in places?
> Could the function type be void? Also other functions in file.
It _could_ I suppose. The intent was to be able to return status
codes that could cause skipping subsequent calls of the function. But
if I'm tracking state with dmarc_abort, it's wasteful to check on both
ends (before the function call, and in the function call).
> "Skip DMARC if connection is SMTP Auth" - Discuss. Should this be
> hardwired in the code, is it specified by a reference standard? Could
> the test, if retained, be earlier in the function?
In the RFC:
This memo defines Domain-based Message Authentication, Reporting and
Compliance (DMARC), a mechanism by which email operators leverage
existing authentication and policy advertisement technologies to
enable both message-stream feedback and enforcement of policies
against unauthenticated email.
The word "authenticated" is overloaded in this document to mean
multiple things, but in this paragraph, I interpreted it to strictly
mean I needed to exclude smtp authenticated submissions.
> switch(libdbm_status) - consider a tabled-based coding.
Ok, I liked the switch, but I'll work on a table.
> "DMARC results:" log line - doesn't look easy to correlate with other log lines
> for the message, on a busy server. Does it get the exim-id also?
No it does not log the exim id. It was done exactly the same as the
DKIM logline because this dmarc capability operates in much the same
way as dkim (dmarc is fully processed at first instantiation, dkim is
fully processed at end of message, before DATA acl begins). If
necessary, this log output can be enhanced with the exim id, or
enabled/disabled with a global variable called, for example,
dmarc_summary_line.
> Library shutdown calls - how expensive are these? Could they be opened
> by the exim daemon process and just used by the child process(es) for the
> message (are the libs safe for that)?
The library is threadsafe, but Murray advised me that the forking
model was not safe.
> dmarc_write_history_file()
> Why is history_buffer global?
I think at one point I moved a couple of variables to be module scoped
and I must have snagged this one too. I changed it to be declared in
the function it is used in.
> Why is history_file_fd static?
> Multiple string_sprintf() calls looks inefficient.
>
> receive.c
> #ifndef DISABLE_DKIM expansion to line 3429 - defining DISABLE_DKIM
> now disables important code... Looks buggy. Has the testsuite been run?
Fixed, closing #endif moved up.
Honestly the test suite has been somewhat of an enigma for me. The
SPF and DKIM libraries require live lookups, whereas I could stuff dns
values into the DMARC library. Phil advised that live lookups were
better so long as we controlled the data it looked up, so that's what
I tried to use (first tried against my own dns server). But when it
came right down to it, the exact same commands run under the test
suite performed differently WRT dkim/dmarc from when I ran the same
command manually. I never did zero in on the problem, so I pushed it
back to "look at it later" status. If no test suite is a show stopper
for an experimental feature, I'll have to delve more deeply into the
behavior I was seeing.
I've got more to work on. Resuming tomorrow.
...Todd
--
The total budget at all receivers for solving senders' problems is $0.
If you want them to accept your mail and manage it the way you want,
send it the way the spec says to. --John Levine
