Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

prevent zfs_acl_chmod() if aclmode restricted and ACL inherited #10748

Merged
merged 1 commit into from
Aug 23, 2020

Conversation

anodos325
Copy link
Contributor

Motivation and Context

In absence of inheriting entry for owner@, group@, or everyone@, zfs_acl_chmod() is called to set these. This can cause confusion for Samba admins who do not expect these entries to appear on newly created files and directories once they have been stripped from from the parent directory.

Description

When aclmode is set to "restricted", chmod is prevented on non-trivial ACLs. It is not a stretch to assume that in this case the administrator does not want ZFS to add the missing special entries. Add check for this aclmode, and if an inherited entry is present skip zfs_acl_chmod().

How Has This Been Tested?

Tested on FreeBSD. Procedure as follows:

root@:/zroot/SMB/FOO # getfacl .
# file: .
# owner: root
# group: wheel
     user:awalker:rwxpDdaARWcCos:fd-----:allow
root@:/zroot/SMB/FOO # mkdir bar
root@:/zroot/SMB/FOO # getfacl bar
# file: bar
# owner: root
# group: wheel
     user:awalker:rwxpDdaARWcCos:fd----I:allow
root@:/zroot/SMB/FOO # touch testfile
root@:/zroot/SMB/FOO # getfacl testfile
# file: testfile
# owner: root
# group: wheel
     user:awalker:rwxpDdaARWcCos:------I:allow

^^^ chmod skipped in due to restricted aclmode and inherited present.

root@:/zroot/SMB/FOO # setfacl -b .
root@:/zroot/SMB/FOO # getfacl .
# file: .
# owner: root
# group: wheel
            owner@:------aARWcCos:-------:allow
            group@:------a-R-c--s:-------:allow
         everyone@:------a-R-c--s:-------:allow
root@:/zroot/SMB/FOO # mkdir subdir
root@:/zroot/SMB/FOO # getfacl subdir
# file: subdir
# owner: root
# group: wheel
            owner@:rwxp--aARWcCos:-------:allow
            group@:r-x---a-R-c--s:-------:allow
         everyone@:r-x---a-R-c--s:-------:allow

^^^ no inherited present. chmod happens

root@:/zroot/SMB/FOO # zfs get aclmode zroot/SMB
NAME       PROPERTY  VALUE        SOURCE
zroot/SMB  aclmode   passthrough  local
root@:/zroot/SMB/FOO # getfacl .
# file: .
# owner: root
# group: wheel
      user:awalker:rwxpDdaARWcCos:fd-----:allow
root@:/zroot/SMB/FOO # mkdir bar
root@:/zroot/SMB/FOO # getfacl bar
# file: bar
# owner: root
# group: wheel
      user:awalker:rwxpDdaARWcCos:fd----I:allow
            owner@:rwxp--aARWcCos:-------:allow
            group@:r-x---a-R-c--s:-------:allow
         everyone@:r-x---a-R-c--s:-------:allow

aclmode is passthrough. chmod occurs.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the contributing document.
  • I have added tests to cover my changes.
  • I have run the ZFS Test Suite with this change applied.
  • All commit messages are properly formatted and contain Signed-off-by.

@codecov
Copy link

codecov bot commented Aug 20, 2020

Codecov Report

Merging #10748 into master will decrease coverage by 0.13%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10748      +/-   ##
==========================================
- Coverage   79.78%   79.64%   -0.14%     
==========================================
  Files         394      394              
  Lines      124637   124640       +3     
==========================================
- Hits        99439    99271     -168     
- Misses      25198    25369     +171     
Flag Coverage Δ
#kernel 80.25% <0.00%> (-0.16%) ⬇️
#user 65.52% <ø> (+0.36%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
module/os/linux/zfs/zfs_acl.c 57.08% <0.00%> (-0.37%) ⬇️
lib/libzutil/zutil_pool.c 38.63% <0.00%> (-54.55%) ⬇️
module/os/linux/spl/spl-zlib.c 55.35% <0.00%> (-28.58%) ⬇️
cmd/ztest/ztest.c 75.53% <0.00%> (-5.82%) ⬇️
module/zcommon/zfs_fletcher.c 75.65% <0.00%> (-2.64%) ⬇️
module/icp/api/kcf_mac.c 38.28% <0.00%> (-1.72%) ⬇️
module/zfs/metaslab.c 94.30% <0.00%> (-1.30%) ⬇️
module/zfs/dmu_traverse.c 96.01% <0.00%> (-1.00%) ⬇️
module/os/linux/zfs/zfs_file_os.c 84.15% <0.00%> (-1.00%) ⬇️
module/zfs/vdev_initialize.c 97.15% <0.00%> (-0.95%) ⬇️
... and 53 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5266a07...88b0d63. Read the comment docs.

@behlendorf behlendorf requested a review from a user August 20, 2020 04:39
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Aug 20, 2020
@ghost
Copy link

ghost commented Aug 20, 2020

I spoke privately with @anodos325 about this PR but here are the current action items planned:

  • It seems this is treating the inherited ACE as a special case, but I think there is a more general rule that any non-trivial ACL does not need chmod when aclmode=restricted, which is probably the way we actually want to hande this
  • I assume this needs to be corrected on Linux as well

In absence of inheriting entry for owner@, group@, or everyone@,
zfs_acl_chmod() is called to set these. This can cause confusion for Samba
admins who do not expect these entries to appear on newly created files and
directories once they have been stripped from from the parent directory.

When aclmode is set to "restricted", chmod is prevented on non-trivial ACLs. It
is not a stretch to assume that in this case the administrator does not want
ZFS to add the missing special entries. Add check for this aclmode, and if an
inherited entry is present skip zfs_acl_chmod().

Signed-off-by: Andrew Walker <awalker@ixsystems.com>
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anodos325 can you address the style issue in your commit message? The change itself LGTM.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Aug 22, 2020
@behlendorf behlendorf merged commit a741b38 into openzfs:master Aug 23, 2020
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
In absence of inheriting entry for owner@, group@, or everyone@,
zfs_acl_chmod() is called to set these. This can cause confusion for Samba
admins who do not expect these entries to appear on newly created files and
directories once they have been stripped from from the parent directory.

When aclmode is set to "restricted", chmod is prevented on non-trivial ACLs.
It is not a stretch to assume that in this case the administrator does not want
ZFS to add the missing special entries. Add check for this aclmode, and if an
inherited entry is present skip zfs_acl_chmod().

Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Andrew Walker <awalker@ixsystems.com>
Closes openzfs#10748
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
In absence of inheriting entry for owner@, group@, or everyone@,
zfs_acl_chmod() is called to set these. This can cause confusion for Samba
admins who do not expect these entries to appear on newly created files and
directories once they have been stripped from from the parent directory.

When aclmode is set to "restricted", chmod is prevented on non-trivial ACLs.
It is not a stretch to assume that in this case the administrator does not want
ZFS to add the missing special entries. Add check for this aclmode, and if an
inherited entry is present skip zfs_acl_chmod().

Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Andrew Walker <awalker@ixsystems.com>
Closes openzfs#10748
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants