Hi,
I realized that my last message was missing a part of the
info I had prepared. I'm resending it with a new subject
to make it clearer what it is.
I slightly revised the patch in that it had a unused variable.
I don't know why -Wc-Wunused didn't work with apxs2.
To compile and install the patch:
apxs2 -i -a -c mod_authz_dbd.c
Looking forward to your feedback or RFE. More details here below.
I'm also opening a bug report for the dbd issue I found (and fixed
in this patch).
Cheers,
-jose
DETAILED REPORT
----------------
Work done:
- As discussed earlier in this thread, new require dbd-query directive
for supporting query parameters. If the query returns at least one
row, we consider that access is granted.
- It's now possible to use query parameters for dbd-login and
dbd-logout. If no parameter is given, the default value is
%{REMOTE-USER} to keep backwards compatibility.
- While testing the changes for dbd-login, a server misconfiguration
revealed a SIGSEV. If the dbd handle preparation fails due to a
misconfiguration of the database server or the access rights to the
table, the server logs something along these lines:
AH00632: failed to prepare SQL statements: UPDATE command denied to
user 'foo'@'exampleorg' for table 'bar'
As a consequence, the call to dbd_handle(r) returns NULL. However,
in mod_authz_dbd, there was no control for the value of dbd before
its being used in both authz_dbd_login() and authzdbd_group():
ap_dbd_t *dbd = dbd_handle(r);
...
query = apr_hash_get(dbd->prepared, <--- SIGSEV
I added a check to protect against this case.
Open issues:
- I wanted to control that the number of bind arguments correspond to
those in the prepared request. However this value is not visible in
apr_dbd_prepared_t *query. Looking at the code in apr_dbd.c, I see
that even though the number of bind arguments are being passed to
apr_dbd_pselect() and apr_dbd_pquery() in nargs, this argument is
discarded before calling the implementation of those functions in
the driver.
At least in the mysql driver, there is no further check that there
is an equivalence between the bind and prepared statement
arguments. In both cases there is just a call to
mysql_stmt_bind_param(statement->stmt, bind);
which expects bind to have the correct number of arguments. I
checked the sqlite3 driver and it has the same issue.
If bind has more arguments, there is no issue. If there is less, we
risk a SIGSEV or unexpected behavior.
To avoid this issue, I wanted to control the number of arguments
when parsing the configuration file and show a configuration error
when necessary. However, as the prepared query goes into opaque
driver implementations, I don't have access to its nargs value. Are
there any ideas on how to get this info? The only hack I could
think of is counting the number of % inside the prepared query and
making sure that we have the corresponding number of bind arguments.
- Would it be worth it to connect this module to mod_socache.c?
Other questions:
- In function dbd_parse_config, I was unable to decide whether it
would be ok to use cmd->temp_pool instead of cmd->pool in my call to
ap_getword_white()
- I am using ap_expr_parse_cmd() on each argument of the require
dbd-query, dbd-login, and dbd-logout directives. I think this is
akin to building an expression tree for each argument. Is there a
more efficient way to do so?
Index: /tmp/httpd-trunk/modules/aaa/mod_authz_dbd.c
===================================================================
--- /tmp/httpd-trunk/modules/aaa/mod_authz_dbd.c (revision 1676575)
+++ /tmp/httpd-trunk/modules/aaa/mod_authz_dbd.c (working copy)
@@ -109,7 +109,45 @@
{NULL}
};
+static int evaluate_query_parameters(request_rec *r,
+ const apr_array_header_t *parsed_require_args,
+ const void **query_parameters)
+{
+ int i;
+ apr_array_header_t *qp;
+
+ const ap_expr_info_t *expr = NULL;
+ const char *parameter;
+
+ const char *err = NULL;
+
+ /* evaluate the query parameters in parsed_require_args */
+ qp = apr_array_make(r->pool,
+ parsed_require_args->nelts,
+ sizeof (char *));
+
+ for (i = 0; i < parsed_require_args->nelts; i++) {
+
+ expr = ((const ap_expr_info_t **)parsed_require_args->elts)[i];
+ parameter = ap_expr_str_exec(r, expr, &err);
+
+ if (err) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO()
+ "authz_dbd in evaluate query_parameters: Can't "
+ "evaluate require expression: %s", err);
+ return HTTP_INTERNAL_SERVER_ERROR;
+ }
+
+ *(const char **)apr_array_push(qp) = parameter;
+ }
+
+ *query_parameters = (void *)qp;
+
+ return OK;
+}
+
static int authz_dbd_login(request_rec *r, authz_dbd_cfg *cfg,
+ const void *parsed_require_args,
const char *action)
{
int rv;
@@ -120,12 +158,18 @@
apr_dbd_prepared_t *query;
apr_dbd_results_t *res = NULL;
apr_dbd_row_t *row = NULL;
+ apr_array_header_t *query_parameters;
if (cfg->query == NULL) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01642)
"No query configured for %s!", action);
return HTTP_INTERNAL_SERVER_ERROR;
}
+ if (dbd == NULL) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO()
+ "No db handle available for %s! Check your database access", action);
+ return HTTP_INTERNAL_SERVER_ERROR;
+ }
query = apr_hash_get(dbd->prepared, cfg->query, APR_HASH_KEY_STRING);
if (query == NULL) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01643)
@@ -133,8 +177,15 @@
return HTTP_INTERNAL_SERVER_ERROR;
}
- rv = apr_dbd_pvquery(dbd->driver, r->pool, dbd->handle, &nrows,
- query, r->user, NULL);
+ rv = evaluate_query_parameters(r, parsed_require_args, (void *)&query_parameters);
+ if (rv != OK) {
+ return HTTP_INTERNAL_SERVER_ERROR;
+ }
+
+ rv = apr_dbd_pquery(dbd->driver, r->pool, dbd->handle, &nrows,
+ query,
+ query_parameters->nelts,
+ (const char **)query_parameters->elts);
if (rv == 0) {
if (nrows != 1) {
ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01644)
@@ -194,6 +245,68 @@
return OK;
}
+static int authz_dbd_query(request_rec *r, authz_dbd_cfg *cfg,
+ const apr_array_header_t *query_parameters,
+ apr_array_header_t *query_result_rows)
+{
+ /* SELECT group FROM authz WHERE col = %s, col = %s, ... */
+ int rv;
+ const char *message;
+ ap_dbd_t *dbd = dbd_handle(r);
+ apr_dbd_prepared_t *query;
+ apr_dbd_results_t *res = NULL;
+ apr_dbd_row_t *row = NULL;
+ const char **query_result_row;
+
+ if (cfg->query == NULL) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO()
+ "No query configured for dbd-query!");
+ return HTTP_INTERNAL_SERVER_ERROR;
+ }
+ if (dbd == NULL) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO()
+ "No db handle available for dbd-query! Check your database access");
+ return HTTP_INTERNAL_SERVER_ERROR;
+ }
+ query = apr_hash_get(dbd->prepared, cfg->query, APR_HASH_KEY_STRING);
+ if (query == NULL) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO()
+ "Error retrieving query for dbd-query!");
+ return HTTP_INTERNAL_SERVER_ERROR;
+ }
+
+ rv = apr_dbd_pselect(dbd->driver, r->pool, dbd->handle, &res,
+ query, 0,
+ query_parameters->nelts,
+ (const char **)query_parameters->elts);
+
+ if (rv == 0) {
+ for (rv = apr_dbd_get_row(dbd->driver, r->pool, res, &row, -1);
+ rv != -1;
+ rv = apr_dbd_get_row(dbd->driver, r->pool, res, &row, -1)) {
+ if (rv == 0) {
+ query_result_row = apr_array_push(query_result_rows);
+ *query_result_row = apr_dbd_get_entry(dbd->driver, row, 0);
+ }
+ else {
+ message = apr_dbd_error(dbd->driver, dbd->handle, rv);
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO()
+ "authz_dbd dbd_query in get_row; query for user=%s [%s]",
+ r->user, message?message:noerror);
+ return HTTP_INTERNAL_SERVER_ERROR;
+ }
+ }
+ }
+ else {
+ message = apr_dbd_error(dbd->driver, dbd->handle, rv);
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO()
+ "authz_dbd, in dbd_query query for %s [%s]",
+ r->user, message?message:noerror);
+ return HTTP_INTERNAL_SERVER_ERROR;
+ }
+ return OK;
+}
+
static int authz_dbd_group_query(request_rec *r, authz_dbd_cfg *cfg,
apr_array_header_t *groups)
{
@@ -211,6 +324,11 @@
"No query configured for dbd-group!");
return HTTP_INTERNAL_SERVER_ERROR;
}
+ if (dbd == NULL) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO()
+ "No db handle available for dbd-group! Check your database access");
+ return HTTP_INTERNAL_SERVER_ERROR;
+ }
query = apr_hash_get(dbd->prepared, cfg->query, APR_HASH_KEY_STRING);
if (query == NULL) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01650)
@@ -246,6 +364,42 @@
return OK;
}
+static authz_status dbdquery_check_authorization(request_rec *r,
+ const char *require_args,
+ const void *parsed_require_args)
+{
+ int rv;
+
+ const apr_array_header_t *parsed_require_args_array = parsed_require_args;
+ apr_array_header_t *query_parameters;
+ apr_array_header_t *query_result_rows = NULL;
+
+ authz_dbd_cfg *cfg = ap_get_module_config(r->per_dir_config,
+ &authz_dbd_module);
+
+ if (!r->user) {
+ return AUTHZ_DENIED_NO_USER;
+ }
+
+ rv = evaluate_query_parameters(r, parsed_require_args, (void *)&query_parameters);
+ if (rv != OK) {
+ return HTTP_INTERNAL_SERVER_ERROR;
+ }
+
+ query_result_rows = apr_array_make(r->pool, 4, sizeof(const char*));
+ rv = authz_dbd_query(r, cfg, query_parameters, query_result_rows);
+ if (rv != OK) {
+ return HTTP_INTERNAL_SERVER_ERROR;
+ }
+
+ /* if we get at least one row result, consider the request is granted */
+ if (query_result_rows->nelts) {
+ return AUTHZ_GRANTED;
+ }
+
+ return AUTHZ_DENIED;
+}
+
static authz_status dbdgroup_check_authorization(request_rec *r,
const char *require_args,
const void *parsed_require_args)
@@ -305,8 +459,8 @@
if (!r->user) {
return AUTHZ_DENIED_NO_USER;
}
-
- return (authz_dbd_login(r, cfg, "login") == OK ? AUTHZ_GRANTED : AUTHZ_DENIED);
+
+ return (authz_dbd_login(r, cfg, parsed_require_args, "login") == OK ? AUTHZ_GRANTED : AUTHZ_DENIED);
}
static authz_status dbdlogout_check_authorization(request_rec *r,
@@ -320,15 +474,70 @@
return AUTHZ_DENIED_NO_USER;
}
- return (authz_dbd_login(r, cfg, "logout") == OK ? AUTHZ_GRANTED : AUTHZ_DENIED);
+ return (authz_dbd_login(r, cfg, parsed_require_args, "logout") == OK ? AUTHZ_GRANTED : AUTHZ_DENIED);
}
-static const char *dbd_parse_config(cmd_parms *cmd, const char *require_line,
- const void **parsed_require_line)
+static const char *dbd_parse_config_query_parameters(cmd_parms *cmd,
+ const char *require_line,
+ const void **parsed_require_line)
{
const char *expr_err = NULL;
ap_expr_info_t *expr;
+ apr_array_header_t *expr_array = NULL;
+
+ const char *t;
+ char *w;
+
+ /* parse each element of the require line and store it in an
+ individual table entry. We will evaluate them in that order
+ when passing the parameters to the db query */
+ t = require_line;
+ while ((w = ap_getword_white(cmd->pool, &t)) && w[0]) {
+
+ expr = ap_expr_parse_cmd(cmd, w, AP_EXPR_FLAG_STRING_RESULT,
+ &expr_err, NULL);
+
+ if (expr_err)
+ return apr_pstrcat(cmd->temp_pool,
+ "Cannot parse expression in require line: ",
+ expr_err, NULL);
+
+ if (expr_array == NULL) {
+ expr_array = apr_array_make(cmd->pool, 1,
+ sizeof(const ap_expr_info_t *));
+ }
+ *(const ap_expr_info_t **)apr_array_push(expr_array) = expr;
+ }
+
+ *parsed_require_line = expr_array;
+
+ return NULL;
+}
+static const char *dbd_parse_config_session(cmd_parms *cmd,
+ const char *require_line,
+ const void **parsed_require_line)
+{
+ const char *t;
+
+ /* to preserve backwards compatibility, if the require line has no
+ arguments, use REMOTE_USER by default. */
+ if (require_line[0] == '\0') {
+ t = apr_pstrdup (cmd->pool, "%{REMOTE_USER}");
+ }
+ else {
+ t = require_line;
+ }
+
+ return dbd_parse_config_query_parameters (cmd, t, parsed_require_line);
+}
+
+static const char *dbd_parse_config_groups(cmd_parms *cmd, const char *require_line,
+ const void **parsed_require_line)
+{
+ const char *expr_err = NULL;
+ ap_expr_info_t *expr;
+
expr = ap_expr_parse_cmd(cmd, require_line, AP_EXPR_FLAG_STRING_RESULT,
&expr_err, NULL);
@@ -342,27 +551,37 @@
return NULL;
}
+static const authz_provider authz_dbdquery_provider =
+{
+ &dbdquery_check_authorization,
+ &dbd_parse_config_query_parameters,
+};
+
static const authz_provider authz_dbdgroup_provider =
{
&dbdgroup_check_authorization,
- &dbd_parse_config,
+ &dbd_parse_config_groups,
};
static const authz_provider authz_dbdlogin_provider =
{
&dbdlogin_check_authorization,
- NULL,
+ dbd_parse_config_session,
};
static const authz_provider authz_dbdlogout_provider =
{
&dbdlogout_check_authorization,
- NULL,
+ dbd_parse_config_session,
};
static void authz_dbd_hooks(apr_pool_t *p)
{
+ ap_register_auth_provider(p, AUTHZ_PROVIDER_GROUP, "dbd-query",
+ AUTHZ_PROVIDER_VERSION,
+ &authz_dbdquery_provider,
+ AP_AUTH_INTERNAL_PER_CONF);
ap_register_auth_provider(p, AUTHZ_PROVIDER_GROUP, "dbd-group",
AUTHZ_PROVIDER_VERSION,
&authz_dbdgroup_provider,
