List:Commits« Previous MessageNext Message »
From:Rafal Somla Date:September 9 2010 1:21pm
Subject:Re: bzr commit into mysql-5.5 branch (Georgi.Kodinov:3082) WL#5366
View as plain text  
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
Thread
bzr commit into mysql-5.5 branch (Georgi.Kodinov:3082) WL#5366Georgi Kodinov16 Aug
  • Re: bzr commit into mysql-5.5 branch (Georgi.Kodinov:3082) WL#5366Rafal Somla10 Sep
  • Re: bzr commit into mysql-5.5 branch (Georgi.Kodinov:3082) WL#5366Rafal Somla10 Sep