Added is_hidden column to the users and groups tables on macOS. #5368
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
akindyakov
reviewed
Jan 24, 2019
There was a problem hiding this comment.
Thank you for working on it, @drakearonhalt !
And may I ask you to create a test for you changes? Or at least to modify existing integration tests for this tables in tests/integration/tables/.
| isHidden = std::string([isHiddenValue[0] UTF8String]); | ||
| boost::algorithm::to_lower(isHidden); | ||
| // isHiddenValue can be 1 || True || YES to hide the user or group | ||
| if (isHidden == "yes" || isHidden == "1" || isHidden == "true") { |
There was a problem hiding this comment.
Would it be better to use tryTo<bool>() from osquery/utils/conversions/tryto.h instead of manual parsing?
Something like:
tryTo<bool>(std::string([isHiddenValue[0] UTF8String])).takeOr(false);| } | ||
| results.push_back(r); |
There was a problem hiding this comment.
May I ask you to convert r to rvalue for push_back, to prevent useless copy?
result.push_back(std::move(r));|
@akindyakov I modified |
|
Perfect, let me run more tests on it and we good to go |
There was a problem hiding this comment.
@akindyakov has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
All tests passed. Lets merge it |
muffins
pushed a commit
to muffins/osquery
that referenced
this pull request
Feb 1, 2019
…ery#5368) Summary: This PR is the result of the discussion in a previous PR (osquery#5348) after we determined account_policy_data was the wrong place for the column. Add `is_hidden` column to the users and groups tables in macOS. `is_hidden` is populated by looking for the `dsAttrTypeNative:IsHidden` attribute in the OpenDirectory record for the user/group if the value is `1`, `True`, or `Yes` is_hidden is 1. If the value is anything else it's set to 0. Invalid values have the same affect as the attribute not existing at all. The `dsAttrTypeNative:IsHidden` attribute controls whether a user account is is visible in the preferences panel similar to having a uid < 500. One test failed when running buck test: ``` ====STANDARD OUT==== tests/integration/tables/helper.cpp:159: Failure Value of: boost::get<CustomCheckerType>(validator)(value) Actual: false Expected: true Custom validator of the column "mask" with value "" failed ``` This also fails when I ran the test on the current experimental branch as well. Important to note I had to remove the optimization on both the user and group tables that just called `getpwnam` if the query specified the `uid` or `gid` since the struct returned doesn't contain the `IsHidden` attribute. I'm not sure if or how much this will affect performance since I wasn't able to get the profiling to work with the new version (very likely I'm just doing it incorrectly). Pull Request resolved: osquery#5368 Differential Revision: D13862375 Pulled By: akindyakov fbshipit-source-id: 1fec88a6ba71884f7e611e1d96ea00630c5be655
directionless
added a commit
to directionless/osquery
that referenced
this pull request
Jul 23, 2019
In osquery#5368 the optimization where the uid/gid constraint was removed. At the time, the PR states this might have adverse performance effects. I think, in fact, we have them. Debugging a performance regression in osquery#5656 I see that we're doing a lot of queries to this table, they are quite expensive. This fix may not be ideal. It removes the `is_hidden` attribute when constraining by uid/gid. Perhaps there's a better query planner related fix
directionless
added a commit
to directionless/osquery
that referenced
this pull request
Jul 23, 2019
…S. (osquery#5368)" This reverts commit e205458.
directionless
added a commit
to directionless/osquery
that referenced
this pull request
Jul 23, 2019
…S. (osquery#5368)" This reverts commit e205458.
directionless
added a commit
to directionless/osquery
that referenced
this pull request
Jul 24, 2019
…S. (osquery#5368)" This reverts commit e205458.
theopolis
added a commit
to theopolis/osquery
that referenced
this pull request
Aug 4, 2019
This PR supersedes osquery#5669 with an alternate approach of moving forward without a revert of osquery#5368.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is the result of the discussion in a previous PR (#5348) after we determined account_policy_data was the wrong place for the column.
Add
is_hiddencolumn to the users and groups tables in macOS.is_hiddenis populated by looking for thedsAttrTypeNative:IsHiddenattribute in the OpenDirectory record for the user/group if the value is1,True, orYesis_hidden is 1. If the value is anything else it's set to 0. Invalid values have the same affect as the attribute not existing at all.The
dsAttrTypeNative:IsHiddenattribute controls whether a user account is is visible in the preferences panel similar to having a uid < 500.One test failed when running buck test:
This also fails when I ran the test on the current experimental branch as well.
Important to note I had to remove the optimization on both the user and group tables that just called
getpwnamif the query specified theuidorgidsince the struct returned doesn't contain theIsHiddenattribute. I'm not sure if or how much this will affect performance since I wasn't able to get the profiling to work with the new version (very likely I'm just doing it incorrectly).