osquery: re-init at 5.5.1

[jdbaldry]

Some open questions I have about implementation:

• Is it OK to strictly require the clangUseLLVM stdenv?
  The CMake file for this project logs a warning if you try to use another compiler.
• Should one attempt to build a derivation for the osquery-toolchain and use that instead of trying to work with the clang stdenv?
  I'm not sure if this is a case of just attempting the path of least resistance of if there might be a practical reason to prefer one mechanism over the other.

Unable to include <sys/sysctl.h> in osquery/tables/system/posix/sysctl_utils.h needed by osquery/tables/system/posix/system_controls.cpp

I believe this header has been long deprecated and finally removed in glibc 2.32 but is still required by osquery until they rewrite the code to read /proc/sys rather than using the library.
For now, I've decided to just remove the tables requiring <sys/sysctl.h>.

Will squash the package and module commits if desired once this looks good to go.

Description of changes

Closes #193673

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 22.11 Release Notes (or backporting 22.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[znewman01]

TYVM for your hard work here, this is great!

Overall looks fine to me, but I'm not a nixpkgs expert so there may be conventions I'm not aware of. Left a bunch of little nitpicks too, sorry 🙂

[jdbaldry]

Thanks for the fast review @znewman01, it's super valuable and great to be able to work on this together!

I've shared some of my musings on the module design and will address all the immediately actionable feedback now :)

[znewman01]

Looking good overall!

Implementation wise, I think putting the command line options into a flagfile or concatenating them into the execStart shell args is a matter of preference. I would personally find the flagfile easier to read but I'm happy with either.

flagfile is great for me.

I agree that exposing the flagfile path as a module option is unnecessary and, if I think back on developing this module, resulted from wanting to share that flagfile with the osqueryi binary in the tests. I think if I pack the command line options into a derivation, that derivation can be used in the test instead of peaking at the option.

+1

In terms of adding flags as module options. I could be convinced that it is worthwhile to promote some and leave the rest to extraFlags.
Do you think we should use the heuristic of "important" vs. "less important"?
Another option is to promote any flags that rely on the FHS. It feels more easily decided but I'm not sure it's that useful.

I haven't used osquery a great deal yet so I haven't really worked out which flags are the most important to me. I perhaps mistakenly assumed that the options exposed by the previous implementation of this module were there as they happened to be useful to the previous maintainer's config rather than being strictly more important.

Yeah, I think let's start with "any flags that get explicitly referenced in the systemd unit file". We can always add more later as-needed.

You might be right about the previous flags 😛

Assuming that we will at some point accept a request to provide first class options for some flags, it makes sense to bake that into the interface from the start rather having a breaking change in the future or having to support two separate mechanisms.

+1

One wrinkle is that a user could specify a different configuration file path to override the use of the configuration file derivation but that would be no different to them using an alternative configuration plugin which we also wouldn't be able to inspect.
I think I can the usage of the runtimeValue function depend on the user using the filesystem config plugin and the configuration file in the derivation and skipping any tasks derived from those functions otherwise.

See my suggestions in flags.nix about discouraging this.

[jdbaldry]

Sorry for the delay in getting back to this PR.

I've fixed the typos and renamed the config option to be settings.

[znewman01]

My turn to apologize now for the delay. Looking good overall, one big comment and one little one, then I'm happy to approve

[jdbaldry]

@znewman01 You'd think the Christmas period would make things quieter :)

I think we are broadly aligned with regards to your table of usage but I don't have time right now to properly integrate and test the warning behavior.
Despite this, I have cleaned up the unused functions in flags.nix

I am keen to get this into the hands of some other users and let us iterate from there if you are happy to leave the warnings for a future PR?

[znewman01]

@znewman01 You'd think the Christmas period would make things quieter :)

I think we are broadly aligned with regards to your table of usage but I don't have time right now to properly integrate and test the warning behavior. Despite this, I have cleaned up the unused functions in flags.nix

I am keen to get this into the hands of some other users and let us iterate from there if you are happy to leave the warnings for a future PR?

SGTM, approved (just need to fix maintainers-list.nix

[RaitoBezarius]

Thank you for the very interesting package & service :-) I am very excited to try it on my systems!
A bit of rework must be done, so it can be merged fine

[RaitoBezarius]

If anyone does pick this up and wants me to explain anything, I'll be more than happy to help to the best of my understanding.

FWIW, @nlewo has been pushing on your branch those last days and he's taking the work, you may get notifications or messages here, but they are not directly for you :P — I sent them to @nlewo who is getting this to merge state. :-)

[nlewo]

Unfortunately, I think I need to step back from this contribution.

@jdbaldry fyi, I then removed you from the maintainers of this package.

[nlewo]

Some squashing could help, this has too many commits to be applied cleanly.

@RaitoBezarius I squashed all commits.

[RaitoBezarius]

Let's go, this is shippable, I confirmed it worked on my machines.

[RaitoBezarius]

Let's address the final comments, then merge, it has waited enough.

[RaitoBezarius]

Let's address the final comments, then merge, it has waited enough.

[RaitoBezarius]

@nlewo The commit message is wrong according to our CONTRIBUTING guide.
You should ideally separate it in 3 commits:

osquery: init at 5.5.1
nixos/osquery: init
nixos/tests/osquery: init

[nlewo]

@RaitoBezarius i rewrote the history as required by the CONTRIBUTING section.

[RaitoBezarius]

Tested it, when CI is good, let's merge.

[necrophcodr]

It would be nice to see fewer patches in the future, since some of the functionality is removed, but this is definitely better than not having it packaged at all.

[nlewo]

Thank you all, especially @jdbaldry who made most of the work!

[jdbaldry]

Thank you for picking it up and getting it over the line. Often the last push is the hardest. I hope it serves as a reasonable starting point!

[vcunat]

BTW, you put platforms.linux into osquery.meta but it won't build on aarch64-linux on Hydra:
https://hydra.nixos.org/job/nixpkgs/trunk/osquery.aarch64-linux

[necrophcodr]

@vcunat The issue likely stems from this patch being specific for x86_64. The issue exists in x86_64 as well, but the patch resolves it. So maybe a new PR with a fix for remaining platforms would have it build in a more satisfying manner in the future.