On Tue, 2014-04-15 at 18:23 +0200, Pavel Reichl wrote:
On Fri, 2014-03-14 at 14:18 -0400, Simo Sorce wrote:
> On Fri, 2014-03-14 at 17:09 +0100, Jakub Hrozek wrote:
> > On Fri, Mar 14, 2014 at 04:44:00PM +0100, Sumit Bose wrote:
> > > On Fri, Mar 14, 2014 at 04:14:15PM +0100, Pavel Reichl wrote:
> > > > Hello,
> > > >
> > > > I noticed that debug message does not reflect condition.
> > > > I suppose it was intended to to check user's gid too.
> > > >
> > > > Please have mercy if I'm wrong :-).
> > > >
> > > > Regards,
> > > >
> > > > Pavel Reichl
> > >
> > > > From ebc90225c5f8141b997e40bebfc08e4f0a0a222b Mon Sep 17 00:00:00
2001
> > > > From: Pavel Reichl <preichl(a)redhat.com>
> > > > Date: Fri, 14 Mar 2014 14:46:45 +0000
> > > > Subject: [PATCH 1/2] KRB: check ccache directory for user's GID
> > > >
> > > > ---
> > > > src/providers/krb5/krb5_utils.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/src/providers/krb5/krb5_utils.c
b/src/providers/krb5/krb5_utils.c
> > > > index
27572c4ea749c6953f73cea10ef184ec539bf25b..858029a5aa46c3582adeae0f912632acd87377d3 100644
> > > > --- a/src/providers/krb5/krb5_utils.c
> > > > +++ b/src/providers/krb5/krb5_utils.c
> > > > @@ -419,8 +419,8 @@ done:
> > > > static errno_t check_parent_stat(struct stat *parent_stat,
> > > > uid_t uid, gid_t gid)
> > > > {
> > > > - if (!((parent_stat->st_uid == 0 &&
parent_stat->st_gid == 0) ||
> > > > - parent_stat->st_uid == uid)) {
> > > > + if ((parent_stat->st_uid != 0 || parent_stat->st_gid != 0)
&&
> > > > + (parent_stat->st_uid != uid || parent_stat->st_gid !=
gid)) {
> > >
> > > I think the original check is ok because it is sufficient that the
> > > directory belongs to the user, it does not need to belong to the users
> > > primary group as well. As a result gid can be removed form
> > > check_parent_stat()'s parameter list.
> > >
> > > But it would nice to hear other opinoins as well.
> > >
> > > bye,
> > > Sumit
> >
> > That's what I thought, too, but I wasn't sure either when we discussed
> > the code with Pavel in the office earlier today.
> >
> > Simo, do you remember?
>
> No, however the debug message is misleading and this code change would
> make it true.
> That said I am not sure we really want to care about the group owner
> given we create the subdirs with 700 permission anyway so no permission
> is associated with group ownership.
>
> Perhaps we should just check:
> if (parent_stat->st_uid != 0 && parent_stat->st_uid != uid) { ...
>
> Simo.
>
Hello,
patch addressing Simo's comments is attached.
Thanks for review.
LGTM
Simo.
--
Simo Sorce * Red Hat, Inc * New York