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

proposal: x/sys/windows: Add GetAce syscall #66850

Closed
claudiubelu opened this issue Apr 16, 2024 · 8 comments
Closed

proposal: x/sys/windows: Add GetAce syscall #66850

claudiubelu opened this issue Apr 16, 2024 · 8 comments

Comments

@claudiubelu
Copy link

Proposal Details

On Windows, access to an object / file can be controlled through the Discretionary Access Control List (DACL). The DACL contains a list of Access Control Entries (ACEs), which can be retrieved through the GetAce syscall.

Currently, we have support for manipulating the DACLs and setting ACEs, but we there's no support for querying / retriving set ACEs on an object / file, and there is no wrapper for GetAce, which would allow us to retrieve them.

References:

@gopherbot gopherbot added this to the Proposal milestone Apr 16, 2024
claudiubelu added a commit to claudiubelu/sys that referenced this issue Apr 16, 2024
GetAce obtains a pointer to an access control entry (ACE) in an
discretionary access control list (DACL), which controls access to
an object.

Adds the ACE_HEADER and ACCESS_ALLOWED_ACE structs.
Adds GetEntriesFromACL function which returns an array of ACEs from the
given ACL if no errors have been encountered.

References:

- https://learn.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-ace_header
- https://learn.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-access_allowed_ace
- https://learn.microsoft.com/en-us/windows/win32/api/securitybaseapi/nf-securitybaseapi-getace

Fixes golang/go#66850
@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Apr 16, 2024
@ianlancetaylor
Copy link
Contributor

Do you have a proposal for what this would look like? Thanks.

CC @golang/windows

@claudiubelu
Copy link
Author

I do. I have sent a PR here: golang/sys#191 / https://go-review.googlesource.com/c/sys/+/578976

@seankhliao seankhliao changed the title proposal: golang.org/x/sys/windows: Add GetAce syscall proposal: x/sys/windows: Add GetAce syscall Apr 19, 2024
@alexbrainman
Copy link
Member

@claudiubelu ,

I suggest we add Windows GetAce API
https://learn.microsoft.com/en-us/windows/win32/api/securitybaseapi/nf-securitybaseapi-getace
instead of GetEntriesFromACL added in https://go-review.googlesource.com/c/sys/+/578976 .

Everyone expects to find standard Windows API in golang.org/x/sys/windows package and not some made up functions.

They can write the GetEntriesFromACL themselves. You can keep getEntriesFromACL as part of your test in https://go-review.googlesource.com/c/sys/+/578976 .

Then you will not need to wait for this proposal, because it is OK to just add standard Windows APIs.

What do you think?

Alex.

claudiubelu added a commit to claudiubelu/sys that referenced this issue Apr 29, 2024
GetAce obtains a pointer to an access control entry (ACE) in an
discretionary access control list (DACL), which controls access to
an object.

Adds the ACE_HEADER and ACCESS_ALLOWED_ACE structs.
Adds GetEntriesFromACL function which returns an array of ACEs from the
given ACL if no errors have been encountered.

References:

- https://learn.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-ace_header
- https://learn.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-access_allowed_ace
- https://learn.microsoft.com/en-us/windows/win32/api/securitybaseapi/nf-securitybaseapi-getace

Fixes golang/go#66850
@claudiubelu
Copy link
Author

@claudiubelu ,

I suggest we add Windows GetAce API https://learn.microsoft.com/en-us/windows/win32/api/securitybaseapi/nf-securitybaseapi-getace instead of GetEntriesFromACL added in https://go-review.googlesource.com/c/sys/+/578976 .

Everyone expects to find standard Windows API in golang.org/x/sys/windows package and not some made up functions.

They can write the GetEntriesFromACL themselves. You can keep getEntriesFromACL as part of your test in https://go-review.googlesource.com/c/sys/+/578976 .

Then you will not need to wait for this proposal, because it is OK to just add standard Windows APIs.

What do you think?

Alex.

Done.

@alexbrainman
Copy link
Member

@claudiubelu

Let's continue conversation on

https://go-review.googlesource.com/c/sys/+/578976

@ianlancetaylor

I hope it is OK to rename existing Windows struct field to make it accessible from outside of "golang.org/x/sys/windows" package. See https://go-review.googlesource.com/c/sys/+/578976/4/windows/security_windows.go#1185 for details. This code is very old, and all struct fields should be public but they are not. I am surprised that no one complained about that earlier.

Thank you.

Alex

@ianlancetaylor
Copy link
Contributor

I don't see a problem with renaming an unexported field to be exported.

claudiubelu added a commit to claudiubelu/sys that referenced this issue Jun 3, 2024
GetAce obtains a pointer to an access control entry (ACE) in an
discretionary access control list (DACL), which controls access to
an object.

Adds the ACE_HEADER and ACCESS_ALLOWED_ACE structs.
Adds GetEntriesFromACL function which returns an array of ACEs from the
given ACL if no errors have been encountered.

References:

- https://learn.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-ace_header
- https://learn.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-access_allowed_ace
- https://learn.microsoft.com/en-us/windows/win32/api/securitybaseapi/nf-securitybaseapi-getace

Fixes golang/go#66850
claudiubelu added a commit to claudiubelu/sys that referenced this issue Jun 3, 2024
GetAce obtains a pointer to an access control entry (ACE) in an
discretionary access control list (DACL), which controls access to
an object.

Adds the ACE_HEADER and ACCESS_ALLOWED_ACE structs.
Adds GetEntriesFromACL function which returns an array of ACEs from the
given ACL if no errors have been encountered.

References:

- https://learn.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-ace_header
- https://learn.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-access_allowed_ace
- https://learn.microsoft.com/en-us/windows/win32/api/securitybaseapi/nf-securitybaseapi-getace

Fixes golang/go#66850
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/578976 mentions this issue: windows: add GetAce Windows API

claudiubelu added a commit to claudiubelu/sys that referenced this issue Jun 17, 2024
GetAce obtains a pointer to an access control entry (ACE) in an
discretionary access control list (DACL), which controls access to
an object.

Adds the ACE_HEADER and ACCESS_ALLOWED_ACE structs.
Adds GetEntriesFromACL function which returns an array of ACEs from the
given ACL if no errors have been encountered.

References:

- https://learn.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-ace_header
- https://learn.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-access_allowed_ace
- https://learn.microsoft.com/en-us/windows/win32/api/securitybaseapi/nf-securitybaseapi-getace

Fixes golang/go#66850
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/599295 mentions this issue: windows: correctly generate GetAce syscall

gopherbot pushed a commit to golang/sys that referenced this issue Jul 22, 2024
GetAce expects a failretval==0, and we shouldn't call GetLastError on
error.

For golang/go#66850

Change-Id: I812d71b066d56e8285324e70b8b5b5fb42b5ce35
GitHub-Last-Rev: 40cf750
GitHub-Pull-Request: #205
Reviewed-on: https://go-review.googlesource.com/c/sys/+/599295
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Incoming
Development

Successfully merging a pull request may close this issue.

4 participants