smbcacls support for automatic inheritance propagation
David Disseldorp
ddiss at suse.de
Wed Jan 29 10:50:36 MST 2014
Hi Noel,
This looks like a really useful feature, I look forward to seeing it
upstream. Please see feedback below...
On Thu, 21 Nov 2013 18:41:14 +0000
Noel Power <nopower at suse.com> wrote:
> It appears the message ( with attachment ) broke some size limit and my
> message is in the moderation queue, instead of the attached patch
> mentioned in the mail below please instead see
You may want to consider using git send-email in future to avoid this.
It sends one mail per-commit and puts the patch content inline.
> http://cgit.freedesktop.org/~noelp/noelp-samba/log/?h=smbcalcs-inherit-squash-v2
> and the commit range
> 929be157d8e6fe9614a071230b89b742395786be...3f77bf2ce318b51547c36f315b34a062ba7afccf
> ( or the top 5 commits if you prefer )
add smbcacls test based on test_smbclient_tarmode.pl
- test_smbcacls.pl should have your copyright, given that you wrote the
majority of the tests.
- is this your latest patch set? I thought you worked on removing some
of the test_smbclient_tarmode.pl code duplication.
smbcacls: parse config file argument prior to load
- looks like this patch picked up some white-space damage. Please remove
the newline above poptGetContext() then add my Signed-off-by.
add new '--propagate-inheritance' option for smbcacl
856 static struct security_descriptor *get_secdesc_with_ctx(TALLOC_CTX *ctx,
857 struct cli_state *cli,
858 const char *filename)
- I'd prefer to just add the ctx argument to get_secdesc() here and add
talloc_tos() to the existing callers, rather than the split/rename.
1958 if (NT_STATUS_IS_OK(ntstatus)) {
1959 /* check if there is actually any need to propagate */
- no need for the condition here, ntstatus is already checked to be OK
at this point.
1648 /* set operation if inheritance is enabled doesn't make sense */
1649 if (mode == SMB_ACL_SET && ((old->type & SEC_DESC_DACL_PROTECTED) !=
1650 SEC_DESC_DACL_PROTECTED)){
- get_secdesc_with_ctx() errors should be handled before this point, as
@old may be NULL.
- ((old->type & SEC_DESC_DACL_PROTECTED) != SEC_DESC_DACL_PROTECTED)
could be shortened to !(old->type & SEC_DESC_DACL_PROTECTED) here.
1651 d_printf("Inheritance enabled at %s, can't apply set operation\n",filename);
1652 result = NT_STATUS_UNSUCCESSFUL;
- why is this considered an error, does icacls.exe behave similarly in
such a case?
1653 goto out;
1654
- whitespace
1686 static NTSTATUS get_parents_inheritable_aces(TALLOC_CTX *ctx, char *filename,
1687 struct cacl_callback_state *cbstate)
1688 {
...
1691 struct security_descriptor *sd = get_secdesc_with_ctx(ctx, cli,
1692 filename);
- check for error
1723 static NTSTATUS cacl_set_cb(const char *mntpoint, struct file_info *f,
1724 const char *mask, void *state)
1725 {
...
1808 /* get acls from parent (e.g. caclfile) */
1809 struct cacl_callback_state dir_cbstate;
- avoid mixed declarations and code
1496 static NTSTATUS propagate_inherited_aces(char *caclfile,
1497 struct cacl_callback_state *cbstate)
1498 {
...
1506 uint32 i, j;
- use uint32_t for new code
...
1516 if ((old->type & SEC_DESC_DACL_PROTECTED) ==
1517 SEC_DESC_DACL_PROTECTED){
- should be: if (old->type & SEC_DESC_DACL_PROTECTED) {
1525 struct security_acl *acl_to_remove = NULL;
- avoid mixed declarations and code
1526 /* find acl(s) that are inherited */
1527 for (j = 0; old->dacl && j < old->dacl->num_aces; j++) {
1528
1529 if (old->dacl->aces[j].flags & SEC_ACE_FLAG_INHERITED_ACE) {
1530 if (!add_ace_with_ctx(aclctx, &acl_to_remove,
1531 &old->dacl->aces[j])) {
1532 status = NT_STATUS_NO_MEMORY;
1533 goto out;
1534 }
1535 }
1536 }
1537
1538 /* remove any acl(s) that are inherited */
1539 if (acl_to_remove) {
1540 for (i = 0; i < acl_to_remove->num_aces; i++) {
1541 struct security_ace ace = acl_to_remove->aces[i];
1542 for (j = 0; old->dacl && j < old->dacl->num_aces; j++) {
1543
1544 if (sec_ace_equal(&ace,
1545 &old->dacl->aces[j])) {
1546 uint32 k;
1547 for (k = j; k < old->dacl->num_aces-1;
1548 k++) {
1549 old->dacl->aces[k] =
1550 old->dacl->aces[k+1];
1551 }
1552 old->dacl->num_aces--;
1553 break;
1554 }
1555 }
1556 }
1557 }
- This code looks waaaaay too complex for what it's doing. IIUC it
determines which ACEs can be removed from the existing ACL, and then
removes them by collapsing the array in the subsequent loop.
- sec_ace_equal() is used to check whether the ACE should be
removed, but couldn't this also match an explicit
(non-inherited) ACE?
- Isn't it possible to build a new ACL in the first iteration,
ignoring inherited ACEs. Then combine it with acl_to_add.
- What happens in the following scenario:
parent_dir
+---child_dir
+-----nested_file
1) setacl(parent_dir, allow:ddiss:read, OI)
2) setacl(child_dir, allow:noelp:write, OI)
Does nested_file end up with both allow: ACEs, or would propagation
during setacl(child_dir,...) remove the inherited allow:ddiss ACE?
1558 /* propagate any inheritable ace to be added */
1559 if (acl_to_add) {
1560 for (i = 0; i < acl_to_add->num_aces; i++) {
- for (i = 0; (acl_to_add != NULL) && (i < acl_to_add->num_aces; i++) {
Could be used to remove one layer of nesting here.
1561 struct security_ace ace = acl_to_add->aces[i];
1562 bool is_objectinherit = (ace.flags &
1563 SEC_ACE_FLAG_OBJECT_INHERIT) ==
1564 SEC_ACE_FLAG_OBJECT_INHERIT;
1565 /* don't propagate flags to a file unless OI */
1566 if (!is_objectinherit && !is_container) {
1567 continue;
1568 }
1569 /*
1570 * adjust flags according to inheritance
1571 * rules
1572 */
1573 ace.flags = get_flags_to_propagate(is_container, &ace);
1574 bool is_inherited = (ace.flags &
- mixed declaration and code
1575 SEC_ACE_FLAG_INHERITED_ACE) ==
1576 SEC_ACE_FLAG_INHERITED_ACE;
1577 /* don't propagate non inherited flags */
1578 if (!is_inherited) {
1579 continue;
1580 }
1581 if (!add_ace_with_ctx(aclctx, &old->dacl, &ace)) {
1582 status = NT_STATUS_NO_MEMORY;
1583 goto out;
1584 }
1585 }
1586 }
- Does this logic retain canonical order?
http://technet.microsoft.com/en-us/library/cc961994.aspx
I'll stop the review of this commit at this point, as I think this needs
to go another round.
doc: describe smbcacls --propagate-inheritance
doc: describe smbcacls --propagate-inheritance expanding INHERITANCE section
- I'm a big fan of the propagation examples you added here :-)
292 <itemizedlist>
293 <listitem><para>Inheritable (OI)(OI) ACE flags can only be
- I think this should be (OI)(CI).
315 <para><programlisting>
...
322 <para><programlisting>
...
- Please drop the EOL white spaces after all <programlisting> lines.
332 | +-nested/ (OI)(CI(I)(READ)
- should be (OI)(CI)(I)(READ).
More information about the samba-technical
mailing list