Bug 155163 - [patch] Add Recursive Functionality to setfacl(1)
Summary: [patch] Add Recursive Functionality to setfacl(1)
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: unspecified
Hardware: Any Any
: Normal Affects Only Me
Assignee: Mark Johnston
URL:
Keywords: patch
Depends on:
Blocks: 228911
  Show dependency treegraph
 
Reported: 2011-03-01 17:00 UTC by Shawn Webb
Modified: 2018-11-10 20:38 UTC (History)
7 users (show)

See Also:
emaste: mfc-stable10-
emaste: mfc-stable11?


Attachments
file.diff (7.05 KB, patch)
2011-03-01 17:00 UTC, Shawn Webb
no flags Details | Diff
patch_acl_recursive_9.1-STABLE_r246451.txt (6.50 KB, text/plain; charset=us-ascii)
2013-04-24 09:02 UTC, c.vasadi
no flags Details
patch_acl_recursive_MAN_PAGE_9.1-STABLE_r246451.txt (505 bytes, text/plain; charset=us-ascii)
2013-04-24 09:02 UTC, c.vasadi
no flags Details
Add recursive support to setfacl(1) (10.98 KB, patch)
2014-12-29 07:11 UTC, Shawn Webb
no flags Details | Diff
Add recurisve support to setfacl(1) (11.84 KB, patch)
2016-12-15 19:10 UTC, Shawn Webb
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shawn Webb 2011-03-01 17:00:17 UTC
The setfacl command is missing recursive functionality. The proposed and attached patch implements said functionality.

Included in the patch is also an enhancement to the -k switch. Solaris allows for zero-number ACL entries on objects stored in ZFS datasets via `chmod A= /path/to/file". FreeBSD does not support zero-number ACL entries so I give owner@ full permissions. This enhancement depends upon another bug report I sent that allows use of ACL sets (read_set, write_set, modify_set, full_set) in ZFS NFSv4 ACLs.

Fix: Patch included.

Patch attached with submission follows:
Comment 1 Edward Tomasz Napierala freebsd_committer 2011-03-02 07:25:28 UTC
Responsible Changed
From-To: freebsd-bugs->trasz

I'll take it.
Comment 2 c.vasadi 2013-04-24 09:02:35 UTC
Hi,

I tried the patch on a 9.0-RELEASE and it applies without any problems.
On 9.1-STABLE (r246451) however, that patch had to be adjusted a bit
(see the new patch below). Attached, you may also find the patch for the
man page.

Feel free to modify if necessary.

PS: Any chance this will be imported?

-- 
Claudiu Vasadi
System Administrator

myphotobook
Oranienstraße 183
10999 Berlin
Germany

Tel.:+49 (0)30 616 508 120
Fax: +49 (0)30 616 508 250
c.vasadi@myphotobook.de

Authorised Executives: Vanessa Dill
Trade Register: HRB 94377, Berlin
Comment 3 Edward Tomasz Napierala freebsd_committer 2014-06-18 12:47:03 UTC
Disown.  I don't even remember how that code works anymore.  One potential problem with this code that it used fts to build a list of all paths in the tree, instead of using fts to iterate over the tree, and that could fail due to out of memory condition.  That said, it doesn't look critical, and that's also how setfacl works right now.
Comment 4 Shawn Webb 2014-12-29 07:11:34 UTC
Created attachment 151059 [details]
Add recursive support to setfacl(1)

Manpage still needs to be updated. This patch works against 11-CURRENT.
Comment 5 clint 2015-08-25 15:33:21 UTC
Is testing and work on this patch still in progress? The inability to recursively set ACLs on FreeBSD is quite an annoyance and I was going to open my own bug report on it when I found this one already open.
Comment 6 Shawn Webb 2015-08-25 15:35:36 UTC
The patch I submitted in December of last year works well in HardenedBSD.
Comment 7 Shawn Webb 2016-12-15 19:10:04 UTC
Created attachment 177998 [details]
Add recurisve support to setfacl(1)

Update the patch to reflect latest HEAD.
Comment 8 Shawn Webb 2017-01-08 17:07:21 UTC
Hey Allan, have you had a chance to look at this?
Comment 9 Ed Maste freebsd_committer 2017-06-18 02:15:20 UTC
This is open for review in https://reviews.freebsd.org/D9096, and it is currently blocked on issues raised in that review.
Comment 10 Ed Maste freebsd_committer 2018-04-10 00:50:34 UTC
Updated patch in review at https://reviews.freebsd.org/D14934
Comment 11 commit-hook freebsd_committer 2018-04-10 23:30:44 UTC
A commit references this bug:

Author: emaste
Date: Tue Apr 10 23:29:57 UTC 2018
New revision: 332396
URL: https://svnweb.freebsd.org/changeset/base/332396

Log:
  setfacl: add recursive functionality

  Add a -R option to setfacl to operate recursively on directories, along
  with the accompanying flags -H, -L, and -P (whose behaviour mimics
  chmod).

  A patch was submitted with PR 155163, but this is a new implementation
  based on comments raised in the Phabricator review for that patch
  (review D9096).

  PR:		155163
  Submitted by:	Mitchell Horne <mhorne063@gmail.com>
  Reviewed by:	jilles
  MFC after:	2 weeks
  Relnotes:	Yes
  Sponsored by:	The FreeBSD Foundation
  Differential Revision:	https://reviews.freebsd.org/D14934

Changes:
  head/bin/setfacl/setfacl.1
  head/bin/setfacl/setfacl.c
  head/bin/setfacl/setfacl.h
  head/bin/setfacl/util.c
Comment 12 Shawn Webb 2018-04-11 16:22:55 UTC
(In reply to commit-hook from comment #11)
The new version of the recursive patch does not remove the inheritance flags from files, so when you do something like:

setfacl -R -m everyone@:full_set:fd:allow /some/path

You get the following error:

setfacl: /usr/src2/tools/tools/track/track.sh: acl_set_link_np() failed: Invalid argument
Comment 13 Mitchell Horne 2018-04-11 19:40:57 UTC
(In reply to Shawn Webb from comment #12)

Are you sure this isn't an existing issue? After enabling NFSv4 ACLs on UFS I get the same error both pre and post patch, regardless of -R.
Comment 14 Shawn Webb 2018-04-11 19:45:37 UTC
(In reply to Mitchell Horne from comment #13)
It is an existing issue that was addressed in the original recursive setfacl patch. The reason to address this issue is that by using recursive functionality, users will now be applying unwittingly inheritance flags on objects (files) in which inheritance doesn't apply.

Thus, a follow-up patch should be implemented to remove inheritance flags on objects that don't support inheritance.
Comment 15 Mitchell Horne 2018-04-13 18:50:08 UTC
(In reply to Shawn Webb from comment #14)

I've submitted a review to https://reviews.freebsd.org/D15061 containing the changes from your original patch which seems to fix this.
Comment 16 Ed Maste freebsd_committer 2018-10-10 17:08:00 UTC
Tag as a FreeBSD 12 issue as the change is currently partially complete in HEAD.
Comment 17 Mark Johnston freebsd_committer 2018-10-26 18:35:28 UTC
(In reply to Mitchell Horne from comment #15)
One problem with this patch is that clear_inheritance_flags() updates the global ACE rather than making a copy.  This basically means that setfacl -R can't reliably be used to recursively set inheritance flags for subdirectories.  With that fixed I think the patch is reasonable.  I'll take care of getting it committed and adding some regression tests.
Comment 18 commit-hook freebsd_committer 2018-10-26 21:17:19 UTC
A commit references this bug:

Author: markj
Date: Fri Oct 26 21:17:07 UTC 2018
New revision: 339793
URL: https://svnweb.freebsd.org/changeset/base/339793

Log:
  Don't set NFSv4 ACL inheritance flags on non-directories.

  They only make sense in the context of directory ACLs, and attempting
  to set them on regular files results in errors, causing a recursive
  setfacl invocation to abort.

  This is derived from patches by Shawn Webb <shawn.webb@hardenedbsd.org>
  and Mitchell Horne <mhorne063@gmail.com>.

  PR:		155163
  MFC after:	2 weeks
  Sponsored by:	The FreeBSD Foundation
  Differential Revision:	https://reviews.freebsd.org/D15061

Changes:
  head/bin/setfacl/setfacl.1
  head/bin/setfacl/setfacl.c
Comment 19 commit-hook freebsd_committer 2018-11-10 20:35:43 UTC
A commit references this bug:

Author: markj
Date: Sat Nov 10 20:35:00 UTC 2018
New revision: 340332
URL: https://svnweb.freebsd.org/changeset/base/340332

Log:
  MFC r339793:
  Don't set NFSv4 ACL inheritance flags on non-directories.

  PR:		155163
  Approved by:	re (gjb)

Changes:
_U  stable/12/
  stable/12/bin/setfacl/setfacl.1
  stable/12/bin/setfacl/setfacl.c