Adding Python extensions support for Windows by muffins · Pull Request #48 · osquery/osquery-python

muffins · 2018-02-01T00:10:01Z

This adds in an implementation of TPipe for windows, which allows us to make use of Python extensions on the Windows platform.

Some samples of the extensions in action. First we startup osquery:

:\Users\Nick\work\repos\osquery [win-python-auto-ext]
λ  .\build\windows10\osquery\RelWithDebInfo\osqueryd.exe -S --flagfile=C:\Users\Nick\work\configs\osquery_extensions\osquery.flags --verbose

Then we start up our extension:

C:\Users\Nick\work\repos\osquery-python [osquery-python-windows-port +1 ~0 -0 !]
λ  python .\foobar_table.ext --socket \\.\pipe\shell.em --verbose

We see the extension connect to our shell:

I0131 16:07:10.660527  4272 init.cpp:380] osquery initialized [version=3.0.0-17-g10c0c60b]
I0131 16:07:10.663009  4272 rocksdb.cpp:132] Opening RocksDB handle: C:\Users\Nick\work\configs\osquery_extensions\osquery.db
I0131 16:07:10.725503 22132 interface.cpp:338] Extension manager service starting: \\.\pipe\shell.em
I0131 16:07:10.741130 21252 interface.cpp:89] Thrift message: TPipe ::GetOverlappedResult errored GLE=errno = 109
I0131 16:07:10.741130 21252 interface.cpp:89] Thrift message: TConnectedClient died: TPipe: GetOverlappedResult failed
I0131 16:07:10.741130  5972 interface.cpp:89] Thrift message: TPipe ::GetOverlappedResult errored GLE=errno = 109
I0131 16:07:10.741130  5972 interface.cpp:89] Thrift message: TConnectedClient died: TPipe: GetOverlappedResult failed
Using a I0131 16:07:11.811651 25176 events.cpp:746] Starting event publisher run loop: windows_event_log
virtual database. Need help, type '.help'
osquery> I0131 16:07:12.191326  8460 interface.cpp:141] Registering extension (foobar_table, 29953, version=1.0.0, sdk=1.8.0)
I0131 16:07:12.191326  8460 registry.cpp:351] Extension 29953 registered table plugin foobar

We then query the extension:

osquery> select * from foobar;
+-----+-----+
| foo | baz |
+-----+-----+
| bar | baz |
| bar | baz |
+-----+-----+

So far I have tested extensions being autloaded with osquery, which currently works from both command line and running as a system service. I tested having a python extension that both provides data to, and queries data from osquery, and this didn't seem to work. I'm still digging into what's happening to prevent bidirectional communication, but throwing this up to get the review process started.

fmanco

Generally I like the code structure. I left a few comments but I should let you know that I'm not familiar with Windows or the win32 API so they're more generic.

fmanco · 2018-02-02T18:40:30Z

osquery/TPipe.py

+            raise TTransportException(TTransportException.ALREADY_OPEN)
+
+        h = None
+        while True:


I'm not familiar with this APIs (and Windows in general) but should we have something like a max_retries here to avoid this getting stuck forever on winerror.ERROR_PIPE_BUSY? It's probably an unlikely scenario but still.

fmanco · 2018-02-02T21:20:09Z

osquery/TPipe.py

+            raise TTransportException(
+                type=TTransportException.UNKNOWN, message='TPipe read failed')
+
+        if (err != 0):


Remove parenthesis for consistency.

fmanco · 2018-02-02T21:37:18Z

osquery/TPipe.py

+                type=TTransportException.UNKNOWN,
+                message='Failed to write to named pipe: ' + e.message)
+
+    def flush(self):


Should check if isOpen().

fmanco · 2018-02-02T21:37:55Z

osquery/TPipe.py

+
+        bytesWritten = None
+        try:
+            (writeError, bytesWritten) = win32file.WriteFile(


Is ignoring writeError intentional? If so maybe should add a comment. We should also handle bytesWritten. How would the caller know if the full buffer was written?

fmanco · 2018-02-02T21:40:15Z

osquery/TPipe.py

+
+    # Create a new named pipe if one doesn't already exist
+    def listen(self):
+        if self._handle == None:


Doesn't make much difference here but should use is instead of == as a rule.

fmanco · 2018-02-02T21:46:32Z

osquery/TPipe.py

+            raise TTransportException(
+                type=TTransportException.NOT_OPEN,
+                message='TCreateNamedPipe failed: {}'.format(err))
+            return False


I think this is unreachable as you're raising an exception before. Also for consistency maybe it makes sense to throw on error and not return as you're doing on other methods.

fmanco · 2018-02-02T21:49:18Z

osquery/TPipe.py

+
+    def accept(self):
+        if self._handle == None:
+            self.createNamedPipe()


Does it make sense to allow an accept before having created a pipe? Maybe this should just fail if self._handle is None.

That definitely makes sense to me, this logic flow was modeled after how the logic happens in Apache thrifts C++ implementation, so my guess is that maybe this happens occasionally? I'd prefer to leave it to stay consistent with the C++ impl, but am open to a discussion about it.

fmanco · 2018-02-02T21:49:29Z

osquery/TPipe.py

+                message='TCreateNamedPipe failed')
+
+    def accept(self):
+        if self._handle == None:


Same here with == vs is.

fmanco · 2018-02-02T21:53:16Z

osquery/TPipe.py

+
+            if ret == winerror.ERROR_PIPE_CONNECTED:
+                win32event.SetEvent(self._overlapped.hEvent)
+                break


Should raise on else.

I think we actually want the same logic as above with the client here. The intent was we'd have another while True loop that continues to attempt the connection, but this definitely needs cleaning!

fmanco · 2018-02-02T21:56:34Z

osquery/management.py

+            self._socket = tempfile.mkstemp(prefix="pyosqsock")
+            self._pidfile = tempfile.mkstemp(prefix="pyosqpid")
+            with open(self._pidfile[1], "w") as fh:
+                fh.write("100000")


I think you forgot to actually get the PID. Also maybe we should handle errors more gracefully here than allowing IOError to propagate.

I think this was testing logic that I forgot to remove, going to just nuke this component, as I think on posix we don't need to worry about the pidfile.

muffins · 2018-02-06T06:18:14Z

Huh, not entirely sure what I'm doing to screw up tests, however funny thing - tests pass on Windows :D

C:\Users\Nick\work\repos\osquery-python [osquery-python-windows-port]
λ  python setup.py test
running test
running egg_info
writing requirements to osquery.egg-info\requires.txt
writing osquery.egg-info\PKG-INFO
writing top-level names to osquery.egg-info\top_level.txt
writing dependency_links to osquery.egg-info\dependency_links.txt
reading manifest file 'osquery.egg-info\SOURCES.txt'
writing manifest file 'osquery.egg-info\SOURCES.txt'
running build_ext
test_simple_call (tests.test_config_plugin.TestConfigPlugin)
Tests for the call method of osquery.TablePlugin ... ok
test_simple_call (tests.test_logger_plugin.TestLoggerPlugin)
Tests for the call method of osquery.TablePlugin ... ok
test_plugin_inheritance (tests.test_plugin.TestBasePlugin)
Test that an object derived from BasePlugin works properly ... ok
test_singleton_creation (tests.test_singleton.TestSingleton)
Test that two singletons are the same object ... ok
test_plugin_was_registered (tests.test_table_plugin.TestTablePlugin)
Tests to ensure that a plugin was registered ... ok
test_routes_are_correct (tests.test_table_plugin.TestTablePlugin)
Tests to ensure that a plugins routes are correct ... ok
test_simple_call (tests.test_table_plugin.TestTablePlugin)
Tests for the call method of osquery.TablePlugin ... ok

----------------------------------------------------------------------
Ran 7 tests in 0.032s

OK

muffins · 2018-02-06T06:25:39Z

Looks like the only failing test is due to Python 2.6 no longer being supported:

isort requires Python '>=2.7, !=3.0.*, !=3.1.*, !=3.2.*, !=3.3.*' but the running Python is 2.6.9
The command "pip install -r requirements.txt" failed and exited with 1 during .
Your build has been stopped.

friedbutter

nits mostly.

friedbutter · 2018-02-15T21:46:52Z

osquery/TPipe.py

+        Generic close method, as both server and client rely on closing pipes
+        in the same way
+        """
+        if self._handle is not None:


if not self._handle

Isn't it always better to specifically test for None when that's what you care about? In this case if not self._handle will call __eq__ that can be overridden by the object I think...

I'm inclined to agree with @fmanco on this one, but that's largely because I'm more keen on explicit instead of implicit comparisons :\

friedbutter · 2018-02-16T18:39:39Z

osquery/TPipe.py

+        while conns < self._maxConnAttempts:
+            try:
+                h = win32file.CreateFile(
+                    self._pipeName,


It's hard to read. Please indent 4 spaces

friedbutter · 2018-02-16T18:40:01Z

osquery/TPipe.py

+            try:
+                h = win32file.CreateFile(
+                    self._pipeName,
+                    win32file.GENERIC_READ | win32file.GENERIC_WRITE, 0, None,


Ideally each param is a new line.

friedbutter · 2018-02-16T18:41:30Z

osquery/TPipe.py

+                        'Failed to open connection to pipe: {}'.format(e))
+
+            # Successfully connected, break out.
+            if h is not None and h.handle != winerror.ERROR_INVALID_HANDLE:


it's fine, nit it's better to check the type of the structure you look for in h.

This also could be if not h and h.handle ...

friedbutter · 2018-02-16T18:42:31Z

osquery/TPipe.py

+            raise TTransportException(
+                type=TTransportException.UNKNOWN, message='TPipe read failed')
+
+        if err != 0:


friedbutter · 2018-02-16T18:45:03Z

osquery/extension_client.py

 from osquery.extensions.ExtensionManager import Client

-DEFAULT_SOCKET_PATH = "/var/osquery/osquery.em"
+if sys.platform == "win32":


"win32" should be a const in thrift spec?

or a const somewhere we can resue?

friedbutter · 2018-02-16T18:46:10Z

osquery/extension_client.py

+    DEFAULT_SOCKET_PATH = r'\\.\pipe\osquery.em'
+else:
+    DEFAULT_SOCKET_PATH = "/var/osquery/osquery.em"
 """The default path for osqueryd sockets"""


this comment seems out of place

friedbutter · 2018-02-16T18:48:50Z

osquery/extension_client.py

+            sock = TPipe(pipeName=self.path)
+        else:
+            if uuid:
+                self.path += ".%s" % str(uuid)


could be:
self.path += ".{}".format(uuid) if uuid else ""

friedbutter · 2018-02-16T18:49:55Z

osquery/management.py

 from thrift.transport import TSocket
 from thrift.transport import TTransport

+if sys.platform == "win32":


"win32" is using a lot. plz consider make it a const or part of thrift spec

friedbutter · 2018-02-16T18:52:01Z

osquery/management.py

+
+    transport = None
+    if sys.platform == 'win32':
+        transport = TPipeServer(args.socket + "." + str(status.uuid))


"{}.{}".format(args.socket, status.uuid)

muffins · 2018-02-25T05:23:30Z

With the most recent commit I've verified this does not impact python extensions on posix platforms.

ryanheffernan · 2018-02-25T23:25:30Z

osquery/TPipe.py

+        saAttr = pywintypes.SECURITY_ATTRIBUTES()
+        saAttr.SetSecurityDescriptorDacl(1, None, 0)
+
+        self._handle = win32pipe.create_named_pipe(


This causes an error since create_named_pipe doesn't exist in the version of pywin32 I have.

Changing it to win32pipe.CreateNamedPipe fixed it.

Note this was using the latest pypiwin32.

ryanheffernan · 2018-02-26T05:55:54Z

osquery/TPipe.py

+        except Exception as e:
+            raise TTransportException(
+                type=TTransportException.UNKNOWN,
+                message='Failed to write to named pipe: ' + e.message)


There was no e.message when I tested this, might want to use str(e)

ryanheffernan · 2018-02-26T15:14:39Z

osquery/TPipe.py

+                raise TTransportException(
+                    type=TTransportException.NOT_OPEN,
+                    message='TConnectNamedPipe failed: {}'.format(e.message))
+


ryanheffernan · 2018-02-26T15:25:11Z

I found a few bugs, see above comments. With those fixed I'd be good to merge, was able to get this working successfully once I made the changes locally.

ryanheffernan · 2018-02-28T04:28:28Z

osquery/TPipe.py

+                        win32file.FILE_FLAG_OVERLAPPED,
+                        None)
+            except Exception as e:
+                if e[0] != winerror.ERROR_PIPE_BUSY:


This line gives an IndexError - I think it should just be 'if e 1= ..'.

Traceback (most recent call last): File "C:\Python36-32\lib\site-packages\osquery-3.0.1-py3.6.egg\osquery\TPipe.py", line 76, in open pywintypes.error: (2, 'CreateFile', 'The system cannot find the file specified.') During handling of the above exception, another exception occurred: Traceback (most recent call last): File "C:\ProgramData\osquery\extensions\bb_e2e_test.ext", line 69, in <module> osquery.start_extension(name="bb_e2e_tests", version="1.0.0") File "C:\Python36-32\lib\site-packages\osquery-3.0.1-py3.6.egg\osquery\management.py", line 198, in start_extension File "C:\Python36-32\lib\site-packages\osquery-3.0.1-py3.6.egg\osquery\extension_client.py", line 62, in open File "C:\Python36-32\lib\site-packages\thrift\transport\TTransport.py", line 153, in open return self.__trans.open() File "C:\Python36-32\lib\site-packages\osquery-3.0.1-py3.6.egg\osquery\TPipe.py", line 78, in open TypeError: 'error' object does not support indexing

Ok will take a look. Just as a heads up though, looks like you’re testing this with Python 3,m given The path to your binary, and I’m honestly not sure how this code will behave. That index does look correct to me though, as e[0] will be the error code which is what we are intending to check against, but I’ll see if I can sort out what’s up with the exception. Thanks for testing!

Ah, good catch. In fact, awesome to know this works out of the box in py3 :).

ryanheffernan · 2018-02-28T21:16:37Z

I'm good with these changes, code looks good and testing proved successful. I am seeing some stability issues but need further testing and am OK with this being shipped in current state.

ryanheffernan · 2018-02-28T21:18:01Z

One note, which i can file as a seperate issue if you prefer, is that when using osqueryi to call an extension table, it works the first time but then doesn't work until i kill and restart osqueryi.

After running the select * from an extension table in osqueryi, after the first time all subsequent runs just return nothing until i restart a new osqueryi process.

ryanheffernan · 2018-02-28T22:34:56Z

BTW some of the stability issues I saw look similar to #3954 on osquery, so this likely isn't related to this diff but a generic windows extensions issue.

theopolis · 2018-04-08T20:22:48Z

👍

theopolis · 2018-04-08T20:23:06Z

We can remove build for 2.6 in a follow up PR, do not let that stop you from merging this.

muffins · 2018-04-11T05:35:38Z

I'm going to move forward with landing this as in it's current form this is working on Windows with osquery running as a system level service, and a configuration that queries against a python extensions table. Any remaining issues that folks have for this can be filed as follow up issues and I'll deal with them :)

SudhirSingh20

Getting error while running on Windows , I guess it should use AF_INET
Traceback (most recent call last):
File "run.py", line 14, in
CLIENT.open()
File "C:\Python27\lib\site-packages\osquery\extension_client.py", line 51, in open
self._transport.open()
File "C:\Python27\lib\site-packages\thrift\transport\TTransport.py", line 153, in open
return self.__trans.open()
File "C:\Python27\lib\site-packages\thrift\transport\TSocket.py", line 95, in open
addrs = self._resolveAddr()
File "C:\Python27\lib\site-packages\thrift\transport\TSocket.py", line 34, in _resolveAddr
return [(socket.AF_UNIX, socket.SOCK_STREAM, None, None,
AttributeError: 'module' object has no attribute 'AF_UNIX'

muffins · 2018-04-13T15:27:56Z

@SudhirSingh20 you’ll wanna check what version of the osquery python module you’ve got installed, as that looks like the old logic. Ensure you’re running off of 3.0.2, and if you have more problems feel free to open an issue

Nick Anderson and others added 18 commits December 4, 2017 16:09

Porting osquery-python to windows

5e9a138

Porting osquery-python to Windows

167d6f6

Adding initial TPipe implementation

477a2e5

Adding TPipeServer implementation

41cfa36

deps: adding windows python dependencies

2be2a25

fixing socket name so daemon can communicate with client

6578564

example extension now registers. server needs more implementations

35223c3

Adding in flush and seek calls

e89d7c5

more updates

a8cc24a

Progress!

611cc23

rework: modifying code flow to use overlapped io

5fe9ba5

changing flow to match mutliprocessing.connection

697f2b9

extensions are now queryable from osquery

cbc8e74

cleaning up code

8fb9a3a

more code cleanup

8eb8c66

more code cleanup and adding license

da0eb21

linting

d78056b

more cleanup

a48d937

muffins added the Windows label Feb 1, 2018

fmanco reviewed Feb 2, 2018

View reviewed changes

muffins force-pushed the osquery-python-windows-port branch 3 times, most recently from 35c6988 to 9e2dceb Compare February 6, 2018 05:26

addressing review feedback

1931549

muffins force-pushed the osquery-python-windows-port branch from 9e2dceb to 1931549 Compare February 6, 2018 06:10

gating imports of TPipe to Windows only

05f2c88

muffins added the ready for review label Feb 6, 2018

friedbutter reviewed Feb 16, 2018

View reviewed changes

more nits and formatting

3ccf6b1

muffins force-pushed the osquery-python-windows-port branch from 9065183 to 3ccf6b1 Compare February 21, 2018 19:07

moving win platform symbol to ext client

4b1ce89

ryanheffernan reviewed Feb 25, 2018

View reviewed changes

ryanheffernan reviewed Feb 26, 2018

View reviewed changes

Addressing notes from @ryanhefernan

a69ecbf

ryanheffernan reviewed Feb 28, 2018

View reviewed changes

addressing notes from @ryanhefernan

5d25b64

muffins merged commit 9e8eaf8 into osquery:master Apr 11, 2018

muffins mentioned this pull request Apr 11, 2018

windows: osquery Python extensions do not appear to work on Windows #34

Closed

SudhirSingh20 reviewed Apr 13, 2018

View reviewed changes

Conversation

muffins commented Feb 1, 2018

Uh oh!

fmanco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muffins commented Feb 6, 2018

Uh oh!

muffins commented Feb 6, 2018

Uh oh!

friedbutter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muffins commented Feb 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanheffernan commented Feb 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

SudhirSingh20 left a comment •

edited

Loading