[PATCH] Merging the privilege code
Andrew Bartlett
abartlet at samba.org
Mon Aug 30 20:31:27 MDT 2010
On Mon, 2010-08-30 at 12:10 +0200, Michael Adam wrote:
> Andrew,
>
> I'd like to second Volker's question/request here,
> hopefully emphasizing it:
>
> What is the benefit of removing the struct and going
> back to a flat integer type or as a compromise (as
> it seems to me) to an enum type.
>
> I also do very much like having these se_... accessor
> functions instead of directly doing bit-operations on
> the integer type. It is enhances readability and portability
> against changes of the type. If the implementation is bad
> (relying on certain global variables or so), this should be
> changed IMHO instead of removing the accessor functions
> altogether.
>
> I would very much like to keep a struct type and the
> accessor functions for the privileges.
>
>
> Also, why not simply port the 128 bit access mask with room
> to grow to source4 instead of backporting the source3 code?
Michael,
When I started this work I looked at the two structures:
source3 had:
#define SE_PRIV_MASKSIZE 4
typedef struct {
uint32 mask[SE_PRIV_MASKSIZE];
} SE_PRIV;
source4 simply used a uint64_t.
However, while the source3 code only defined 8 bitmap values, it
allocated space for 128. The source4 code allocated 28 bitmap values,
and was quite content to use a uint64_t. Perhaps Jerry would have used
a uint64_t if that was available at the time, I simply don't know.
His original construction of this interface is here:
http://gitweb.samba.org/?p=samba.git;a=commitdiff;h=46e5effea948931509283cb84b27007d34b521c8
I never imagined there would be so much fuss over this change, but I
still feel it is correct. We have many different bitmask values in
Samba, and no other bitmask justifies such a degree of abstraction. I
respect your view, but strongly disagree with your assessment that the
se_ accessor functions make the code clearer.
However, the concerns raised about type checking did make me think about
the API. In my previous proposal, the interface still remained split
between functions from source3/ which operated on bitmaps, and those
from source4/ which operated on an enum. There was indeed potential for
confusion, particularly as C provides no protection between these
types.
As I examined the code, and determined that in almost all the instances,
the caller only dealt with one bit at a time, I reworked all the calls
to use the security_token_has_privilege() call. (and a few other changes
to finish removing the bitmap interface, removing a potential source of
type confusion).
As you will see in the patches, this interface has a much simpler
calling convention. I hope you will see in my attached patch (showing
the changes as a whole, because the split-out has made it hard to follow
what benefits the changes bring) that we have obtained even greater
clarity in the new interfaces.
As to why I feel the SE_PRIV bitmap is not suitable for use in an IDL
defined structure, our IDL does not as far as I know provide an easy way
to generate defines such as:
#define SE_NETWORK_LOGON { { 0x00000001, 0x00000000,
0x00000000, 0x00000000 } }
And given that we will need to transport this structure over the named
pipe forwarding in future (to gain consistent privileges), I felt it
best to use a bitmap that would be described completely in IDL (and then
printable etc using the IDL routines).
i.e.
typedef [public,bitmap64bit] bitmap {
SE_NETWORK_LOGON 0x01
}
generates:
#define SE_NETWORK_LOGON 0x00000001
I hope this clarifies the approach I've taken in this area.
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: session-info.diff
Type: text/x-patch
Size: 201525 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20100831/594721c8/attachment-0001.bin>
-------------- next part --------------
b/libcli/security/privileges.c | 440 +++++++++++++++++++++++
b/libcli/security/privileges.h | 119 ++++++
b/libcli/security/wscript_build | 2
b/libgpo/gpext/gpext.c | 6
b/libgpo/gpext/gpext.h | 8
b/libgpo/gpo.h | 14
b/libgpo/gpo_ldap.c | 10
b/libgpo/gpo_sec.c | 8
b/libgpo/gpo_util.c | 10
b/librpc/idl/security.idl | 151 ++++++--
b/source3/Makefile.in | 2
b/source3/auth/auth_util.c | 4
b/source3/auth/token_util.c | 56 +--
b/source3/groupdb/mapping_tdb.c | 13
b/source3/include/auth.h | 2
b/source3/include/includes.h | 2
b/source3/include/proto.h | 103 ++---
b/source3/include/registry.h | 12
b/source3/include/smb.h | 9
b/source3/lib/netapi/localgroup.c | 4
b/source3/lib/privileges.c | 243 ++++++-------
b/source3/lib/sharesec.c | 2
b/source3/lib/smbconf/smbconf_reg.c | 4
b/source3/lib/util_nttoken.c | 29 -
b/source3/lib/util_seaccess.c | 10
b/source3/lib/util_sid.c | 16
b/source3/libgpo/gpext/registry.c | 4
b/source3/libgpo/gpext/scripts.c | 6
b/source3/libgpo/gpext/security.c | 2
b/source3/libgpo/gpo_proto.h | 8
b/source3/libgpo/gpo_reg.c | 24 -
b/source3/passdb/pdb_ldap.c | 5
b/source3/printing/nt_printing.c | 3
b/source3/registry/reg_api.c | 10
b/source3/registry/reg_backend_smbconf.c | 4
b/source3/registry/reg_dispatcher.c | 2
b/source3/registry/reg_dispatcher.h | 2
b/source3/registry/reg_util_legacy.c | 2
b/source3/registry/reg_util_legacy.h | 2
b/source3/registry/reg_util_token.c | 9
b/source3/registry/reg_util_token.h | 2
b/source3/rpc_server/srv_eventlog_nt.c | 2
b/source3/rpc_server/srv_lsa_nt.c | 148 ++------
b/source3/rpc_server/srv_samr_nt.c | 117 ++----
b/source3/rpc_server/srv_spoolss_nt.c | 51 +-
b/source3/rpc_server/srv_srvsvc_nt.c | 12
b/source3/rpc_server/srv_svcctl_nt.c | 8
b/source3/rpc_server/srv_winreg_nt.c | 9
b/source3/rpc_server/srv_wkssvc_nt.c | 8
b/source3/rpcclient/cmd_samr.c | 2
b/source3/services/services_db.c | 10
b/source3/smbd/globals.h | 2
b/source3/smbd/open.c | 2
b/source3/smbd/posix_acls.c | 6
b/source3/smbd/sec_ctx.c | 2
b/source3/smbd/share_access.c | 8
b/source3/smbd/uid.c | 2
b/source3/utils/net_ads_gpo.c | 6
b/source3/utils/net_proto.h | 12
b/source3/utils/net_registry.c | 2
b/source3/utils/net_rpc.c | 14
b/source3/utils/net_sam.c | 27 -
b/source3/winbindd/winbindd_ads.c | 14
b/source3/winbindd/winbindd_async.c | 4
b/source3/winbindd/winbindd_getsidaliases.c | 2
b/source3/winbindd/winbindd_pam.c | 6
b/source3/winbindd/winbindd_proto.h | 16
b/source3/winbindd/winbindd_util.c | 4
b/source4/dsdb/samdb/samdb_privilege.c | 2
b/source4/libcli/security/security.h | 1
b/source4/libcli/security/wscript_build | 2
b/source4/rpc_server/lsa/dcesrv_lsa.c | 16
source3/include/privileges.h | 98 -----
source3/lib/privileges_basic.c | 515 ----------------------------
source4/libcli/security/privilege.c | 245 -------------
75 files changed, 1209 insertions(+), 1530 deletions(-)
-------------- 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/20100831/594721c8/attachment-0001.pgp>
More information about the samba-technical
mailing list