maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexey!

Looks good, of course.
See few minor comments below.

On Mar 18, Alexey Botchkov wrote:
> revision-id: 108cdbf854df6af1ed2c0fc3a63e1786f6b4b7b4 (mariadb-10.1.31-61-g108cdbf)
> parent(s): a0c722d853071e605ea025168da1b01bfe21092c
> committer: Alexey Botchkov
> timestamp: 2018-03-18 12:03:59 +0400
> message:
> 
> MDEV-10871 Add logging capability to pam_user_map.c.
> 
>         'debug' option added to the pam_user_map.so.
>         It writes the verbose report to the syslog.

don't indent the commit comment, please

> 
> ---
>  plugin/auth_pam/mapper/pam_user_map.c | 74 +++++++++++++++++++++++++++++++++--
>  1 file changed, 71 insertions(+), 3 deletions(-)
> 
> diff --git a/plugin/auth_pam/mapper/pam_user_map.c b/plugin/auth_pam/mapper/pam_user_map.c
> index e62be94..1706630 100644
> --- a/plugin/auth_pam/mapper/pam_user_map.c
> +++ b/plugin/auth_pam/mapper/pam_user_map.c
> @@ -26,10 +26,13 @@ top:  accounting
>  
>  #include <stdlib.h>
>  #include <stdio.h>
> +#include <ctype.h>
> +#include <string.h>
>  #include <syslog.h>
>  #include <grp.h>
>  #include <pwd.h>
>  
> +#include <security/pam_ext.h>
>  #include <security/pam_modules.h>
>  
>  #define FILENAME "/etc/security/user_map.conf"
> @@ -90,9 +93,40 @@ static int user_in_group(const gid_t *user_groups, int ng,const char *group)
>  }
>  
>  
> +static void print_groups(pam_handle_t *pamh, const gid_t *user_groups, int ng)
> +{
> +  char buf[256];
> +  char *c_buf= buf, *buf_end= buf+sizeof(buf)-1;
> +  struct group *gr;
> +
> +  for (; ng > 0; user_groups++, ng--)
> +  {
> +    char *c;
> +    if (!(gr= getgrgid(*user_groups)) ||
> +        !(c= gr->gr_name))
> +      continue;
> +    while (*c)
> +    {
> +      if (c_buf == buf_end)
> +        break;
> +      *(c_buf++)= *c;
> +    }
> +    if (c_buf == buf_end)
> +      break;
> +    *(c_buf++)= ',';
> +  }
> +  *c_buf= 0;
> +  pam_syslog(pamh, LOG_DEBUG, "User belongs to groups [%s].\n", buf);

s/pam_syslog/SYSLOG_DEBUG/ ?

> +}
> +
> +
> +static const char debug_keyword[]= "debug";
> +#define SYSLOG_DEBUG if (mode_debug) pam_syslog 
> +
>  int pam_sm_authenticate(pam_handle_t *pamh, int flags,
>      int argc, const char *argv[])
>  {
> +  int mode_debug= 0;
>    int pam_err, line= 0;
>    const char *username;
>    char buf[256];
> @@ -101,6 +135,14 @@ int pam_sm_authenticate(pam_handle_t *pamh, int flags,
>    gid_t *groups= group_buffer;
>    int n_groups= -1;
>  
> +  for (; argc > 0; argv++)
> +  {
> +    if (strcasecmp(*argv, debug_keyword) == 0)
> +      mode_debug= 1;
> +  }

can there be many arguments?

> +
> +  SYSLOG_DEBUG(pamh, LOG_DEBUG, "Opening file '%s'.\n", FILENAME);
>
> +
>    f= fopen(FILENAME, "r");
>    if (f == NULL)
>    {
> @@ -110,12 +152,18 @@ int pam_sm_authenticate(pam_handle_t *pamh, int flags,
>  
>    pam_err = pam_get_item(pamh, PAM_USER, (const void**)&username);
>    if (pam_err != PAM_SUCCESS)
> +  {
> +    pam_syslog(pamh, LOG_ERR, "Cannot get username.\n");
>      goto ret;
> +  }
> +
> +  SYSLOG_DEBUG(pamh, LOG_DEBUG, "Incoming username '%s'.\n", username);
>  
>    while (fgets(buf, sizeof(buf), f) != NULL)
>    {
>      char *s= buf, *from, *to, *end_from, *end_to;
>      int check_group;
> +    int cmp_result;
>  
>      line++;
>  
> @@ -124,7 +172,11 @@ int pam_sm_authenticate(pam_handle_t *pamh, int flags,
>      if ((check_group= *s == '@'))
>      {
>        if (n_groups < 0)
> +      {
>          n_groups= populate_user_groups(username, &groups);
> +        if (mode_debug)
> +          print_groups(pamh, groups, n_groups);
> +      }
>        s++;
>      }
>      from= s;
> @@ -139,14 +191,29 @@ int pam_sm_authenticate(pam_handle_t *pamh, int flags,
>      if (end_to == to) goto syntax_error;
>  
>      *end_from= *end_to= 0;
> -    if (check_group ?
> -          user_in_group(groups, n_groups, from) :
> -          (strcmp(username, from) == 0))
> +
> +    cmp_result= 0;

looks redundant ^^^

> +    if (check_group)
> +    {
> +      cmp_result= user_in_group(groups, n_groups, from);
> +      SYSLOG_DEBUG(pamh, LOG_DEBUG, "Check if user is in gourp '%s': %s\n",

s/gourp/group/

> +                                    from, cmp_result ? "YES":"NO");
> +    }
> +    else
> +    {
> +      cmp_result= (strcmp(username, from) == 0);
> +      SYSLOG_DEBUG(pamh, LOG_DEBUG, "Check if username '%s': %s\n",
> +                                    from, cmp_result ? "YES":"NO");
> +    }
> +    if (cmp_result)
>      {
>        pam_err= pam_set_item(pamh, PAM_USER, to);
> +      SYSLOG_DEBUG(pamh, LOG_DEBUG, "User authenticated as '%s'\n", to);

may be "user mapped to %s" ?

>        goto ret;
>      }
>    }
> +
> +  SYSLOG_DEBUG(pamh, LOG_DEBUG, "User not found in the list.\n");
>    pam_err= PAM_AUTH_ERR;
>    goto ret;
>  
> @@ -162,6 +229,7 @@ int pam_sm_authenticate(pam_handle_t *pamh, int flags,
>    return pam_err;
>  }
>  
> +
>  int pam_sm_setcred(pam_handle_t *pamh, int flags,
>                     int argc, const char *argv[])
>  {

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx