Chris Darroch wrote:
KaiGai Kohei wrote:
I think a new directive with formats support is preferable to
keep compatibility with existing directives.
We definitely need compatibility with existing directives.
That's why I figured the extra parameter would be optional --
if you only provided one parameter, the query, it would be handled
as it is now (i.e., user and realm interpolated, in that order).
Indeed, apache/httpd allows us to handle an extra parameter easily.
The reason why I wondered the extra parameter approch was that I think
the custom log format caracters (like %a, %u) should be included within
the query string. But it is not a fundamental thing. :-)
For example:
AuthDBDUserRealmQueryFmt \
"SELECT md5(uname || ':' || %R || ':' || upass) FROM uaccount \
WHERE uname = %u AND uaddr >>= %a::inet"
When the directive is given, mod_authn_dbd can register the type and
order of the charater to be replaced. Then it can set up as a paramter
list on query execution phase.
I think you'll find that trying to change the format specifiers
(e.g., "%u", "%a::inet", etc.) is not the way to go. These are
handled by APR-util, not mod_authn_dbd. At least in APR trunk,
there's a large set of format specifiers supported and these were
hashed through some time ago (e.g., "%pa" for date/timestamps, etc.)
on the APR lists. I don't think it's a good idea to start adding
more DB query format specifier parsing in httpd.
I din't have a plan to add support for whole of custom log format
and all mod_authn_dbd should do is replacing them to '%s' and register
the appeared order. However, it is not a fundamental thing.
That's why I figured you might stick with APR-util's specifiers
in the query itself, but then provide a comma-delimited list of
specifiers in the optional argument to the AuthDBD* directives.
These would be follow mod_log_config and would represent the various
request-related data one could interpolate into the query.
(E.g., "u" for user, "H" for protocol, "a" for remote address, etc.)
That would provide a vast number of possibilities (more, certainly,
than anyone really needs) but would at least stick to existing
format specifiers as defined for both APR-util DBD and httpd now.
Should I submit a patch to support the feature?
Patches are always welcome (although I'm the first to admit
that I'm too slow at reviewing and committing them).
The attached patch is a proof of the concept.
It allows to specify the order of parameters, as follows:
AuthDBDUserRealmQuery \
"SELECT md5(uname || ':' || %s || ':' || upass) FROM uaccount \
WHERE uname = %s" "realm,user"
^^^^^^^^^^^^
The second argument is optional. If nothing was given, mod_authn_dbd
considers the parameter should be extracted with the default order
("user" for basic authentication, and "user,realm" for digest one).
Currently, it only supports "user", "password", "realm" and "remote_addr".
Its coverage is now far from custom log format, and I'm not clear whether
different stuffs should have similar configuration (may be confusable?),
so this patches uses different token.
Any comments please.
Thanks,
--
KaiGai Kohei <kai...@kaigai.gr.jp>
Index: httpd-2.2.x/modules/aaa/mod_authn_dbd.c
===================================================================
--- httpd-2.2.x/modules/aaa/mod_authn_dbd.c (revision 763638)
+++ httpd-2.2.x/modules/aaa/mod_authn_dbd.c (working copy)
@@ -30,7 +30,9 @@
typedef struct {
const char *user;
+ const char *user_params;
const char *realm;
+ const char *realm_params;
} authn_dbd_conf;
typedef struct {
const char *label;
@@ -51,8 +53,25 @@
authn_dbd_conf *add = ADD;
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->realm = (add->realm == NULL) ? base->realm : add->realm;
+
+ if (add->user == NULL) {
+ ret->user = base->user;
+ ret->user_params = base->user_params;
+ }
+ else {
+ ret->user = add->user;
+ ret->user = add->user_params;
+ }
+
+ if (add->realm == NULL) {
+ ret->realm = base->realm;
+ ret->realm_params = base->realm_params;
+ }
+ else {
+ ret->realm = add->realm;
+ ret->realm_params = add->realm_params;
+ }
+
return ret;
}
static const char *authn_dbd_prepare(cmd_parms *cmd, void *cfg, const char *query)
@@ -74,16 +93,88 @@
/* save the label here for our own use */
return ap_set_string_slot(cmd, cfg, label);
}
+static const char *set_authn_dbd_user(cmd_parms *cmd, void *cfg,
+ const char *query, const char *params)
+{
+ authn_dbd_conf *conf = cfg;
+
+ conf->user_params = (!params ? "user" : params);
+
+ return authn_dbd_prepare(cmd, cfg, query);
+}
+static const char *set_authn_dbd_realm(cmd_parms *cmd, void *cfg,
+ const char *query, const char *params)
+{
+ authn_dbd_conf *conf = cfg;
+
+ conf->realm_params = (!params ? "user,realm" : params);
+
+ return authn_dbd_prepare(cmd, cfg, query);
+}
static const command_rec authn_dbd_cmds[] =
{
- AP_INIT_TAKE1("AuthDBDUserPWQuery", authn_dbd_prepare,
- (void *)APR_OFFSETOF(authn_dbd_conf, user), ACCESS_CONF,
- "Query used to fetch password for user"),
- 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_TAKE12("AuthDBDUserPWQuery", set_authn_dbd_user,
+ (void *)APR_OFFSETOF(authn_dbd_conf, user), ACCESS_CONF,
+ "Query used to fetch password for user"),
+ AP_INIT_TAKE12("AuthDBDUserRealmQuery", set_authn_dbd_realm,
+ (void *)APR_OFFSETOF(authn_dbd_conf, realm), ACCESS_CONF,
+ "Query used to fetch password for user+realm"),
{NULL}
};
+static int authn_dbd_setup_params(request_rec *r, const char *params,
+ const char ***args, int *nargs,
+ const char *user,
+ const char *password,
+ const char *realm)
+{
+ const char **ptable = NULL;
+ const char *delim = ",";
+ char *buffer, *tok, *pos;
+ int index = 0, limit = 0;
+
+ buffer = apr_pstrdup(r->pool, params);
+
+ for (tok = strtok_r(buffer, delim, &pos);
+ tok != NULL;
+ tok = strtok_r(NULL, delim, &pos)) {
+
+ if (index == limit) {
+ const char **new_ptable;
+ int new_limit = limit + 10;
+
+ new_ptable = apr_pcalloc(r->pool, sizeof(const char *) * new_limit);
+ if (ptable != NULL) {
+ memcpy(new_ptable, ptable, sizeof(const char *) * limit);
+ }
+ ptable = new_ptable;
+ limit = new_limit;
+ }
+
+ if (strcasecmp(tok, "user") == 0) {
+ ptable[index++] = user;
+ }
+ else if (strcasecmp(tok, "password") == 0) {
+ ptable[index++] = password;
+ }
+ else if (strcasecmp(tok, "realm") == 0) {
+ ptable[index++] = realm;
+ }
+ else if (strcasecmp(tok, "remote_addr") == 0) {
+ ptable[index++] = r->connection->remote_ip;
+ }
+ else {
+ return -1;
+ }
+ }
+
+ if (index < 1)
+ return -1;
+
+ *args = ptable;
+ *nargs = index;
+
+ return 0;
+}
static authn_status authn_dbd_password(request_rec *r, const char *user,
const char *password)
{
@@ -92,6 +183,8 @@
apr_dbd_prepared_t *statement;
apr_dbd_results_t *res = NULL;
apr_dbd_row_t *row = NULL;
+ const char **args = NULL;
+ int nargs;
authn_dbd_conf *conf = ap_get_module_config(r->per_dir_config,
&authn_dbd_module);
@@ -116,9 +209,15 @@
"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) {
+ if (authn_dbd_setup_params(r, conf->user_params, &args, &nargs,
+ user, password, NULL) < 0) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
+ "Unsupported parameter format '%s'", conf->user_params);
+ return AUTH_GENERAL_ERROR;
+ }
+ 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);
return AUTH_GENERAL_ERROR;
@@ -184,6 +283,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,9 +307,15 @@
"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) {
+ if (authn_dbd_setup_params(r, conf->realm_params, &args, &nargs,
+ user, NULL, realm) < 0) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
+ "Unsupported parameter format '%s'", conf->realm_params);
+ return AUTH_GENERAL_ERROR;
+ }
+ 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);
return AUTH_GENERAL_ERROR;
