Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upWindows Event Log support #3887
Conversation
|
Thanks for separating this out @alessandrogario! Is it possible to move all of the "tooling", aka, the generated files and binary content into a folder within "./tools", like ./tools/wel"? |
|
As @theopolis mentioned, I think the last big component we'd need for this is to get more of the rendered files contained under the |
The Windows Event Log manifest file (osquery.man) is now shipped with the package. The manage-osqueryd.ps1 script has been updated to (un)install as part of service configuration (-install and -uninstall switches).
Changes on the 'manage-osqueryd.ps1' script: The Windows Event Log manifest installation/uninstallation has been moved out of the '-install' and '-uninstall' commands. The script now supports the following two parameters: '-install_wel_manifest' and -uninstall_wel_manifest.
|
@alessandrogario has updated the pull request. |
|
@alessandrogario has updated the pull request. View: changes |
|
I've moved the files in the tools/wel directory, but I still have an issue; the absolute path of the osquery executable must be written inside the manifest (see line 5 in tools/wel/osquery.man) before we compile everything. Right now, I'm using the installation path for the chocolatey package, but it will not work with the MSI if it is being installed elsewhere. |
|
@alessandrogario that sounds fine to me, I'd say let's place an assumption on it being in the |
|
2 very small nits. I think that we're lookin good here after those changes hit. @theopolis do you have any other comments for this? I'd say we're good to merge. |
|
|
||
| std::string error_message = error_output.str(); | ||
| if (!error_message.empty()) | ||
| return Status(1, error_message); |
muffins
Nov 30, 2017
Contributor
Do you mind wrapping this return with curlys?
if (!error_message.empty()){
return Status(1, error_message);
}
Do you mind wrapping this return with curlys?
if (!error_message.empty()){
return Status(1, error_message);
}
|
|
||
| [switch] $install_wel_manifest = $false, | ||
| [switch] $uninstall_wel_manifest = $false, | ||
| [string] $wel_manifest_path = (Join-Path $PSScriptRoot "osquery.man") |
muffins
Nov 30, 2017
Contributor
nit: can you change these to have camlCasing to match the format of the rest?
nit: can you change these to have camlCasing to match the format of the rest?
> Add missing if brackets > Fix the variable and command names in the manage-osqueryd.ps1 script. > Update the documentation.
|
@alessandrogario has updated the pull request. View: changes |
|
Is there anyway to have unit tests for this code? If we by accident forget to include the |
|
Hello Teddy! I don't think it will compile if you forget to include it; the systemLog() function in include/osquery/logger.h is now wired to Windows Event Log (it wasn't implemented before for Windows). I'm open to suggestions to improve this! |
|
Ok, good to know, I’m still worried about not having tests for it, but we don’t have tests for logging to syslog either unfortunately. :( |
|
I'm going to merge this PR, and we can use #3996 to track building out tests for this, as it'd be nice to have assurances around it's performance. Thanks @alessandrogario! |
Testing the new logger plugin
Things to keep in mind
Any feedback is really appreciated!