Bug 58025 - mod_authz_dbd : new dbd-query and AuthzDBDQuery options
Summary: mod_authz_dbd : new dbd-query and AuthzDBDQuery options
Status: NEW
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: mod_authz_dbd (show other bugs)
Version: 2.5-HEAD
Hardware: PC Linux
: P2 enhancement (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
URL: http://mail-archives.apache.org/mod_m...
Keywords: PatchAvailable
Depends on:
Reported: 2015-06-11 15:14 UTC by Jose Kahan
Modified: 2015-11-18 11:50 UTC (History)
0 users

mod_authz_dbd.c.diff patch adding dbd-query / AuthzDBDQuery options (11.28 KB, patch)
2015-06-11 15:14 UTC, Jose Kahan
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jose Kahan 2015-06-11 15:14:24 UTC
Created attachment 32809 [details]
mod_authz_dbd.c.diff patch adding dbd-query / AuthzDBDQuery options

This patch implements the enhancement proposed by Graham Leggett on the apache-dev list[1]. It is tested and fully working against 2.4.10.

Usage examples:

 AuthzDBDQuery "UPDATE authn SET uri = %s, method = %s WHERE user = %s”

 Require dbd-logout %{TIME} %{REMOTE_USER}
 AuthzDBDQuery "UPDATE authn SET logout_time = %s WHERE user = %s”

 To be backwards compatible, "Require dbd-login” on it’s own would imply "Require dbd-login %{REMOTER_USER}”.

One possible approach to support completely generic queries might be as follows:

 Require dbd-query %{REQUEST_URI} %{REMOTE_USER}
 AuthzDBDQuery “SELECT count(user) FROM authn WHERE uri=%s AND user = %s”

Although I developed and tested this against the 2.4.10 source
tree (the latest debian jessie version), I checked and this
module as well as mod_dbd.c have not changed since then neither
in the latest 2.4.x nor in trunk. I don't think there will be
an issue running it there.  Installing that environment over the
default debian one could be a hazzle for me that I'd prefer
to avoid , but that I could do if needed.

To compile and install the patch (after merging):

         apxs2  -i -a -c mod_authz_dbd.c

[1] http://mail-archives.apache.org/mod_mbox/httpd-dev/201504.mbox/%3C304DB914-ED71-4476-B875-F1ABDD1E370C@sharp.fm%3E

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 not directly related to this patch, but which is good to keep in mind:

- 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 re implementation of the patch that may be modified:

- 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

- 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?
Comment 1 Jose Kahan 2015-11-18 11:50:28 UTC
The related SIGSEV issue I reported and sent a separate patch for has been resolved:https://bz.apache.org/bugzilla/show_bug.cgi?id=57868

I integrated those fixes into my patch for the extension of mod_authz_dbd to use database query parameters. In order to facilitate its tracking and sharing with other people who are interested on it, as well as having a visual diff from the version shipped in apache, I created a repository on github for it:


Feel free to contact me if you are interested in adding this enhancement to apache and require further work on it.