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

tables: update Windows codename column to be that of name #5887

Merged
merged 1 commit into from
Oct 17, 2019
Merged

tables: update Windows codename column to be that of name #5887

merged 1 commit into from
Oct 17, 2019

Conversation

muffins
Copy link
Contributor

@muffins muffins commented Oct 15, 2019

Summary:
This addresses Issue #4080 by removing the code_name column on Windows whole-sale, and replacing it's contents with that of the name column. See the discussion in #4080 for additional details.

Test Plan:

Ran on Windows 10:

PS C:\Users\Nicholas\work\repos\osquery\build\windows10> .\osquery\RelWithDebInfo\osqueryd.exe -S                                                                 Using a �[1mvirtual database�[0m. Need help, type '.help'
osquery> select * from os_version;
+--------------------------+------------+-------+-------+-------+-------+----------+---------------+--------------------------+---------------------------+
| name                     | version    | major | minor | patch | build | platform | platform_like | codename                 | install_date              |
+--------------------------+------------+-------+-------+-------+-------+----------+---------------+--------------------------+---------------------------+
| Microsoft Windows 10 Pro | 10.0.18362 | 10    | 0     |       | 18362 | windows  | windows       | Microsoft Windows 10 Pro | 20190522202404.000000-420 |
+--------------------------+------------+-------+-------+-------+-------+----------+---------------+--------------------------+---------------------------+
osquery>

Ran on Windows 7:

PS C:\Users\RE\Desktop> .\osqueryd.exe -S
Using a ←[1mvirtual database←[0m. Need help, type '.help'
osquery> select * from os_version;
+---------------------------------+----------+-------+-------+-------+-------+----------+---------------+---------------
------------------+---------------------------+
| name                            | version  | major | minor | patch | build | platform | platform_like | codename
                  | install_date              |
+---------------------------------+----------+-------+-------+-------+-------+----------+---------------+---------------
------------------+---------------------------+
| Microsoft Windows 7 Enterprise  | 6.1.7601 | 6     | 1     |       | 7601  | windows  | windows       | Microsoft Wind
ows 7 Enterprise  | 20150317162012.000000-360 |
+---------------------------------+----------+-------+-------+-------+-------+----------+---------------+---------------
------------------+---------------------------+
osquery>

@muffins muffins force-pushed the fix4080-win-codename-fix branch 2 times, most recently from bf7b122 to ed6a09b Compare October 15, 2019 04:24
@theopolis theopolis added the flaky test A unit test that sometimes or on some systems does not return success, when it should label Oct 15, 2019
@theopolis
Copy link
Member

I think you got bit by a flaky test:

11: Test command: /__w/1/b/build/osquery/logger/tests/osquery_logger_tests-test
11: Test timeout computed to be: 10000000
11: Running main() from /__w/1/b/build/libs/src/patched-source/googletest/src/googletest/src/gtest_main.cc
11: [==========] Running 12 tests from 1 test case.
11: [----------] Global test environment set-up.
11: [----------] 12 tests from LoggerTests
11: [ RUN      ] LoggerTests.test_plugin
11: [       OK ] LoggerTests.test_plugin (1 ms)
11: [ RUN      ] LoggerTests.test_logger_init
11: W1015 04:47:52.000437 57552 logger.cpp:152] Logger test is generating a warning status (1)
11: [       OK ] LoggerTests.test_logger_init (0 ms)
11: [ RUN      ] LoggerTests.test_log_string
11: [       OK ] LoggerTests.test_log_string (0 ms)
11: [ RUN      ] LoggerTests.test_logger_log_status
11: W1015 04:47:52.000552 57552 logger.cpp:186] Logger test is generating a warning status (2)
11: /__w/1/s/osquery/logger/tests/logger.cpp:194: Failure
11: Expected: (now) >= (LoggerTests::last_status.time), actual: 1571114871 vs 1571114872
11: [  FAILED  ] LoggerTests.test_logger_log_status (0 ms)

@muffins
Copy link
Contributor Author

muffins commented Oct 16, 2019

@keeleysam @FritzX6 @directionless y'all mind taking a look considering this is relevant to the issue discussing this problem? Curious of folks' thoughts on removing the list whole sale.

@keeleysam
Copy link
Contributor

This looks like a completely reasonable change to me, and good to not maintain that list in the source code any longer.

theopolis
theopolis requested changes Oct 16, 2019
View reviewed changes
osquery/tables/system/windows/os_version.cpp Outdated
@@ -142,7 +31,10 @@ QueryData genOSVersion(QueryContext& context) {
}

wmiResults[0].GetString("InstallDate", r["install_date"]);
wmiResults[0].GetString("Caption", r["name"]);
std::string osName{""};
Copy link
Member

Choose a reason for hiding this comment

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

I think the {""} is superfluous.

@theopolis
Copy link
Member

@muffins, one nitpick, but do you mind rebasing onto master too?

@muffins muffins force-pushed the fix4080-win-codename-fix branch from ed6a09b to 97c9e44 Compare October 17, 2019 00:17
@muffins
version: fix windows os codename lookup
a8f54d7 
Summary: This updates the logic for lookups on Windows codenames to
correctly return '6' as the minimum version, which corresponds with
Windows 7.

Test Plan: Ran the code on Windows 7, will update with results:
```

```
@muffins muffins force-pushed the fix4080-win-codename-fix branch from 97c9e44 to a8f54d7 Compare October 17, 2019 00:18
@directionless
Copy link
Member

directionless commented Oct 17, 2019 •
edited
Loading

I agree maintaining the mapping here seems hard. I don't know what the underlying intent is, but I'm generally onboard here. I also observe that removing the leading Microsoft Windows gets you pretty close to the codenames. With some weirdness around language specific editions. I'm curious if @FritzX6 weighs in.

name,codename
Microsoft Windows 7 Ultimate ,Ultimate
Microsoft Windows 10 Pro for Workstations,
Microsoft Windows 10 Enterprise Insider Preview,Windows 10 Enterprise
Microsoft Windows 10 Education,
Microsoft Windows 10 Home,Windows 10 Home
Microsoft Windows 10 企业版 2016 长期服务版,
Microsoft Windows 7 Enterprise ,Windows 10 Enterprise
Microsoft Windows Server 2016 Datacenter,Server Datacenter (full installation)
Microsoft Windows 10 Business,Windows 10 Pro
Microsoft Windows Server 2012 R2 Datacenter,Server Datacenter (full installation)
Microsoft Windows Server 2016 Datacenter Evaluation,Server Datacenter (evaluation installation)
Microsoft Windows Server 2016 Standard,Server Standard
Microsoft Windows 10 Enterprise Evaluation,Windows 10 Enterprise Evaluation
Microsoft Windows 10 Pro N,Windows 10 Pro N
Microsoft Windows Server Datacenter,
Майкрософт Windows 10 Pro,Windows 10 Pro
Microsoft Windows 10 Home Single Language,Windows 10 Home Single Language
Microsoft Windows 10 Enterprise,Windows 10 Enterprise
Microsoft Windows 8.1 Pro,Windows 10 Pro
Microsoft Windows 10 Pro Education,
Microsoft Windows 10 Famille,Windows 10 Home
Microsoft Windows Server 2012 R2 Standard,Server Standard
Microsoft Windows Server 2012 R2 Foundation,Server Foundation
Microsoft Windows Server 2008 R2 Standard ,Server Standard
Microsoft Windows 10 Enterprise 2016 LTSB,
Microsoft Windows Server 2019 Standard,Server Standard
Microsoft Windows 10 Pro,Windows 10 Pro
Microsoft Windows Server 2016 Standard Evaluation,Server Standard (evaluation installation)
Microsoft Windows 7 Professional ,Windows 10 Pro
Майкрософт Windows 10 Домашняя для одного языка,Windows 10 Home Single Language

@muffins
Copy link
Contributor Author

muffins commented Oct 17, 2019

@directionless good call, I'm happy to hear what @FritzX6 has to say. I'm going to close this out, but am happy to do follow-up PRs if folks think it's necessary. Overall I feel this might be worth just calling codename whatever name is. I don't think that codename gives that much extra value for the cost of maintaining that list :\

@muffins muffins merged commit 3957d8e into osquery:master Oct 17, 2019
@directionless
Copy link
Member

Works for me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theopolis theopolis theopolis approved these changes

Assignees
No one assigned
Labels
bug flaky test A unit test that sometimes or on some systems does not return success, when it should Hacktoberfest Windows
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@muffins @theopolis @keeleysam @directionless