-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
osquery: 2.5.2 -> 3.2.2 #39336
osquery: 2.5.2 -> 3.2.2 #39336
Conversation
No attempt on aarch64-linux (full log) The following builds were skipped because they don't evaluate on aarch64-linux: osquery Partial log (click to expand)
|
Failure on x86_64-linux (full log) Attempted: osquery Partial log (click to expand)
|
ok, rebasing onto the latest master seems to have killed it because of newly broken builds... |
No attempt on aarch64-linux (full log) The following builds were skipped because they don't evaluate on aarch64-linux: osquery Partial log (click to expand)
|
Success on x86_64-linux (full log) Attempted: osquery Partial log (click to expand)
|
When I run your command as root for the daemon (and having corrected $ sudo result/bin/osqueryd --pidfile /tmp/osq.pid --database_path /tmp/test.db --logger_path /tmp/osq.log
E0422 23:39:28.478080 19220 init.cpp:565] Cannot activate filesystem logger plugin:
Could not create file: /tmp/osq.log/osqueryd.results.log Even running with root doesn't make it work. Here is the strace tail:
I thought it might have something to do with me using ZFS, but I also tried it with a tmpfs but I get exactly the same error. |
The interesting part where it creates the log file is not included in strace here. |
@Mic92 Good point, but even in the full strace it doesn't show up: https://gist.github.com/Infinisil/eb224302b74d3d56a1a4ef2cfce46208 (search for "osq.log"). I think it's the |
Use |
Ah yes this makes it a bit clearer: https://gist.github.com/Infinisil/568c8915033266df0d18280951e4e012#file-strace-L6185-L6203
It seems to have the logger path set to "" at that point :/ |
Ouch, that's been an avoidable typo, thx! :/
As stated in the docs (https://github.com/facebook/osquery/blob/3.2.3/docs/wiki/deployment/logging.md) they write files into the The module creates the |
@Ma27 Thanks a lot for doing that bump! I'll soon give this a try as well :-) I recently filed osquery/osquery#4267 adding support for NixOS to Can you set |
@Ma27 Ahh, how did I not think of creating that directory, it works indeed. But that error was rather confusing for such a basic problem. Now that I can verify it working, I'm going to suggest this patch which removes unnecessary dependencies: diff --git a/pkgs/tools/system/osquery/default.nix b/pkgs/tools/system/osquery/default.nix
index 8fb2226b7f5..93c971b3998 100644
--- a/pkgs/tools/system/osquery/default.nix
+++ b/pkgs/tools/system/osquery/default.nix
@@ -1,23 +1,14 @@
{ stdenv, lib, fetchFromGitHub, pkgconfig, cmake, pythonPackages
, udev, audit, aws-sdk-cpp, cryptsetup, lvm2, libgcrypt, libarchive
-, libgpgerror, libuuid, iptables, apt, dpkg, lzma, lz4, bzip2, rpm
+, libgpgerror, libuuid, iptables, dpkg, lzma, bzip2, rpm
, beecrypt, augeas, libxml2, sleuthkit, yara, lldpd, google-gflags
-, thrift, boost, rocksdb_lite, cpp-netlib, glog, gbenchmark, snappy
-, openssl, file, doxygen, devicemapper
+, thrift, boost, rocksdb_lite, glog, gbenchmark, snappy
+, openssl, file, doxygen
, gtest, sqlite, fpm, zstd, rdkafka, rapidjson
}:
let
- aws-sdk-cpp' = aws-sdk-cpp.overrideAttrs (_: {
- src = fetchFromGitHub {
- owner = "awslabs";
- repo = "aws-sdk-cpp";
- rev = "1.2.7";
- sha256 = "182wr6j4pqz7g7j9b9cdgkpmflv3b17kl52gkwfz5b2qr8dix2ca";
- };
- });
-
thirdparty = fetchFromGitHub {
owner = "osquery";
repo = "third-party";
@@ -44,7 +35,7 @@ stdenv.mkDerivation rec {
patches = [ ./misc.patch ] ++ lib.optional stdenv.isLinux ./platform-nixos.patch;
nativeBuildInputs = [
- pkgconfig cmake pythonPackages.python pythonPackages.jinja2
+ pkgconfig cmake pythonPackages.python pythonPackages.jinja2 doxygen fpm
];
buildInputs = let
@@ -54,17 +45,16 @@ stdenv.mkDerivation rec {
in [
udev audit
- (aws-sdk-cpp'.override {
+ (aws-sdk-cpp.override {
apis = [ "firehose" "kinesis" "sts" "ec2" ];
customMemoryManagement = false;
}).dev
-
- lvm2 libgcrypt libarchive libgpgerror libuuid iptables.dev apt dpkg
- lzma lz4 bzip2 rpm beecrypt augeas libxml2 sleuthkit
+ lvm2 libgcrypt libarchive libgpgerror libuuid iptables dpkg
+ lzma bzip2 rpm beecrypt augeas libxml2 sleuthkit
yara lldpd gflags' thrift boost
- cpp-netlib glog gbenchmark snappy openssl
- file doxygen devicemapper cryptsetup
- gtest sqlite fpm zstd rdkafka rapidjson rocksdb_lite
+ glog gbenchmark snappy openssl
+ file cryptsetup
+ gtest sqlite zstd rdkafka rapidjson rocksdb_lite
];
preConfigure = ''
|
Awesome! I've just seen the ticket regarding compilation errors on NixOS, but I've obviously missed that. This looks indeed like a much better solution, I'll give it a try!
You're absolutely right! I stumbled about this as well :-)
Thanks! I didn't like the AWS override hack either, so I'll try it out :-) |
I fixed the commit with the comments from @infinisil and @flokli, thanks a lot! |
No attempt on aarch64-linux (full log) The following builds were skipped because they don't evaluate on aarch64-linux: osquery Partial log (click to expand)
|
Success on x86_64-linux (full log) Attempted: osquery Partial log (click to expand)
|
patches = [ | ||
./misc.patch | ||
(fetchpatch { | ||
url = "https://patch-diff.githubusercontent.com/raw/facebook/osquery/pull/4267.patch"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This url will break if changes are made to the pull request. @flokli could you make a dedicated branch for this that is independent from the pull request and that will not change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need that patch here at all? This tools/get_platform.py
shouldn't even run if OSQUERY_PLATFORM
is set…
cpp-netlib glog gbenchmark snappy openssl linenoise-ng | ||
file doxygen devicemapper cryptsetup | ||
gtest sqlite | ||
}).dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why .dev specifically? I'm pretty sure that's not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.dev
is the output of the derivation which contains the development headers. Just search with rg "\.dev"
the nixpkgs
repo and you'll see that this is needed sometimes (Obviously the aws-sdk
doesn't provide the headers in out
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
buildInputs
chooses dev outputs automatically:
dependencies = map (map lib.chooseDevOutputs) [ |
@@ -1 +1 @@ | |||
18.09 | |||
18.09 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the history of this .version file, there were a couple times where this has been updated to remove the trailing newline, and the reason for that is the nixpkgs/.editorconfig
file which specifies that a trailing newline should be inserted for all files. You can add the following section to that file to prevent further newline insertions for that file:
[.version]
insert_final_newline = false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be addressed in a different commit or PR for the git history?
|
||
# this is what `osquery --help` will show as the version. | ||
OSQUERY_BUILD_VERSION = version; | ||
OSQUERY_PLATFORM = "nixos;${builtins.readFile "${builtins.toString path}/.version"}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toString
is available at top-level scope, so you can remove the builtins.
here.
thanks for the suggestions, I'll make the requested changes tonight :-) |
@infinisil you're absolutely right, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! One last thing though: I would put the .editorconfig update and newline removal in a first commit and the osquery update in a second one.
Success on x86_64-linux (full log) Attempted: osquery Partial log (click to expand)
|
No attempt on aarch64-linux (full log) The following builds were skipped because they don't evaluate on aarch64-linux: osquery Partial log (click to expand)
|
No attempt on aarch64-linux (full log) The following builds were skipped because they don't evaluate on aarch64-linux: osquery Partial log (click to expand)
|
Success on x86_64-linux (full log) Attempted: osquery Partial log (click to expand)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @zimbatm indicated, the .version thing really needs to be in a different commit or PR
When reading the `nixpkgs` version from `.version` you always have a `\n` at the end because of the final newline. This issue exists since b7d15ed and had to be fixed several times according to the history of `.version`. Furthermore @infinisil recommended I explicitly configured `.editorconfig` to avoid newlines in `.version`.
The package was originally broken as reported in NixOS#38940 and osquery/osquery#4257. The latest version (3.x) contains several important fixes for GCC 7, so now we can compile without a much less complicated patches. The following changes were needed to fix the derivation: * Upgrade `osquery/third-party` to the latest rev to be compliant with osquery 3. * Keep using an override for the AWS SDK (for a lower closure size and less compile time), but make the `ec2` API available. * Added the dependencies `fpm`, `zstd`, `rdkafka`, `rapidjson` to the build. `linenoise-ng` is obsolete as it's directly bundled with `osquery/third-party`. * Fixed the linking issue with `gflags` as recommended in the mailing list: https://groups.google.com/d/msg/nix-devel/l1blj-mWxtI/J3CwPATBCAAJ * Dropped the obsolete dependencies `cpp-netlib`, `lz4`, `apt` and `devicemapper` (thanks @infinisil). * Override `OSQUERY_PLATFORM` to provide `nixos:version` for sandbox and non-NixOS based builds. The `platform-nixos.patch` file is now obsolete (thanks @flokli). The patch was rebased against the 3.x branch of `osquery` and contains mostly old changes. Additionally several testing targets were skipped as they broke the build. The functionality has been testing using the following command: ``` mkdir /tmp/osq.log/ ./result/bin/osqueryd --pidfile /tmp/osq.pid \ --database_path /tmp/test.db --logger_path /tmp/osq.log ``` With the daemon running the database can be queried easily using `./result/bin/osqueryi`. Fixes ticket NixOS#38940 See ticket NixOS#36453 Further reference can be gathered from the affected Hydra logs for the master branch: https://hydra.nixos.org/job/nixos/trunk-combined/nixpkgs.osquery.x86_64-linux
@infinisil @zimbatm done %) |
Success on x86_64-linux (full log) Attempted: osquery Partial log (click to expand)
|
No attempt on aarch64-linux (full log) The following builds were skipped because they don't evaluate on aarch64-linux: osquery Partial log (click to expand)
|
Some time ago I fixed the broken package `osquery` (see NixOS#39336). I had to test the package manually by starting the daemon locally, however this doesn't ensure that the module is still functional. In order to cover the package *and* the integration with the NixOS module I thought that adding a testcase might be the best idea. The current testcase does the following things: * Starts an `osqueryd` service in a test machine with customized logger path and PID file * Ensures that the `osqueryd.service` unit is running * Checks if the customized flags (`pidfile`, `logger_path`) are applied to `osquery`. * Performs a simple test query against the `etc_hosts` database to check if the basic funcitonality of `osquery` (storing system information into a database) works fine.
Some time ago I fixed the broken package `osquery` (see NixOS#39336). I had to test the package manually by starting the daemon locally, however this doesn't ensure that the module is still functional. In order to cover the package *and* the integration with the NixOS module I thought that adding a testcase might be the best idea. The current testcase does the following things: * Starts an `osqueryd` service in a test machine with customized logger path and PID file * Ensures that the `osqueryd.service` unit is running * Checks if the customized flags (`pidfile`, `logger_path`) are applied to `osquery`. * Performs a simple test query against the `etc_hosts` database to check if the basic funcitonality of `osquery` (storing system information into a database) works fine.
Motivation for this change
The package was originally broken as reported in #38940 and
osquery/osquery#4257. The latest version (3.x) contains several
important fixes for GCC 7, so now we can compile without a much less
complicated patches.
The following changes were needed to fix the derivation:
Upgrade
osquery/third-party
to the latest rev to be compliant withosquery 3.
Keep using an override for the AWS SDK (for a lower closure size and
less compile time), but make the
ec2
API available.Added the dependencies
fpm
,zstd
,rdkafka
,rapidjson
to thebuild.
linenoise-ng
is obsolete as it's directly bundled withosquery/third-party
.Fixed the linking issue with
gflags
as recommended in the mailinglist: https://groups.google.com/d/msg/nix-devel/l1blj-mWxtI/J3CwPATBCAAJ
Dropped the obsolete dependencies
cpp-netlib
,lz4
,apt
anddevicemapper
(thanks @infinisil).Applied the patch from tools/get_platform.py: add support for nixos osquery/osquery#4267 for NixOS support during
configure phase, override
OSQUERY_PLATFORM
to providenixos:version
for sandbox and non-NixOS based builds. The
platform-nixos.patch
file is now obsolete (thanks @flokli).
Dropped the newline character from
.version
to avoid\n
in thedistro version provided by
osquery
The patch was rebased against the 3.x branch of
osquery
and containsmostly old changes. Additionally several testing targets were skipped as
they broke the build.
The functionality has been testing using the following command:
With the daemon running the database can be queried easily using
./result/bin/osqueryi
.Fixes ticket #38940
See ticket #36453
Further reference can be gathered from the affected Hydra logs for
the master branch: https://hydra.nixos.org/job/nixos/trunk-combined/nixpkgs.osquery.x86_64-linux
Things done
build-use-sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)