You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Jose Kahan <jo...@w3.org> on 2015/04/14 19:38:52 UTC
Extending mod_authz_dbd
Hi,
We're preparing the migration to apache 2.4 and we're happy
to see that many features now available allow us to put to
rest most of our custom developed modules.
Looking at mod_authz_dbd, we would like to make a request
against a dbase passing different parameters (REQUEST_URI,
REQUEST_METHOD, REMOTE_USER, ...) to support a fine-grained
ACL mechanism. However, this module doesn't seem to support
resolving those variables when preparing the SQL query.
Looking at the code, the only one that seems supported is
a hard-coded r->user in authz_dbd_group_query:
rv = apr_dbd_pvselect(dbd->driver, r->pool, dbd->handle, &res,
query, 0, r->user, NULL);
Is there any historical reason for this? Would you be interested
in our contributing time to extend this module to support more
generic queries such as (invented query)
require sql-query "SELECT * FROM foo WHERE user=${REMOTE_USER}
AND uri=${REQUEST_URI} AND ${REQUEST_METHOD} in
('GET', 'POST')"
While browsing for information regarding this point, I only
found people asking if this was possible, but no hints if there
had been (or will be) plans to do this.
The defunct mod_auth_mysql used to propose something similar. i
This module is not officially maintained anymore.
A possible point of confusion in the module's doc [1] is the
phrase saying
"Since v2.4.8, expressions are supported within the DBD
require directives."
while the requires in the paragraphs below can't support expressions,
as far as I can tell. I may be missing something.
Please keep me update if you'd be interested in this generic
contribution, which we would be happy to make, rather than forking
and maintaining a derived and non-generic patch to this module.
If you're interested, I'd appreciate some feedback on how the feature
should look like so that it's designed that way before coding it,
rather than coding, proposing the contribution and getting feedback
it wasn't what x had in mind.
Of course, it's understood that your showing interest for this doesn't
mean you will end up accepting the contribution.
Thanks and KUDOS! Apache 2.4 rocks!
-jose
Re: Extending mod_authz_dbd
Posted by Jose Kahan <jo...@w3.org>.
Hello Graham,
Thanks for your feedback. It all makes sense to me.
Following your reasoning, the only directive that will have i
to stay as is for backwards compatibility will be require
dbd-group, as it expects either no parameter or the value
the query will return.
I had initially thought of only using one directive and
extracting the variables from the AuthzDBDQuery arguments,
substituting them for %s and then preparing the request. Your
proposal is much simpler so I'll go with it.
I'm already looking at the code. Will keep you (and the list)
updated as soon as I have something to share.
Regards,
-jose
On Wed, Apr 15, 2015 at 11:17:04AM +0200, Graham Leggett wrote:
>
> The limitations we’d have to work with is that all the queries are prepared statements, and are reused for multiple requests. At the same time any query that is interpreted purely as a string would need to be protected against SQL injection.
> 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”
>
> The bit above where you’d limit the requests to GET or POST you’d probably do with Limit or LimitExcept.
Re: patch to mod_authz_dbd to handle query parameters
Posted by Jose Kahan <jo...@w3.org>.
Hi,
Not having heard back since submitting this enhancement, I decided
to put it on github to share it with other people who may be
interested by it [1]. I integrated the changes from [2] and used
2.4.17 as the "base version".
My original submission is [3].
Feel free to contact me if you are interested to integrate this
into apache and require further changes.
-jose
[1] https://github.com/jkbzh/apache2_mod_authz_dbd
[2] https://bz.apache.org/bugzilla/show_bug.cgi?id=57868
[3] https://bz.apache.org/bugzilla/show_bug.cgi?id=58025
Re: Re: patch to mod_authz_dbd to handle query parameters
Posted by Jose Kahan <jo...@w3.org>.
Argh, I forgot to include the link to the message! Sorry about
that.
http://mail-archives.apache.org/mod_mbox/httpd-dev/201504.mbox/%3C20150428165148.GA17745@kiribati.inrialpes.fr%3E
-jk
Re: patch to mod_authz_dbd to handle query parameters
Posted by Jose Kahan <jo...@w3.org>.
Hi!
A gentle reminder that I had submitted this patch and would like
to know if someone has had time to review it and/or would like
further modifications.
Should I add a bugzilla entry so that it doesn't fall under the radar?
Thanks! Kudos.
-jk
patch to mod_authz_dbd to handle query parameters
Posted by Jose Kahan <jo...@w3.org>.
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?
Re: Extending mod_authz_dbd
Posted by Jose Kahan <jo...@w3.org>.
Hi Graham,
Here's a first patch that adds the functionality that you suggested.
> 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.
Here below are some development notes.
Please keep me update if I can contribute something else to this
patch, that it be documentation or further changes. I understand
there's no engagement to accept this code from you. We can chat
on irc too if needed.
Cheers,
-jose
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?
Re: Extending mod_authz_dbd
Posted by Graham Leggett <mi...@sharp.fm>.
On 14 Apr 2015, at 7:38 PM, Jose Kahan <jo...@w3.org> wrote:
> require sql-query "SELECT * FROM foo WHERE user=${REMOTE_USER}
> AND uri=${REQUEST_URI} AND ${REQUEST_METHOD} in
> ('GET', 'POST')"
>
> While browsing for information regarding this point, I only
> found people asking if this was possible, but no hints if there
> had been (or will be) plans to do this.
>
> The defunct mod_auth_mysql used to propose something similar. i
> This module is not officially maintained anymore.
>
> A possible point of confusion in the module's doc [1] is the
> phrase saying
>
> "Since v2.4.8, expressions are supported within the DBD
> require directives.”
Hmmm…
The limitations we’d have to work with is that all the queries are prepared statements, and are reused for multiple requests. At the same time any query that is interpreted purely as a string would need to be protected against SQL injection.
One possible way of approaching this would be to extend the dbd-login and dbd-logout require directives with optional expression parameters, which can then be referred to in the prepared statement, so you could do this:
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”
The bit above where you’d limit the requests to GET or POST you’d probably do with Limit or LimitExcept.
Regards,
Graham
—