On Fri, Nov 15, 2013 at 06:08:30PM +0100, Jakub Hrozek wrote:
On Fri, Nov 15, 2013 at 06:01:14PM +0100, Sumit Bose wrote:
> On Fri, Nov 15, 2013 at 05:47:08PM +0100, Jakub Hrozek wrote:
> > On Wed, Nov 13, 2013 at 01:12:02PM +0100, Sumit Bose wrote:
> > > On Mon, Nov 11, 2013 at 06:28:33PM -0500, Simo Sorce wrote:
> > > > On Mon, 2013-11-11 at 22:03 +0100, Sumit Bose wrote:
> > > > > On Mon, Nov 11, 2013 at 10:41:46AM -0500, Simo Sorce wrote:
> > > > > > Why should we reserve these names ?
> > > > > > These SID are never used for actual users anyway, so I do
not understand
> > > > > > why would you care about their names ?
> > > > > > How would you create a collision ?
> > > > >
> > > > > The main purpose is to resolve the SIDs for [gs]etcifsacl.
Currently the
> > > > > output for getcifsacl with SSSD looks like:
> > > > >
> > > > > REVISION:0x1
> > > > > CONTROL:0x8404
> > > > > OWNER:S-1-5-32-544
> > > > > GROUP:domain users(a)ad18.ipa18.devel
> > > > > ACL:posix_test_group@ad18.ipa18.devel:ALLOWED/0x0/FULL
> > > > > ACL:S-1-2-1:ALLOWED/0x0/READ
> > > > > ACL:S-1-5-32-556:ALLOWED/0x0/READ
> > > > > ACL:S-1-5-18:ALLOWED/I/FULL
> > > > > ACL:S-1-5-32-544:ALLOWED/I/FULL
> > > > > ACL:administrator@ad18.ipa18.devel:ALLOWED/I/FULL
> > > > >
> > > > > the well-Known SIDs are not resolved. With the patches it looks
like:
> > > > >
> > > > > REVISION:0x1
> > > > > CONTROL:0x8404
> > > > > OWNER:Administrators@BUILTIN
> > > > > GROUP:domain users(a)ad18.ipa18.devel
> > > > > ACL:posix_test_group@ad18.ipa18.devel:ALLOWED/0x0/FULL
> > > > > ACL:CONSOLE LOGON:ALLOWED/0x0/READ
> > > > > ACL:Network Configuration Operators@BUILTIN:ALLOWED/0x0/READ
> > > > > ACL:SYSTEM@NT AUTHORITY:ALLOWED/I/FULL
> > > > > ACL:Administrators@BUILTIN:ALLOWED/I/FULL
> > > > > ACL:administrator@ad18.ipa18.devel:ALLOWED/I/FULL
> > > > >
> > > > > Everything is resolved. Maybe I've gone a little bit to far
becasuse I
> > > > > just realized that winbind only resolves the BUILTIN SIDs:
> > > > >
> > > > > REVISION:0x1
> > > > > CONTROL:0x8404
> > > > > OWNER:BUILTIN\Administrators
> > > > > GROUP:domain users(a)ad18.ipa18.devel
> > > > > ACL:posix_test_group@ad18.ipa18.devel:ALLOWED/0x0/FULL
> > > > > ACL:S-1-2-1:ALLOWED/0x0/READ
> > > > > ACL:BUILTIN\Network Configuration Operators:ALLOWED/0x0/READ
> > > > > ACL:S-1-5-18:ALLOWED/I/FULL
> > > > > ACL:BUILTIN\Administrators:ALLOWED/I/FULL
> > > > > ACL:administrator@ad18.ipa18.devel:ALLOWED/I/FULL
> > > > >
> > > > > setcifsacl can use the names instead of the SIDs to modify the
ACLs,
> > > > > hence it is useful to support the name-to-SID translation as
well. A
> > > > > collision might be possible if for some reason a called e.g.
'CONSOLE
> > > > > LOGIN' is created. In this case this user is only accessible
by its
> > > > > fully qualified name.
> > > >
> > > > Ok I think it is fair to require the fully qualified name in that
case,
> > > > and not try to black list potentially valid names ?
> > >
> > > ok, thanks. The new version always uses fully qualified names, add a man
> > > page section and updates the cifs-utils plugin to reflect the changes.
> > >
> > > bye,
> > > Sumit
> >
> > These patches work really well and I haven't found any issues. Also
> > Coverity scan didn't bring any new defects. I tested mostly with
> > pysss_nss_idmap, both user and group sids and various builtin sids.
> >
> > Thanks for providing detailed unit tests!
> >
> > I only have one question about return codes. Currently if the first part
> > of the SID matches any of the well known categories but the RID is not
> > known, we return EINVAL. This return code is then passed all the way to
> > the client. Would it make sense to return ENOENT instead?
>
> The idea was that if ENOENT is returned SSSD can continue to consult
> his backends to see if the given SID can be resolved. Since the backends
> do not know how to handle SIDs from those categories we can error out
> if we cannot find the matching RID.
>
> bye,
> Sumit
Ah, right. That makese sense.
Sorry, can't resist one remark -- the handling might have been more
readable if there was a new error code just for the situation where the
SID is valid but not built-in. But that's not a reason to hold a
patchset back..
ACK.
Pushed to master.