Re: [PATCH][SMB3] mount.cifs integration with PAM

[Pavel Shilovsky <piastryyy@gmail.com>]

Sure. Removed the patch and updated the next branch.
--
Best regards,
Pavel Shilovsky

вт, 10 нояб. 2020 г. в 05:20, Shyam Prasad N <nspmangalore@gmail.com>:
>
> Hi Pavel,
>
> There is more that needs to be done on this item. Otherwise, this will
> depend on user behaviour to cleanup unused krb5 tickets.
> The pending items on this is to propagate this mount option to cifs.ko
> and write an umount.cifs utility to read that mount option to do PAM
> logoff.
> So please rollback this patch for now.
>
> Regards,
> Shyam
>
> On Tue, Nov 10, 2020 at 5:12 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> >
> > Merged into next. Please let me know if something needs to be fixed. Thanks!
> > --
> > Best regards,
> > Pavel Shilovsky
> >
> > чт, 24 сент. 2020 г. в 03:39, Shyam Prasad N <nspmangalore@gmail.com>:
> > >
> > > Hi Aurélien,
> > >
> > > I've implemented most of your review comments. Also fixed the issue.
> > >
> > > On Wed, Sep 23, 2020 at 7:26 PM Aurélien Aptel <aaptel@suse.com> wrote:
> > > >
> > > > Shyam Prasad N <nspmangalore@gmail.com> writes:
> > > > > Also, I'll test this out with DFS once I figure out how to set it up. :)
> > > > > Re-attaching the patch with some minor changes with just the
> > > > > "force_pam" mount option.
> > > >
> > > > You will need 2 Windows VM. DFS is basically symlinks across
> > > > servers. The DFS root VM will host the links (standalone namespace) and
> > > > you have to make them point to shares on the 2nd VM. You don't need to
> > > > setup replication to test.
> > > >
> > > > When you mount the root in cifs.ko and access a link, the server will
> > > > reply that the file is remote. cifs.ko then does an FSCTL on the link to
> > > > resolve the target it points to and then connects to the target and
> > > > mounts it under the link seemlessly.
> > > >
> > > >
> > > > Regarding the patch:
> > > >
> > > > * need to update the man page with option and explanation
> > > >
> > > > I have some comments with the style, I know it's annoying.. but it
> > > > would be best to keep the same across the code.
> > > >
> > > > * use the existing indent style (tabs) and avoid adding trailing whitespaces.
> > > > * no () for return statements
> > > > * no casting for memory allocation
> > > > * if (X) free(X)  => free(X)
> > > >
> > > > Below some comments about pam_auth_krb5_conv():
> > > >
> > > > > @@ -1809,6 +1824,119 @@ get_password(const char *prompt, char *input, int capacity)
> > > > >       return input;
> > > > >  }
> > > > >
> > > > > +#ifdef HAVE_KRB5PAM
> > > > > +#define PAM_CIFS_SERVICE "cifs"
> > > > > +
> > > > > +static int
> > > > > +pam_auth_krb5_conv(int n, const struct pam_message **msg,
> > > > > +    struct pam_response **resp, void *data)
> > > > > +{
> > > > > +    struct parsed_mount_info *parsed_info;
> > > > > +     struct pam_response *reply;
> > > > > +     int i;
> > > > > +
> > > > > +     *resp = NULL;
> > > > > +
> > > > > +    parsed_info = data;
> > > > > +    if (parsed_info == NULL)
> > > > > +             return (PAM_CONV_ERR);
> > > > > +     if (n <= 0 || n > PAM_MAX_NUM_MSG)
> > > > > +             return (PAM_CONV_ERR);
> > > > > +
> > > > > +     if ((reply = calloc(n, sizeof(*reply))) == NULL)
> > > > > +             return (PAM_CONV_ERR);
> > > > > +
> > > > > +     for (i = 0; i < n; ++i) {
> > > > > +             switch (msg[i]->msg_style) {
> > > > > +             case PAM_PROMPT_ECHO_OFF:
> > > > > +            if ((reply[i].resp = (char *) malloc(MOUNT_PASSWD_SIZE + 1)) == NULL)
> > > > > +                goto fail;
> > > > > +
> > > > > +            if (parsed_info->got_password && parsed_info->password != NULL) {
> > > > > +                strncpy(reply[i].resp, parsed_info->password, MOUNT_PASSWD_SIZE + 1);
> > > > > +            } else if (get_password(msg[i]->msg, reply[i].resp, MOUNT_PASSWD_SIZE + 1) == NULL) {
> > > > > +                goto fail;
> > > > > +            }
> > > > > +            reply[i].resp[MOUNT_PASSWD_SIZE] = '\0';
> > > > > +
> > > > > +                     reply[i].resp_retcode = PAM_SUCCESS;
> > > > > +                     break;
> > > > > +             case PAM_PROMPT_ECHO_ON:
> > > > > +                     fprintf(stderr, "%s: ", msg[i]->msg);
> > > > > +            if ((reply[i].resp = (char *) malloc(MOUNT_PASSWD_SIZE + 1)) == NULL)
> > > > > +                goto fail;
> > > > > +
> > > > > +                     if (fgets(reply[i].resp, MOUNT_PASSWD_SIZE + 1, stdin) == NULL)
> > > >
> > > > Do we need to remove the trailing \n from the buffer?
> > > >
> > > > > +                goto fail;
> > > > > +
> > > > > +            reply[i].resp[MOUNT_PASSWD_SIZE] = '\0';
> > > > > +
> > > > > +                     reply[i].resp_retcode = PAM_SUCCESS;
> > > > > +                     break;
> > > > > +             case PAM_ERROR_MSG:
> > > >
> > > > Shouldn't this PAM_ERROR_MSG case goto fail?
> > > >
> > > > > +             case PAM_TEXT_INFO:
> > > > > +                     fprintf(stderr, "%s: ", msg[i]->msg);
> > > > > +
> > > > > +                     reply[i].resp_retcode = PAM_SUCCESS;
> > > > > +                     break;
> > > > > +             default:
> > > > > +                     goto fail;
> > > > > +             }
> > > > > +     }
> > > > > +     *resp = reply;
> > > > > +     return (PAM_SUCCESS);
> > > > > +
> > > > > + fail:
> > > > > +     for(i = 0; i < n; i++) {
> > > > > +        if (reply[i].resp)
> > > > > +            free(reply[i].resp);
> > > >
> > > > free(NULL) is a no-op, remove the checks.
> > > >
> > > > > +     }
> > > > > +     free(reply);
> > > > > +     return (PAM_CONV_ERR);
> > > > > +}
> > > >
> > > > I gave this a try with a properly configured system joined to AD from
> > > > local root account:
> > > >
> > > > aaptel$ ./configure
> > > > ...
> > > > checking krb5.h usability... yes
> > > > checking krb5.h presence... yes
> > > > checking for krb5.h... yes
> > > > checking krb5/krb5.h usability... yes
> > > > checking krb5/krb5.h presence... yes
> > > > checking for krb5/krb5.h... yes
> > > > checking for keyvalue in krb5_keyblock... no
> > > > ...
> > > > checking keyutils.h usability... yes
> > > > checking keyutils.h presence... yes
> > > > checking for keyutils.h... yes
> > > > checking for krb5_init_context in -lkrb5... yes
> > > > ...
> > > > checking for WBCLIENT... yes
> > > > checking for wbcSidsToUnixIds in -lwbclient... yes
> > > > ...
> > > > checking for keyutils.h... (cached) yes
> > > > checking security/pam_appl.h usability... yes
> > > > checking security/pam_appl.h presence... yes
> > > > checking for security/pam_appl.h... yes
> > > > checking for pam_start in -lpam... yes
> > > > checking for security/pam_appl.h... (cached) yes
> > > > checking for krb5_auth_con_getsendsubkey... yes
> > > > checking for krb5_principal_get_realm... no
> > > > checking for krb5_free_unparsed_name... yes
> > > > checking for krb5_auth_con_setaddrs... yes
> > > > checking for krb5_auth_con_set_req_cksumtype... yes
> > > > ...
> > > > aaptel$ make
> > > > ....(ok)
> > > >
> > > > Without force_pam:
> > > >
> > > > root# ./mount.cifs -v //adnuc.nuc.test/data /x -o sec=krb5,username=administrator,domain=NUC
> > > > mount.cifs kernel mount options: ip=192.168.2.111,unc=\\adnuc.nuc.test\data,sec=krb5,user=administrator,domain=NUC
> > > > mount error(2): No such file or directory
> > > > Refer to the mount.cifs(8) manual page (e.g. man mount.cifs) and kernel log messages (dmesg)
> > > >
> > > > With force_pam:
> > > >
> > > > root# ./mount.cifs -v //adnuc.nuc.test/data /x -o sec=krb5,username=administrator,domain=NUC,force_pam
> > > > Authenticating as user: administrator
> > > > Error in authenticating user with PAM: Authentication failure
> > > > Attempt to authenticate user with PAM unsuccessful. Still, proceeding with mount.
> > > >
> > > > => no further message but mount failed and no msg in dmesg, it didn't
> > > >    reach the mount() call
> > > >
> > > > Not sure what is going on. Does the domain need to be passed to PAM?
> > > >
> > > > Cheers,
> > > > --
> > > > Aurélien Aptel / SUSE Labs Samba Team
> > > > GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> > > > SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> > > > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
> > >
> > >
> > >
> > > --
> > > -Shyam
>
>
>
> --
> -Shyam