Hi Joro,
Before I can approve this patch we need to fix one issue - I think there is
a logical error in map_group_to_user() function in the place where group
members are searched. See the FIXME: note below.
Apart from that, treat rest of my comments as suggestions which you can
follow if you like but can also leave things as they are.
Georgi Kodinov wrote:
> #At file:///home/kgeorge/mysql/work/wl5366-5.5/ based on
> revid:georgi.kodinov@stripped
>
> 3082 Georgi Kodinov 2010-08-16
> WL#5366: moved the implementation to the the new mysql-5.5 based WL1054
> tree.
>
> added:
> plugin/authentication_pam/
> plugin/authentication_pam/CMakeLists.txt
> plugin/authentication_pam/Makefile.am
> plugin/authentication_pam/authentication_pam.c
> plugin/authentication_pam/plug.in
> plugin/authentication_pam/safe.h
> plugin/authentication_pam/safe_getgrnam.c
> plugin/authentication_pam/safe_getpwnam.c
> === added directory 'plugin/authentication_pam'
How about shortening the name to 'plugin/auth_pam'?
...
> === added file 'plugin/authentication_pam/Makefile.am'
> --- a/plugin/authentication_pam/Makefile.am 1970-01-01 00:00:00 +0000
> +++ b/plugin/authentication_pam/Makefile.am 2010-08-16 15:38:01 +0000
> @@ -0,0 +1,11 @@
> +pkgplugindir=$(pkglibdir)/plugin
> +
> +AM_LDFLAGS=-module -rpath $(pkgplugindir)
> +AM_CPPFLAGS=-DMYSQL_DYNAMIC_PLUGIN -Wno-pointer-sign -I$(top_srcdir)/include
> +
> +if HAVE_PAM
> +pkgplugin_LTLIBRARIES= authentication_pam.la
> +authentication_pam_la_SOURCES= authentication_pam.c safe_getgrnam.c safe_getpwnam.c
> +endif
> +
> +EXTRA_DIST= CMakeLists.txt plug.in
>
I do not fully understand the role of plug.in file. Where can I get
information about this?
> === added file 'plugin/authentication_pam/authentication_pam.c'
> --- a/plugin/authentication_pam/authentication_pam.c 1970-01-01 00:00:00 +0000
> +++ b/plugin/authentication_pam/authentication_pam.c 2010-08-16 15:38:01 +0000
...
> +
> +
> +/**
> + Walk through the comma separated list of name/value pairs and call a handler
> +
> + @param out authenticated_as an output buffer to pass to the map function.
> + @param in size_authenticated_as length of authenticated_as
> + @param in pam_user the user data to pass to the map function
> + @param in group_mapping the comma separated list of name=value pairs
> + @param in map_group_to_user the mapping function to check and substitute
> + the mysql user name if the current group is
> + found in the groups assigned to the user
If this callback function has so well defined semantics - it should check if
given user belongs to a given group - then why we make it a callback
argument? I did not notice walk_namevalue_list() being used with a different
callback function, so why not to use a normal, static function instead?
> + @param out substitution_was_made set to non-zero when a substitution was made.
> +
> + @return status
> + @retval 0 success
> + @retval 1 error
> +*/
...
> +/***************** os_group=mysql_user list handling *************************/
> +
> +/**
> + Walk through groups assigned to a user and substitute the mysql name if a match
> + is found.
I find this description unclear. My understanding is like this:
A callback function for walk_namevalue_list(). Checks if user described by
'pam_user' is a member of the group given by 'name' parameter. If this is
the case, it indicates a match by setting 'substitution_was_made' flag and
copies the corresponding mysql_user name given in 'value' to 'auth_as'
buffer. Otherwise it sets 'substitution_was_made' to 0.
> +
> + @param out auth_as an output buffer for the resulting user name.
> + @param in size_auth_as length of authenticated_as
> + @param in pam_user info about the current user
> + @param in name the group name
> + @param in name_len length of name
> + @param in value the mysql user name to use
> + @param in value_len length of value
> + @param out substitution_was_made set to non-zero when a substitution was made.
> +
> + @return status
> + @retval 0 success
> + @retval 1 error
> +*/
> +
> +static int
> +map_grop_to_user(char *auth_as, size_t size_auth_as, void *pam_user,
> + const char *name, const char *value,
> + int *substitution_was_made)
Typo: should be 'map_group_to_user'.
> +{
> + struct passwd *pwd= (struct passwd *) pam_user;
> + struct group grp_buf, *grp;
> + char buf[512], *to_free= NULL;
> + char **members;
> +
> + PAM_DBUG_ENTER("map_grop_to_user");
> + PAM_DBUG_ASSERT(size_auth_as > 0);
> +
> + PAM_DBUG_PRINT("info", ("map_grop_to_user:pam_user=%s, name=%s, value=%s",
> + pwd->pw_name, name, value));
> +
> + grp= auth_pam_safe_getgrnam_r(name, buf, sizeof(buf), &grp_buf,
> &to_free);
> + if (!grp)
> + {
> + if (to_free)
> + free (to_free);
> + *substitution_was_made= 0;
> + /* return success here to let the caller to continue searching */
> + PAM_DBUG_RETURN(0);
> + }
> +
> + *substitution_was_made= 1;
> +
> + if (grp->gr_gid == pwd->pw_gid)
> + {
> + PAM_DBUG_PRINT("info", ("map_grop_to_user:primary group match"));
> + goto found;
> + }
> +
> + members= grp->gr_mem;
> + while (*members)
> + {
> + PAM_DBUG_PRINT("info", ("examining member %s", *members));
> + if (!strcmp(*members, name))
FIXME: This seems to be wrong: the name of a group member shoudl be compared
against pwd->pw_name, not against name which contains name of the gorup...
> + goto found;
> + members++;
> + }
> +
> + *substitution_was_made= 0;
> +
> +found:
> + if (*substitution_was_made)
> + {
> + strncpy(auth_as, value, size_auth_as - 1);
> + auth_as[size_auth_as - 1]= 0;
> + PAM_DBUG_PRINT("info", ("substitution was made to mysql user %s", auth_as));
> + }
> + if (to_free)
> + free(to_free);
> + PAM_DBUG_RETURN(0);
> +}
> +
> +
...
> +static struct st_mysql_auth auth_pam_handler=
> +{
> + MYSQL_AUTHENTICATION_INTERFACE_VERSION,
> + "authentication_pam", /* requires auth_pam_plugin client's plugin */
Confusing comment: the name of client-side plugin is "authentication_pam" I
think.
> + auth_pam_server
> +};
> +
> +mysql_declare_plugin(auth_pam)
> +{
> + MYSQL_AUTHENTICATION_PLUGIN,
> + &auth_pam_handler,
> + "authentication_pam",
> + "Georgi Kodinov",
> + "PAM authentication plugin",
> + PLUGIN_LICENSE_GPL,
> + NULL,
> + NULL,
> + 0x0100,
> + NULL,
> + NULL,
> + NULL
> +}
> +mysql_declare_plugin_end;
> +
> === added file 'plugin/authentication_pam/plug.in'
> --- a/plugin/authentication_pam/plug.in 1970-01-01 00:00:00 +0000
> +++ b/plugin/authentication_pam/plug.in 2010-08-16 15:38:01 +0000
> @@ -0,0 +1,19 @@
> +MYSQL_PLUGIN(authentication_pam, [PAM authentication plugin],
> + [Plugin to authenticate MySQL users using PAM])
> +MYSQL_PLUGIN_DYNAMIC(authentication_pam, [authentication_pam.la])
> +
> +AC_CHECK_LIB(pam, pam_start)
> +AC_CHECK_HEADER(security/pam_appl.h,
> + [AC_DEFINE([HAVE_PAM_APPL_H],[1],
> + [Define to 1 if you have
> <security/pam_appl.h>.])],[])
> +AC_CHECK_FUNCS(getpwnam_r getgrnam_r)
> +
> +AC_COMPILE_IFELSE([
> + AC_LANG_PROGRAM([[
> + #include <security/pam_appl.h>
> +]],[
> + pam_handle_t *pamh;
> + struct pam_conv conv;
> + pam_start("mysql", "user", &conv, &pamh);
> +])],have_pam=yes)
> +AM_CONDITIONAL(HAVE_PAM, test x$have_pam = xyes)
>
...
> === added file 'plugin/authentication_pam/safe_getgrnam.c'
> --- a/plugin/authentication_pam/safe_getgrnam.c 1970-01-01 00:00:00 +0000
> +++ b/plugin/authentication_pam/safe_getgrnam.c 2010-08-16 15:38:01 +0000
> @@ -0,0 +1,74 @@
> +/* Copyright (C) 2010 Oracle */
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/types.h>
> +#include <sys/errno.h>
> +#include <grp.h>
> +#include <config.h>
> +#include "safe.h"
> +
> +struct group *
> +auth_pam_safe_getgrnam_r(const char *name, char *buf_arg, size_t buf_size,
> + struct group *g_arg, char **to_free)
Would be nice to add documentation: return value, meaning of arguments,
notes about responsibiliets of the caller to free allocated memory etc...
...
> === added file 'plugin/authentication_pam/safe_getpwnam.c'
> --- a/plugin/authentication_pam/safe_getpwnam.c 1970-01-01 00:00:00 +0000
> +++ b/plugin/authentication_pam/safe_getpwnam.c 2010-08-16 15:38:01 +0000
> @@ -0,0 +1,77 @@
> +/* Copyright (C) 2010 Oracle */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/types.h>
> +#include <sys/errno.h>
> +#include <pwd.h>
> +#include <config.h>
> +#include "safe.h"
> +
> +struct passwd *
> +auth_pam_safe_getpwnam_r(const char *name, char *buf_arg, size_t buf_size,
> + struct passwd *pwd_arg, char **to_free)
Would be nice to add documentation: return value, meaning of arguments,
notes about responsibiliets of the caller to free allocated memory etc...
Rafal
