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 developer accessible backdoor to VM tests infrastructure. #47418

Merged
merged 1 commit into from
Sep 28, 2018

Conversation

domenkozar
Copy link
Member

@domenkozar domenkozar commented Sep 27, 2018

Thanks to @dezgeg for prototype implementation, I've
cleaned it up and added documentation.

I admit it's not clear to me if console access via $TMPDIR is considered a security issue, on a builder running multiple tests, I'd expect this could be considered an adversary.

@domenkozar domenkozar added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation labels Sep 27, 2018
enable = true;
permitRootLogin = "yes";
};
users.extraUsers.root.initialPassword = "backdoor";
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need to set a password?

      services.openssh.enable = true;
      services.openssh.permitRootLogin = "yes";
      services.openssh.extraConfig = "PermitEmptyPasswords yes";
      users.extraUsers.root.password = "";

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really. I was hesitant to do that, but then again once it has a backdoor, why even bother with passwords.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

@GrahamcOfBorg GrahamcOfBorg added the 8.has: module (update) This PR changes an existing module in `nixos/` label Sep 27, 2018
@GrahamcOfBorg GrahamcOfBorg added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Sep 27, 2018
@domenkozar domenkozar force-pushed the nixos-tests-debug branch 2 times, most recently from 8a8fb34 to e19316a Compare September 27, 2018 11:42
@domenkozar
Copy link
Member Author

@nlewo @dezgeg I've removed redundant SSH, let me know what you think.

Thanks to @dezgeg for prototype implementation, I've
cleaned it up and added documentation.
@domenkozar domenkozar changed the title Add ssh backdoor to VM tests infrastructure. Add developer accessible backdoor to VM tests infrastructure. Sep 27, 2018
@roberth
Copy link
Member

roberth commented Sep 27, 2018

👍

I'd expect the temporary directory to have sufficiently restrictive permissions anyway. That's a problem that Nix should already solve. An adversary will have just as hard a time attacking a backdoored test as he would attacking a non-backdoored test.

Some potential improvements:

A similar backdoor to the perl script could be added, to provide access to library functions on machines and to relay the vm backdoors.

Also the tester could print the backdoor instructions, although it won't be able to point the user to the right path because of that one security measure to prevent dangerous accidental /tmp references in built packages, or do we have an obscure impure environment variable with that info?

@domenkozar
Copy link
Member Author

A similar backdoor to the perl script could be added, to provide access to library functions on machines and to relay the vm backdoors.

Sounds good to me - anyone should feel free to contribute that, my motivation was to have a system that works and is documented. Hopefully that sparkles simplification :)

Also the tester could print the backdoor instructions, although it won't be able to point the user to the right path because of that one security measure to prevent dangerous accidental /tmp references in built packages, or do we have an obscure impure environment variable with that info?

Shouldn't be an issue to echo $TMPDIR to logs. My willpower to do more test debugging is very low, as I was deep into it in last days, can we also consider this an improvement yet to be done? :)

@roberth
Copy link
Member

roberth commented Sep 27, 2018

Iirc, $TMPDIR is /build/..... in the sandbox, which is backed by /tmp/..... on the host.
This is a great addition. I won't let perfect get in the way of better.

@domenkozar domenkozar added 9.needs: reporter feedback This issue needs the person who filed it to respond 9.needs: community feedback and removed 9.needs: reporter feedback This issue needs the person who filed it to respond labels Sep 27, 2018
@aszlig
Copy link
Member

aszlig commented Sep 28, 2018

Hm... what if we add a .debug attribute to the result of makeTest that turns the whole thing into a fixed output derivation (where we have networking) with a dummy hash, allowing to connect directly via SPICE for example?

@roberth
Copy link
Member

roberth commented Sep 28, 2018

@aszlig That's also an interesting approach. What do you think of the PR as is?

@domenkozar
Copy link
Member Author

I'm merging this by discarding my concern about backdoor being a security issue given that shell is also placed into the temp directory - which is then used by testing infrastructure.

I know this is far from perfect, but I've managed to debug my test failures easily - so it does the job. Any improvements are welcome to be done on top of this work.

Going to backport to 18.09 as well.

@domenkozar domenkozar merged commit d6e3db4 into master Sep 28, 2018
@Mic92 Mic92 deleted the nixos-tests-debug branch October 3, 2018 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` 9.needs: community feedback 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants