round 2 of mod_authn_mysql

[Paul Querna <ch...@cyan.com>]

I have put an updated version of mod_auth_mysql onto http://open.cyanworlds.com/

Changes from Initial Release:
+ Removed evil signal() calls
+ Changed Config Style
+ Added Database Pooling
+ Added Multiple Database Connections
+ General Cleanup

TODO:
- Make Database Pool Size config options (right now they are hard coded!)
- General Cleanup
- Make Configuring Not so "hackish"
- Add Much Better Error Detection/Logging.
- Add end user SQL query as suggested on apache-dev
- Test Scalling/Stability

I e-mailed davida@mysql.com as was suggested regardling licensing, but I
haven't heard anything back from him yet

comments?

-chip

Re: round 2 of mod_authn_mysql

[Dirk-Willem van Gulik <di...@webweaving.org>]

On Tue, 18 Feb 2003, Paul Querna wrote:

> DW> mysql_init can return a NULL;

> According the MySQL Website and documentaion, only
> mysql_real_connect/mysql_connect are not thread safe.

Ack - I had a bang around the close - must have been something else.

> I can't find any function called "apr_psnprintf" in the apr documentation, and
> grep`ing the APR source I couldn't find it either.  It doesn't look like there
> is a apr_psprintf implmetation that checks the size.

Hmm - yes you are 100% right. My bad. Only psprintf. and snprintf but not
the combination. Shame. Big shame :-)

> I have added Error Checking to it now.  I haven't seen any APR wrapper for it,

Nope; that never got written. Just checked.

> and I am using tmpnam in the same way mod_auth_digest creates its own global
> mutex. ( modules/aaa/mod_auth_digest.c - line 329 )

Right - but somewhere else may be just as wrong :-) I've added it to my
little todo list to look at one time (not so soon :-(.

> I am still waiting to hear anything from DavidA@mysql.com regarding the
> licensing issues of linking to the libmysqlclient. I would like to get them
> sorted out soon.

Dw.

Re: round 2 of mod_authn_mysql

[Paul Querna <ch...@cyan.com>]

DW> mysql_init can return a NULL;  

Added a check for this in 0.0.5 ( now on http://open.cyanworlds.com )

DW> are we sure that mysql_close is 
DW> thread safe ? 

According the MySQL Website and documentaion, only
mysql_real_connect/mysql_connect are not thread safe.

DW> And I'd make the psprintf's into 'psnprintf's with a,
DW> say 1-2k limit as some of the fields may be under potential 
DW> malicious http-wire or .htaccess control (note the NAME_LEN and a 
DW> few others in mysql.h or mysql_com.h); mysql does little checking 
DW> afaik and simply barfs/cores.

I can't find any function called "apr_psnprintf" in the apr documentation, and
grep`ing the APR source I couldn't find it either.  It doesn't look like there
is a apr_psprintf implmetation that checks the size.

DW> this also has another issue; a local user could cause apache to 
DW> create a -lot- of connections to the database with rogue .htaccess 
DW> files. Not sure that that is -really- an issue. But given that 
DW> you've very nicely mutexed all the connects; a simple counter may 
DW> help.

I have added a MYSQL_HARD_MAX_CONNS for a total Hard Max.  Ive set it to 255
for now, as i don't see anyone needing that many concurant mysql connections.
(and if they do, the source is always there) 

DW> init_authn_mysql
DW> 	tmpnam()	-> no error trapping
DW> 
DW> Trusted Solaris barfed on this without it being clear that this was the
DW> cause of my problem. It also makes a file in some random location; 
DW> did apr_ not have a nice version of it which has some more control 
DW> for the admin over where ? Or was that never written ?

I have added Error Checking to it now.  I haven't seen any APR wrapper for it,
and I am using tmpnam in the same way mod_auth_digest creates its own global
mutex. ( modules/aaa/mod_auth_digest.c - line 329 )

I don't have a Win32 build enviroment for Apache2.1 and mod_authn_mysql, if
anyone could try it out and give me feedback on it, that would be great.

I am still waiting to hear anything from DavidA@mysql.com regarding the
licensing issues of linking to the libmysqlclient. I would like to get them
sorted out soon.

thank you for the comments.

-chip

Re: round 2 of mod_authn_mysql

[Dirk-Willem van Gulik <di...@webweaving.org>]

Your 0.6 version works splendidly. Does not seem to leak (well; less than
I can detect as I have other stuff leaking) and survives restarts and
other harrasment well.

Last nit:

	#define MYSQL_HARD_MAX_CONNS (255)
to
	#ifndef MYSQL_HARD_MAX_CONNS
	#define MYSQL_HARD_MAX_CONNS (255)
	#endif

would allow for makefile override. You also *may* (not sure) be able to
easily use apr_strerror() to get a human readable error out of the APR;
rather than report a %d. Though that may require a buffer and other
expensive pain which is prohibitive.

Dw

Re: round 2 of mod_authn_mysql

[Dirk-Willem van Gulik <di...@webweaving.org>]

> I have put the version(0.0.3) with these and a couple over small changes on
> http://open.cyanworlds.com

Compiles and Works for me gov.

Few minor nits below. Feel free to ignore. Nothing major.

mysql_init can return a NULL;  are we sure that mysql_close is thread safe
? And I'd make the psprintf's into 'psnprintf's with a, say 1-2k limit as
some of the fields may be under potential malicious http-wire or .htaccess
control (note the NAME_LEN and a few others in mysql.h or mysql_com.h);
mysql does little checking afaik and simply barfs/cores.

#define MYSQL_MAX_QUERY_STRING (1024)
...
    if (conf->rec.isactive_field) {
        query = apr_psnprintf(r->pool, MYSQL_MAX_QUERY_STRING,
		"SELECT %s FROM %s WHERE %s='%s' AND %s!=0 LIMIT 0,1",
                                         conf->rec.password_field, conf->rec.mysql_table,
                                         conf->rec.username_field, esc_user, conf->rec.isactive_field);

this also has another issue; a local user could cause apache to create a
-lot- of connections to the database with rogue .htaccess files. Not sure
that that is -really- an issue. But given that you've very nicely mutexed
all the connects; a simple counter may help. Though file descriptors would
run out early I'd imagine. But then again; I could imagine this not being
an issue at all. If you where -really- paranoid you could do another
sanity check on (m/A-Z\-_0-9/ && len<NAME_LENGTH) on the conf->rec.*
fields. Also - mysql - is quite happy with 0x01 and \n's and stuff like
UTF8 in its '' fields ??

init_authn_mysql
	tmpnam()	-> no error trapping

Trusted Solaris barfed on this without it being clear that this was the
cause of my problem. It also makes a file in some random location; did
apr_ not have a nice version of it which has some more control for the
admin over where ? Or was that never written ?

Dw.

Re: round 2 of mod_authn_mysql

[Paul Querna <ch...@cyan.com>]

On Mon, 17 Feb 2003 23:02:39 +0100 (CET), Dirk-Willem van Gulik wrote
> And or change the apr_pstrcat into things like select "%s" from %s
> with an apr_pstrNprintf( with a nice limit; as some of the values 
> are from potentially doggy sources; such as .htaccess file made by 
> possibly hostile users and from the network.

changed to apr_psprintf, and added LIMIT to the queries.

I supose another thing to put in the documentation is to say to create a
seperate MySQL user just for this, and set them to have ONLY SELECT permissions.

> Aye - you want to triple/double check your mysql_free()'s I'd guess. 
> Or have one exit after the claim you go to with an 'return e' set to 
> AUTH_USER_X Y or Z. Just to make it a bit more defensive. Or wrap 
> inside a function or soemthing :-).

yep, changed it to set an authn_status, and then do the mysql_free and
releasing the server in one spot.

I have put the version(0.0.3) with these and a couple over small changes on
http://open.cyanworlds.com

thanks for the suggestions.

-chip

Re: round 2 of mod_authn_mysql

[Dirk-Willem van Gulik <di...@webweaving.org>]

On Mon, 17 Feb 2003, Paul Querna wrote:

> - Add end user SQL query as suggested on apache-dev

And or change the apr_pstrcat into things like select "%s" from %s
with an apr_pstrNprintf( with a nice limit; as some of the values are from
potentially doggy sources; such as .htaccess file made by possibly hostile
users and from the network.

> - Test Scalling/Stability

Aye - you want to triple/double check your mysql_free()'s I'd guess. Or
have one exit after the claim you go to with an 'return e' set to
AUTH_USER_X Y or Z. Just to make it a bit more defensive. Or wrap inside a
function or soemthing :-).

You could also move/remove some of your DEBUG_AUTH_MYSQL to a APLOG_DEBUG;
that may make more sense ?

Dw,