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

[Shyam Prasad N <nspmangalore@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6629 bytes --]

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

[-- Attachment #2: 0001-mount.cifs-Have-an-option-to-authenticate-against-PA.patch --]
[-- Type: application/octet-stream, Size: 9158 bytes --]

From 67fa592800d74a0dc85717c5edeb06bc26c6f476 Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@microsoft.com>
Date: Thu, 13 Aug 2020 08:53:08 -0700
Subject: [PATCH] mount.cifs: Have an option to authenticate against PAM.

Authentication against PAM has two benefits:
- The PAM module (winbind or sssd) will perform the house-keeping of
the krb5 tickets and make sure they aren't expired.
- The mount.cifs utility need not rely on sshd or su to authenticate
against PAM and arrange the krb5 TGT. In case of ssh with private keys,
ssh doesn't involve PAM. So krb5 TGT may be missing.

This introduces a new PAM application named "cifs".
The user may use this to customize the PAM stack for just this application.
Otherwise, the default PAM configuration is used.
Also, to disable this feature at the time of build, one can run
configure with --enable-krb5pam=no.
---
 Makefile.am  |   2 +-
 configure.ac |  27 +++++++++
 mount.cifs.c | 158 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 183 insertions(+), 4 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index fe9cd34..51c5c47 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -4,7 +4,7 @@ ACLOCAL_AMFLAGS = -I aclocal
 root_sbindir = $(ROOTSBINDIR)
 root_sbin_PROGRAMS = mount.cifs
 mount_cifs_SOURCES = mount.cifs.c mtab.c resolve_host.c util.c
-mount_cifs_LDADD = $(LIBCAP) $(CAPNG_LDADD) $(RT_LDADD)
+mount_cifs_LDADD = $(LIBCAP) $(LIBPAM) $(CAPNG_LDADD) $(RT_LDADD)
 include_HEADERS = cifsidmap.h
 rst_man_pages = mount.cifs.8
 
diff --git a/configure.ac b/configure.ac
index 22e78ef..fa8af0f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -55,6 +55,11 @@ AC_ARG_ENABLE(pam,
 	enable_pam=$enableval,
 	enable_pam="maybe")
 
+AC_ARG_ENABLE(krb5pam,
+    [AS_HELP_STRING([--enable-krb5pam],[Add PAM authentication support when using sec=krb5 @<:@default=yes@])],,
+    enable_krb5pam=$enableval
+    enable_krb5pam="yes")
+
 AC_ARG_ENABLE(systemd,
 	[AS_HELP_STRING([--enable-systemd],[Enable systemd specific behavior for mount.cifs @<:@default=yes@:>@])],
 	enable_systemd=$enableval,
@@ -241,6 +246,27 @@ if test $enable_pam != "no"; then
 			])
 fi
 
+if test "x$enable_krb5pam" = "xno"; then
+	enable_krb5pam="no"
+else
+	AC_CHECK_LIB([pam], [pam_start], enable_krb5pam="yes", enable_krb5pam="no", )
+	AC_CHECK_HEADERS([security/pam_appl.h], ,
+			 [
+				if test $enable_krb5pam = "yes"; then
+					AC_MSG_ERROR([security/pam_appl.h not found, consider installing keyutils-libs-devel.])
+				else
+					AC_MSG_WARN([security/pam_appl.h not found, consider installing pam-devel. Disabling cifscreds PAM module.])
+					enable_krb5pam="no"
+				fi
+			 ])
+fi
+
+if test "$enable_krb5pam" = "yes"; then
+	AC_DEFINE([HAVE_KRB5PAM],[1], [Define if PAM auth is enabled for krb5])
+	LIBPAM=-lpam
+	AC_SUBST(LIBPAM)
+fi
+
 # ugly, but I'm not sure how to check for functions in a library that's not in $LIBS
 cu_saved_libs=$LIBS
 LIBS="$LIBS $KRB5_LDADD"
@@ -288,6 +314,7 @@ AM_CONDITIONAL(CONFIG_CIFSACL, [test "$enable_cifsacl" != "no"])
 AM_CONDITIONAL(CONFIG_SMBINFO, [test "$enable_smbinfo" != "no"])
 AM_CONDITIONAL(CONFIG_PYTHON_TOOLS, [test "$enable_pythontools" != "no"])
 AM_CONDITIONAL(CONFIG_PAM, [test "$enable_pam" != "no"])
+AM_CONDITIONAL(CONFIG_KRB5PAM, [test "$enable_krb5pam" != "no"])
 AM_CONDITIONAL(CONFIG_PLUGIN, [test "$enable_cifsidmap" != "no" -o "$enable_cifsacl" != "no"])
 
 LIBCAP_NG_PATH
diff --git a/mount.cifs.c b/mount.cifs.c
index 4feb397..271966b 100644
--- a/mount.cifs.c
+++ b/mount.cifs.c
@@ -46,6 +46,9 @@
 #include <time.h>
 #include <sys/mman.h>
 #include <sys/wait.h>
+#ifdef HAVE_KRB5PAM
+#include <security/pam_appl.h>
+#endif /* HAVE_KRB5PAM */
 #ifdef HAVE_SYS_FSUID_H
 #include <sys/fsuid.h>
 #endif /* HAVE_SYS_FSUID_H */
@@ -163,6 +166,7 @@
 #define OPT_BKUPGID    31
 #define OPT_NOFAIL     32
 #define OPT_SNAPSHOT   33
+#define OPT_FORCE_PAM  34
 
 #define MNT_TMP_FILE "/.mtab.cifs.XXXXXX"
 
@@ -189,6 +193,8 @@ struct parsed_mount_info {
 	unsigned int verboseflag:1;
 	unsigned int nofail:1;
 	unsigned int got_domain:1;
+	unsigned int is_krb5:1;
+	unsigned int force_pam:1;
 };
 
 static const char *thisprogram;
@@ -713,6 +719,8 @@ static int parse_opt_token(const char *token)
 		return OPT_PASS;
 	if (strcmp(token, "sec") == 0)
 		return OPT_SEC;
+	if (strcmp(token, "force_pam") == 0)
+		return OPT_FORCE_PAM;
 	if (strcmp(token, "ip") == 0 ||
 		strcmp(token, "addr") == 0)
 		return OPT_IP;
@@ -891,12 +899,19 @@ parse_options(const char *data, struct parsed_mount_info *parsed_info)
 
 		case OPT_SEC:
 			if (value) {
-				if (!strncmp(value, "none", 4) ||
-				    !strncmp(value, "krb5", 4))
+				if (!strncmp(value, "none", 4))
 					parsed_info->got_password = 1;
+				if (!strncmp(value, "krb5", 4))
+					parsed_info->is_krb5 = 1;
 			}
 			break;
 
+#ifdef HAVE_KRB5PAM
+		case OPT_FORCE_PAM:
+			parsed_info->force_pam = 1;
+			goto nocopy;
+#endif /* HAVE_KRB5PAM */
+			
 		case OPT_IP:
 			if (!value || !*value) {
 				fprintf(stderr,
@@ -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)
+				goto fail;
+
+			reply[i].resp[MOUNT_PASSWD_SIZE] = '\0';
+			null_terminate_endl(reply[i].resp);
+
+			reply[i].resp_retcode = PAM_SUCCESS;
+			break;
+		case PAM_ERROR_MSG:
+		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++) {
+		free(reply[i].resp);
+	}
+	free(reply);
+	return PAM_CONV_ERR;
+}
+
+	static int
+pam_auth_krb5_user(struct parsed_mount_info *parsed_info)
+{
+	int rc = -1;
+	pam_handle_t *pamh = NULL;
+	struct pam_conv pam_conv = {
+		.conv = pam_auth_krb5_conv,
+		.appdata_ptr = (void *) parsed_info
+	};
+
+	fprintf(stdout, "Authenticating as user: %s\n", parsed_info->username);
+	rc = pam_start(PAM_CIFS_SERVICE, parsed_info->username, &pam_conv, &pamh);
+	if (rc != PAM_SUCCESS) {
+		fprintf(stderr, "Error starting PAM transaction: %s\n", pam_strerror(pamh, rc));
+		return rc;
+	}
+
+	rc = pam_authenticate(pamh, 0);
+	if (rc != PAM_SUCCESS) {
+		fprintf(stderr, "Error in authenticating user with PAM: %s\n", pam_strerror(pamh, rc));
+		goto end;
+	}
+
+	rc = pam_acct_mgmt(pamh, 0);
+	if (rc != PAM_SUCCESS) {
+		fprintf(stderr, "User account invalid: %s\n", pam_strerror(pamh, rc));
+		goto end;
+	}
+
+	rc = pam_setcred(pamh, PAM_ESTABLISH_CRED);
+	if (rc != PAM_SUCCESS) {
+		fprintf(stderr, "Error in setting PAM credentials: %s\n", pam_strerror(pamh, rc));
+		goto end;
+	}
+
+end:
+	pam_end(pamh, rc);
+	return rc;
+}
+#endif /* HAVE_KRB5PAM */
+
 static int
 assemble_mountinfo(struct parsed_mount_info *parsed_info,
 		   const char *thisprogram, const char *mountpoint,
@@ -1891,7 +2019,31 @@ assemble_mountinfo(struct parsed_mount_info *parsed_info,
 		parsed_info->got_user = 1;
 	}
 
-	if (!parsed_info->got_password) {
+#ifdef HAVE_KRB5PAM
+	if (parsed_info->is_krb5 && parsed_info->force_pam) {
+		/*
+		 * Attempt to authenticate with PAM.
+		 * If PAM is configured properly, let it get the krb5 tickets necessary for the mount.
+		 * Even if this fails, it could be the case of PAM not configured properly.
+		 * In that case, retain the current behavior. So this is just a best-effort.
+		 */
+		rc = pam_auth_krb5_user(parsed_info);
+		if (rc) {
+			fprintf(stderr, "Attempt to authenticate user with " \
+					"PAM unsuccessful. Still, proceeding with mount.\n");
+			/*
+			 * Even if this is a failure, fallthrough and see if cifs.ko can still
+			 * authenticate the user.
+			 */
+			rc = 0;
+		}
+
+		parsed_info->got_password = 1;
+	}
+#endif /* HAVE_KRB5PAM */
+
+	/* If sec=krb5, then password is collected by other means */
+	if (!parsed_info->is_krb5 && !parsed_info->got_password) {
 		char tmp_pass[MOUNT_PASSWD_SIZE + 1];
 		char *prompt = NULL;
 
-- 
2.25.1