Bug 14002 - samba.ntacls python module uses invalid "state dir" smb.conf parameter
samba.ntacls python module uses invalid "state dir" smb.conf parameter
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Tools
unspecified
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2019-06-19 13:06 UTC by Björn Baumbach
Modified: 2019-06-27 10:31 UTC (History)
3 users (show)

See Also:


Attachments
proposed patch for 4.10 (4.76 KB, patch)
2019-06-19 13:10 UTC, Björn Baumbach
dbagnall: review+
timbeale: review+
bbaumbach: ci‑passed+
Details
proposed patch for 4.9 (1.44 KB, patch)
2019-06-20 13:09 UTC, Björn Baumbach
abartlet: review+
dbagnall: review+
bbaumbach: ci‑passed+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Björn Baumbach 2019-06-19 13:06:50 UTC
The samba.ntacls python module queries for an invalid "state dir" smb.conf parameter.

This leads to errors like:

# samba-tool ntacl get new --as-sddl --xattr-backend=tdb --use-ntvfs
Unknown parameter encountered: "state dir"
ERROR(<type 'exceptions.AttributeError'>): uncaught exception - 'NoneType' object has no attribute 'endswith'
  File "/usr/lib/python2.7/dist-packages/samba/netcmd/__init__.py", line 185, in _run
    return self.run(*args, **kwargs)
  File "/usr/lib/python2.7/dist-packages/samba/netcmd/ntacl.py", line 204, in run
    session_info=system_session_unix())
  File "/usr/lib/python2.7/dist-packages/samba/ntacls.py", line 103, in getntacl
    (backend_obj, dbname) = checkset_backend(lp, backend, eadbfile)
  File "/usr/lib/python2.7/dist-packages/samba/ntacls.py", line 80, in checkset_backend
    return (samba.xattr_tdb, os.path.abspath(os.path.join(lp.get("state dir"), "xattr.tdb")))
  File "/usr/lib/python2.7/posixpath.py", line 77, in join
    elif path == '' or path.endswith('/'):


Patch is already upstream:
test: 1b0184a9562689a658e75a0cfc69bdd23277cff6
fix: 670a12df52df63a067b638d37bec71341bf18bdd
Comment 1 Björn Baumbach 2019-06-19 13:10:55 UTC
Created attachment 15257 [details]
proposed patch for 4.10
Comment 2 Björn Baumbach 2019-06-19 20:08:26 UTC
Comment on attachment 15257 [details]
proposed patch for 4.10

https://gitlab.com/samba-team/devel/samba/pipelines/66978074
Comment 3 Tim Beale 2019-06-19 21:44:15 UTC
Patch looks good. I just had a couple of questions:
- The bad line of code looks like it's been there since 2012. Should this go back to 4.9 as well?
- The NTVFS bit confused me a bit. I take it this samba-tool parameter is perhaps just a bit badly named, and in this case it means 'direct access of the TDB xattr backend' rather than 'access the NTVFS file server'?
Comment 4 Björn Baumbach 2019-06-20 13:05:02 UTC
(In reply to Tim Beale from comment #3)

> - The bad line of code looks like it's been there since 2012. Should
> this go back to 4.9 as well?

Yes, I'll add a patch.

> - The NTVFS bit confused me a bit. I take it this samba-tool parameter
> is perhaps just a bit badly named, and in this case it means 'direct
> access of the TDB xattr backend' rather than 'access the NTVFS file
> server'?

Yes, it is confusing. It sets the NT ACLs directly "for use with the ntvfs file server", not using the ntvfs file server.

I did some research and figured out that this option has been introduced to have an option to enable the "old behavior". For the ntvfs file server the POSIX ACLs are not relevant. The following commit introduced a new default, which uses the smbd/s3fs. Maybe this makes this more clear:

commit a778662da8b1dfc65bde55644703f2a3146ef7a8
Author: Andrew Bartlett <abartlet@samba.org>
Date:   Thu Aug 2 16:15:27 2012 +1000

  s4-provision: set POSIX ACLs to for use with the smbd file server (s3fs)
  
  This handles the fact that smbd will rarely override the POSIX ACL enforced by
  the kernel.  This has caused issues with the creation of group policies by
  other members of the Domain Admins group.
  
  Andrew Bartlett
Comment 5 Björn Baumbach 2019-06-20 13:09:06 UTC
Created attachment 15260 [details]
proposed patch for 4.9
Comment 6 Björn Baumbach 2019-06-20 15:41:50 UTC
Comment on attachment 15260 [details]
proposed patch for 4.9

CI passed:
https://gitlab.com/samba-team/devel/samba/pipelines/67157795
Comment 7 Björn Baumbach 2019-06-22 18:13:05 UTC
Thanks for the review.

Please correct me if I am wrong regarding the explanation of the ntvfs server or --use-ntvfs option. It is a long time ago, when I've used the ntvfs. If it will be removed soon, we should change the name of the parameters to make it more clear what they do.

Karo, please add the patches to 4.10 and 4.9.
Thank you.
Comment 8 Andrew Bartlett 2019-06-23 02:22:45 UTC
(In reply to Björn Baumbach from comment #7)
Yeah, it sets a simpler form of xattr containing the NTACL, which does not include a hash of the posix ACL (as the NTVFS file server never had that feature) and sets it directly from python, not via the python smbd vfs wrappers. 

We use that a lot in test scripts to avoid complexity of the full smbd integration when we are trying to test other things.
Comment 9 Karolin Seeger 2019-06-26 07:30:10 UTC
Pushed to autobuild-v4-{10,9}-test.
Comment 10 Karolin Seeger 2019-06-27 10:31:49 UTC
(In reply to Karolin Seeger from comment #9)
Pushed to both branches.
Closing out bug report.

Thanks!