smbcacls support for automatic inheritance propagation
Noel Power
nopower at suse.com
Thu Jan 30 08:41:00 MST 2014
On 29/01/14 17:50, David Disseldorp wrote:
> Hi Noel,
>
[...]
> - is this your latest patch set? I thought you worked on removing some
> of the test_smbclient_tarmode.pl code duplication.
I have done that separately, this patch is complicated enough I think,
I'd really like to avoid dealing with this in the context of this
patch/feature. However I do commit to pushing those patches for review
immediately after this, hope that is ok.
> 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.
done
> 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.
yup, I agree (in fact already did that in the version that supports a
-r|recursive option), anyway done
> 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.
correct and now removed
> 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.
fine, fixed now
> 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?
No, but.. there is no real equivalent to set with iacacls. Also because
smbcacls --set replaces the ACL with the
ACL specified on the command-line (with no regard to any inherited
parent acls) and we issue the --propagate-inheritance indicating we wish
to honour inheritance we would more than likely be setting non-senisical
values here. Actually probably what was intended was --add, for that
reason I inhibit set via an error, if we wish we could remove that error
and issue a warning instead and continue to propagate, I'm ok with that.
I see in some of the previous email some confusing comments from me)
about this (including a comment indicating I was no longer uncomfortable
with '--set' in the context of --propagate-inheritance) I have no idea
what I meant there (because I am uncomfortable with it :-)) So.. what do
you think change this to a warning?
> 1653 goto out;
> 1654
> - whitespace
done
> 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
done
> 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
seems I can't :-) anyway done
> 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
done
> ...
> 1516 if ((old->type & SEC_DESC_DACL_PROTECTED) ==
> 1517 SEC_DESC_DACL_PROTECTED){
> - should be: if (old->type & SEC_DESC_DACL_PROTECTED) {
done
> 1525 struct security_acl *acl_to_remove = NULL;
> - avoid mixed declarations and code
done
> 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.
hmm, not sure I agree with the 'waaaay too complex' possibly I am tunnel
visioned :-)
> - sec_ace_equal() is used to check whether the ACE should be
> removed, but couldn't this also match an explicit
> (non-inherited) ACE?
not sure I understand what you mean ?
> - Isn't it possible to build a new ACL in the first iteration,
> ignoring inherited ACEs. Then combine it with acl_to_add.
sure, I guess, it would save a loop, will do that
> - 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?
it would remove it (if set would work) but remember from above
attempting to set is prohibited
> 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.
oh yeah indeed, done
> 1573 ace.flags = get_flags_to_propagate(is_container, &ace);
> 1574 bool is_inherited = (ace.flags &
> - mixed declaration and code
done
> 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 call cacl_set_from_sd which calls acl_sort which uses ace_compare
which claims iiuc to consider cannonical order
> I'll stop the review of this commit at this point, as I think this needs
> to go another round.
ok
> doc: describe smbcacls --propagate-inheritance expanding INHERITANCE section
>
> 292 <itemizedlist>
> 293 <listitem><para>Inheritable (OI)(OI) ACE flags can only be
> - I think this should be (OI)(CI).
yup, done
> 315 <para><programlisting>
> ...
> 322 <para><programlisting>
> ...
> - Please drop the EOL white spaces after all <programlisting> lines.
done
> 332 | +-nested/ (OI)(CI(I)(READ)
> - should be (OI)(CI)(I)(READ).
and again, done
thanks (updated patches to follow)
Noel
More information about the samba-technical
mailing list