Hi Shyam, in what way is this specific to kerberos? Would it make sense to call the configure option just --enable-pam? And also remove the 'krb5' from other variables? PAM_CIFS_SERVICE "cifs" seems to be unspecific, "mount.cifs" or "cifs.mount". Maybe even "mount.smb3"? Can you fix the indentation of the patch? There seems to be a strange mix of leading tabs and whitespaces, which make it hard to read the patch. With force_pam I would not expect a failure to be ignored. Why can this only be used with sec=krb5? Wouldn't it be possible to fetch the password from the pam stack in order to pass it to the kernel? metze Am 27.11.20 um 11:43 schrieb Shyam Prasad N via samba-technical: > 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 wrote: >> >> Sure. Removed the patch and updated the next branch. >> -- >> Best regards, >> Pavel Shilovsky >> >> вт, 10 нояб. 2020 г. в 05:20, Shyam Prasad N : >>> >>> 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 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 : >>>>> >>>>> 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 wrote: >>>>>> >>>>>> Shyam Prasad N 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 > > >