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:

 Require dbd-login %{REQUEST_URI} %{REQUEST_METHOD} %{REMOTE_USER}
 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
  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?