From: Keean Schupke (keeanfry-it.com)
Date: Sat Dec 01 2007 - 13:24:16 CST

• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

Comments inline:

On 01/12/2007, Victor Duchovni <Victor.Duchovnimorganstanley.com> wrote:
>
> Given that there is also a "var_cache_service", to allow different
> clients to choose split caches, this seems reasonable. Given the generic
> nature of the cache, the parameter that controls the cache database name
> is mis-named.
>
> #define VAR_CACHE_NAME "smtp_sasl_auth_cache_name"
>
> It should be something like "cache_map" (along the lines of
> address_verify_map). And then in master.cf something along the lines of:
>
> master.cf:
> auth_cache unix ... cache
> -o cache_map=$smtp_auth_cache_map
>
> main.cf:
> # Optional:
> # smtp_auth_cache_map = btree:/var/lib/postfix/smtp_auth_cache
>
> Minor drawback is that the "smtp_auth_cache_map" parameter won't be
> known to "postconf".

This all looks good to me... will do!

>
> Violation of Postfix indentation style... :-) The opening brace of a
> function should be its own line... Variable declarations are followed
> by a blank line and then the code.

Okay, will fix too.

>
> > +static int smtp_sasl_parse_cache_value(char *buf, long *timestamp,
> > + char **respdsn, char **respstr) {
> > + if ((*respdsn = split_at(buf,':')) != 0
> > + && (*respstr = split_at(*respdsn,':')) != 0 && alldig(buf)) {
> > + *timestamp = atol(buf);
> > + return (0);
> > + }
> > + msg_warn("bad smtp_sasl_auth_cache_map entry: %.100s", buf);
> > + return (-1);
> > +}
>
> May want to validate the DSN string here, and perhaps use strtol()
> or sscanf() instead of atol() to check for trailing junk, ...

The alldig(buf) check makes sure there is no trailing junk, but proper
validation would be better.

> > + smtp_sasl_make_cache_key(buf, session->host,
> > session->sasl_username, session->sasl_passwd);
> > + if (cache_client_query(STR(buf), get_buf) == 0) {
> > + if (vstring_strcpy(put_buf,STR(get_buf))
> > + && smtp_sasl_parse_cache_value(STR(get_buf),&timestamp,
> > + &cached_respdsn,&cached_respstr) == 0
> > + && timestamp + var_smtp_sasl_auth_cache_time > now) {
> > + dsb_update(why, cached_respdsn, DSB_DEF_ACTION, DSB_MTYPE_DNS,
> > + session->host, var_procname, cached_respstr,
> > + "SASL [CACHED] authentication failed; server %s : %s",
> > + session->namaddr, STR(put_buf));
> > + status = -1;
> > + } else {
> > + /* timeout or corrupt cache entry */
> > + cache_client_delete(STR(buf));
> > + }
> > + }
> > + vstring_free(put_buf);
> > + vstring_free(get_buf);
> > + vstring_free(buf);
> > + return (status);
> > +}
>
> With SASL soft failures, the DSN should perhaps be downgraded from 5XX
> to 4XX here? And validated somewhere to be either a 5XX or 4XX?
>

The downgrading happens automatically, the cache_update happens after
the resp->dsn has been changed by the soft_bounce patch. So no need to
alter things on the query side.

>
> Here, you should be more selective (code == 535) about which SASL auth
> failures are cached as "bad password" errors. While soft-fail on AUTH
> failure is reason agnostic, blocking all deliveries for the same password
> requires more care.
>

No problem with that... will do

Regards,
Keean Schupke, Fry-IT Ltd.

• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]