[PATCH] PAM Support for Busybox Login

[Thaddeus Ternes]

Thank you Denis and Bernhard for the constructive comments.  I've
included an updated version of the patch that incorporates your
suggestions.

Again, comments appreciated.  Feel free to apply to svn if it's
something that others could benefit from.

-Thaddeus

On 10/1/06, Denis Vlasenko <vda.linux at googlemail.com> wrote:
> On Friday 29 September 2006 23:26, Thaddeus Ternes wrote:
> > Previously, a patch for Busybox login was shared on the list that
> > added PAM support to 1.0.0.  That patch no longer applies to 1.2.1, so
> > I did some work to fix it up, as well as make it a bit easier to build
> > (thanks for the help on that, Jason).
> >
> > Here's the original patch:
> > http://www.busybox.net/lists/busybox/2004-December/013257.html
> >
> > And attached is my swing at it (should patch cleanly against 1.2.1).
> >
> > All comments appreciated.
>
> +#define PWLOOKUP \
> +               if (!( pw > +                        pw_copy.pw_name   > +                        pw_copy.pw_passwd > +                        opt_fflag > +                        failed > +                } else                                 \
> +                        pw_copy > +                pw >
> Please do it this way:
>
> #define PWLOOKUP do { statement; statement; } while(0)
>
> It will behave exactly like single C statement then.
>
> +#ifdef CONFIG_PAM
>
> #ifdef does not detect typos in macro names. #if ENABLE_PAM is better.
>
> +        // TODO: decide how this comment should be worded (since we have the MACRO above)
> +                /* PAM authentication goes before (and instead of) all the
> +                 * other stuff.  The reason we initialize the pw struct
> +                 * before going on with PAM is that the pw struct is used
> +                 * later on, and this way saved us some changing of that code.
> +                 */
> +                if ((pamret >
> Generally avoid assignments inside if().
>                  pamret >                  if (pamret !> generates the same assembly code, but easier to read.
> It is not an iron rule. In nested "else ifs", for example,
> it is ok...
>
> +                {
> +                        /* pam_start() failed.  We failed to initialize the PAM
> +                         * structures, and should probably carry on with login,
> +                         * so we avoid locking people out.
> +                         */
> +                         fprintf(stderr, "PAM initialization failed: %s\n", pam_strerror(pamh, pamret));
> +                                failed >
> 1. bb_error_msg will do the same, but smaller (no stderr parameter, no \n)
> 2. Indentation is broken. (At least
>
> +                         goto auth_ok;
> +                }
> +                else
> +                {   // continuing with pam authentication
>
> Regarding '{': usually we do
>         if() {
>                 ...
>         } else {
>
> +                    // set TTY (so things like securetty work)
> +                               if((pamret > +                               {
> +                                       fprintf(stderr, "Failed to pam_set_item TTY: %s\n", pam_strerror(pamh, pamret));
> +                                       failed > +                                       goto auth_ok;
> +                               }
> +                    else if ( ( pamret > +                               {
> +                               // Then check that the account is healthy.
> +                               if ( ( pamret > +                                       fprintf( stderr, "User not allowed access: %s\n",pam_strerror( pamh, pamret ) );
> +                                       else
> +                                       {       // read user back
> +                                char *pamuser;
> +                                               if(pam_get_item(pamh, PAM_USER, (const void **) &pamuser)!> +                                                       fprintf(stderr, "pam_get_item failed on username\n");
> +                                               else
> +                                    strcpy(username, pamuser);
> +                                       }
> +                    }
> +                    // If we get here, the user was authenticated, and is
> +                    // granted access.
> +                    if ( pam_end( pamh, pamret ) !>
> We typically format it like this: if (pam_end(pamh, pamret) !>
> +                        fprintf( stderr, "PAM cleaning up failed\n" );
> +
> +                    if ( pamret = PAM_SUCCESS )
> +                        failed > +                    else
> +                        failed > +                       PWLOOKUP
> +                               goto auth_ok;
> +                }
> +                // Everything from here to auth_ok: is skipped when running
> +                // PAM.  This is all PAM's responsibility anyway.  The exception
> +                // is if pam_start() failed, which would indicate a more serious
> +                // error in the PAM library.  In that case, we carry on with
> +                // standard procedure.
> +#else
> +               PWLOOKUP
> +#endif /* CONFIG_PAM */
>
>                 if (( pw-> pw_passwd [0] = '!' ) || ( pw-> pw_passwd[0] = '*' ))
>                         failed >
> Generally I like it.
>
> Maybe will add CONFIG_DESKTOP to make such options
> visible only for fat "desktop replacement"-class bbox.
> So that embedded people will not feel like
> bbox is going to be horribly bloated now.
> --
> vda
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox_pam_1.2.1.patch
Type: application/octet-stream
Size: 5490 bytes
Desc: not available
Url : http://busybox.net/lists/busybox/attachments/20061002/496cc50d/attachment.obj