Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: pass (nike.apache.org: domain of kaigai@ak.jp.nec.com designates
 202.32.8.193 as permitted sender)
Message-ID: <4A24C6BE.4010803@ak.jp.nec.com>
Date: Tue, 02 Jun 2009 15:29:18 +0900
From: KaiGai Kohei <kaigai@ak.jp.nec.com>
User-Agent: Thunderbird 2.0.0.6 (Windows/20070728)
MIME-Version: 1.0
To: dev@httpd.apache.org
Subject: Re: User/Realm order in AuthDBDUserRealmQuery (mod_authn_dbd)
References: <4A0D1CED.1070004@ak.jp.nec.com> <4A247270.90401@ak.jp.nec.com>
In-Reply-To: <4A247270.90401@ak.jp.nec.com>
Content-Type: multipart/mixed;
 boundary="------------040905080307010502080702"

This is a multi-part message in MIME format.
--------------040905080307010502080702
Content-Type: text/plain; charset=ISO-2022-JP
Content-Transfer-Encoding: 7bit

I filed the patch as:
  https://issues.apache.org/bugzilla/show_bug.cgi?id=47295

Similar implementation shall also be possible on mod_dbd_session.
If necessary, I'll implement it next to the mod_authn_dbd.

--------
This patch adds a two new directives (AuthDBDUserPWQueryFmt,
AuthDBDUserRealmQueryFmt) on mod_authn_dbd.

These options allow to deploy various kind of query parameters (not only
username, password and realm) in discretionary order.
Needless to say, you can use the existing directives, if here is no concerns.

This patch enables to apply mod_authn_dbd on the following cases also.

1. Hardwired parameter order is not suitable for the database.

SELECT md5(uname || ':' || %s || ':' upass) FROM uaccount WHERE uname = %s;

If we want to execute the query (the 1st %s should be realm, and the 2nd %s
should be username) for digest authentication, the hardwired parameter order
is not suitable for the current AuthDBDUserRealmQuery option.

The new AuthDBDUserRealmQueryFmt allows to specify the order as follows:

AuthDBDUserRealmQueryFmt \
    "SELECT md5(uname || ':' || $(realm) || ':' upass) \
         FROM uaccount WHERE uname = $(username)"

2. Additional conditions more than username/password.

When we want to restrict available users depending on remote address or
other factors, the current directive does not support it.

This patch allows to put $(remote_addr) other than username, password and
realm, as a proof of the concept. It can be used to implement a user who
is available only from local networks, for example.

Thanks,

KaiGai Kohei wrote:
> KaiGai Kohei wrote:
>> I'm now trying to set up mod_authn_dbb for authentication purpose.
>> However, I faced to a concern for AuthDBDUserRealmQuery directive.
>>
>> The example shows the query:
>>   AuthDBDUserRealmQuery \
>>       "SELECT password FROM authn WHERE user = %s AND realm = %s"
>>
>> But, I would like to set up the query as follows:
>>   AuthDBDUserRealmQuery \
>>       "SELECT md5(uname || ':' || %s || ':' || upass) FROM uaccount WHERE uname = %s"
>>                                   ^^... to be realm                to be user ... ^^
>>
>> It seems to me we have no way to put the replacement of the given
>> realm prior to username. Am I missing anything?
> 
> Here, I could find a short hack.
> 
>   AuthDBDUserRealmQuery \
>       "SELECT md5(uname || ':' || $2 || ':' || upass), udomain, %s=%s AS dummy \
>            FROM uaccount WHERE uname = $1"
> 
> The first %s is replaced to '$1' as username, and the second %s is replaced
> to '$2' as a realm, but $n is not touched by mod_dbd.
> The dummy field is just put to consume the parameters in correct order, and
> it refers meaningful parameters with $n.
> 
> However, I don't think it is a straightforward approach. :-(
> 
> Chris Darroch suggested me to add an optional second argument to suggest
> the order of parameters, like:
> 
>   AuthDBDUserRealmQuery \
>     "SELECT md5(uname || ':' || %s || ':' || upass) FROM uaccount \
>          WHERE uname = %s" "realm,username"
> 
> However, my preference is still an inline replacement approach, like:
> 
>   AuthDBDUserRealmQueryFmt \
>     "SELECT md5(uname || ':' || $(realm) || ':' || upass) FROM uaccount \
>          WHERE uname = $(username) and unetwork >>= $(remote_host)::inet"
> 
> Needless to say, the current behavior of AuthDBDUserRealmQuery should be
> kept as is. The new directive only suggest an another way to set up the
> query.
> 
> Chris also mentioned we should use the custome log format as much as possible.
>   http://httpd.apache.org/docs/2.2/mod/mod_log_config.html#formats
> 
> However, Tom Donovan mentioned that the upcoming mod_session_dbd module also
> applies hardwired parameters, and it requires to replace session keys and
> so on. But we could not find these parameters in the formats.
> 
> Therefore, it seems to me these identical marks should be defined independent
> from the custom log format.
> 
> Examples:
>  -- mod_auth_dbd --
>  $(username) ... replaced by %s as the given authenticated username
>  $(password) ... replaced by %s as the given authenticated password
>  $(realm)    ... replaced by %s as the realm string
>  $(remote_addr) ... replaced by %s as the remote address
>  -- mod_session_dbd --
>  $(key)      ... replaced by %s as the session key
>  $(value)    ... replaced by %s as the session value
>  $(expiry)   ... replaced by %lld as the session expity
> 
> Thanks,


-- 
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

--------------040905080307010502080702
Content-Type: text/x-patch;
 name="httpd-mod_authn_dbd-params.2.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="httpd-mod_authn_dbd-params.2.patch"

Index: modules/aaa/mod_authn_dbd.c
===================================================================
--- modules/aaa/mod_authn_dbd.c	(revision 780893)
+++ modules/aaa/mod_authn_dbd.c	(working copy)
@@ -30,13 +30,23 @@
 
 typedef struct {
     const char *user;
+    const char *uparams;
     const char *realm;
+    const char *rparams;
 } authn_dbd_conf;
 typedef struct {
     const char *label;
     const char *query;
 } authn_dbd_rec;
 
+/* replacement in query format */
+#define QUERY_FMT_USERNAME      'u'
+#define QUERY_FMT_PASSWORD      'p'
+#define QUERY_FMT_REALM         'r'
+#define QUERY_FMT_REMOTE_ADDR   'a'
+#define QUERY_FMT_USER_DEFAULT  "p"
+#define QUERY_FMT_REALM_DEFAULT "ur"
+
 /* optional function - look it up once in post_config */
 static ap_dbd_t *(*authn_dbd_acquire_fn)(request_rec*) = NULL;
 static void (*authn_dbd_prepare_fn)(server_rec*, const char*, const char*) = NULL;
@@ -52,7 +62,9 @@
     authn_dbd_conf *base = BASE;
     authn_dbd_conf *ret = apr_palloc(pool, sizeof(authn_dbd_conf));
     ret->user = (add->user == NULL) ? base->user : add->user;
+    ret->uparams = (add->user == NULL) ? base->uparams : add->uparams;
     ret->realm = (add->realm == NULL) ? base->realm : add->realm;
+    ret->rparams = (add->realm == NULL) ? base->rparams : add->rparams;
     return ret;
 }
 static const char *authn_dbd_prepare(cmd_parms *cmd, void *cfg, const char *query)
@@ -74,6 +86,75 @@
     /* save the label here for our own use */
     return ap_set_string_slot(cmd, cfg, label);
 }
+static const char *setup_query_fmt(apr_pool_t *p, const char *query,
+                                   const char **result_params)
+{
+    char   *copy, *pos, *params, *tmp;
+    int     pnums = 0, plimit = 0;
+
+    copy = apr_palloc(p, strlen(query) + 1);
+    params = apr_palloc(p, plimit + 1);
+
+    for (pos = copy; *query != '\0'; query++) {
+        if (*query != '$') {
+            *pos++ = *query;
+            continue;
+        }
+        /* expand parameter buffer */
+        if (pnums == plimit) {
+            plimit += 10;
+            tmp = apr_palloc(p, plimit + 1);
+            strcpy(tmp, params);
+            params = tmp;
+        }
+        /* replace format string to %s */
+        if (strncmp(query, "$(username)", 11) == 0) {
+            query += 10;
+            pos += sprintf(pos, "%%s");
+            params[pnums++] = QUERY_FMT_USERNAME;
+        }
+        else if (strncmp(query, "$(password)", 11) == 0) {
+            query += 10;
+            pos += sprintf(pos, "%%s");
+            params[pnums++] = QUERY_FMT_PASSWORD;
+        }
+        else if (strncmp(query, "$(realm)", 8) == 0) {
+            query += 7;
+            pos += sprintf(pos, "%%s");
+            params[pnums++] = QUERY_FMT_REALM;
+        }
+        else if (strncmp(query, "$(remote_addr)", 14) == 0) {
+            query += 13;
+            pos += sprintf(pos, "%%s");
+            params[pnums++] = QUERY_FMT_REMOTE_ADDR;
+        }
+        else {
+            /* keep the unknown $... */
+            *pos++ = *query;
+        }
+    }
+    *pos = '\0';
+    params[pnums++] = '\0';
+
+    *result_params = params;
+    return copy;
+}
+static const char *set_user_query_fmt(cmd_parms *cmd, void *cfg, const char *query)
+{
+    authn_dbd_conf *conf = cfg;
+    const char *new_query
+        = setup_query_fmt(cmd->pool, query, &conf->uparams);
+    /* do the normal setups */
+    return authn_dbd_prepare(cmd, cfg, new_query);
+}
+static const char *set_realm_query_fmt(cmd_parms *cmd, void *cfg, const char *query)
+{
+    authn_dbd_conf *conf = cfg;
+    const char *new_query
+        = setup_query_fmt(cmd->pool, query, &conf->rparams);
+    /* do the normal setups */
+    return authn_dbd_prepare(cmd, cfg, new_query);
+}
 static const command_rec authn_dbd_cmds[] =
 {
     AP_INIT_TAKE1("AuthDBDUserPWQuery", authn_dbd_prepare,
@@ -82,8 +163,49 @@
     AP_INIT_TAKE1("AuthDBDUserRealmQuery", authn_dbd_prepare,
                   (void *)APR_OFFSETOF(authn_dbd_conf, realm), ACCESS_CONF,
                   "Query used to fetch password for user+realm"),
+    AP_INIT_TAKE1("AuthDBDUserPWQueryFmt", set_user_query_fmt,
+                  (void *)APR_OFFSETOF(authn_dbd_conf, user), ACCESS_CONF,
+                  "Query used to fetch password for user"),
+    AP_INIT_TAKE1("AuthDBDUserRealmQueryFmt", set_realm_query_fmt,
+                  (void *)APR_OFFSETOF(authn_dbd_conf, realm), ACCESS_CONF,
+                  "Query used to fetch password for user+realm"),
     {NULL}
 };
+static void authn_dbd_setup_params(request_rec *r, const char *params,
+                                   const char ***args, int *nargs,
+                                   const char *username,
+                                   const char *password,
+                                   const char *realm)
+{
+    const char **ptable = NULL;
+    int c, index = 0;
+
+    ptable = apr_palloc(r->pool, sizeof(const char *) * strlen(params));
+
+    while ((c = *params++) != '\0') {
+        switch (c) {
+        case QUERY_FMT_USERNAME:
+            ptable[index++] = username;
+            break;
+        case QUERY_FMT_PASSWORD:
+            ptable[index++] = password;
+            break;
+        case QUERY_FMT_REALM:
+            ptable[index++] = realm;
+            break;
+        case QUERY_FMT_REMOTE_ADDR:
+            ptable[index++] = r->connection->remote_ip;
+            break;
+        default:
+            ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
+                          "Bug? unexpected format: %c", c);
+            break;
+        }
+    }
+
+    *args = ptable;
+    *nargs = index;
+}
 static authn_status authn_dbd_password(request_rec *r, const char *user,
                                        const char *password)
 {
@@ -92,6 +214,8 @@
     apr_dbd_prepared_t *statement;
     apr_dbd_results_t *res = NULL;
     apr_dbd_row_t *row = NULL;
+    const char **args;
+    int nargs;
 
     authn_dbd_conf *conf = ap_get_module_config(r->per_dir_config,
                                                 &authn_dbd_module);
@@ -116,8 +240,11 @@
                       "AuthDBDUserPWQuery with the key '%s'", conf->user);
         return AUTH_GENERAL_ERROR;
     }
-    if (apr_dbd_pvselect(dbd->driver, r->pool, dbd->handle, &res, statement,
-                              0, user, NULL) != 0) {
+
+    authn_dbd_setup_params(r, conf->uparams ? conf->uparams : QUERY_FMT_USER_DEFAULT,
+                           &args, &nargs, user, password, NULL);
+    if (apr_dbd_pselect(dbd->driver, r->pool, dbd->handle, &res,
+                        statement, 0, nargs, args) != 0) {
         ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
                       "Query execution error looking up '%s' "
                       "in database", user);
@@ -184,6 +311,8 @@
     apr_dbd_prepared_t *statement;
     apr_dbd_results_t *res = NULL;
     apr_dbd_row_t *row = NULL;
+    const char **args;
+    int nargs;
 
     authn_dbd_conf *conf = ap_get_module_config(r->per_dir_config,
                                                 &authn_dbd_module);
@@ -206,8 +335,11 @@
                       "AuthDBDUserRealmQuery with the key '%s'", conf->realm);
         return AUTH_GENERAL_ERROR;
     }
-    if (apr_dbd_pvselect(dbd->driver, r->pool, dbd->handle, &res, statement,
-                              0, user, realm, NULL) != 0) {
+
+    authn_dbd_setup_params(r, conf->rparams ? conf->rparams : QUERY_FMT_REALM_DEFAULT,
+                           &args, &nargs, user, NULL, realm);
+    if (apr_dbd_pselect(dbd->driver, r->pool, dbd->handle, &res,
+                        statement, 0, nargs, args) != 0) {
         ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
                       "Query execution error looking up '%s:%s' "
                       "in database", user, realm);

--------------040905080307010502080702--