[PATCH] cifs: Support for an upcall to map SID to an uid and a gid

[PATCH] cifs: Support for an upcall to map SID to an uid and a gid

[shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w]

From: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>


Use a cifs upcall to map a SID to either an uid or a gid using
winbind.

There is a corrosponding patch for the cifs.upcall binary in cifs-utils rpm 
that is being posted also.

A new type of key, cifs_acl_key_type, is used.
To map a SID, which can be either a Onwer SID or a Group SID, key
description starts with the string "os" or "gs" followed by SID converted
to a string. Without "os" or "gs", cifs.upcall does not know whether
SID needs to be mapped to either an uid or a gid.

Once a key is instantiated, get rid of it since cifs does not need to
use in any way.
winbind does the id mapping and looks up name for the newly mapped
SID/uid or SID/gid combination.

For now, cifs.upcall is only used to map a SID to an id (uid or gid) but
it would be used to obtain an SID (string) for an id.

An entry such as this 

create  cifs.cifs_acl   *       *               /usr/sbin/cifs.upcall %k

is needed in the file /etc/request-key.conf.


Signed-off-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 fs/cifs/cifsacl.c |  117 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 fs/cifs/cifsacl.h |    8 ++++
 fs/cifs/cifsfs.c  |    7 +++
 3 files changed, 122 insertions(+), 10 deletions(-)

diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
index a520091..d3ac6c8 100644
--- a/fs/cifs/cifsacl.c
+++ b/fs/cifs/cifsacl.c
@@ -23,6 +23,9 @@
 
 #include <linux/fs.h>
 #include <linux/slab.h>
+#include <linux/string.h>
+#include <keys/user-type.h>
+#include <linux/key-type.h>
 #include "cifspdu.h"
 #include "cifsglob.h"
 #include "cifsacl.h"
@@ -52,6 +55,102 @@ static const struct cifs_sid sid_authusers = {
 /* group users */
 static const struct cifs_sid sid_user = {1, 2 , {0, 0, 0, 0, 0, 5}, {} };
 
+static int
+cifs_acl_key_instantiate(struct key *key, const void *data, size_t datalen)
+{
+	char *payload;
+
+	payload = kmalloc(datalen, GFP_KERNEL);
+	if (!payload)
+		return -ENOMEM;
+
+	memcpy(payload, data, datalen);
+	key->payload.data = payload;
+	return 0;
+}
+
+static void
+cifs_acl_key_destroy(struct key *key)
+{
+	kfree(key->payload.data);
+}
+
+struct key_type cifs_acl_key_type = {
+	.name        = "cifs.cifs_acl",
+	.instantiate = cifs_acl_key_instantiate,
+	.destroy     = cifs_acl_key_destroy,
+	.describe    = user_describe,
+	.match       = user_match,
+};
+
+static void
+sid_to_str(struct cifs_sid *sidptr, char *sidstr)
+{
+	int i;
+	unsigned long saval;
+	char *strptr;
+
+	strptr = sidstr;
+
+	sprintf(strptr, "%s", "S");
+	strptr = sidstr + strlen(sidstr);
+
+	sprintf(strptr, "-%d", sidptr->revision);
+	strptr = sidstr + strlen(sidstr);
+
+	for (i = 0; i < 6; ++i) {
+		if (sidptr->authority[i]) {
+			sprintf(strptr, "-%d", sidptr->authority[i]);
+			strptr = sidstr + strlen(sidstr);
+		}
+	}
+
+	for (i = 0; i < sidptr->num_subauth; ++i) {
+		saval = le32_to_cpu(sidptr->sub_auth[i]);
+		sprintf(strptr, "-%ld", saval);
+		strptr = sidstr + strlen(sidstr);
+	}
+}
+
+static int
+sid_to_id(struct cifs_sid *psid, struct cifs_fattr *fattr, uint sidtype)
+{
+	int rc = 0;
+	char *sidstr, *strptr;
+	struct key *idkey;
+
+	sidstr = kzalloc(SIDLEN, GFP_KERNEL);
+	if (!sidstr)
+		return -ENOMEM;
+	strptr = sidstr;
+
+	if (sidtype == SIDOWNER)
+		sprintf(strptr, "%s", "os:");
+	else if (sidtype == SIDGROUP)
+		sprintf(strptr, "%s", "gs:");
+	else {
+		rc = -EINVAL;
+		goto idresolve_err;
+	}
+	strptr = sidstr + strlen(sidstr);
+
+	sid_to_str(psid, strptr);
+
+	idkey = request_key(&cifs_acl_key_type, sidstr, "");
+	if (IS_ERR(idkey))
+		cFYI(1, "%s: idkey error: %d\n", __func__, -ENOKEY);
+	else {
+		if (sidtype == SIDOWNER)
+			fattr->cf_uid = *(unsigned long *)idkey->payload.value;
+		else if (sidtype == SIDGROUP)
+			fattr->cf_gid = *(unsigned long *)idkey->payload.value;
+		key_put(idkey);
+	}
+
+idresolve_err:
+	kfree(sidstr);
+	return rc;
+}
 
 int match_sid(struct cifs_sid *ctsid)
 {
@@ -438,7 +537,6 @@ static int set_chmod_dacl(struct cifs_acl *pndacl, struct cifs_sid *pownersid,
 	return 0;
 }
 
-
 static int parse_sid(struct cifs_sid *psid, char *end_of_acl)
 {
 	/* BB need to add parm so we can store the SID BB */
@@ -476,7 +574,7 @@ static int parse_sid(struct cifs_sid *psid, char *end_of_acl)
 static int parse_sec_desc(struct cifs_ntsd *pntsd, int acl_len,
 			  struct cifs_fattr *fattr)
 {
-	int rc;
+	int rc = 0;
 	struct cifs_sid *owner_sid_ptr, *group_sid_ptr;
 	struct cifs_acl *dacl_ptr; /* no need for SACL ptr */
 	char *end_of_acl = ((char *)pntsd) + acl_len;
@@ -500,10 +598,16 @@ static int parse_sec_desc(struct cifs_ntsd *pntsd, int acl_len,
 	rc = parse_sid(owner_sid_ptr, end_of_acl);
 	if (rc)
 		return rc;
+	rc = sid_to_id(owner_sid_ptr, fattr, SIDOWNER);
+	if (rc)
+		cFYI(1, "Can't resolve SID to an uid");
 
 	rc = parse_sid(group_sid_ptr, end_of_acl);
 	if (rc)
 		return rc;
+	rc = sid_to_id(group_sid_ptr, fattr, SIDGROUP);
+	if (rc)
+		cFYI(1, "Can't resolve SID to a gid");
 
 	if (dacloffset)
 		parse_dacl(dacl_ptr, end_of_acl, owner_sid_ptr,
@@ -511,14 +615,7 @@ static int parse_sec_desc(struct cifs_ntsd *pntsd, int acl_len,
 	else
 		cFYI(1, "no ACL"); /* BB grant all or default perms? */
 
-/*	cifscred->uid = owner_sid_ptr->rid;
-	cifscred->gid = group_sid_ptr->rid;
-	memcpy((void *)(&(cifscred->osid)), (void *)owner_sid_ptr,
-			sizeof(struct cifs_sid));
-	memcpy((void *)(&(cifscred->gsid)), (void *)group_sid_ptr,
-			sizeof(struct cifs_sid)); */
-
-	return 0;
+	return rc;
 }
 
 
diff --git a/fs/cifs/cifsacl.h b/fs/cifs/cifsacl.h
index 6c8096c..74e69ca 100644
--- a/fs/cifs/cifsacl.h
+++ b/fs/cifs/cifsacl.h
@@ -39,6 +39,10 @@
 #define ACCESS_ALLOWED	0
 #define ACCESS_DENIED	1
 
+#define SIDOWNER 1
+#define SIDGROUP 2
+#define SIDLEN 150 /* S- 1 revision- 6 authorities- max 5 sub authorities */
+
 struct cifs_ntsd {
 	__le16 revision; /* revision level */
 	__le16 type;
@@ -76,6 +80,10 @@ struct cifs_wksid {
 
 #ifdef CONFIG_CIFS_EXPERIMENTAL
 
+#ifdef __KERNEL__
+extern struct key_type cifs_acl_key_type;
+#endif /* KERNEL */
+
 extern int match_sid(struct cifs_sid *);
 extern int compare_sids(const struct cifs_sid *, const struct cifs_sid *);
 
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 56a4b75..2753d54 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -46,6 +46,7 @@
 #include <linux/mm.h>
 #include <linux/key-type.h>
 #include "cifs_spnego.h"
+#include "cifsacl.h"
 #include "fscache.h"
 #define CIFS_MAGIC_NUMBER 0xFF534D42	/* the first four bytes of SMB PDUs */
 
@@ -973,11 +974,16 @@ init_cifs(void)
 	rc = register_key_type(&cifs_spnego_key_type);
 	if (rc)
 		goto out_unregister_filesystem;
+	rc = register_key_type(&cifs_acl_key_type);
+	if (rc)
+		goto out_unregister_keytype;
 #endif
 
 	return 0;
 
 #ifdef CONFIG_CIFS_UPCALL
+out_unregister_keytype:
+	unregister_key_type(&cifs_spnego_key_type);
 out_unregister_filesystem:
 	unregister_filesystem(&cifs_fs_type);
 #endif
@@ -1004,6 +1010,7 @@ exit_cifs(void)
 	cifs_dfs_release_automount_timer();
 #endif
 #ifdef CONFIG_CIFS_UPCALL
+	unregister_key_type(&cifs_acl_key_type);
 	unregister_key_type(&cifs_spnego_key_type);
 #endif
 	unregister_filesystem(&cifs_fs_type);
-- 
1.6.0.2

Re: [PATCH] cifs: Support for an upcall to map SID to an uid and a gid

[Shirish Pargaonkar]

On Sat, Dec 11, 2010 at 10:17 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Tue,  7 Dec 2010 11:11:12 -0600
> shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>
>> From: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>>
>> Use a cifs upcall to map a SID to either an uid or a gid using
>> winbind.
>>
>> There is a corrosponding patch for the cifs.upcall binary in cifs-utils rpm
>> that is being posted also.
>>
>> A new type of key, cifs_acl_key_type, is used.
>> To map a SID, which can be either a Onwer SID or a Group SID, key
>> description starts with the string "os" or "gs" followed by SID converted
>> to a string. Without "os" or "gs", cifs.upcall does not know whether
>> SID needs to be mapped to either an uid or a gid.
>>
>> Once a key is instantiated, get rid of it since cifs does not need to
>> use in any way.
>> winbind does the id mapping and looks up name for the newly mapped
>> SID/uid or SID/gid combination.
>>
>> For now, cifs.upcall is only used to map a SID to an id (uid or gid) but
>> it would be used to obtain an SID (string) for an id.
>>
>> An entry such as this
>>
>> create  cifs.cifs_acl   *       *               /usr/sbin/cifs.upcall %k
>>
>> is needed in the file /etc/request-key.conf.
>>
>>
>> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  fs/cifs/cifsacl.c |  117 ++++++++++++++++++++++++++++++++++++++++++++++++-----
>>  fs/cifs/cifsacl.h |    8 ++++
>>  fs/cifs/cifsfs.c  |    7 +++
>>  3 files changed, 122 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
>> index a520091..d3ac6c8 100644
>> --- a/fs/cifs/cifsacl.c
>> +++ b/fs/cifs/cifsacl.c
>> @@ -23,6 +23,9 @@
>>
>>  #include <linux/fs.h>
>>  #include <linux/slab.h>
>> +#include <linux/string.h>
>> +#include <keys/user-type.h>
>> +#include <linux/key-type.h>
>>  #include "cifspdu.h"
>>  #include "cifsglob.h"
>>  #include "cifsacl.h"
>> @@ -52,6 +55,102 @@ static const struct cifs_sid sid_authusers = {
>>  /* group users */
>>  static const struct cifs_sid sid_user = {1, 2 , {0, 0, 0, 0, 0, 5}, {} };
>>
>> +static int
>> +cifs_acl_key_instantiate(struct key *key, const void *data, size_t datalen)
>> +{
>> +     char *payload;
>> +
>> +     payload = kmalloc(datalen, GFP_KERNEL);
>> +     if (!payload)
>> +             return -ENOMEM;
>> +
>> +     memcpy(payload, data, datalen);
>> +     key->payload.data = payload;
>> +     return 0;
>> +}
>> +
>> +static void
>> +cifs_acl_key_destroy(struct key *key)
>> +{
>> +     kfree(key->payload.data);
>> +}
>> +
>> +struct key_type cifs_acl_key_type = {
>> +     .name        = "cifs.cifs_acl",
>> +     .instantiate = cifs_acl_key_instantiate,
>> +     .destroy     = cifs_acl_key_destroy,
>> +     .describe    = user_describe,
>> +     .match       = user_match,
>> +};
>> +
>
> Nit: these don't have so much to do with ACL's per-se, as much as
> idmapping. Maybe these functions and the key type should be called
> "cifs_idmap_*" and "cifs.idmap" ? It might be clearer to admins what
> this is for.

Yes, will change the name as you suggested, that sounds right, rather
that acl.

>
>> +static void
>> +sid_to_str(struct cifs_sid *sidptr, char *sidstr)
>> +{
>> +     int i;
>> +     unsigned long saval;
>> +     char *strptr;
>> +
>> +     strptr = sidstr;
>> +
>> +     sprintf(strptr, "%s", "S");
>> +     strptr = sidstr + strlen(sidstr);
>> +
>> +     sprintf(strptr, "-%d", sidptr->revision);
>> +     strptr = sidstr + strlen(sidstr);
>> +
>> +     for (i = 0; i < 6; ++i) {
>> +             if (sidptr->authority[i]) {
>> +                     sprintf(strptr, "-%d", sidptr->authority[i]);
>> +                     strptr = sidstr + strlen(sidstr);
>> +             }
>> +     }
>> +
>> +     for (i = 0; i < sidptr->num_subauth; ++i) {
>> +             saval = le32_to_cpu(sidptr->sub_auth[i]);
>> +             sprintf(strptr, "-%ld", saval);
>> +             strptr = sidstr + strlen(sidstr);
>> +     }
>> +}
>> +
>> +static int
>> +sid_to_id(struct cifs_sid *psid, struct cifs_fattr *fattr, uint sidtype)
>> +{
>> +     int rc = 0;
>> +     char *sidstr, *strptr;
>> +     struct key *idkey;
>> +
>> +     sidstr = kzalloc(SIDLEN, GFP_KERNEL);
>> +     if (!sidstr)
>> +             return -ENOMEM;
>> +     strptr = sidstr;
>> +
>> +     if (sidtype == SIDOWNER)
>> +             sprintf(strptr, "%s", "os:");
>> +     else if (sidtype == SIDGROUP)
>> +             sprintf(strptr, "%s", "gs:");
>> +     else {
>> +             rc = -EINVAL;
>> +             goto idresolve_err;
>> +     }
>> +     strptr = sidstr + strlen(sidstr);
>> +
>> +     sid_to_str(psid, strptr);
>> +
>> +     idkey = request_key(&cifs_acl_key_type, sidstr, "");
>> +     if (IS_ERR(idkey))
>> +             cFYI(1, "%s: idkey error: %d\n", __func__, -ENOKEY);
>> +     else {
>> +             if (sidtype == SIDOWNER)
>> +                     fattr->cf_uid = *(unsigned long *)idkey->payload.value;
>> +             else if (sidtype == SIDGROUP)
>> +                     fattr->cf_gid = *(unsigned long *)idkey->payload.value;
>> +             key_put(idkey);
>> +     }
>> +
>> +idresolve_err:
>> +     kfree(sidstr);
>> +     return rc;
>> +}
>>
>
> What about security? With this code, it'll be possible for a user to
> "stuff" the cache with idmapping. In the situation where the kernel is
> trying to enforce permissions on the client, it'll be possible to
> trick it into mapping a sid to a uid of your choosing. I think you need
> to guard against that.

That means cifs module will have to know what was the range of ids for
uids and gids to be allocated by winbind as dictated by entries in
smb.conf and if a returned id happens not to be in that range, discard it
and assign default id of 0 (root).
winbind know the range from smb.conf, it could be any smb.conf file,
not necessarily under /etc/samba/smb.conf.
How would cifs module know that range.
Not sure if winbind has an API to query the ranges.  And even if it had,
it is possible that winbind deamon itself is not running.

>
> You'll also be doing an upcall every time a different user needs to map
> a SID to a UID/GID.
>
> Finally, calling into the keys API every time you want to map an ID
> sounds rather inefficient. This might take quite some time if you were
> doing "ls -l" on a large directory.
>
> Would it be better to consider taking the info in the key and
> populating a different cache? You could register it with a shrinker and
> prune off LRU entries if memory gets tight.
>

Will look into this.  One thing that concerns me is if a cached etnry
for a SID with its name and an id (either an uid or a gid), if that SID
now represents a different object and has differernt name, would
not cached info be incorrect?  Not sure if this can ever happen
or how would it happen and if it does, what would be a trigger
for a cache revalidation and purges!

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

Re: [PATCH] cifs: Support for an upcall to map SID to an uid and a gid

[Jeff Layton]

On Sat, 11 Dec 2010 15:47:12 -0600
Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:

> On Sat, Dec 11, 2010 at 10:17 AM, Jeff Layton <jlayton@samba.org> wrote:
> > On Tue,  7 Dec 2010 11:11:12 -0600
> > shirishpargaonkar@gmail.com wrote:
> >
> >> From: Shirish Pargaonkar <shirishpargaonkar@gmail.com>
> >>
> >>
> >> Use a cifs upcall to map a SID to either an uid or a gid using
> >> winbind.
> >>
> >> There is a corrosponding patch for the cifs.upcall binary in cifs-utils rpm
> >> that is being posted also.
> >>
> >> A new type of key, cifs_acl_key_type, is used.
> >> To map a SID, which can be either a Onwer SID or a Group SID, key
> >> description starts with the string "os" or "gs" followed by SID converted
> >> to a string. Without "os" or "gs", cifs.upcall does not know whether
> >> SID needs to be mapped to either an uid or a gid.
> >>
> >> Once a key is instantiated, get rid of it since cifs does not need to
> >> use in any way.
> >> winbind does the id mapping and looks up name for the newly mapped
> >> SID/uid or SID/gid combination.
> >>
> >> For now, cifs.upcall is only used to map a SID to an id (uid or gid) but
> >> it would be used to obtain an SID (string) for an id.
> >>
> >> An entry such as this
> >>
> >> create  cifs.cifs_acl   *       *               /usr/sbin/cifs.upcall %k
> >>
> >> is needed in the file /etc/request-key.conf.
> >>
> >>
> >> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar@gmail.com>
> >> ---
> >>  fs/cifs/cifsacl.c |  117 ++++++++++++++++++++++++++++++++++++++++++++++++-----
> >>  fs/cifs/cifsacl.h |    8 ++++
> >>  fs/cifs/cifsfs.c  |    7 +++
> >>  3 files changed, 122 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
> >> index a520091..d3ac6c8 100644
> >> --- a/fs/cifs/cifsacl.c
> >> +++ b/fs/cifs/cifsacl.c
> >> @@ -23,6 +23,9 @@
> >>
> >>  #include <linux/fs.h>
> >>  #include <linux/slab.h>
> >> +#include <linux/string.h>
> >> +#include <keys/user-type.h>
> >> +#include <linux/key-type.h>
> >>  #include "cifspdu.h"
> >>  #include "cifsglob.h"
> >>  #include "cifsacl.h"
> >> @@ -52,6 +55,102 @@ static const struct cifs_sid sid_authusers = {
> >>  /* group users */
> >>  static const struct cifs_sid sid_user = {1, 2 , {0, 0, 0, 0, 0, 5}, {} };
> >>
> >> +static int
> >> +cifs_acl_key_instantiate(struct key *key, const void *data, size_t datalen)
> >> +{
> >> +     char *payload;
> >> +
> >> +     payload = kmalloc(datalen, GFP_KERNEL);
> >> +     if (!payload)
> >> +             return -ENOMEM;
> >> +
> >> +     memcpy(payload, data, datalen);
> >> +     key->payload.data = payload;
> >> +     return 0;
> >> +}
> >> +
> >> +static void
> >> +cifs_acl_key_destroy(struct key *key)
> >> +{
> >> +     kfree(key->payload.data);
> >> +}
> >> +
> >> +struct key_type cifs_acl_key_type = {
> >> +     .name        = "cifs.cifs_acl",
> >> +     .instantiate = cifs_acl_key_instantiate,
> >> +     .destroy     = cifs_acl_key_destroy,
> >> +     .describe    = user_describe,
> >> +     .match       = user_match,
> >> +};
> >> +
> >
> > Nit: these don't have so much to do with ACL's per-se, as much as
> > idmapping. Maybe these functions and the key type should be called
> > "cifs_idmap_*" and "cifs.idmap" ? It might be clearer to admins what
> > this is for.
> 
> Yes, will change the name as you suggested, that sounds right, rather
> that acl.
> 
> >
> >> +static void
> >> +sid_to_str(struct cifs_sid *sidptr, char *sidstr)
> >> +{
> >> +     int i;
> >> +     unsigned long saval;
> >> +     char *strptr;
> >> +
> >> +     strptr = sidstr;
> >> +
> >> +     sprintf(strptr, "%s", "S");
> >> +     strptr = sidstr + strlen(sidstr);
> >> +
> >> +     sprintf(strptr, "-%d", sidptr->revision);
> >> +     strptr = sidstr + strlen(sidstr);
> >> +
> >> +     for (i = 0; i < 6; ++i) {
> >> +             if (sidptr->authority[i]) {
> >> +                     sprintf(strptr, "-%d", sidptr->authority[i]);
> >> +                     strptr = sidstr + strlen(sidstr);
> >> +             }
> >> +     }
> >> +
> >> +     for (i = 0; i < sidptr->num_subauth; ++i) {
> >> +             saval = le32_to_cpu(sidptr->sub_auth[i]);
> >> +             sprintf(strptr, "-%ld", saval);
> >> +             strptr = sidstr + strlen(sidstr);
> >> +     }
> >> +}
> >> +
> >> +static int
> >> +sid_to_id(struct cifs_sid *psid, struct cifs_fattr *fattr, uint sidtype)
> >> +{
> >> +     int rc = 0;
> >> +     char *sidstr, *strptr;
> >> +     struct key *idkey;
> >> +
> >> +     sidstr = kzalloc(SIDLEN, GFP_KERNEL);
> >> +     if (!sidstr)
> >> +             return -ENOMEM;
> >> +     strptr = sidstr;
> >> +
> >> +     if (sidtype == SIDOWNER)
> >> +             sprintf(strptr, "%s", "os:");
> >> +     else if (sidtype == SIDGROUP)
> >> +             sprintf(strptr, "%s", "gs:");
> >> +     else {
> >> +             rc = -EINVAL;
> >> +             goto idresolve_err;
> >> +     }
> >> +     strptr = sidstr + strlen(sidstr);
> >> +
> >> +     sid_to_str(psid, strptr);
> >> +
> >> +     idkey = request_key(&cifs_acl_key_type, sidstr, "");
> >> +     if (IS_ERR(idkey))
> >> +             cFYI(1, "%s: idkey error: %d\n", __func__, -ENOKEY);
> >> +     else {
> >> +             if (sidtype == SIDOWNER)
> >> +                     fattr->cf_uid = *(unsigned long *)idkey->payload.value;
> >> +             else if (sidtype == SIDGROUP)
> >> +                     fattr->cf_gid = *(unsigned long *)idkey->payload.value;
> >> +             key_put(idkey);
> >> +     }
> >> +
> >> +idresolve_err:
> >> +     kfree(sidstr);
> >> +     return rc;
> >> +}
> >>
> >
> > What about security? With this code, it'll be possible for a user to
> > "stuff" the cache with idmapping. In the situation where the kernel is
> > trying to enforce permissions on the client, it'll be possible to
> > trick it into mapping a sid to a uid of your choosing. I think you need
> > to guard against that.
> 
> That means cifs module will have to know what was the range of ids for
> uids and gids to be allocated by winbind as dictated by entries in
> smb.conf and if a returned id happens not to be in that range, discard it
> and assign default id of 0 (root).
> winbind know the range from smb.conf, it could be any smb.conf file,
> not necessarily under /etc/samba/smb.conf.
> How would cifs module know that range.
> Not sure if winbind has an API to query the ranges.  And even if it had,
> it is possible that winbind deamon itself is not running.
> 

Maybe I wasn't clear... The danger here is that I have the ability to
put info of my own choosing into my keyring via an add_key() syscall
from userspace.

You need to do something like the override_creds trick that dns_query
does.

> >
> > You'll also be doing an upcall every time a different user needs to map
> > a SID to a UID/GID.
> >
> > Finally, calling into the keys API every time you want to map an ID
> > sounds rather inefficient. This might take quite some time if you were
> > doing "ls -l" on a large directory.
> >
> > Would it be better to consider taking the info in the key and
> > populating a different cache? You could register it with a shrinker and
> > prune off LRU entries if memory gets tight.
> >
> 
> Will look into this.  One thing that concerns me is if a cached etnry
> for a SID with its name and an id (either an uid or a gid), if that SID
> now represents a different object and has differernt name, would
> not cached info be incorrect?  Not sure if this can ever happen
> or how would it happen and if it does, what would be a trigger
> for a cache revalidation and purges!
> 

Sure, mappings can change. But, you still have the same problem with
what you're proposing in these patches. The userspace program isn't
setting a timeout on the key. Once a mapping is put in the keyring,
it's there until it's revoked. You probably want to set a max TTL for
the entries in the cache regardless of what scheme is used.

idmap lookups need to be pretty fast as you'll be calling them a lot.
Consider the case of "ls -l" on a large directory. Some slowness for
the upcall to populate the entry in the cache is probably acceptable.
Slowness on subsequent cached lookups of that same entry will be
painful.

-- 
Jeff Layton <jlayton@samba.org>

Re: [PATCH] cifs: Support for an upcall to map SID to an uid and a gid

[Richard Sharpe]

On Sat, Dec 11, 2010 at 7:30 PM, Jeff Layton <jlayton@samba.org> wrote:
>>
>> Will look into this.  One thing that concerns me is if a cached etnry
>> for a SID with its name and an id (either an uid or a gid), if that SID
>> now represents a different object and has differernt name, would
>> not cached info be incorrect?  Not sure if this can ever happen
>> or how would it happen and if it does, what would be a trigger
>> for a cache revalidation and purges!
>>
>
> Sure, mappings can change. But, you still have the same problem with
> what you're proposing in these patches. The userspace program isn't
> setting a timeout on the key. Once a mapping is put in the keyring,
> it's there until it's revoked. You probably want to set a max TTL for
> the entries in the cache regardless of what scheme is used.

I was under the impression that SIDs are never reused. Perhaps I am mistaken.

-- 
Regards,
Richard Sharpe

Re: [PATCH] cifs: Support for an upcall to map SID to an uid and a gid

[Jeff Layton]

On Sat, 11 Dec 2010 19:57:11 -0500
Richard Sharpe <realrichardsharpe-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Sat, Dec 11, 2010 at 7:30 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> >>
> >> Will look into this.  One thing that concerns me is if a cached etnry
> >> for a SID with its name and an id (either an uid or a gid), if that SID
> >> now represents a different object and has differernt name, would
> >> not cached info be incorrect?  Not sure if this can ever happen
> >> or how would it happen and if it does, what would be a trigger
> >> for a cache revalidation and purges!
> >>
> >
> > Sure, mappings can change. But, you still have the same problem with
> > what you're proposing in these patches. The userspace program isn't
> > setting a timeout on the key. Once a mapping is put in the keyring,
> > it's there until it's revoked. You probably want to set a max TTL for
> > the entries in the cache regardless of what scheme is used.
> 
> I was under the impression that SIDs are never reused. Perhaps I am mistaken.
> 

That may be, but the mapping of a SID is dependent upon settings in
config files that could change. It seems reasonable to me to only cache
these mappings for a period of time in the event that they do. That
period of time could default to being rather long and be tunable.

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

Re: [PATCH] cifs: Support for an upcall to map SID to an uid and a gid

[Andrew Bartlett]

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

On Sat, 2010-12-11 at 22:11 -0500, Jeff Layton wrote:
> On Sat, 11 Dec 2010 19:57:11 -0500
> Richard Sharpe <realrichardsharpe-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > On Sat, Dec 11, 2010 at 7:30 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> > >>
> > >> Will look into this.  One thing that concerns me is if a cached etnry
> > >> for a SID with its name and an id (either an uid or a gid), if that SID
> > >> now represents a different object and has differernt name, would
> > >> not cached info be incorrect?  Not sure if this can ever happen
> > >> or how would it happen and if it does, what would be a trigger
> > >> for a cache revalidation and purges!
> > >>
> > >
> > > Sure, mappings can change. But, you still have the same problem with
> > > what you're proposing in these patches. The userspace program isn't
> > > setting a timeout on the key. Once a mapping is put in the keyring,
> > > it's there until it's revoked. You probably want to set a max TTL for
> > > the entries in the cache regardless of what scheme is used.
> > 
> > I was under the impression that SIDs are never reused. Perhaps I am mistaken.
> > 
> 
> That may be, but the mapping of a SID is dependent upon settings in
> config files that could change. It seems reasonable to me to only cache
> these mappings for a period of time in the event that they do. That
> period of time could default to being rather long and be tunable.

I think that instead some explicit signal should be made to indicate
that a mapping has changed, so you don't have to worry about cache
times.  It should change *very* rarely and only on specific
administrator intervention.  We do a lot of things to avoid this
happening in the normal course of events. 

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org
Samba Developer, Cisco Inc.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] cifs: Support for an upcall to map SID to an uid and a gid

[Jeff Layton]

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

On Sun, 12 Dec 2010 14:48:04 +1100
Andrew Bartlett <abartlet-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:

> On Sat, 2010-12-11 at 22:11 -0500, Jeff Layton wrote:
> > On Sat, 11 Dec 2010 19:57:11 -0500
> > Richard Sharpe <realrichardsharpe-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > 
> > > On Sat, Dec 11, 2010 at 7:30 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> > > >>
> > > >> Will look into this.  One thing that concerns me is if a cached etnry
> > > >> for a SID with its name and an id (either an uid or a gid), if that SID
> > > >> now represents a different object and has differernt name, would
> > > >> not cached info be incorrect?  Not sure if this can ever happen
> > > >> or how would it happen and if it does, what would be a trigger
> > > >> for a cache revalidation and purges!
> > > >>
> > > >
> > > > Sure, mappings can change. But, you still have the same problem with
> > > > what you're proposing in these patches. The userspace program isn't
> > > > setting a timeout on the key. Once a mapping is put in the keyring,
> > > > it's there until it's revoked. You probably want to set a max TTL for
> > > > the entries in the cache regardless of what scheme is used.
> > > 
> > > I was under the impression that SIDs are never reused. Perhaps I am mistaken.
> > > 
> > 
> > That may be, but the mapping of a SID is dependent upon settings in
> > config files that could change. It seems reasonable to me to only cache
> > these mappings for a period of time in the event that they do. That
> > period of time could default to being rather long and be tunable.
> 
> I think that instead some explicit signal should be made to indicate
> that a mapping has changed, so you don't have to worry about cache
> times.  It should change *very* rarely and only on specific
> administrator intervention.  We do a lot of things to avoid this
> happening in the normal course of events. 
> 

What would provide this signal? winbindd? I suppose we could add a knob
or something under /sys that tells cifs to dump the idmap cache.

We would also have to consider however how to deal with someone running
an old winbindd that doesn't signal the kernel properly.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] cifs: Support for an upcall to map SID to an uid and a gid

[Andrew Bartlett]

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

On Sun, 2010-12-12 at 06:39 -0500, Jeff Layton wrote:
> On Sun, 12 Dec 2010 14:48:04 +1100
> Andrew Bartlett <abartlet-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> 
> > On Sat, 2010-12-11 at 22:11 -0500, Jeff Layton wrote:
> > > On Sat, 11 Dec 2010 19:57:11 -0500
> > > Richard Sharpe <realrichardsharpe-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > > 
> > > > On Sat, Dec 11, 2010 at 7:30 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> > > > >>
> > > > >> Will look into this.  One thing that concerns me is if a cached etnry
> > > > >> for a SID with its name and an id (either an uid or a gid), if that SID
> > > > >> now represents a different object and has differernt name, would
> > > > >> not cached info be incorrect?  Not sure if this can ever happen
> > > > >> or how would it happen and if it does, what would be a trigger
> > > > >> for a cache revalidation and purges!
> > > > >>
> > > > >
> > > > > Sure, mappings can change. But, you still have the same problem with
> > > > > what you're proposing in these patches. The userspace program isn't
> > > > > setting a timeout on the key. Once a mapping is put in the keyring,
> > > > > it's there until it's revoked. You probably want to set a max TTL for
> > > > > the entries in the cache regardless of what scheme is used.
> > > > 
> > > > I was under the impression that SIDs are never reused. Perhaps I am mistaken.
> > > > 
> > > 
> > > That may be, but the mapping of a SID is dependent upon settings in
> > > config files that could change. It seems reasonable to me to only cache
> > > these mappings for a period of time in the event that they do. That
> > > period of time could default to being rather long and be tunable.
> > 
> > I think that instead some explicit signal should be made to indicate
> > that a mapping has changed, so you don't have to worry about cache
> > times.  It should change *very* rarely and only on specific
> > administrator intervention.  We do a lot of things to avoid this
> > happening in the normal course of events. 
> > 
> 
> What would provide this signal? winbindd? I suppose we could add a knob
> or something under /sys that tells cifs to dump the idmap cache.

I think a /sys knob seems appropriate, perhaps easily sent a command
option on the same utility used for the upcall?

> We would also have to consider however how to deal with someone running
> an old winbindd that doesn't signal the kernel properly.

That's a very interesting question, as after a manual reconfiguration
perhaps even winbind might not know it changed.  It depends how deeply
the administrator changed things (changing the idmap_rid config settings
might matter for example).  I'll let others who deal with idmap more
often comment. 

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org
Samba Developer, Cisco Inc.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] cifs: Support for an upcall to map SID to an uid and a gid

[Jeff Layton]

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

On Mon, 13 Dec 2010 14:22:09 +1100
Andrew Bartlett <abartlet-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:

> On Sun, 2010-12-12 at 06:39 -0500, Jeff Layton wrote:
> > On Sun, 12 Dec 2010 14:48:04 +1100
> > Andrew Bartlett <abartlet-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> > 
> > > On Sat, 2010-12-11 at 22:11 -0500, Jeff Layton wrote:
> > > > On Sat, 11 Dec 2010 19:57:11 -0500
> > > > Richard Sharpe <realrichardsharpe-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > > > 
> > > > > On Sat, Dec 11, 2010 at 7:30 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> > > > > >>
> > > > > >> Will look into this.  One thing that concerns me is if a cached etnry
> > > > > >> for a SID with its name and an id (either an uid or a gid), if that SID
> > > > > >> now represents a different object and has differernt name, would
> > > > > >> not cached info be incorrect?  Not sure if this can ever happen
> > > > > >> or how would it happen and if it does, what would be a trigger
> > > > > >> for a cache revalidation and purges!
> > > > > >>
> > > > > >
> > > > > > Sure, mappings can change. But, you still have the same problem with
> > > > > > what you're proposing in these patches. The userspace program isn't
> > > > > > setting a timeout on the key. Once a mapping is put in the keyring,
> > > > > > it's there until it's revoked. You probably want to set a max TTL for
> > > > > > the entries in the cache regardless of what scheme is used.
> > > > > 
> > > > > I was under the impression that SIDs are never reused. Perhaps I am mistaken.
> > > > > 
> > > > 
> > > > That may be, but the mapping of a SID is dependent upon settings in
> > > > config files that could change. It seems reasonable to me to only cache
> > > > these mappings for a period of time in the event that they do. That
> > > > period of time could default to being rather long and be tunable.
> > > 
> > > I think that instead some explicit signal should be made to indicate
> > > that a mapping has changed, so you don't have to worry about cache
> > > times.  It should change *very* rarely and only on specific
> > > administrator intervention.  We do a lot of things to avoid this
> > > happening in the normal course of events. 
> > > 
> > 
> > What would provide this signal? winbindd? I suppose we could add a knob
> > or something under /sys that tells cifs to dump the idmap cache.
> 
> I think a /sys knob seems appropriate, perhaps easily sent a command
> option on the same utility used for the upcall?
> 
> > We would also have to consider however how to deal with someone running
> > an old winbindd that doesn't signal the kernel properly.
> 
> That's a very interesting question, as after a manual reconfiguration
> perhaps even winbind might not know it changed.  It depends how deeply
> the administrator changed things (changing the idmap_rid config settings
> might matter for example).  I'll let others who deal with idmap more
> often comment. 
> 

The other option is just to have a manual knob that flushes the cache,
and add something like this to the cifs.upcall manpage: "If you change
your idmapping configuration, then you'll probably also want to flush
the idmap cache." Maybe it's a rare enough thing that we shouldn't
sweat trying to make it too automatic.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] cifs: Support for an upcall to map SID to an uid and a gid

[Shirish Pargaonkar]

On Mon, Dec 13, 2010 at 5:16 AM, Jeff Layton <jlayton@samba.org> wrote:
> On Mon, 13 Dec 2010 14:22:09 +1100
> Andrew Bartlett <abartlet@samba.org> wrote:
>
>> On Sun, 2010-12-12 at 06:39 -0500, Jeff Layton wrote:
>> > On Sun, 12 Dec 2010 14:48:04 +1100
>> > Andrew Bartlett <abartlet@samba.org> wrote:
>> >
>> > > On Sat, 2010-12-11 at 22:11 -0500, Jeff Layton wrote:
>> > > > On Sat, 11 Dec 2010 19:57:11 -0500
>> > > > Richard Sharpe <realrichardsharpe@gmail.com> wrote:
>> > > >
>> > > > > On Sat, Dec 11, 2010 at 7:30 PM, Jeff Layton <jlayton@samba.org> wrote:
>> > > > > >>
>> > > > > >> Will look into this.  One thing that concerns me is if a cached etnry
>> > > > > >> for a SID with its name and an id (either an uid or a gid), if that SID
>> > > > > >> now represents a different object and has differernt name, would
>> > > > > >> not cached info be incorrect?  Not sure if this can ever happen
>> > > > > >> or how would it happen and if it does, what would be a trigger
>> > > > > >> for a cache revalidation and purges!
>> > > > > >>
>> > > > > >
>> > > > > > Sure, mappings can change. But, you still have the same problem with
>> > > > > > what you're proposing in these patches. The userspace program isn't
>> > > > > > setting a timeout on the key. Once a mapping is put in the keyring,
>> > > > > > it's there until it's revoked. You probably want to set a max TTL for
>> > > > > > the entries in the cache regardless of what scheme is used.
>> > > > >
>> > > > > I was under the impression that SIDs are never reused. Perhaps I am mistaken.
>> > > > >
>> > > >
>> > > > That may be, but the mapping of a SID is dependent upon settings in
>> > > > config files that could change. It seems reasonable to me to only cache
>> > > > these mappings for a period of time in the event that they do. That
>> > > > period of time could default to being rather long and be tunable.
>> > >
>> > > I think that instead some explicit signal should be made to indicate
>> > > that a mapping has changed, so you don't have to worry about cache
>> > > times.  It should change *very* rarely and only on specific
>> > > administrator intervention.  We do a lot of things to avoid this
>> > > happening in the normal course of events.
>> > >
>> >
>> > What would provide this signal? winbindd? I suppose we could add a knob
>> > or something under /sys that tells cifs to dump the idmap cache.
>>
>> I think a /sys knob seems appropriate, perhaps easily sent a command
>> option on the same utility used for the upcall?
>>
>> > We would also have to consider however how to deal with someone running
>> > an old winbindd that doesn't signal the kernel properly.
>>
>> That's a very interesting question, as after a manual reconfiguration
>> perhaps even winbind might not know it changed.  It depends how deeply
>> the administrator changed things (changing the idmap_rid config settings
>> might matter for example).  I'll let others who deal with idmap more
>> often comment.
>>
>
> The other option is just to have a manual knob that flushes the cache,
> and add something like this to the cifs.upcall manpage: "If you change
> your idmapping configuration, then you'll probably also want to flush
> the idmap cache." Maybe it's a rare enough thing that we shouldn't
> sweat trying to make it too automatic.
>
> --
> Jeff Layton <jlayton@samba.org>
>

# date; cat /proc/keys | grep cifs
Tue Dec 14 16:16:20 CST 2010
26fb212d I-----     1 perm 1f030000     0     0 keyring   .cifs_idmap: empty
2fb33fb5 IR----     2 expd 1f030000     0     0 keyring   .cifs_idmap: empty

# date; cat /proc/keys | grep cifs
Tue Dec 14 16:16:29 CST 2010
26fb212d I-----     1 perm 1f030000     0     0 keyring   .cifs_idmap: 2/4
2fb33fb5 IR----     2 expd 1f030000     0     0 keyring   .cifs_idmap: empty

# date; cat /proc/keys | grep cifs
Tue Dec 14 16:20:10 CST 2010
26fb212d I-----     1 perm 1f030000     0     0 keyring   .cifs_idmap: 2/4
2fb33fb5 IR----     2 expd 1f030000     0     0 keyring   .cifs_idmap: empty


Should not these two entries expire after a minute if

key->expiry = KEY_EXPIRY_TIMEOUT;

in function cifs_idmap_key_instantiate()

if

#define KEY_EXPIRY_TIMEOUT 60*HZ

I was expecting this to be empty instead of 2/4 after more than a minute
of inactivity.

26fb212d I-----     1 perm 1f030000     0     0 keyring   .cifs_idmap: 2/4

Re: [PATCH] cifs: Support for an upcall to map SID to an uid and a gid

[Andrew Bartlett]

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

On Mon, 2010-12-13 at 06:16 -0500, Jeff Layton wrote:

> 
> The other option is just to have a manual knob that flushes the cache,
> and add something like this to the cifs.upcall manpage: "If you change
> your idmapping configuration, then you'll probably also want to flush
> the idmap cache." Maybe it's a rare enough thing that we shouldn't
> sweat trying to make it too automatic.

I think that's a reasonable approach. 

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org
Samba Developer, Cisco Inc.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]