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