Comments on the auth_serversupplied_info rework in Samba3

[Andrew Bartlett]

I'm very glad to see the rework of auth_serversupplied_info in the
source3 auth subsystem.  I know I was involved in adding the passdb
structures in there in the first place, but I agree they no longer
belong there.

However, I have a few concerns:

My biggest concern is the way that this patch uses 'magic values' that
are only defined in Samba.  

s3:auth handle unix domain sids in samu
http://git.samba.org/?p=idra/samba.git;a=commitdiff;h=5648836a26f39966ff3f1fa9bc5a287ea368afb6

+/* special bits used to identify non domain users
+ * the may be generated in a samba server
+ * These must not conflict with assigned values
+ * in samr_GroupAttrs
+ */
+#define SE_SAMBA_USER_SID ( 0x00100000 )
+#define SE_SAMBA_PGID_SID ( 0x00200000 )


s3:auth create nt token from info3 directly
http://git.samba.org/?p=idra/samba.git;a=commitdiff;h=56c1b9648324f3bd8f0b386298beaa24b859dc3c

+       /* USER SID */
+       if (info3->base.rid == (uint32_t)(-1)) {
+               /* this is a signal the user was fake and generated,
+                * the actual SID we want to use is stored in the extra
+                * sids */

I would strongly prefer that we pass in a 'wrapper' or associated
structure that contains the info3, that has a flag of 'user rid fake'
etc.  We do not control these structures, and while we can fill them in
with values that are semantically correct for their original use, I'm
happy, but I don't like the use of magic values.

This isn't just a preference - it took quite some time to understand why
a new windows client failed against Samba4 with this same trick
(overloading a bitfield with Samba-only internal values) was used to
pass down special meanings in the Samba4 file server. 

On a positive note:

s3:passdb Export function to calculate the proper primary group sid
http://git.samba.org/?p=idra/samba.git;a=commitdiff;h=7bff8010bffcd8835e650f84ba9a4400bfb7924f

I really like that this horror show has been moved out of passdb.  It
was a real surprise to find in there when I started working on
s3compat. 

s3:smbd make yp cache local.
http://git.samba.org/?p=idra/samba.git;a=commitdiff;h=e219253cfd7c13f718cf8515d219eb4ef693c72e

I really like the clean-up this has allowed in the callers.  

Regarding the rest of what is currently in:
http://git.samba.org/?p=idra/samba.git;a=shortlog;h=refs/heads/wip
(I know this will change as you keep working, but anyway)

While I've note tested these, I've given the whole patch queue a good
look over, and aside from the changes I note above I look forward to
seeing these changes in master soon. 

(Once these are in, I'll propose a patch to move all of Samba to using
dom_sid_dup() as the standard SID duplication function). 

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20100601/cc6047ea/attachment.pgp>