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

Shyam Prasad N
2020-11-27T10:44:17Z

Discussed this with Aurelien today.

With the patch last sent, users are authenticated using the PAM stack.
However, there's no call to cleanup the PAM credentials. Which could
leave the kerberos tickets around, even after the umount.

To complete this fix, we need a mechanism to tell the umount helper
umount.cifs (a new executable to be created) that PAM authentication
was used for the mount.

There are two possible approaches which I could think of:
1. Add a new mount option in cifs.ko. Inside cifs.ko, this option
would be non-functional. But will be used by umount.cifs to call PAM
for cleanup.
2. In mount.cifs, keep this info in a temporary file (maybe
/var/run/cifs/). umount.cifs will read this and call PAM for cleanup.

I feel approach 1 is a cleaner approach to take. Aurelien seems to
favour option 1 as well.
Please feel free to comment if you feel otherwise.
Once we agree on the right approach, I'll send a patch for the changes.

Regards,
Shyam

On Wed, Nov 11, 2020 at 12:53 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> 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

--
-Shyam