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

Expand SSH tables to support Windows #6161

Merged
merged 8 commits into from
Feb 5, 2020
Merged

Conversation

puffyCid
Copy link
Contributor

@puffyCid puffyCid commented Jan 8, 2020

Starting with Windows 10 1803, Windows now includes a SSH client (OpenSSH) by default.
This minor PR simply expands the existing ssh_configs and user_ssh_keys tables to support Windows.
Let me know if there are any issues.

muffins
muffins previously approved these changes Jan 12, 2020
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! @puffyCid do you happen to know if we could get the unit/integration tests for these tables working on Windows too? Are they already running? That might make a really nice addition to this diff, or a good follow-up issue for someone to take on.

@theopolis
Copy link
Member

Yeah that's a good call out @muffins, we should change the CMake logic such that the integration tests for these tables run on Windows too. https://github.com/osquery/osquery/blob/master/tests/integration/tables/CMakeLists.txt#L125

@puffyCid
Copy link
Contributor Author

I moved the tables to support Windows tests. But currently it looks like the tests are not implemented to test anything.
Is it even possible to create a specific test for these tables?
Are there dummy user_ssh keys in the Windows VM used to build/test osquery?
Also by default I believe there is no ssh_config for the builtin ssh client for Windows, you have to manually make a config file/directory

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.

@puffyCid ah good call, I'd say we don't need to stress too much about adding in new tests, (if you are feeling up for it though, yeah a good test would be to write out some simple test SSH keys and clean them up after, or do some clever mocking). I mostly wanted to ensure that we had tests hooked up, so if we get tests for the other tables they'll apply to Windows too. I'd say just moving the integration tests to build for all platforms should be sufficient. Thanks!

@theopolis theopolis merged commit c722c68 into osquery:master Feb 5, 2020
@puffyCid puffyCid deleted the windows_ssh branch February 11, 2021 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants