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.

Sign up for GitHub

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

Jump to bottom

macOS: Add a bsd_flags column to the file table #5981

Merged
merged 11 commits into from Dec 4, 2019
Merged

macOS: Add a bsd_flags column to the file table #5981

merged 11 commits into from Dec 4, 2019

Conversation

GarretReece
Copy link
Contributor

Initial work by @alessandrogario , git wrangling to get it playing with current master by @GarretReece

This PR implements support for the BSD flags on macOS as documented in issue #4189.

garretreece@latke-2:~$ ./osqueryd -S "select path, bsd_flags from file where path='/Users/garretreece/test-file'"
+------------------------------+-----------+
| path                         | bsd_flags |
+------------------------------+-----------+
| /Users/garretreece/test-file |           |
+------------------------------+-----------+
garretreece@latke-2:~$ chflags nodump,hidden test-file 
garretreece@latke-2:~$ ./osqueryd -S "select path, bsd_flags from file where path='/Users/garretreece/test-file'"
+------------------------------+----------------+
| path                         | bsd_flags      |
+------------------------------+----------------+
| /Users/garretreece/test-file | HIDDEN, NODUMP |
+------------------------------+----------------+
garretreece@latke-2:~$

@GarretReece
add bsd_flags column to the file table on macos
8b4e7a3 
initial work by alessandro, git wrangling by garret
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 1, 2019 •
edited

CLA Check
The committers are authorized under a signed CLA.

@alessandrogario alessandrogario mentioned this pull request Nov 1, 2019
Closed
@theopolis
Copy link
Member

IMO if we can avoid pre-processor logic we should, and instead use the platform-testing APIs in ./utils.

theopolis
theopolis requested changes Nov 6, 2019
View reviewed changes
osquery/core/darwin/bsd_file_flags.cpp Outdated Show resolved Hide resolved
osquery/tables/utility/file.cpp Outdated Show resolved Hide resolved
osquery/filesystem/fileops.h Show resolved Hide resolved
osquery/core/tests/darwin/bsd_file_flags_tests.cpp Outdated Show resolved Hide resolved
osquery/filesystem/fileops.h Outdated Show resolved Hide resolved
osquery/tables/utility/file.cpp Show resolved Hide resolved
@theopolis
Copy link
Member

I just noticed that files are being added to ./osquery/core. I think we should avoid this unless the code is very internal to osquery's operation. I suggest placing this within the ./osquery/filesystem abstraction or alongside table implementation code within ./osquery/tables.

directionless
directionless previously approved these changes Nov 20, 2019
View reviewed changes
Copy link
Member

@directionless directionless left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me. I'll leave for @theopolis to merge when he's happy

osquery/filesystem/darwin/bsd_file_flags.cpp

std::vector<std::string> label_list;

for (const auto& p : kBsdFlagMap) {
Copy link
Member

Choose a reason for hiding this comment

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

Will this always iterate in the same order? I think it's desirable for output to have a stable order.

directionless
directionless previously approved these changes Nov 20, 2019
View reviewed changes
theopolis
theopolis previously requested changes Nov 22, 2019
View reviewed changes
osquery/filesystem/BUCK Outdated
@@ -104,7 +110,8 @@ osquery_cxx_test(
platform_srcs = [
(
MACOSX,
["tests/darwin/plist_tests.cpp"],
["tests/darwin/plist_tests.cpp",
Copy link
Member

Choose a reason for hiding this comment

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

Can you write this as:

            [
                "tests/darwin/plist_tests.cpp",
                "tests/darwin/bsd_file_flags_tests.cpp"
            ],

osquery/filesystem/CMakeLists.txt Outdated
@@ -101,7 +107,10 @@ function(generateOsqueryFilesystemTest)
)

if(DEFINED PLATFORM_MACOS)
list(APPEND source_files tests/darwin/plist_tests.cpp)
list(APPEND source_files
tests/darwin/plist_tests.cpp
Copy link
Member

Choose a reason for hiding this comment

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

The indent is wrong here, it should be:

    list(APPEND source_files 
      tests/darwin/plist_tests.cpp
      tests/darwin/bsd_file_flags_tests.cpp
    )

directionless
directionless approved these changes Nov 22, 2019
View reviewed changes
Copy link
Member

@directionless directionless left a comment

Choose a reason for hiding this comment

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

updates seem fine

muffins
muffins approved these changes Dec 4, 2019
View reviewed changes
Copy link
Contributor

@muffins muffins left a comment

Choose a reason for hiding this comment

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

lgtm too! I'm going to go ahead and merge as it looks like all of the issues have been addressed from folks. Nice work!

@muffins muffins merged commit 5c3a463 into osquery:master Dec 4, 2019
rachelcipkins pushed a commit to trailofbits/osquery that referenced this pull request Dec 10, 2019
@GarretReece @rachelcipkins
macOS: Add a bsd_flags column to the file table (osquery#5981)
14e6e98
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@directionless directionless directionless approved these changes

@muffins muffins muffins approved these changes

@theopolis theopolis theopolis left review comments

Assignees
No one assigned
Labels
macOS virtual tables
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@GarretReece @theopolis @directionless @muffins