Re: proposed API breaks with async DNS

From: Daniel Black <daniel.subs_at_internode.on.net>
Date: Tue, 3 Nov 2009 21:02:57 +1100

On Tuesday 03 November 2009 19:15:55 Murray S. Kucherawy wrote:
> > -----Original Message-----
> > Some are aware that I'm developing some libopendkim code to consolidate
> > DNS querying and make it asynchronous. The plan is for queries for ADSP,
> > DKIM- Signatures and reputation to occur as soon as the relevant header
> > is passed in for processing.
>
> One thing to consider is that an author signature that validates removes
> the need for an ADSP query, so starting one as soon as From: arrives might
> generate work that's not actually needed. It's impossible to say yet how
> often this might occur, so I've no idea whether to say it's a good idea or
> not-so-much.

1. I guess its a latency vs effort trade off. Probably easy enough to make it
compile time configurable
2. hopefully statistics reporting can answer this better.

> > #1.
> >
> > As this will make policy and signature information available earlier
> > would you like to set call back functions for these?
> >
> > e.g.
> >
> > DKIM_STAT dkim_set_signature_callback
> > DKIM_LIB *libopendkim,
> > DKIM_CBSTAT (*func)(DKIM *dkim, DKIM_SIGINFO *sigs,
> > DKIM_SIGERROR
> > status)
> > );
> >
> > will do a calllback as soon as something invalid is known about the
> > signature or DKIM_SIGERROR_OK - if all is finished and the signature
> > validates.
>
> Is the idea that the resolver, upon receiving a reply, would process the
> signature validation itself rather than just returning a DNS reply and
> letting libopendkim do the parsing? I'm wondering if we're blurring the
> demarcation between the resolver and libopendkim if so.
it goes beyond DNS resolution and extends to all signature errors.

http://gitorious.org/opendkim/libopendkim/blobs/asyncdns/dkim.c should give
you some idea I what I mean. Look for the dkim_sig_error function calls.

> If not, the resolver would need a callback to perform reply processing once
> it's available,
function dkim_process_key
> and it would have to handle timeouts as well (call the
> callback with a specific result code, maybe?),
buried in the DKIM_DNS structure that I haven't written the code for either.
> and a condition variable of
> some kind so that the requesting thread can block and be awakened when the
> reply arrives.

this is the bit I haven't done yet. I was thinking this conditional wait would
also handle the keep alive callbacks. Some locking is also needed to prevent
memory deallocation while in the callbacks to the application.

> > #3.
> >
> > The result of this is that DKIM-Signature header fields need to be
> > parsed earlier and, than unlike previous version, allocation of a
> > continuous block of signatures is not practical.
>
> I'm not sure I see the connection between earlier parsing and the necessity
> for linked lists.

the total number of DKIM signatures isn't known in the dkim_header() function.

> Can you describe what you have in mind there?
Make DKIM_SIGINFO a linked list. dkim_header adds to the list. Retrieving the
list give the DKIM_SIGINFO of the header with a dkim_sig_next(DKIM_SIGINFO *)
function to return the next element.

Some callbacks like prescreen could just pass a single signature for the
caller's evaluation.

http://gitorious.org/opendkim/libopendkim/blobs/asyncdns/dkim-types.h#line193

> I'm a
> little sensitive about changing the API there because there are some large
> sites that use this code as-is, so minimal changes to the existing public
> data structures and the API would be preferable.

To preserve API maximum signatures supported could be set or some
reallocation after a preset number is seen.

Speaking of public data structures, we have some variables not visible in the
API that are in the shared library generating potential linker conflicts (nm
nm -g --defined-only ./libopendkim/.libs/libopendkim.so | fgrep -v dkim_

libopendkim_la_LDFLAGS = -export-symbols-regex '^dkim_.*'

> > #4
> >
> > The current FFR_DKIM_REPUTATION uses the API:
> >
> > DKIM_STAT dkim_get_reputation __P((DKIM *dkim, DKIM_SIGINFO *sig,
> > char *qroot, int *rep));
> >
> >
> > Passing of the qroot value here make async DNS here impossible so here
> > are
> > some options:
> > 1. make qroot a DKIM option
> > 2. make qroot a list of DKIM options (more that one reputation service
> > supported)
>
> These are OK with me. If (2) then "rep" would need to become an array or
> list whose size matches the number of services being queried.

yes. ok. Currently implemented as 1. however to support future API consistency
going down path 2 is probably easier now.

> > 3. remove it and let the application fetch the domain with
> > dkim_sig_getdomain and do its own reputation service.
>
> I'm undecided about what seems best here. On one hand I'd like to support
> DKIM reputation by including it in the library to promote its use, but
> then again so far I've also sought to restrict the library to functions
> that are part of DKIM or ADSP and their extensions.
 
> So maybe it should go in its own reputation library or, as you said, be
> moved directly to the application. As the currently supported reputation
> mechanism is only one experiment with many more to come, maybe
> library-izing it on its own is the right idea.

its boarder line here and easy enough to keep in on the promotion premise.

library-izing may add to latency as the application would need to do the
lookup synchronously and/or handle its own author domain and signature domain
logic (that I've hopefully done already in dkim_header)

> > #5 Namespace consistency:
> >
> > rfc2822_mailbox_split is the only function that doesn't have a dkim_
> > prefix.
> > So that we don't conflict with functions elsewhere that happen to be
> > called
> > the same thing lets use dkim_rfc2822_mailbox_split instead.
>
> This is more for historical reasons (it was borrowed from another library).
> At this point I'm fine with renaming it on the trunk (i.e. in 1.2.0 and
> later).
k.

> > #6 ASYNC DNS API exported
> >
> > The async DNS may provide API for doing your own async DNS for things
> > like
> > VBR, SPF, CSV (insert other three letter email acronym).
> >
> > DKIM_STAT dkim_dns_callback(const char *query, int type, void (*f)(char
> > *result, DKIM_DNSSEC, DKIM_DNSERROR), unsigned timeoutms);
> >
> > here you setup the query and receive a callback when its done. it
> > follows
> > CNAMES etc.
>
> You'll also need a mechanism to see if a query has been completed yet so if
> the requesting code wakes up, it knows if the query is pending, has timed
> out, arrived but was corrupt, is being parsed, or has completed.
parsing would be the application's responsibility. The reset I'm going to need
to write /repackage anyway.

> As I
> suggested, it also needs a mechanism for the caller to go synchronous if
> it has nothing at all to do until the reply arrives or a timeout occurs.
wrapping this behind a simple function is easy enough.
Received on Tue Nov 03 2009 - 10:03:30 PST

This archive was generated by hypermail 2.3.0 : Mon Oct 29 2012 - 23:32:29 PST