| Message ID | 1249324456-8194-1-git-send-email-jlayton@redhat.com |
|---|---|
| State | New |
| Headers | show |
Wouldn't it give more flexibility to the upcall program if we sent both or changed the name (and be less confusing) e.g. to "mount_uid" or something else distinct On Mon, Aug 3, 2009 at 1:34 PM, Jeff Layton <jlayton@redhat.com> wrote: > The ownership of files on the mount has no direct relationship to the > credentials used to do the mount. Instead of sending the uid > corresponding to the owner of files on the mount, instead send the real > uid of the process that initiated the upcall. > > Usually this will be the real uid of the process running /bin/mount. > Eventually however, we want to be able to establish new sessions for > users that walk into a cifs mount. For that we need the real uid of > those users. > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > --- > fs/cifs/cifs_spnego.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/fs/cifs/cifs_spnego.c b/fs/cifs/cifs_spnego.c > index 8ec7736..1d0d8fc 100644 > --- a/fs/cifs/cifs_spnego.c > +++ b/fs/cifs/cifs_spnego.c > @@ -140,7 +140,7 @@ cifs_get_spnego_key(struct cifsSesInfo *sesInfo) > goto out; > > dp = description + strlen(description); > - sprintf(dp, ";uid=0x%x", sesInfo->linux_uid); > + sprintf(dp, ";uid=0x%x", current_uid()); > > dp = description + strlen(description); > sprintf(dp, ";user=%s", sesInfo->userName); > -- > 1.6.0.6 > >
On Mon, 3 Aug 2009 13:41:03 -0500 Steve French <smfrench@gmail.com> wrote: > Wouldn't it give more flexibility to the upcall program if we sent both or > changed the name (and be less confusing) e.g. to "mount_uid" or something > else distinct > We already send a bunch of fields that we don't actually use... ...and TBH, I'm a little leery of the existing situation. Might someone be able to trick the kernel into using a credcache to which the user doesn't have access by setting the right uid= option on a mount? One of the main problems we have with "uid=" is that it means too much. I'd like to remove it from having any meaning at all to the krb5 auth subsys. Seems saner and more secure to me.
On Mon, Aug 3, 2009 at 1:52 PM, Jeff Layton <jlayton@redhat.com> wrote: > be able to trick the kernel into using a credcache to which the user doesn't > have access by setting the right uid= option on a mount? ... > Seems saner and more secure to me. Are you saying "saner" to rename "uid=" for the purposes of the upcall... not just the change to pass the "uid of the process that initiated the upcall" (not the "owner of the files" uid) (If so, I agree)
On Mon, 3 Aug 2009 14:40:19 -0500 Steve French <smfrench@gmail.com> wrote: > On Mon, Aug 3, 2009 at 1:52 PM, Jeff Layton <jlayton@redhat.com> wrote: > > be able to trick the kernel into using a credcache to which the user > doesn't > > have access by setting the right uid= option on a mount? > ... > > Seems saner and more secure to me. > > Are you saying "saner" to rename "uid=" for the purposes of the upcall... > not > just the change to pass the "uid of the process that initiated the upcall" > (not > the "owner of the files" uid) (If so, I agree) > > I'm not sure I follow what you're saying... I'm saying that we should pass the real uid of the process that initiated the upcall to the upcall. There's no point in passing the uid of the process that owns the files on the mount to the upcall. I don't see the point of renaming the uid= "key" here. Can you elaborate as to why you think it would be good to do so?
On Mon, Aug 3, 2009 at 3:20 PM, Jeff Layton <jlayton@redhat.com> wrote: > On Mon, 3 Aug 2009 14:40:19 -0500 > Steve French <smfrench@gmail.com> wrote: > > > On Mon, Aug 3, 2009 at 1:52 PM, Jeff Layton <jlayton@redhat.com> wrote: > > > be able to trick the kernel into using a credcache to which the user > > doesn't > > > have access by setting the right uid= option on a mount? > > ... > > > Seems saner and more secure to me. > > > > Are you saying "saner" to rename "uid=" for the purposes of the upcall... > > not > > just the change to pass the "uid of the process that initiated the > upcall" > > (not > > the "owner of the files" uid) (If so, I agree) > > > > > > I'm not sure I follow what you're saying... > > I'm saying that we should pass the real uid of the process that > initiated the upcall to the upcall. There's no point in passing the uid > of the process that owns the files on the mount to the upcall. > Yes - I was agreeing on this :) > > I don't see the point of renaming the uid= "key" here. Can you > elaborate as to why you think it would be good to do so? > I thought that you had also noted that using "uid" to mean more than one thing on mount was confusing, and thus could be confusing to also keep the same name in cifs.upcall. If cifs.upcall does not care about the "default file owner" but does care about the process that initiated the upcall - seems like it would make sense to call it something distinct to eliminate any possible confusion. Thanks, Steve
On Mon, 3 Aug 2009 15:45:51 -0500 Steve French <smfrench@gmail.com> wrote: > On Mon, Aug 3, 2009 at 3:20 PM, Jeff Layton <jlayton@redhat.com> wrote: > > > On Mon, 3 Aug 2009 14:40:19 -0500 > > Steve French <smfrench@gmail.com> wrote: > > > > > On Mon, Aug 3, 2009 at 1:52 PM, Jeff Layton <jlayton@redhat.com> wrote: > > > > be able to trick the kernel into using a credcache to which the user > > > doesn't > > > > have access by setting the right uid= option on a mount? > > > ... > > > > Seems saner and more secure to me. > > > > > > Are you saying "saner" to rename "uid=" for the purposes of the upcall... > > > not > > > just the change to pass the "uid of the process that initiated the > > upcall" > > > (not > > > the "owner of the files" uid) (If so, I agree) > > > > > > > > > > I'm not sure I follow what you're saying... > > > > I'm saying that we should pass the real uid of the process that > > initiated the upcall to the upcall. There's no point in passing the uid > > of the process that owns the files on the mount to the upcall. > > > > Yes - I was agreeing on this :) > Ok good. Though I mis-wrote that above. That should have read "There's no point in passing the uid that owns the files on the mount..." ...but you get the idea. > > > > > I don't see the point of renaming the uid= "key" here. Can you > > elaborate as to why you think it would be good to do so? > > > > I thought that you had also noted that using "uid" to mean more than > one thing on mount was confusing, and thus could be confusing > to also keep the same name in cifs.upcall. If cifs.upcall does > not care about the "default file owner" but does care about > the process that initiated the upcall - seems like it would make > sense to call it something distinct to eliminate any > possible confusion. > Ahh ok. The problem there is that would break older cifs.upcall programs. cifs.upcall currently looks for an option called "uid=". I'm just proposing that we change how we populate that option. If we call it something new, then older cifs.upcall programs won't know about the change and wouldn't setuid to a user before trying to access the credcache. The catch here is that this change might be problematic for people that expect that setting uid= to a value will point cifs.upcall at that uid's credcache. If we're concerned about that, it might be better to make this change more gradually. Thoughts?
On Mon, 3 Aug 2009 18:37:20 -0400 Jeff Layton <jlayton@redhat.com> wrote: > On Mon, 3 Aug 2009 15:45:51 -0500 > Steve French <smfrench@gmail.com> wrote: > > > On Mon, Aug 3, 2009 at 3:20 PM, Jeff Layton <jlayton@redhat.com> wrote: > > > > > On Mon, 3 Aug 2009 14:40:19 -0500 > > > Steve French <smfrench@gmail.com> wrote: > > > > > > > On Mon, Aug 3, 2009 at 1:52 PM, Jeff Layton <jlayton@redhat.com> wrote: > > > > > be able to trick the kernel into using a credcache to which the user > > > > doesn't > > > > > have access by setting the right uid= option on a mount? > > > > ... > > > > > Seems saner and more secure to me. > > > > > > > > Are you saying "saner" to rename "uid=" for the purposes of the upcall... > > > > not > > > > just the change to pass the "uid of the process that initiated the > > > upcall" > > > > (not > > > > the "owner of the files" uid) (If so, I agree) > > > > > > > > > > > > > > I'm not sure I follow what you're saying... > > > > > > I'm saying that we should pass the real uid of the process that > > > initiated the upcall to the upcall. There's no point in passing the uid > > > of the process that owns the files on the mount to the upcall. > > > > > > > Yes - I was agreeing on this :) > > > > Ok good. Though I mis-wrote that above. That should have read "There's > no point in passing the uid that owns the files on the mount..." > > ...but you get the idea. > > > > > > > > > I don't see the point of renaming the uid= "key" here. Can you > > > elaborate as to why you think it would be good to do so? > > > > > > > I thought that you had also noted that using "uid" to mean more than > > one thing on mount was confusing, and thus could be confusing > > to also keep the same name in cifs.upcall. If cifs.upcall does > > not care about the "default file owner" but does care about > > the process that initiated the upcall - seems like it would make > > sense to call it something distinct to eliminate any > > possible confusion. > > > > Ahh ok. The problem there is that would break older cifs.upcall > programs. > > cifs.upcall currently looks for an option called "uid=". I'm just > proposing that we change how we populate that option. If we call it > something new, then older cifs.upcall programs won't know about the > change and wouldn't setuid to a user before trying to access the > credcache. > > The catch here is that this change might be problematic for people that > expect that setting uid= to a value will point cifs.upcall at that > uid's credcache. If we're concerned about that, it might be better to > make this change more gradually. > > Thoughts? > Actually...I think I'm going to withdraw this patch. Sending the real uid still isn't quite what we need here. The problem is that there's no guarantee that the "correct" uid will be the one that initiates the upcall during a reconnect. I need to rethink this (and will plan to respin and resend in the future).
diff --git a/fs/cifs/cifs_spnego.c b/fs/cifs/cifs_spnego.c index 8ec7736..1d0d8fc 100644 --- a/fs/cifs/cifs_spnego.c +++ b/fs/cifs/cifs_spnego.c @@ -140,7 +140,7 @@ cifs_get_spnego_key(struct cifsSesInfo *sesInfo) goto out; dp = description + strlen(description); - sprintf(dp, ";uid=0x%x", sesInfo->linux_uid); + sprintf(dp, ";uid=0x%x", current_uid()); dp = description + strlen(description); sprintf(dp, ";user=%s", sesInfo->userName);
The ownership of files on the mount has no direct relationship to the credentials used to do the mount. Instead of sending the uid corresponding to the owner of files on the mount, instead send the real uid of the process that initiated the upcall. Usually this will be the real uid of the process running /bin/mount. Eventually however, we want to be able to establish new sessions for users that walk into a cifs mount. For that we need the real uid of those users. Signed-off-by: Jeff Layton <jlayton@redhat.com> --- fs/cifs/cifs_spnego.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)