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

[Stefan Metzmacher <metze@samba.org>]

[-- Attachment #1.1: Type: text/plain, Size: 10297 bytes --]

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 <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
> 
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]