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

Add SMB2_0_INFO_SECURITY request support #65

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

principis
Copy link

@principis principis commented Jul 14, 2022

This PR implements support for the SMB2 QUERY_INFO request with InfoType SMB2_0_INFO_SECURITY.

I chose the OWNER_SECURITY_INFORMATION, GROUP_SECURITY_INFORMATION and DACL_SECURITY_INFORMATION security attributes, since this is what pysmb queries for and is everything I need. I'm not sure if more is necessary at this time.

Let me know if any changes are necessary!

Edit: force push to fix typo

Copy link
Owner

@hirochachacha hirochachacha left a comment

Choose a reason for hiding this comment

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

Hi, thank you for the huge work! I didn't check it in detail yet, but here is my initial review.
Also, please consider implementing SetSecurity feature in the same PR, so that we can add test as well.

client.go Outdated Show resolved Hide resolved
client.go Outdated
Owner *Sid
Group *Sid
Flags uint16
Sacl []ACE
Copy link
Owner

Choose a reason for hiding this comment

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

same as above. we should define ACE(ACL?) type on the top package instead of internal package.

Copy link
Author

Choose a reason for hiding this comment

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

I refactored the way ACE's are decoded. Let me know if you agree with this approach!
An other option is renaming the internal ACE struct and just copy everything to the external ACE.

internal/smb2/dtyp.go Outdated Show resolved Hide resolved
internal/smb2/dtyp.go Outdated Show resolved Hide resolved
@principis
Copy link
Author

Also, please consider implementing SetSecurity feature in the same PR, so that we can add test as well.

I started implementing setSecurity on this branch. It sadly doesn't work (the server responds with STATUS_INVALID_INFO_CLASS), and I can't find the problem easily.

Implementing setSecurity in an abstract way is also incredibly difficult. My current implementation (if it would work) isn't very useful.

I'd like to omit it from this PR. My time is unfortunately limited, and if it's something I need in the future, I'll be happy to open a PR.

I'll add some tests to check if the returned FileSecurityInfo is well formed.

@hirochachacha
Copy link
Owner

Thanks, give me some time to review and think. I will revisit this on this weekend.

@principis
Copy link
Author

Take your time, thanks!

I've added a Security method on Share type, since that seemed necessary. Not sure if the other methods are still useful, but I'll let you decide! :)

@hirochachacha
Copy link
Owner

Hi, sorry for the delay. I did some investigation and I concluded that current Security/SetSecurity design doesn't work well because sacl and dacl require different permissions. Instead, I want to split its functionalities into 6 pieces.

func (f *File) GetDacl() (*ACL, error)
func (f *File) SetDacl(acl *ACL) error

func (f *File) GetSacl() (*ACL, error)
func (f *File) SetSacl(acl *ACL) error

func (f *File) GetOwner() (user, group *SID, err error)
func (f *File) SetOwner(user, group *SID) error

I rethought about exposing SID on the top package for convenience of encoding of SID.
So, I want to introduce helper functions for SID

func MustSID(s string) *SID
func ParseSID(s string) (*SID, error)

we also need to add constants like SecurityDescriptor's control flags, AceType, AccessMask.
because of current usage of dot import on the package, it will incur collisions. so we need to do refactoring.

There're a lot things to solve this issue. I made a draft on top of your work, but I need more time for investigations and tests.

@principis
Copy link
Author

Sounds good! Let me know if you want me to help with something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants