[cifs-utils PATCHv2 0/6] cifs.upcall: cleanup and overhaul of the cifs.upcall krb5 handling code

[cifs-utils PATCHv2 0/6] cifs.upcall: cleanup and overhaul of the cifs.upcall krb5 handling code

[Jeff Layton]

This is an update of the patchset that I posted to linux-cifs mailing
list last week, with the main change being a few extra cleanups to
the code, to make it a bit more efficient. This also removes a large
chunk of (largely unneeded) code from the program.

Only lightly tested so far, but it seems to do the right thing. The
main assumption here is that no one actually relies on the code to
scan /tmp for credcaches, and that we should just rely on krb5.conf to
tell us where they are.

While this is a step in the right direction, what I think we might
want to do longer-term is to make this use gss_init_sec_context
instead of micromanaging it like we do now. The only part I'm a
little unclear on is how to extract the session key in that case.

Comments welcome!

Jeff Layton (6):
  aclocal: fix typo in idmap.m4
  cifs.upcall: use krb5 routines to get default ccname
  cifs.upcall: make the krb5_context a static global variable
  cifs.upcall: remove KRB5_TC_OPENCLOSE
  cifs.upcall: make get_tgt_time take a ccache arg
  cifs.upcall: stop passing around ccache name strings

 aclocal/idmap.m4 |   2 +-
 cifs.upcall.c    | 266 +++++++++++++------------------------------------------
 2 files changed, 61 insertions(+), 207 deletions(-)

-- 
2.7.4

[cifs-utils PATCHv2 1/6] aclocal: fix typo in idmap.m4

[Jeff Layton]

We really don't want to do the same check twice.

Signed-off-by: Jeff Layton <jlayton@samba.org>
---
 aclocal/idmap.m4 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/aclocal/idmap.m4 b/aclocal/idmap.m4
index 3ccdae3ab968..4e16a46568a1 100644
--- a/aclocal/idmap.m4
+++ b/aclocal/idmap.m4
@@ -19,7 +19,7 @@ if test $enable_cifsidmap != "no" -o $enable_cifsacl != "no"; then
 			])
 fi
 
-if test $enable_cifsacl != "no" -o $enable_cifsacl != "no"; then
+if test $enable_cifsidmap != "no" -o $enable_cifsacl != "no"; then
 	ac_wbc_save_LDFLAGS="$LDFLAGS"
 	ac_wbc_save_LIBS="$LIBS"
 	LDFLAGS="$LDFLAGS $WBCLIENT_LIBS"
-- 
2.7.4

[cifs-utils PATCHv2 2/6] cifs.upcall: use krb5 routines to get default ccname

[Jeff Layton]

Currently we end up groveling around in /tmp, trying to guess what the
credcache will be. Instead, just get the default ccname for the user,
and then see if it has a valid tgt. If it doesn't then we try to use
the keytab to init the credcache before proceeding.

Signed-off-by: Jeff Layton <jlayton@samba.org>
---
 cifs.upcall.c | 148 +++++++++++-----------------------------------------------
 1 file changed, 27 insertions(+), 121 deletions(-)

diff --git a/cifs.upcall.c b/cifs.upcall.c
index e8544c2b68ad..d0f6d089d8e1 100644
--- a/cifs.upcall.c
+++ b/cifs.upcall.c
@@ -52,12 +52,6 @@
 #include "spnego.h"
 #include "cifs_spnego.h"
 
-#define	CIFS_DEFAULT_KRB5_DIR		"/tmp"
-#define	CIFS_DEFAULT_KRB5_USER_DIR	"/run/user/%U"
-#define	CIFS_DEFAULT_KRB5_PREFIX	"krb5cc"
-
-#define	MAX_CCNAME_LEN			PATH_MAX + 5
-
 static const char *prog = "cifs.upcall";
 typedef enum _sectype {
 	NONE = 0,
@@ -178,13 +172,34 @@ err_cache:
 	return credtime;
 }
 
-static int krb5cc_filter(const struct dirent *dirent)
+static char *
+get_default_cc(void)
 {
-	/* subtract 1 for the null terminator */
-	return !strncmp(dirent->d_name, CIFS_DEFAULT_KRB5_PREFIX,
-			sizeof(CIFS_DEFAULT_KRB5_PREFIX) - 1);
+	krb5_error_code ret;
+	const char *ccname;
+	char *rcc = NULL;
+	krb5_context context = NULL;
+
+	ret = krb5_init_context(&context);
+	if (ret) {
+		syslog(LOG_DEBUG, "krb5_init_context: %d", (int)ret);
+		return NULL;
+	}
+
+	ccname = krb5_cc_default_name(context);
+	if (!ccname) {
+		syslog(LOG_DEBUG, "krb5_cc_default returned NULL.");
+		goto out_free_context;
+	}
+
+	if (get_tgt_time(ccname))
+		rcc = strdup(ccname);
+out_free_context:
+	krb5_free_context(context);
+	return rcc;
 }
 
+
 static char *
 init_cc_from_keytab(const char *keytab_name, const char *user)
 {
@@ -263,109 +278,6 @@ icfk_cleanup:
 	return ccname;
 }
 
-/* resolve a pattern to an actual directory path */
-static char *resolve_krb5_dir(const char *pattern, uid_t uid)
-{
-	char name[MAX_CCNAME_LEN];
-	int i;
-	size_t j;
-	for (i = 0, j = 0; (pattern[i] != '\0') && (j < sizeof(name)); i++) {
-		switch (pattern[i]) {
-		case '%':
-			switch (pattern[i + 1]) {
-			case '%':
-				name[j++] = pattern[i];
-				i++;
-				break;
-			case 'U':
-				j += snprintf(name + j, sizeof(name) - j,
-					      "%lu", (unsigned long) uid);
-				i++;
-				break;
-			}
-			break;
-		default:
-			name[j++] = pattern[i];
-			break;
-		}
-	}
-	if ((j > 0) && (j < sizeof(name)))
-		return strndup(name, MAX_CCNAME_LEN);
-	else
-		return NULL;
-}
-
-/* search for a credcache that looks like a likely candidate */
-static char *find_krb5_cc(const char *dirname, uid_t uid,
-			  char **best_cache, time_t *best_time)
-{
-	struct dirent **namelist;
-	struct stat sbuf;
-	char ccname[MAX_CCNAME_LEN], *credpath;
-	int i, n;
-	time_t cred_time;
-
-	n = scandir(dirname, &namelist, krb5cc_filter, NULL);
-	if (n < 0) {
-		syslog(LOG_DEBUG, "%s: scandir error on directory '%s': %s",
-		       __func__, dirname, strerror(errno));
-		return NULL;
-	}
-
-	for (i = 0; i < n; i++) {
-		snprintf(ccname, sizeof(ccname), "FILE:%s/%s", dirname,
-			 namelist[i]->d_name);
-		credpath = ccname + 5;
-		syslog(LOG_DEBUG, "%s: considering %s", __func__, credpath);
-
-		if (lstat(credpath, &sbuf)) {
-			syslog(LOG_DEBUG, "%s: stat error on '%s': %s",
-			       __func__, credpath, strerror(errno));
-			free(namelist[i]);
-			continue;
-		}
-		if (sbuf.st_uid != uid) {
-			syslog(LOG_DEBUG, "%s: %s is owned by %u, not %u",
-			       __func__, credpath, sbuf.st_uid, uid);
-			free(namelist[i]);
-			continue;
-		}
-		if (S_ISDIR(sbuf.st_mode)) {
-			snprintf(ccname, sizeof(ccname), "DIR:%s/%s", dirname,
-				 namelist[i]->d_name);
-			credpath = ccname + 4;
-		} else
-		if (!S_ISREG(sbuf.st_mode)) {
-			syslog(LOG_DEBUG, "%s: %s is not a regular file",
-			       __func__, credpath);
-			free(namelist[i]);
-			continue;
-		}
-		if (!(cred_time = get_tgt_time(ccname))) {
-			syslog(LOG_DEBUG, "%s: %s is not a valid credcache.",
-			       __func__, ccname);
-			free(namelist[i]);
-			continue;
-		}
-
-		if (cred_time <= *best_time) {
-			syslog(LOG_DEBUG, "%s: %s expires sooner than current "
-			       "best.", __func__, ccname);
-			free(namelist[i]);
-			continue;
-		}
-
-		syslog(LOG_DEBUG, "%s: %s is valid ccache", __func__, ccname);
-		free(*best_cache);
-		*best_cache = strndup(ccname, MAX_CCNAME_LEN);
-		*best_time = cred_time;
-		free(namelist[i]);
-	}
-	free(namelist);
-
-	return *best_cache;
-}
-
 static int
 cifs_krb5_get_req(const char *host, const char *ccname,
 		  DATA_BLOB * mechtoken, DATA_BLOB * sess_key)
@@ -841,13 +753,12 @@ int main(const int argc, char *const argv[])
 	unsigned int have;
 	long rc = 1;
 	int c, try_dns = 0, legacy_uid = 0;
-	char *buf, *ccdir = NULL, *ccname = NULL, *best_cache = NULL;
+	char *buf, *ccname = NULL;
 	char hostbuf[NI_MAXHOST], *host;
 	struct decoded_args arg;
 	const char *oid;
 	uid_t uid;
 	char *keytab_name = NULL;
-	time_t best_time = 0;
 
 	hostbuf[0] = '\0';
 	memset(&arg, 0, sizeof(arg));
@@ -954,13 +865,8 @@ int main(const int argc, char *const argv[])
 		syslog(LOG_ERR, "setuid: %s", strerror(errno));
 		goto out;
 	}
-	ccdir = resolve_krb5_dir(CIFS_DEFAULT_KRB5_USER_DIR, uid);
-	if (ccdir != NULL)
-		find_krb5_cc(ccdir, uid, &best_cache, &best_time);
-	ccname = find_krb5_cc(CIFS_DEFAULT_KRB5_DIR, uid, &best_cache,
-			      &best_time);
-	SAFE_FREE(ccdir);
 
+	ccname = get_default_cc();
 	/* Couldn't find credcache? Try to use keytab */
 	if (ccname == NULL && arg.username != NULL)
 		ccname = init_cc_from_keytab(keytab_name, arg.username);
-- 
2.7.4

[cifs-utils PATCHv2 3/6] cifs.upcall: make the krb5_context a static global variable

[Jeff Layton]

There's no need to keep initing a new context for every function. Just
do it once and reuse as needed.

Signed-off-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
 cifs.upcall.c | 61 ++++++++++++++++-------------------------------------------
 1 file changed, 16 insertions(+), 45 deletions(-)

diff --git a/cifs.upcall.c b/cifs.upcall.c
index d0f6d089d8e1..8448d00f6061 100644
--- a/cifs.upcall.c
+++ b/cifs.upcall.c
@@ -52,7 +52,9 @@
 #include "spnego.h"
 #include "cifs_spnego.h"
 
-static const char *prog = "cifs.upcall";
+static krb5_context	context;
+static const char	*prog = "cifs.upcall";
+
 typedef enum _sectype {
 	NONE = 0,
 	KRB5,
@@ -69,9 +71,7 @@ typedef enum _sectype {
  * @return pointer to the realm
  *
  */
-
-static char *cifs_krb5_principal_get_realm(krb5_context context __attribute__ ((unused)),
-					   krb5_principal principal)
+static char *cifs_krb5_principal_get_realm(krb5_principal principal)
 {
 #ifdef HAVE_KRB5_PRINCIPAL_GET_REALM	/* Heimdal */
 	return krb5_principal_get_realm(context, principal);
@@ -104,7 +104,6 @@ krb5_auth_con_getsendsubkey(krb5_context context,
 /* does the ccache have a valid TGT? */
 static time_t get_tgt_time(const char *ccname)
 {
-	krb5_context context;
 	krb5_ccache ccache;
 	krb5_cc_cursor cur;
 	krb5_creds creds;
@@ -112,11 +111,6 @@ static time_t get_tgt_time(const char *ccname)
 	time_t credtime = 0;
 	char *realm = NULL;
 
-	if (krb5_init_context(&context)) {
-		syslog(LOG_DEBUG, "%s: unable to init krb5 context", __func__);
-		return 0;
-	}
-
 	if (krb5_cc_resolve(context, ccname, &ccache)) {
 		syslog(LOG_DEBUG, "%s: unable to resolve krb5 cache", __func__);
 		goto err_cache;
@@ -137,7 +131,7 @@ static time_t get_tgt_time(const char *ccname)
 		goto err_ccstart;
 	}
 
-	if ((realm = cifs_krb5_principal_get_realm(context, principal)) == NULL) {
+	if ((realm = cifs_krb5_principal_get_realm(principal)) == NULL) {
 		syslog(LOG_DEBUG, "%s: unable to get realm", __func__);
 		goto err_ccstart;
 	}
@@ -168,34 +162,23 @@ err_princ:
 #endif
 	krb5_cc_close(context, ccache);
 err_cache:
-	krb5_free_context(context);
 	return credtime;
 }
 
 static char *
 get_default_cc(void)
 {
-	krb5_error_code ret;
 	const char *ccname;
 	char *rcc = NULL;
-	krb5_context context = NULL;
-
-	ret = krb5_init_context(&context);
-	if (ret) {
-		syslog(LOG_DEBUG, "krb5_init_context: %d", (int)ret);
-		return NULL;
-	}
 
 	ccname = krb5_cc_default_name(context);
 	if (!ccname) {
 		syslog(LOG_DEBUG, "krb5_cc_default returned NULL.");
-		goto out_free_context;
+		return NULL;
 	}
 
 	if (get_tgt_time(ccname))
 		rcc = strdup(ccname);
-out_free_context:
-	krb5_free_context(context);
 	return rcc;
 }
 
@@ -203,7 +186,6 @@ out_free_context:
 static char *
 init_cc_from_keytab(const char *keytab_name, const char *user)
 {
-	krb5_context context = NULL;
 	krb5_error_code ret;
 	krb5_creds my_creds;
 	krb5_keytab keytab = NULL;
@@ -213,12 +195,6 @@ init_cc_from_keytab(const char *keytab_name, const char *user)
 
 	memset((char *) &my_creds, 0, sizeof(my_creds));
 
-	ret = krb5_init_context(&context);
-	if (ret) {
-		syslog(LOG_DEBUG, "krb5_init_context: %d", (int)ret);
-		goto icfk_cleanup;
-	}
-
 	if (keytab_name)
 		ret = krb5_kt_resolve(context, keytab_name, &keytab);
 	else
@@ -273,8 +249,6 @@ icfk_cleanup:
 		krb5_cc_close(context, cc);
 	if (keytab)
 		krb5_kt_close(context, keytab);
-	if (context)
-		krb5_free_context(context);
 	return ccname;
 }
 
@@ -284,7 +258,6 @@ cifs_krb5_get_req(const char *host, const char *ccname,
 {
 	krb5_error_code ret;
 	krb5_keyblock *tokb;
-	krb5_context context;
 	krb5_ccache ccache;
 	krb5_creds in_creds, *out_creds;
 	krb5_data apreq_pkt, in_data;
@@ -292,26 +265,19 @@ cifs_krb5_get_req(const char *host, const char *ccname,
 #if defined(HAVE_KRB5_AUTH_CON_SETADDRS) && defined(HAVE_KRB5_AUTH_CON_SET_REQ_CKSUMTYPE)
 	static const uint8_t gss_cksum[24] = { 0x10, 0x00, /* ... */};
 #endif
-
-	ret = krb5_init_context(&context);
-	if (ret) {
-		syslog(LOG_DEBUG, "%s: unable to init krb5 context", __func__);
-		return ret;
-	}
-
 	if (ccname) {
 		ret = krb5_cc_resolve(context, ccname, &ccache);
 		if (ret) {
 			syslog(LOG_DEBUG, "%s: unable to resolve %s to ccache\n",
 			       __func__, ccname);
-			goto out_free_context;
+			return ret;
 		}
 	} else {
 		ret = krb5_cc_default(context, &ccache);
 		if (ret) {
 			syslog(LOG_DEBUG, "%s: krb5_cc_default: %d",
 				__func__, (int)ret);
-			goto out_free_context;
+			return ret;
 		}
 	}
 
@@ -383,7 +349,6 @@ cifs_krb5_get_req(const char *host, const char *ccname,
 	/* MIT krb5 < 1.7 is missing the prototype, but still has the symbol */
 #if !HAVE_DECL_KRB5_AUTH_CON_SET_REQ_CKSUMTYPE
 	krb5_error_code krb5_auth_con_set_req_cksumtype(
-		krb5_context      context,
 		krb5_auth_context auth_context,
 		krb5_cksumtype    cksumtype);
 #endif
@@ -427,8 +392,6 @@ out_free_ccache:
 	krb5_cc_set_flags(context, ccache, KRB5_TC_OPENCLOSE);
 #endif
 	krb5_cc_close(context, ccache);
-out_free_context:
-	krb5_free_context(context);
 	return ret;
 }
 
@@ -866,6 +829,12 @@ int main(const int argc, char *const argv[])
 		goto out;
 	}
 
+	rc = krb5_init_context(&context);
+	if (rc) {
+		syslog(LOG_ERR, "unable to init krb5 context: %ld", rc);
+		goto out;
+	}
+
 	ccname = get_default_cc();
 	/* Couldn't find credcache? Try to use keytab */
 	if (ccname == NULL && arg.username != NULL)
@@ -1006,6 +975,8 @@ out:
 	}
 	data_blob_free(&secblob);
 	data_blob_free(&sess_key);
+	if (context)
+		krb5_free_context(context);
 	SAFE_FREE(ccname);
 	SAFE_FREE(arg.hostname);
 	SAFE_FREE(arg.ip);
-- 
2.7.4

[cifs-utils PATCHv2 4/6] cifs.upcall: remove KRB5_TC_OPENCLOSE

[Jeff Layton]

The header file says that this is deprecated, and all of the info I've
seen about it mentioned that it was for performance more than
correctness. It dates back to the original code dump from Igor, so I
think we're safe to just drop it at this point.

Signed-off-by: Jeff Layton <jlayton@samba.org>
---
 cifs.upcall.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/cifs.upcall.c b/cifs.upcall.c
index 8448d00f6061..a25833592440 100644
--- a/cifs.upcall.c
+++ b/cifs.upcall.c
@@ -157,9 +157,6 @@ err_endseq:
 err_ccstart:
 	krb5_free_principal(context, principal);
 err_princ:
-#if defined(KRB5_TC_OPENCLOSE)
-	krb5_cc_set_flags(context, ccache, KRB5_TC_OPENCLOSE);
-#endif
 	krb5_cc_close(context, ccache);
 err_cache:
 	return credtime;
@@ -388,9 +385,6 @@ out_free_creds:
 out_free_principal:
 	krb5_free_principal(context, in_creds.client);
 out_free_ccache:
-#if defined(KRB5_TC_OPENCLOSE)
-	krb5_cc_set_flags(context, ccache, KRB5_TC_OPENCLOSE);
-#endif
 	krb5_cc_close(context, ccache);
 	return ret;
 }
-- 
2.7.4

[cifs-utils PATCHv2 5/6] cifs.upcall: make get_tgt_time take a ccache arg

[Jeff Layton]

...instead of dealing with the ccname. Push resolution of the cache
into the caller.

Signed-off-by: Jeff Layton <jlayton@samba.org>
---
 cifs.upcall.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/cifs.upcall.c b/cifs.upcall.c
index a25833592440..a20576654a95 100644
--- a/cifs.upcall.c
+++ b/cifs.upcall.c
@@ -102,20 +102,14 @@ krb5_auth_con_getsendsubkey(krb5_context context,
 #endif
 
 /* does the ccache have a valid TGT? */
-static time_t get_tgt_time(const char *ccname)
+static time_t get_tgt_time(krb5_ccache ccache)
 {
-	krb5_ccache ccache;
 	krb5_cc_cursor cur;
 	krb5_creds creds;
 	krb5_principal principal;
 	time_t credtime = 0;
 	char *realm = NULL;
 
-	if (krb5_cc_resolve(context, ccname, &ccache)) {
-		syslog(LOG_DEBUG, "%s: unable to resolve krb5 cache", __func__);
-		goto err_cache;
-	}
-
 	if (krb5_cc_set_flags(context, ccache, 0)) {
 		syslog(LOG_DEBUG, "%s: unable to set flags", __func__);
 		goto err_cache;
@@ -156,8 +150,6 @@ err_endseq:
 	krb5_cc_end_seq_get(context, ccache, &cur);
 err_ccstart:
 	krb5_free_principal(context, principal);
-err_princ:
-	krb5_cc_close(context, ccache);
 err_cache:
 	return credtime;
 }
@@ -167,15 +159,22 @@ get_default_cc(void)
 {
 	const char *ccname;
 	char *rcc = NULL;
+	krb5_ccache ccache;
 
 	ccname = krb5_cc_default_name(context);
 	if (!ccname) {
-		syslog(LOG_DEBUG, "krb5_cc_default returned NULL.");
+		syslog(LOG_DEBUG, "%s: krb5_cc_default returned NULL.", __func__);
 		return NULL;
 	}
 
-	if (get_tgt_time(ccname))
+	if (krb5_cc_resolve(context, ccname, &ccache)) {
+		syslog(LOG_DEBUG, "%s: unable to resolve krb5 cache", __func__);
+		return NULL;
+	}
+
+	if (get_tgt_time(ccache))
 		rcc = strdup(ccname);
+	krb5_cc_close(context, ccache);
 	return rcc;
 }
 
-- 
2.7.4

[cifs-utils PATCHv2 6/6] cifs.upcall: stop passing around ccache name strings

[Jeff Layton]

Instead, get a ccache handle and pass that around. That way we can keep
the cache open until the program is complete as well.

Signed-off-by: Jeff Layton <jlayton@samba.org>
---
 cifs.upcall.c | 98 +++++++++++++++++++++++++----------------------------------
 1 file changed, 41 insertions(+), 57 deletions(-)

diff --git a/cifs.upcall.c b/cifs.upcall.c
index a20576654a95..8f146c92b4a5 100644
--- a/cifs.upcall.c
+++ b/cifs.upcall.c
@@ -117,7 +117,7 @@ static time_t get_tgt_time(krb5_ccache ccache)
 
 	if (krb5_cc_get_principal(context, ccache, &principal)) {
 		syslog(LOG_DEBUG, "%s: unable to get principal", __func__);
-		goto err_princ;
+		goto err_cache;
 	}
 
 	if (krb5_cc_start_seq_get(context, ccache, &cur)) {
@@ -154,32 +154,27 @@ err_cache:
 	return credtime;
 }
 
-static char *
+static krb5_ccache
 get_default_cc(void)
 {
-	const char *ccname;
-	char *rcc = NULL;
-	krb5_ccache ccache;
+	krb5_error_code ret;
+	krb5_ccache cc;
 
-	ccname = krb5_cc_default_name(context);
-	if (!ccname) {
-		syslog(LOG_DEBUG, "%s: krb5_cc_default returned NULL.", __func__);
+	ret = krb5_cc_default(context, &cc);
+	if (ret) {
+		syslog(LOG_DEBUG, "%s: krb5_cc_default returned %d", __func__, ret);
 		return NULL;
 	}
 
-	if (krb5_cc_resolve(context, ccname, &ccache)) {
-		syslog(LOG_DEBUG, "%s: unable to resolve krb5 cache", __func__);
-		return NULL;
+	if (!get_tgt_time(cc)) {
+		krb5_cc_close(context, cc);
+		cc = NULL;
 	}
-
-	if (get_tgt_time(ccache))
-		rcc = strdup(ccname);
-	krb5_cc_close(context, ccache);
-	return rcc;
+	return cc;
 }
 
 
-static char *
+static krb5_ccache
 init_cc_from_keytab(const char *keytab_name, const char *user)
 {
 	krb5_error_code ret;
@@ -187,7 +182,6 @@ init_cc_from_keytab(const char *keytab_name, const char *user)
 	krb5_keytab keytab = NULL;
 	krb5_principal me = NULL;
 	krb5_ccache cc = NULL;
-	char *ccname = NULL;
 
 	memset((char *) &my_creds, 0, sizeof(my_creds));
 
@@ -229,61 +223,46 @@ init_cc_from_keytab(const char *keytab_name, const char *user)
 	}
 
 	ret = krb5_cc_store_cred(context, cc, &my_creds);
-	if (ret)
+	if (ret) {
 		syslog(LOG_DEBUG, "krb5_cc_store_cred: %d", (int)ret);
-
-	ccname = strdup(krb5_cc_default_name(context));
-	if (ccname == NULL)
-		syslog(LOG_ERR, "Unable to allocate memory");
-icfk_cleanup:
+		goto icfk_cleanup;
+	}
+out:
 	my_creds.client = (krb5_principal)0;
 	krb5_free_cred_contents(context, &my_creds);
 
 	if (me)
 		krb5_free_principal(context, me);
-	if (cc)
-		krb5_cc_close(context, cc);
 	if (keytab)
 		krb5_kt_close(context, keytab);
-	return ccname;
+	return cc;
+icfk_cleanup:
+	if (cc) {
+		krb5_cc_close(context, cc);
+		cc = NULL;
+	}
+	goto out;
 }
 
 static int
-cifs_krb5_get_req(const char *host, const char *ccname,
+cifs_krb5_get_req(const char *host, krb5_ccache ccache,
 		  DATA_BLOB * mechtoken, DATA_BLOB * sess_key)
 {
 	krb5_error_code ret;
 	krb5_keyblock *tokb;
-	krb5_ccache ccache;
 	krb5_creds in_creds, *out_creds;
 	krb5_data apreq_pkt, in_data;
 	krb5_auth_context auth_context = NULL;
 #if defined(HAVE_KRB5_AUTH_CON_SETADDRS) && defined(HAVE_KRB5_AUTH_CON_SET_REQ_CKSUMTYPE)
 	static const uint8_t gss_cksum[24] = { 0x10, 0x00, /* ... */};
 #endif
-	if (ccname) {
-		ret = krb5_cc_resolve(context, ccname, &ccache);
-		if (ret) {
-			syslog(LOG_DEBUG, "%s: unable to resolve %s to ccache\n",
-			       __func__, ccname);
-			return ret;
-		}
-	} else {
-		ret = krb5_cc_default(context, &ccache);
-		if (ret) {
-			syslog(LOG_DEBUG, "%s: krb5_cc_default: %d",
-				__func__, (int)ret);
-			return ret;
-		}
-	}
-
 	memset(&in_creds, 0, sizeof(in_creds));
 
 	ret = krb5_cc_get_principal(context, ccache, &in_creds.client);
 	if (ret) {
 		syslog(LOG_DEBUG, "%s: unable to get client principal name",
 		       __func__);
-		goto out_free_ccache;
+		return ret;
 	}
 
 	ret = krb5_sname_to_principal(context, host, "cifs", KRB5_NT_UNKNOWN,
@@ -383,8 +362,6 @@ out_free_creds:
 	krb5_free_creds(context, out_creds);
 out_free_principal:
 	krb5_free_principal(context, in_creds.client);
-out_free_ccache:
-	krb5_cc_close(context, ccache);
 	return ret;
 }
 
@@ -410,7 +387,7 @@ out_free_ccache:
  */
 static int
 handle_krb5_mech(const char *oid, const char *host, DATA_BLOB * secblob,
-		 DATA_BLOB * sess_key, const char *ccname)
+		 DATA_BLOB * sess_key, krb5_ccache ccache)
 {
 	int retval;
 	DATA_BLOB tkt, tkt_wrapped;
@@ -418,7 +395,7 @@ handle_krb5_mech(const char *oid, const char *host, DATA_BLOB * secblob,
 	syslog(LOG_DEBUG, "%s: getting service ticket for %s", __func__, host);
 
 	/* get a kerberos ticket for the service and extract the session key */
-	retval = cifs_krb5_get_req(host, ccname, &tkt, sess_key);
+	retval = cifs_krb5_get_req(host, ccache, &tkt, sess_key);
 	if (retval) {
 		syslog(LOG_DEBUG, "%s: failed to obtain service ticket (%d)",
 		       __func__, retval);
@@ -709,12 +686,13 @@ int main(const int argc, char *const argv[])
 	unsigned int have;
 	long rc = 1;
 	int c, try_dns = 0, legacy_uid = 0;
-	char *buf, *ccname = NULL;
+	char *buf;
 	char hostbuf[NI_MAXHOST], *host;
 	struct decoded_args arg;
 	const char *oid;
 	uid_t uid;
 	char *keytab_name = NULL;
+	krb5_ccache ccache = NULL;
 
 	hostbuf[0] = '\0';
 	memset(&arg, 0, sizeof(arg));
@@ -828,10 +806,15 @@ int main(const int argc, char *const argv[])
 		goto out;
 	}
 
-	ccname = get_default_cc();
+	ccache = get_default_cc();
 	/* Couldn't find credcache? Try to use keytab */
-	if (ccname == NULL && arg.username != NULL)
-		ccname = init_cc_from_keytab(keytab_name, arg.username);
+	if (ccache == NULL && arg.username != NULL)
+		ccache = init_cc_from_keytab(keytab_name, arg.username);
+
+	if (ccache == NULL) {
+		rc = 1;
+		goto out;
+	}
 
 	host = arg.hostname;
 
@@ -859,7 +842,7 @@ int main(const int argc, char *const argv[])
 
 retry_new_hostname:
 		lowercase_string(host);
-		rc = handle_krb5_mech(oid, host, &secblob, &sess_key, ccname);
+		rc = handle_krb5_mech(oid, host, &secblob, &sess_key, ccache);
 		if (!rc)
 			break;
 
@@ -904,7 +887,7 @@ retry_new_hostname:
 				break;
 			}
 
-			rc = handle_krb5_mech(oid, fqdn, &secblob, &sess_key, ccname);
+			rc = handle_krb5_mech(oid, fqdn, &secblob, &sess_key, ccache);
 			if (!rc)
 				break;
 		}
@@ -968,9 +951,10 @@ out:
 	}
 	data_blob_free(&secblob);
 	data_blob_free(&sess_key);
+	if (ccache)
+		krb5_cc_close(context, ccache);
 	if (context)
 		krb5_free_context(context);
-	SAFE_FREE(ccname);
 	SAFE_FREE(arg.hostname);
 	SAFE_FREE(arg.ip);
 	SAFE_FREE(arg.username);
-- 
2.7.4

Re: [cifs-utils PATCHv2 0/6] cifs.upcall: cleanup and overhaul of the cifs.upcall krb5 handling code

[Isaac Boukris]

Hello,

On Thu, Aug 25, 2016 at 5:17 PM, Jeff Layton <jlayton@samba.org> wrote:
> While this is a step in the right direction, what I think we might
> want to do longer-term is to make this use gss_init_sec_context
> instead of micromanaging it like we do now. The only part I'm a
> little unclear on is how to extract the session key in that case.


Coincidentally, I was thinking the same way recently that it could be
better to use GSSAPI in cifs upcall.
The idea is to save to trouble of SPNEGO wrapping and to allow real
KRB and NTLM negotiation in userspace.

After some fiddling I managed to make something work using MIT Kerberos library.
First I call gss_init_sec_context() with GSS_C_NO_OID so I get a raw
KRB token and it returns GSS_S_COMPLETE (since I didn't ask for mutual
auth and using KRB mech and not SPNEGO).

Then I call the below (found mentioned on mailing list archive):

maj = gss_inquire_sec_context_by_oid(&min, context,

GSS_C_INQ_SSPI_SESSION_KEY,
                                                         &skey);
if (GSS_ERROR(maj) || skey == GSS_C_NO_BUFFER_SET || !skey->count) {
    syslog(LOG_DEBUG, "%s: failed to inquire for session key (%d)",
__func__, maj);
    goto done;
}

*sess_key = data_blob(skey->elements[0].value, skey->elements[0].length);

This works against my test server (win2k3) since it seem to accept raw
KRB tokens (similar to servers that accept raw NTLMSSP I guess, the
token is probably passed to an equivalent of gss_accept_sec_context()
which can identify the mechanism).

However, it gets more complicated when it comes to SPNEGO.
First, when I call gss_init_sec_context with SPNEGO mech it will
always return GSS_S_CONTINUE_NEEDED as it need server response in
order to complete (even if mutual auth is not requested).
This causes (I think) the call to gss_inquire_sec_context_by_oid to fail.
Also, we need to support continuation anyway for mutual auth and NTLM fallback.

So I thought I'd use gss_export_sec_context() when we get
continuation-needed and send the serialized context back to the
kernel, which in its turn will send it back to the next upcall once it
receives server's response, which in its turn will import back the
context till it completes and then export the session key.
However, unfortunately the spec does not guarantee the export to
succeed with a partial context (not completed).
In practice the MIT library seem to allow export of a partial KRB
context (where the continuation is only needed for mutual auth) but
does not allow export of a partial SPNEGO context.
I've asked about it on MIT's IRC channel and was pointed out to PR
#356 which adds support for exporting partial SPNEGO context, so I'm
currently trying this patch but I'm unsure where this PR stands in
terms of MIT team priorities.

Thanks for reading :)

Regards,
Isaac B.

Re: [cifs-utils PATCHv2 0/6] cifs.upcall: cleanup and overhaul of the cifs.upcall krb5 handling code

[Jeff Layton]

On Thu, 2016-08-25 at 19:05 +0300, Isaac Boukris wrote:
> Hello,
> 
> On Thu, Aug 25, 2016 at 5:17 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> wrote:
> > 
> > While this is a step in the right direction, what I think we might
> > want to do longer-term is to make this use gss_init_sec_context
> > instead of micromanaging it like we do now. The only part I'm a
> > little unclear on is how to extract the session key in that case.
> 
> 
> Coincidentally, I was thinking the same way recently that it could be
> better to use GSSAPI in cifs upcall.
> The idea is to save to trouble of SPNEGO wrapping and to allow real
> KRB and NTLM negotiation in userspace.
> 
> After some fiddling I managed to make something work using MIT
> Kerberos library.
> First I call gss_init_sec_context() with GSS_C_NO_OID so I get a raw
> KRB token and it returns GSS_S_COMPLETE (since I didn't ask for
> mutual
> auth and using KRB mech and not SPNEGO).
> 
> Then I call the below (found mentioned on mailing list archive):
> 
> maj = gss_inquire_sec_context_by_oid(&min, context,
> 
> GSS_C_INQ_SSPI_SESSION_KEY,
>                                                          &skey);
> if (GSS_ERROR(maj) || skey == GSS_C_NO_BUFFER_SET || !skey->count) {
>     syslog(LOG_DEBUG, "%s: failed to inquire for session key (%d)",
> __func__, maj);
>     goto done;
> }
> 
> *sess_key = data_blob(skey->elements[0].value, skey-
> >elements[0].length);
> 
> This works against my test server (win2k3) since it seem to accept
> raw
> KRB tokens (similar to servers that accept raw NTLMSSP I guess, the
> token is probably passed to an equivalent of gss_accept_sec_context()
> which can identify the mechanism).
> 
> However, it gets more complicated when it comes to SPNEGO.
> First, when I call gss_init_sec_context with SPNEGO mech it will
> always return GSS_S_CONTINUE_NEEDED as it need server response in
> order to complete (even if mutual auth is not requested).
> This causes (I think) the call to gss_inquire_sec_context_by_oid to
> fail.
> Also, we need to support continuation anyway for mutual auth and NTLM
> fallback.
> 

Ahh that's interesting. I didn't realize we'd be stuck having to handle
a multi-stage negotiation if we use gss_init_sec_context. That is sort
of difficult to handle with the way the upcall currently works.

I may still play with it just to verify that I understand what you're
saying but it does make sense.

> So I thought I'd use gss_export_sec_context() when we get
> continuation-needed and send the serialized context back to the
> kernel, which in its turn will send it back to the next upcall once
> it
> receives server's response, which in its turn will import back the
> context till it completes and then export the session key.
> However, unfortunately the spec does not guarantee the export to
> succeed with a partial context (not completed).
> In practice the MIT library seem to allow export of a partial KRB
> context (where the continuation is only needed for mutual auth) but
> does not allow export of a partial SPNEGO context.
> I've asked about it on MIT's IRC channel and was pointed out to PR
> #356 which adds support for exporting partial SPNEGO context, so I'm
> currently trying this patch but I'm unsure where this PR stands in
> terms of MIT team priorities.
> 
> Thanks for reading :)
> 
> Regards,
> Isaac B.


If we're going to go down the road of changing the upcall to handle
that then I think I'd prefer to just change cifs.ko over to use
gssproxy directly, like knfsd already does.

It would be one less special snowflake to deal with and most of the
plumbing is already in place. We could leave the old upcall in place
for legacy userland case, and only do it if the gssproxy negotiation
failed. It's a bit of a project, but would be totally doable.

Thoughts?
-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [cifs-utils PATCHv2 0/6] cifs.upcall: cleanup and overhaul of the cifs.upcall krb5 handling code

[Isaac Boukris]

On Thu, Aug 25, 2016 at 7:44 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Thu, 2016-08-25 at 19:05 +0300, Isaac Boukris wrote:
>> However, it gets more complicated when it comes to SPNEGO.
>> First, when I call gss_init_sec_context with SPNEGO mech it will
>> always return GSS_S_CONTINUE_NEEDED as it need server response in
>> order to complete (even if mutual auth is not requested).
>> This causes (I think) the call to gss_inquire_sec_context_by_oid to
>> fail.
>> Also, we need to support continuation anyway for mutual auth and NTLM
>> fallback.
>>
>
> Ahh that's interesting. I didn't realize we'd be stuck having to handle
> a multi-stage negotiation if we use gss_init_sec_context. That is sort
> of difficult to handle with the way the upcall currently works.
>
> I may still play with it just to verify that I understand what you're
> saying but it does make sense.


You may take look at my (dirty) draft:
https://github.com/frenche/cifs-utils/tree/gssapi_upcall

> If we're going to go down the road of changing the upcall to handle
> that then I think I'd prefer to just change cifs.ko over to use
> gssproxy directly, like knfsd already does.
>
> It would be one less special snowflake to deal with and most of the
> plumbing is already in place. We could leave the old upcall in place
> for legacy userland case, and only do it if the gssproxy negotiation
> failed. It's a bit of a project, but would be totally doable.


The cifs kernel module will need some changes anyway to handle
multi-stage negotiation I think.
Using gssproxy sounds like an excellent idea, though I'm only vaguely
familiar with it so I'll have to look it up.

On the other hand I think there may still be an interest for stateless
mechanism (alongside) which wouldn't require a running daemon.
I'm trying to think of an alternative to exporting the gss context,
perhaps by keeping the upcall process running and continuing the
conversation with the kernel module somehow.

With best regards,
Isaac B.

Re: [cifs-utils PATCHv2 0/6] cifs.upcall: cleanup and overhaul of the cifs.upcall krb5 handling code

[Jeff Layton]

On Thu, 2016-08-25 at 22:59 +0300, Isaac Boukris wrote:
> On Thu, Aug 25, 2016 at 7:44 PM, Jeff Layton <jlayton@samba.org>
> wrote:
> > 
> > On Thu, 2016-08-25 at 19:05 +0300, Isaac Boukris wrote:
> > > 
> > > However, it gets more complicated when it comes to SPNEGO.
> > > First, when I call gss_init_sec_context with SPNEGO mech it will
> > > always return GSS_S_CONTINUE_NEEDED as it need server response in
> > > order to complete (even if mutual auth is not requested).
> > > This causes (I think) the call to gss_inquire_sec_context_by_oid
> > > to
> > > fail.
> > > Also, we need to support continuation anyway for mutual auth and
> > > NTLM
> > > fallback.
> > > 
> > 
> > Ahh that's interesting. I didn't realize we'd be stuck having to
> > handle
> > a multi-stage negotiation if we use gss_init_sec_context. That is
> > sort
> > of difficult to handle with the way the upcall currently works.
> > 
> > I may still play with it just to verify that I understand what
> > you're
> > saying but it does make sense.
> 
> 
> You may take look at my (dirty) draft:
> https://github.com/frenche/cifs-utils/tree/gssapi_upcall
> 

Neat. Do you have a companion kernel patch? I'm wondering about how to
handle backward compatibility there. I guess we could try SPNEGO first
and fall back to using the *KRB5 if that fails.

Another thing (not directly related) that I've been considering is
whether we can use anonymous krb5 creds to handle the initial mount for
sec=krb5 mounts. It would be nice to be able to do multiuser mounts
without any client side setup.

> > 
> > If we're going to go down the road of changing the upcall to handle
> > that then I think I'd prefer to just change cifs.ko over to use
> > gssproxy directly, like knfsd already does.
> > 
> > It would be one less special snowflake to deal with and most of the
> > plumbing is already in place. We could leave the old upcall in
> > place
> > for legacy userland case, and only do it if the gssproxy
> > negotiation
> > failed. It's a bit of a project, but would be totally doable.
> 
> 
> The cifs kernel module will need some changes anyway to handle
> multi-stage negotiation I think.
> Using gssproxy sounds like an excellent idea, though I'm only vaguely
> familiar with it so I'll have to look it up.
> 
> On the other hand I think there may still be an interest for
> stateless
> mechanism (alongside) which wouldn't require a running daemon.
> I'm trying to think of an alternative to exporting the gss context,
> perhaps by keeping the upcall process running and continuing the
> conversation with the kernel module somehow.
> 

Yeah, not having a running daemon is certainly simpler for users. 

-- 
Jeff Layton <jlayton@samba.org>

Re: [cifs-utils PATCHv2 0/6] cifs.upcall: cleanup and overhaul of the cifs.upcall krb5 handling code

[Simo]

On Thu, 2016-08-25 at 12:44 -0400, Jeff Layton wrote:
> On Thu, 2016-08-25 at 19:05 +0300, Isaac Boukris wrote:
> > 
> > Hello,
> > 
> > On Thu, Aug 25, 2016 at 5:17 PM, Jeff Layton <jlayton@samba.org>
> > wrote:
> > > 
> > > 
> > > While this is a step in the right direction, what I think we
> > > might
> > > want to do longer-term is to make this use gss_init_sec_context
> > > instead of micromanaging it like we do now. The only part I'm a
> > > little unclear on is how to extract the session key in that case.
> > 
> > Coincidentally, I was thinking the same way recently that it could
> > be
> > better to use GSSAPI in cifs upcall.
> > The idea is to save to trouble of SPNEGO wrapping and to allow real
> > KRB and NTLM negotiation in userspace.
> > 
> > After some fiddling I managed to make something work using MIT
> > Kerberos library.
> > First I call gss_init_sec_context() with GSS_C_NO_OID so I get a
> > raw
> > KRB token and it returns GSS_S_COMPLETE (since I didn't ask for
> > mutual
> > auth and using KRB mech and not SPNEGO).
> > 
> > Then I call the below (found mentioned on mailing list archive):
> > 
> > maj = gss_inquire_sec_context_by_oid(&min, context,
> > 
> > GSS_C_INQ_SSPI_SESSION_KEY,
> >                                                          &skey);
> > if (GSS_ERROR(maj) || skey == GSS_C_NO_BUFFER_SET || !skey->count)
> > {
> >     syslog(LOG_DEBUG, "%s: failed to inquire for session key (%d)",
> > __func__, maj);
> >     goto done;
> > }
> > 
> > *sess_key = data_blob(skey->elements[0].value, skey-
> > > 
> > > elements[0].length);
> > This works against my test server (win2k3) since it seem to accept
> > raw
> > KRB tokens (similar to servers that accept raw NTLMSSP I guess, the
> > token is probably passed to an equivalent of
> > gss_accept_sec_context()
> > which can identify the mechanism).
> > 
> > However, it gets more complicated when it comes to SPNEGO.
> > First, when I call gss_init_sec_context with SPNEGO mech it will
> > always return GSS_S_CONTINUE_NEEDED as it need server response in
> > order to complete (even if mutual auth is not requested).
> > This causes (I think) the call to gss_inquire_sec_context_by_oid to
> > fail.
> > Also, we need to support continuation anyway for mutual auth and
> > NTLM
> > fallback.
> > 
> Ahh that's interesting. I didn't realize we'd be stuck having to
> handle
> a multi-stage negotiation if we use gss_init_sec_context. That is
> sort
> of difficult to handle with the way the upcall currently works.
> 
> I may still play with it just to verify that I understand what you're
> saying but it does make sense.
> 
> > 
> > So I thought I'd use gss_export_sec_context() when we get
> > continuation-needed and send the serialized context back to the
> > kernel, which in its turn will send it back to the next upcall once
> > it
> > receives server's response, which in its turn will import back the
> > context till it completes and then export the session key.
> > However, unfortunately the spec does not guarantee the export to
> > succeed with a partial context (not completed).
> > In practice the MIT library seem to allow export of a partial KRB
> > context (where the continuation is only needed for mutual auth) but
> > does not allow export of a partial SPNEGO context.
> > I've asked about it on MIT's IRC channel and was pointed out to PR
> > #356 which adds support for exporting partial SPNEGO context, so
> > I'm
> > currently trying this patch but I'm unsure where this PR stands in
> > terms of MIT team priorities.
> > 
> > Thanks for reading :)
> > 
> > Regards,
> > Isaac B.
> 
> If we're going to go down the road of changing the upcall to handle
> that then I think I'd prefer to just change cifs.ko over to use
> gssproxy directly, like knfsd already does.
> 
> It would be one less special snowflake to deal with and most of the
> plumbing is already in place. We could leave the old upcall in place
> for legacy userland case, and only do it if the gssproxy negotiation
> failed. It's a bit of a project, but would be totally doable.
> 
> Thoughts?

Yes please.
We've been planning to do this forever but never found enough time to
do it.
But it is the correct way to go, it makes no sense to invent different
upstream protocols for the exact same problem when we already solved
it.

The only thing we need to add kernel side (besides interfaces in
cifs.ko to use sunrpc) is a few xdr unmarshalling functions that were
removed at the time of the knfsd submission because unused.

Simo.

Re: [cifs-utils PATCHv2 0/6] cifs.upcall: cleanup and overhaul of the cifs.upcall krb5 handling code

[Simo]

On Thu, 2016-08-25 at 16:51 -0400, Jeff Layton wrote:
> On Thu, 2016-08-25 at 22:59 +0300, Isaac Boukris wrote:
> > 
> > On Thu, Aug 25, 2016 at 7:44 PM, Jeff Layton <jlayton@samba.org>
> > wrote:
> > > 
> > > 
> > > On Thu, 2016-08-25 at 19:05 +0300, Isaac Boukris wrote:
> > > > 
> > > > 
> > > > However, it gets more complicated when it comes to SPNEGO.
> > > > First, when I call gss_init_sec_context with SPNEGO mech it
> > > > will
> > > > always return GSS_S_CONTINUE_NEEDED as it need server response
> > > > in
> > > > order to complete (even if mutual auth is not requested).
> > > > This causes (I think) the call to
> > > > gss_inquire_sec_context_by_oid
> > > > to
> > > > fail.
> > > > Also, we need to support continuation anyway for mutual auth
> > > > and
> > > > NTLM
> > > > fallback.
> > > > 
> > > Ahh that's interesting. I didn't realize we'd be stuck having to
> > > handle
> > > a multi-stage negotiation if we use gss_init_sec_context. That is
> > > sort
> > > of difficult to handle with the way the upcall currently works.
> > > 
> > > I may still play with it just to verify that I understand what
> > > you're
> > > saying but it does make sense.
> > 
> > You may take look at my (dirty) draft:
> > https://github.com/frenche/cifs-utils/tree/gssapi_upcall
> > 
> Neat. Do you have a companion kernel patch? I'm wondering about how
> to
> handle backward compatibility there. I guess we could try SPNEGO
> first
> and fall back to using the *KRB5 if that fails.
> 
> Another thing (not directly related) that I've been considering is
> whether we can use anonymous krb5 creds to handle the initial mount
> for
> sec=krb5 mounts. It would be nice to be able to do multiuser mounts
> without any client side setup.

You could use the workstation credentials if you are joined to a
domain, but even better would be to let gss-proxy decide it by telling
it you are acting as the "root" user, or passing an option in the gss-
proxy protocol.

Really please let's not add another half-assed "gssapi upcall mechanism
here".

> > 
> > > 
> > > 
> > > If we're going to go down the road of changing the upcall to
> > > handle
> > > that then I think I'd prefer to just change cifs.ko over to use
> > > gssproxy directly, like knfsd already does.
> > > 
> > > It would be one less special snowflake to deal with and most of
> > > the
> > > plumbing is already in place. We could leave the old upcall in
> > > place
> > > for legacy userland case, and only do it if the gssproxy
> > > negotiation
> > > failed. It's a bit of a project, but would be totally doable.
> > 
> > The cifs kernel module will need some changes anyway to handle
> > multi-stage negotiation I think.
> > Using gssproxy sounds like an excellent idea, though I'm only
> > vaguely
> > familiar with it so I'll have to look it up.
> > 
> > On the other hand I think there may still be an interest for
> > stateless
> > mechanism (alongside) which wouldn't require a running daemon.
> > I'm trying to think of an alternative to exporting the gss context,
> > perhaps by keeping the upcall process running and continuing the
> > conversation with the kernel module somehow.
> >  
> Yeah, not having a running daemon is certainly simpler for users. 

Although it may be simpler it makes little sense, in most cases when
you use kerberos you are probably joined to a domain and already have
winbindd or sssd running, and kerberos/gssapi is complex enough that
having to run a daemon is not a big deal.
However you could always try to start the gssproxy daemon if it is not
running. It won't do much good if it has no configuration, however
distributions that allow to start it via the kernel could make sure to
have defaults that would work as well as any upcall mechanism you can
come up with.

I say this because we are working to add more and more features to the
system to handle GSSAPI well, both for krb5 and NTLM protocols, and
other less known ones eventually, and doing it in one place and
avoiding duplications everywhere would concentrate efforts and get you
expert help, as well as a consistent experience across protocols (both
nfs and cifs).

Simo.

Re: [cifs-utils PATCHv2 0/6] cifs.upcall: cleanup and overhaul of the cifs.upcall krb5 handling code

[Jeff Layton]

On Fri, 2016-08-26 at 08:53 -0400, Simo wrote:
> On Thu, 2016-08-25 at 16:51 -0400, Jeff Layton wrote:
> > 
> > On Thu, 2016-08-25 at 22:59 +0300, Isaac Boukris wrote:
> > > 
> > > 
> > > > > > On Thu, Aug 25, 2016 at 7:44 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> > > wrote:
> > > > 
> > > > 
> > > > 
> > > > On Thu, 2016-08-25 at 19:05 +0300, Isaac Boukris wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > However, it gets more complicated when it comes to SPNEGO.
> > > > > First, when I call gss_init_sec_context with SPNEGO mech it
> > > > > will
> > > > > always return GSS_S_CONTINUE_NEEDED as it need server response
> > > > > in
> > > > > order to complete (even if mutual auth is not requested).
> > > > > This causes (I think) the call to
> > > > > gss_inquire_sec_context_by_oid
> > > > > to
> > > > > fail.
> > > > > Also, we need to support continuation anyway for mutual auth
> > > > > and
> > > > > NTLM
> > > > > fallback.
> > > > > 
> > > > Ahh that's interesting. I didn't realize we'd be stuck having to
> > > > handle
> > > > a multi-stage negotiation if we use gss_init_sec_context. That is
> > > > sort
> > > > of difficult to handle with the way the upcall currently works.
> > > > 
> > > > I may still play with it just to verify that I understand what
> > > > you're
> > > > saying but it does make sense.
> > > 
> > > You may take look at my (dirty) draft:
> > > https://github.com/frenche/cifs-utils/tree/gssapi_upcall
> > > 
> > Neat. Do you have a companion kernel patch? I'm wondering about how
> > to
> > handle backward compatibility there. I guess we could try SPNEGO
> > first
> > and fall back to using the *KRB5 if that fails.
> > 
> > Another thing (not directly related) that I've been considering is
> > whether we can use anonymous krb5 creds to handle the initial mount
> > for
> > sec=krb5 mounts. It would be nice to be able to do multiuser mounts
> > without any client side setup.
> 
> You could use the workstation credentials if you are joined to a
> domain, but even better would be to let gss-proxy decide it by telling
> it you are acting as the "root" user, or passing an option in the gss-
> proxy protocol.
> 
> Really please let's not add another half-assed "gssapi upcall mechanism
> here".
> 

Agreed. I'm planning to merge these cleanup patches unless anyone has
objections, but I'm less inclined to spend time on a new upcall
mechanism as well.

I wouldn't NAK it outright, since there is some value in not needing to
run a daemon, but I'd rather just wire cifs.ko up to gssproxy and be
done with it.

> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > 
> > > > If we're going to go down the road of changing the upcall to
> > > > handle
> > > > that then I think I'd prefer to just change cifs.ko over to use
> > > > gssproxy directly, like knfsd already does.
> > > > 
> > > > It would be one less special snowflake to deal with and most of
> > > > the
> > > > plumbing is already in place. We could leave the old upcall in
> > > > place
> > > > for legacy userland case, and only do it if the gssproxy
> > > > negotiation
> > > > failed. It's a bit of a project, but would be totally doable.
> > > 
> > > The cifs kernel module will need some changes anyway to handle
> > > multi-stage negotiation I think.
> > > Using gssproxy sounds like an excellent idea, though I'm only
> > > vaguely
> > > familiar with it so I'll have to look it up.
> > > 
> > > On the other hand I think there may still be an interest for
> > > stateless
> > > mechanism (alongside) which wouldn't require a running daemon.
> > > I'm trying to think of an alternative to exporting the gss context,
> > > perhaps by keeping the upcall process running and continuing the
> > > conversation with the kernel module somehow.
> > >  
> > Yeah, not having a running daemon is certainly simpler for users. 
> 
> Although it may be simpler it makes little sense, in most cases when
> you use kerberos you are probably joined to a domain and already have
> winbindd or sssd running, and kerberos/gssapi is complex enough that
> having to run a daemon is not a big deal.
> However you could always try to start the gssproxy daemon if it is not
> running. It won't do much good if it has no configuration, however
> distributions that allow to start it via the kernel could make sure to
> have defaults that would work as well as any upcall mechanism you can
> come up with.
> 
> I say this because we are working to add more and more features to the
> system to handle GSSAPI well, both for krb5 and NTLM protocols, and
> other less known ones eventually, and doing it in one place and
> avoiding duplications everywhere would concentrate efforts and get you
> expert help, as well as a consistent experience across protocols (both
> nfs and cifs).
> 
> 

Yeah, that makes sense. There's also the problem that the UMH upcalls
(like that used by the keys API) don't play well in containers
currently. That may change one day, but for now that's also a good
reason to avoid them.

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [cifs-utils PATCHv2 0/6] cifs.upcall: cleanup and overhaul of the cifs.upcall krb5 handling code

[Simo]

On Fri, 2016-08-26 at 09:44 -0400, Jeff Layton wrote:
> On Fri, 2016-08-26 at 08:53 -0400, Simo wrote:
> > 
> > On Thu, 2016-08-25 at 16:51 -0400, Jeff Layton wrote:
> > > 
> > > 
> > > On Thu, 2016-08-25 at 22:59 +0300, Isaac Boukris wrote:
> > > > 
> > > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > On Thu, Aug 25, 2016 at 7:44 PM, Jeff Layton <jlayton@sam
> > > > > > > ba.org>
> > > > wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > On Thu, 2016-08-25 at 19:05 +0300, Isaac Boukris wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > However, it gets more complicated when it comes to SPNEGO.
> > > > > > First, when I call gss_init_sec_context with SPNEGO mech it
> > > > > > will
> > > > > > always return GSS_S_CONTINUE_NEEDED as it need server
> > > > > > response
> > > > > > in
> > > > > > order to complete (even if mutual auth is not requested).
> > > > > > This causes (I think) the call to
> > > > > > gss_inquire_sec_context_by_oid
> > > > > > to
> > > > > > fail.
> > > > > > Also, we need to support continuation anyway for mutual
> > > > > > auth
> > > > > > and
> > > > > > NTLM
> > > > > > fallback.
> > > > > > 
> > > > > Ahh that's interesting. I didn't realize we'd be stuck having
> > > > > to
> > > > > handle
> > > > > a multi-stage negotiation if we use gss_init_sec_context.
> > > > > That is
> > > > > sort
> > > > > of difficult to handle with the way the upcall currently
> > > > > works.
> > > > > 
> > > > > I may still play with it just to verify that I understand
> > > > > what
> > > > > you're
> > > > > saying but it does make sense.
> > > > You may take look at my (dirty) draft:
> > > > https://github.com/frenche/cifs-utils/tree/gssapi_upcall
> > > > 
> > > Neat. Do you have a companion kernel patch? I'm wondering about
> > > how
> > > to
> > > handle backward compatibility there. I guess we could try SPNEGO
> > > first
> > > and fall back to using the *KRB5 if that fails.
> > > 
> > > Another thing (not directly related) that I've been considering
> > > is
> > > whether we can use anonymous krb5 creds to handle the initial
> > > mount
> > > for
> > > sec=krb5 mounts. It would be nice to be able to do multiuser
> > > mounts
> > > without any client side setup.
> > You could use the workstation credentials if you are joined to a
> > domain, but even better would be to let gss-proxy decide it by
> > telling
> > it you are acting as the "root" user, or passing an option in the
> > gss-
> > proxy protocol.
> > 
> > Really please let's not add another half-assed "gssapi upcall
> > mechanism
> > here".
> > 
> Agreed. I'm planning to merge these cleanup patches unless anyone has
> objections, but I'm less inclined to spend time on a new upcall
> mechanism as well.
> 
> I wouldn't NAK it outright, since there is some value in not needing
> to
> run a daemon, but I'd rather just wire cifs.ko up to gssproxy and be
> done with it.
> 
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > If we're going to go down the road of changing the upcall to
> > > > > handle
> > > > > that then I think I'd prefer to just change cifs.ko over to
> > > > > use
> > > > > gssproxy directly, like knfsd already does.
> > > > > 
> > > > > It would be one less special snowflake to deal with and most
> > > > > of
> > > > > the
> > > > > plumbing is already in place. We could leave the old upcall
> > > > > in
> > > > > place
> > > > > for legacy userland case, and only do it if the gssproxy
> > > > > negotiation
> > > > > failed. It's a bit of a project, but would be totally doable.
> > > > The cifs kernel module will need some changes anyway to handle
> > > > multi-stage negotiation I think.
> > > > Using gssproxy sounds like an excellent idea, though I'm only
> > > > vaguely
> > > > familiar with it so I'll have to look it up.
> > > > 
> > > > On the other hand I think there may still be an interest for
> > > > stateless
> > > > mechanism (alongside) which wouldn't require a running daemon.
> > > > I'm trying to think of an alternative to exporting the gss
> > > > context,
> > > > perhaps by keeping the upcall process running and continuing
> > > > the
> > > > conversation with the kernel module somehow.
> > > >  
> > > Yeah, not having a running daemon is certainly simpler for
> > > users. 
> > Although it may be simpler it makes little sense, in most cases
> > when
> > you use kerberos you are probably joined to a domain and already
> > have
> > winbindd or sssd running, and kerberos/gssapi is complex enough
> > that
> > having to run a daemon is not a big deal.
> > However you could always try to start the gssproxy daemon if it is
> > not
> > running. It won't do much good if it has no configuration, however
> > distributions that allow to start it via the kernel could make sure
> > to
> > have defaults that would work as well as any upcall mechanism you
> > can
> > come up with.
> > 
> > I say this because we are working to add more and more features to
> > the
> > system to handle GSSAPI well, both for krb5 and NTLM protocols, and
> > other less known ones eventually, and doing it in one place and
> > avoiding duplications everywhere would concentrate efforts and get
> > you
> > expert help, as well as a consistent experience across protocols
> > (both
> > nfs and cifs).
> > 
> > 
> Yeah, that makes sense. There's also the problem that the UMH upcalls
> (like that used by the keys API) don't play well in containers
> currently. That may change one day, but for now that's also a good
> reason to avoid them.

Indeed, with a socket we can be a lot more container friendly (or
unfriendly if we want) by just bind mounting the right stuff in the
right place.

We are going to handle ccaches for containers via SSSD in the not too
distant future and we are adding (yet another I know) ccache mechanism
to address better all use cases that are cropping up.
We'll make sure this interacts properly with gssproxy, how other tools
will handle it will vary...

Simo.

Re: [cifs-utils PATCHv2 0/6] cifs.upcall: cleanup and overhaul of the cifs.upcall krb5 handling code

[Isaac Boukris]

On Thu, Aug 25, 2016 at 7:44 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> Ahh that's interesting. I didn't realize we'd be stuck having to handle
> a multi-stage negotiation if we use gss_init_sec_context. That is sort
> of difficult to handle with the way the upcall currently works.


Note that technically I think the current upcall code could still
switch from krb5 api to gssapi, but the manual spngo wrapping will
still be needed.

Re: [cifs-utils PATCHv2 0/6] cifs.upcall: cleanup and overhaul of the cifs.upcall krb5 handling code

[Isaac Boukris]

On Fri, Aug 26, 2016 at 3:46 PM, Simo <simo-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> The only thing we need to add kernel side (besides interfaces in
> cifs.ko to use sunrpc) is a few xdr unmarshalling functions that were
> removed at the time of the knfsd submission because unused.


Are you referring to missing gssx*init_sec_context? Could this removed
code be found online?

Re: [cifs-utils PATCHv2 0/6] cifs.upcall: cleanup and overhaul of the cifs.upcall krb5 handling code

[Simo]

On Sat, 2016-08-27 at 21:06 +0300, Isaac Boukris wrote:
> On Fri, Aug 26, 2016 at 3:46 PM, Simo <simo-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> > 
> > The only thing we need to add kernel side (besides interfaces in
> > cifs.ko to use sunrpc) is a few xdr unmarshalling functions that
> > were
> > removed at the time of the knfsd submission because unused.
> 
> 
> Are you referring to missing gssx*init_sec_context? Could this
> removed
> code be found online?

I do not remember if I had fully implemented that one, but I know I
implemented a number of other unmarshalling/marshalling functions that
will be needed to implement it.

I can dig the code tomorrow, I should still have a git tree around with
the old patches if the garbage collector have not run in the meanwhile.

Simo.