[PATCH][SMB3] mount.cifs integration with PAM
Stefan Metzmacher
metze at samba.org
Mon Dec 14 18:03:36 UTC 2020
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 at gmail.com> wrote:
>>
>> Sure. Removed the patch and updated the next branch.
>> --
>> Best regards,
>> Pavel Shilovsky
>>
>> вт, 10 нояб. 2020 г. в 05:20, Shyam Prasad N <nspmangalore at 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 at 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 at 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 at suse.com> wrote:
>>>>>>
>>>>>> Shyam Prasad N <nspmangalore at 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
>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20201214/48c714a9/signature.sig>
More information about the samba-technical
mailing list