Re: [PATCH] Ensure that the default ACL contains the standard entries
On Dec 14 22:22, Christian Franke wrote: Hi Corinna, Corinna Vinschen wrote: Hi Christian, On Dec 10 23:05, Christian Franke wrote: The ACL from Cygwin always contains the three (USER|GROUP|OTHER)_OBJ entries. It might be existing practice elsewhere to return these entries also in the default ACL. The attached patch adds these entries with empty permissions if necessary. The patch would fix this rsync issue: http://cygwin.com/ml/cygwin/2010-11/msg00429.html The logic for DEF_CLASS_OBJ is unchanged. This looks good, except that the faked default entries for group and other are set to 0. That's rather unexpected. ... This is rather easy to fix (and you added comments in that place), ... New patch attached. Thanks, applied. I'm not entirely sure yet, but maybe the acl function should not fake these default entries. From my POV it seems a better approach if acl(SETACL) actually creates these entries if *any* default entry is in the incoming acl. And, it should create these entries with useful permission values. This seems to reflect the Linux behaviour much closer. What do you think? AFIAK a minimal ACL must contain the three entries u/g/o which are equivalent to the mode permission bits. The default ACL has likely the same requirement. Apparently yes. I just tested this on Linux and Solaris. On Linux the missing entries are added with default values, on Solaris 10 you are required to enter at least the three default o/g/u entries, otherwise setfacl prints an error message Missing user/group owner, other, mask entry If this is the case, then I would suggest to do both: 1. Fake these entries in acl(GETACL) if required. This would ensure that the default ACL is complete even if the Windows ACL was not created by Cygwin. 2. Create these entries in acl(SETACL) if required. This would ensure that the following reasonable requirement holds if the Windows ACL was created by Cygwin before: - getfacl foo | setfacl -f - foo should not change the (Windows) ACL of foo. Ok, fine with me. Would you implement this? Yes, but may take some time. No worries. We won't release 1.7.8 before January. Btw., while testing your patch I found a bug in setfacl which disallowed to delete user/group-specific default entries. I opted for rewriting the function which examines an incoming acl entry (getaclentry). Would you mind to test this bit, too? The new code accepts a trailing colon, but I think that's ok. The SGI setfacl tool used on Linux is even more relaxed syntax-wise and also accepts trailing colons. I've done a few test, looks good. Thank you! An unrelated issue found during testing: mkdir() may duplicate Windows ACL entries. Testcase (German XP SP3): [...] Problem in security.cc:alloc_sd() ? Indeed. Thanks for the report. I fixed that in CVS, hopefully. Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Project Co-Leader cygwin AT cygwin DOT com Red Hat
Re: [PATCH] Ensure that the default ACL contains the standard entries
Corinna Vinschen wrote: New patch attached. Thanks, applied. Thanks - rsync issue is now fixed. mkdir() may duplicate Windows ACL entries. Testcase (German XP SP3): [...] Problem in security.cc:alloc_sd() ? Indeed. Thanks for the report. I fixed that in CVS, hopefully. At least the testcase is now OK :-) BTW: Are there any long term plans to actually implement the acl mask ? Should be possible by mapping the mask restrictions to deny acl entries for each named entry: Adds further complexity - might or might not be worth the effort, I'm not sure. Christian
Re: [PATCH] Ensure that the default ACL contains the standard entries
Hi Christian, On Dec 10 23:05, Christian Franke wrote: The ACL from Cygwin always contains the three (USER|GROUP|OTHER)_OBJ entries. It might be existing practice elsewhere to return these entries also in the default ACL. The attached patch adds these entries with empty permissions if necessary. The patch would fix this rsync issue: http://cygwin.com/ml/cygwin/2010-11/msg00429.html The logic for DEF_CLASS_OBJ is unchanged. Thanks for the patch. There are two problem with it, unfortunately. Consider the setfacl tool. The -m option basically works like this: acl (path, GETACL); modify_acl (); acl (path, SETACL); Now, what happens with your patch is this. Let's assume I add a single default entry: $ getfacl dir [...] user::rwx group::r-x mask::rwx other::r-x $ setfacl -m d:u:corinna:rwx dir [...] user::rwx group::r-x mask::rwx other::r-x default:user::--- default:user:corinna:rwx default:group::--- default:mask::rwx default:other::--- This looks good, except that the faked default entries for group and other are set to 0. That's rather unexpected. Actually, by default the default entries should reflect the standard permission bits. At least that's what happens in the above example on Linux (I tried with different values for the permission bits): $ setfacl -m d:u:corinna:rwx dir [...] user::rwx group::r-x mask::rwx other::r-x default:user::rwx default:user:corinna:rwx default:group::r-x default:mask::rwx default:other::r-x This is rather easy to fix (and you added comments in that place), but here comes problem #2. In reality, the Windows ACL does not contain any default entries except for the default entry for user corinna: $ icacls dir c:\cygwin\home\corinna\dir VINSCHEN\corinna:(F) VINSCHEN\vinschen:(RX) Everyone:(RX) VINSCHEN\corinna:(OI)(CI)(IO)(RX,W,DC) Ok, but, what happens if I call setfacl again? The first call to acl in setfacl returns the faked default entries. So, after modifying the acl according to the command line, the SETACL call now still contains the faked acl entry. Which means, they are now written back to the dir's ACL. Just call setfacl with the same command line again: $ setfacl -m d:u:corinna:rwx dir $ icacls dir c:\cygwin\home\corinna\tmp VINSCHEN\corinna:(F) VINSCHEN\vinschen:(RX) Everyone:(RX) CREATOR OWNER:(OI)(CI)(IO)(D,Rc,WDAC,WO,RA,WA) VINSCHEN\corinna:(OI)(CI)(IO)(RX,W,DC) CREATOR GROUP:(OI)(CI)(IO)(Rc,RA) Everyone:(OI)(CI)(IO)(Rc,RA) Even though nothing has changed, the ACL is now different since it actually reflects the so far faked default entries. I'm not sure if that's feasible behaviour. Besides, due to the faked default entries defaulting to 0 permissions, subsequently created files in that directory will have 000 permissions by default. Uh oh. I'm not entirely sure yet, but maybe the acl function should not fake these default entries. From my POV it seems a better approach if acl(SETACL) actually creates these entries if *any* default entry is in the incoming acl. And, it should create these entries with useful permission values. This seems to reflect the Linux behaviour much closer. What do you think? Would you implement this? Btw., while testing your patch I found a bug in setfacl which disallowed to delete user/group-specific default entries. I opted for rewriting the function which examines an incoming acl entry (getaclentry). Would you mind to test this bit, too? The new code accepts a trailing colon, but I think that's ok. The SGI setfacl tool used on Linux is even more relaxed syntax-wise and also accepts trailing colons. Corinna
