-
Notifications
You must be signed in to change notification settings - Fork 231
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
Change from Set/GetNamedSecurityInfo to Nt{Query,Set}SecurityInfo #1589
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change looks straight forward. I have some concerns about the package we're using tho.
uint32(len(buf)), | ||
&bufLen) | ||
}, &buf, &bufLen) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we verify that changes are working as expected by any unit test?
TODO: an automated test to double check |
79f9a8c
to
c39fee4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall changes look good. Just some Nitpicking.
QQ: On the validation size: E2E test involving Local resource use the changes you've done, correct?
This PR changes the way that we were obtaining and setting ACLs to ensure accuracy. Set/GetNamedSecurityInfo would do some merging, which made it not entirely accurate for backups. However, the Nt{Query,Set}SecurityInfo APIs do not do merging, and allow for considerably more accurate preservation of ACLs.