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

Add Nix flake #234

Closed
wants to merge 1 commit into from
Closed

Add Nix flake #234

wants to merge 1 commit into from

Conversation

ilkecan
Copy link
Contributor

@ilkecan ilkecan commented Oct 3, 2021

Hi. As a participant of Summer of Nix, I created a Nix flake for cwe_checker.

Nix is a cross-platform package manager. It could build and test packages without an external dependency by managing everything itself. Flakes are a relatively new Nix feature that improves reproducibility, composability and usability in the Nix ecosystem1. The things flake can be used include but not limited to build packages, run tests, create development environments or NixOS modules2. The one added with this PR includes the cwe_checker, cwe_checker_to_ida, ghidra plugin and a development shell for it.

I tried to package the acceptance test suite but couldn't find a sane way to do it. I tried to create the environment created with the Dockerfile but some of the compilers don't exist in the Nixpkgs or the binary cache. While I think it would work in the end, I didn't want to build GCC a couple of times from source.

I also thought about trying to package the Docker image, which would require using fixed output derivation (basically specifying the hash of the output beforehand to circumvent the sandbox restriction). But since there is no pre-built image and building the image is not reproducible, trying to keep up with the changes in output hash would be too much work.

Note that the flake doesn't have to be in this repository to be useful, so no effort would be wasted if you don't accept the PR.

I don't want to repeat things about Nix or Flakes you could find online. I think it would be better if I could answer your questions instead.

P.S.: Here is the description of Summer of Nix:

• NGI0 aims to contribute to an open internet that is resilient,
trustworthy, and sustainable. NGI0 wants to universally enable
internet users in Europe and beyond to get early access to the
innovative technologies developed inside the research and de-
velopment efforts it funds. It also wants to encourage robust
software engineering practices such as continuous integration
and testing.
• Nix is a purely functional package manager for software appli-
cations, a new class of deployment tooling which can be used on
different operating systems and platforms including the purely
functional operating system NixOS. Use of Nix allows experi-
mental newer technologies to be used alongside proven tech-
nologies without dependency conflicts.
• In order to provide uniform and convenient access and to assist
with real-world deployment, the NGI0 consortium and the wider
internet community would like to see the software produced by
the NGI0 projects (and any dependencies) to be packaged in
Nix.
• The Summer of Nix is a program organized by the NixOS Foun-
dation in collaboration with the NLnet Foundation and Tweag
to educate and train a new generation of packagers, and to pack-
age applications relevant to NGI Zero.

Footnotes

  1. https://www.tweag.io/blog/2020-05-25-flakes/

  2. https://nixos.wiki/wiki/Flakes#Output_schema

Comment on lines +3 to +6
--- a/cwe_checker_to_ida.py
+++ b/cwe_checker_to_ida.py
@@ -1,3 +1,5 @@
+#!/usr/bin/env python
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used a patch file to add a shebang line to this, but if you want I can apply the changes to the original file.

@ilkecan ilkecan force-pushed the nix-flake-squashed branch from b9243bf to 9cacdd2 Compare October 3, 2021 18:18
Comment on lines +100 to +105
pkgs.mkShell {
packages = packageList ++ [
defaultPackage.rustToolchain.defaultToolchain
];
inputsFrom = packageList;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The development shell currently includes the packages provided by the flake and their inputs. It also includes the default profile of the stable channel. Other packages or bash code (aliases or function) can also be added here.

;

# ghidra-bin.meta.platforms
ghidraPlatforms = [ "x86_64-linux" "x86_64-darwin" ];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ghidra derivation in Nixpkgs only supports these two platforms. I think this is a limitation of Nixpkgs rather than upstream project. But since cwe_checker depends on ghidra, I could only support these two platforms.

@ilkecan ilkecan marked this pull request as ready for review October 3, 2021 18:29
@ilkecan
Copy link
Contributor Author

ilkecan commented Oct 3, 2021

I created this branch to make the flake usable outside the source repository. While I am willing to to further work on this until it is in an acceptable state, the current effort will not be wasted if this PR is not accepted.

@Enkelmann
Copy link
Contributor

At the current time I will not be able to accept the PR: To only support Docker for easy deployment was a deliberate choice. This keeps our CI pipeline relatively simple and sane while hopefully also not raising false expectations about the reliability of the cwe_checker results (it is still a research project). Accepting the PR would mean that we would take responsibility for maintaining the package, i.e. extending our CI pipeline for it and probably also adding some stability guarantees to the project that I am not comfortable to make at the current time.

But if you are willing to maintain the package yourself (outside of the official cwe_checker repository), I will gladly answer all your questions before I close the PR. Her are some notes on what you have written so far:

  • The acceptance test suite should not be packaged at all! Cross-compiling C programs for dozens of architectures at the same time will always be a flaky business and users do not need any of that to use the cwe_checker at all. So they should also not need to pay for it.
  • The official Docker images on Dockerhub of the stable versions (i.e. with the stable tag) usually only change with new stable releases, i.e. their hash also changes only once every few months. So this could be a solution for you. If you are willing to wait for the next stable release, I plan to release Docker images with the version number as a tag. These should not change at all (aside from emergency bugfixes maybe).
  • The cwe_checker_to_ida.py is currently broken (see Broken IDA export script? #217). So it probably should not be packaged until it is fixed.
  • Yes, you can only support the same architectures that are also supported by the Ghidra package of the same package manager, since Ghidra is an essential dependency.

@ilkecan
Copy link
Contributor Author

ilkecan commented Oct 4, 2021

Thank you for your comment.

Accepting the PR would mean that we would take responsibility for maintaining the package, i.e. extending our CI pipeline for it and probably also adding some stability guarantees to the project that I am not comfortable to make at the current time.

Yeah, that's fair.

But if you are willing to maintain the package yourself (outside of the official cwe_checker repository)

I could help maintain the nix part if it was merged but otherwise I don't have any reason to maintain the out-of-repository flake going forward. I have worked on cwe_checker as part of Summer of Nix program and the goal was to either upstream the flake or create an out-of-repository flake for the main repository.

Since flakes use a lock file, the out of repository flake will continue to work in the future without any maintenance but the packaged versions will be outdated.

* The acceptance test suite should not be packaged at all! Cross-compiling C programs for dozens of architectures at the same time will always be a flaky business and users do not need any of that to use the cwe_checker at all. So they should also not need to pay for it.

I feel the need to clarify something. The packaging in this context probably differs from what it usually means. Packaging for other distro's package manager is mostly done for the end users. But nix is different, in that it can also be used for the development of a project by creating a development shell (like a language agnostic python virtual environments) among other things.

So in my mind, a flake for a project should make every component of the project available for the Nix ecosystem. That includes the acceptance test suite. Nixpkgs and the binary cache already contains some of the cross compilers but not all of them. If all the compilers would exist in the binary cache, like in the case of Ubuntu, I could just run the test suite with Nix without using Docker.

Alternatively, if the image that contained the cross-compilers was published somewhere, I could also use it. But at that point you might wonder why we are even using Nix if it is going to use Docker underneath. One reason would be to be able to run the test suite in an ephemeral NixOS VM so we would be certain there wasn't any leftover state/data. And of course we could just to `nix run '.#acceptance-test-suite' to do all this.

* The official Docker images on Dockerhub of the stable versions (i.e. with the stable tag) usually only change with new stable releases, i.e. their hash also changes only once every few months. So this could be a solution for you. If you are willing to wait for the next stable release, I plan to release Docker images with the version number as a tag. These should not change at all (aside from emergency bugfixes maybe).

There is no need for the docker images that contain cwe_checker and ghidra. The main tool is already packaged for Nix natively.

* The `cwe_checker_to_ida.py` is currently broken (see Broken IDA export script? #217). So it probably should not be packaged until it is fixed.

Packaging this is really trivial, it just adds a shebang line that points to the specified python version in the Nix store. It also runs test. So I don't think the way it is packaged will need to changed in the future. But marking the derivation as broken is also an option by setting the meta.broken attribute to true.

I don't have any more questions, so the PR can be closed whenever you want.

@Enkelmann
Copy link
Contributor

Ah, thanks for the clarifications! Ok, since we cannot commit to maintaining an in-repository flake at this time and an out-of-repository flake is doomed to be outdated very soon, it is probably best not to create such a package right now.

Closing the PR. But still thanks for the effort! And we will definitely keep Nix in mind when we feel ready to maintain cwe_checker packages in the future!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants