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

Tighten the actions' sandbox #36

Merged
merged 3 commits into from
Feb 23, 2024
Merged

Tighten the actions' sandbox #36

merged 3 commits into from
Feb 23, 2024

Conversation

pnmadelaine
Copy link
Member

@pnmadelaine pnmadelaine commented Feb 20, 2024

  • unset environment variables
  • unmount /etc
  • fix actions accordingly

see NixOS/nix#8074 about why $HOME is now set before calling Nix from an action

@pnmadelaine pnmadelaine requested a review from W95Psp February 20, 2024 21:18
@pnmadelaine pnmadelaine marked this pull request as draft February 20, 2024 21:35
@pnmadelaine pnmadelaine changed the title Unset environment variables inside the action's sandbox Tighten the action's sandbox Feb 20, 2024
@pnmadelaine pnmadelaine marked this pull request as ready for review February 20, 2024 22:27
@phaer
Copy link
Contributor

phaer commented Feb 21, 2024

I think we should either mount /etc/nix/nix.conf or even write a separate one for the builds?

Without the /etc mount, i get error: experimental Nix feature 'nix-command' is disabled; use '--extra-experimental-features nix-command' to override in the begin and end phases.

This could be worked around by adding the flag to each nix invocation, but one would probably like stuff such as custom substituters and max-jobs as well?

@pnmadelaine
Copy link
Member Author

I think we should either mount /etc/nix/nix.conf or even write a separate one for the builds?

I have been thinking about this. I would note that the sandbox is only used for the actions, no the builds. In spirit an action should not run a Nix build. The only current occurence of Nix in an action is in lib.common.mkStatus and it is an evaluation. For the time being I patched this call with --extra-experimental-features "nix-command flakes", but I am not opposed to modify lib.builders.mkActionScript to set both $HOME and nix.conf if more actions end up using Nix.

I would oppose mounting /etc/nix/nix.conf though, as it seems like too big a hole to keep in the sandbox if we can avoid it.

@pnmadelaine
Copy link
Member Author

pnmadelaine commented Feb 21, 2024

This could be worked around by adding the flag to each nix invocation, but one would probably like stuff such as custom substituters and max-jobs as well?

As I quickly wrote in my previous comment, this is only relevant if actions are running Nix builds, which is against the spirit of actions. I just want to give a few details about why : an action is meant to be a quick operation (EDIT: well actually not necessarily quick, but light on resources). Long arbitrary computations should be left to jobs, and any dependency an action uses should be built by the action's derivation, at the time of a project's refresh.

Of course we can't force people to follow these principles and prevent them from calling something like nix run nixpkgs#something from an action, but let's not help them shoot themselves in the foot 🙂

@phaer
Copy link
Contributor

phaer commented Feb 21, 2024

I would oppose mounting /etc/nix/nix.conf though, as it seems like too big a hole to keep in the sandbox if we can avoid it.

Agree, this was more a thought about a stop-gap as we just gave up on mounting all of /etc/, including /etc/nix/nix.conf, anyway.

But your design considerations sound good! :)

Sorry if this is too off-topic (there isn't a forum/matrix channel yet, right?), but are you aware of https://docs.hercules-ci.com/hercules-ci-effects/ and if so what's your thoughts on that in comparison to typhons approach?

@pnmadelaine
Copy link
Member Author

pnmadelaine commented Feb 21, 2024

(there isn't a forum/matrix channel yet, right?)

I reserved https://typhon.zulipchat.com, which is ready as soon as there is demand for it!

are you aware of https://docs.hercules-ci.com/hercules-ci-effects/ and if so what's your thoughts on that in comparison to typhons approach?

I know about them but I have not dived too deep into it yet. Now that I am starting to work on extending the library beyond basic features I think I should take a closer look. From a brief overview of the documentation, I could spot two main differences of design: Hercules CI seems focused on GitHub, and the effects have native access to some state (which is not the case in Typhon without hacks).

What I don't know but am interested in is how easily Hercules' effects can be run locally for testing. This was a major frustration point with GitHub actions that contributed to the creation of Typhon.

And I loop back to the subject of this PR, because the reason I noticed the issues with the sandbox is that I was writing https://github.com/typhon-ci/typhon-github-action to test wether we could actually run Typhon's actions in different contexts :)

@phaer
Copy link
Contributor

phaer commented Feb 21, 2024

I reserved https://typhon.zulipchat.com/, which is ready as soon as there is demand for it!

Just tried to sign up, but would seemingly need an invite atm.

Hercules CI seems focused on GitHub

It says something about Gitlab in the docs as well, and conceptually there should be nothing that blocks usage of Gitea or so?

and the effects have native access to some state (which is not the case in Typhon without hacks).

Yes, but I think that's a useful feature to have, at long as it's explicit?

What I don't know but am interested in is how easily Hercules' effects can be run locally for testing.

Hercules implements local actions in it's agent: https://docs.hercules-ci.com/hercules-ci-agent/hci/effect/run, which effectively runs them in a local OCI container after preparing it's environment.

@pnmadelaine
Copy link
Member Author

pnmadelaine commented Feb 21, 2024

Just tried to sign up, but would seemingly need an invite atm.

I just opened up the instance!

It says something about Gitlab in the docs as well, and conceptually there should be nothing that blocks usage of Gitea or so?

Well at least it requires a GitHub account to sign up, and it asks for an amount of permissions I am not confortable with. That's the main reason I did not really test it…

Hercules implements local actions in it's agent: https://docs.hercules-ci.com/hercules-ci-agent/hci/effect/run, which effectively runs them in a local OCI container after preparing it's environment.

That actually sounds really nice :) I think it would definitely be a good thing to do a proper comparison at some point!

@pnmadelaine
Copy link
Member Author

Yes, but I think that's a useful feature to have, at long as it's explicit?

I am not yet sold on the feature. My reasonning is that introducing state complicates things both for the implementation and for the user. There are design decisions to make that seem arbitrary to me: is the state shared by all actions of the same project? by all actions of the same jobset? At the very least these are decisions that I want to leave for later.

I am not saying that it is a bad idea either. I am just inclined to keep things as simple as possible at first, and wait for people to come up with scenarios that we are interested in handling that require the feature.

@W95Psp
Copy link
Collaborator

W95Psp commented Feb 23, 2024

I'm also in favor of setting nice (flake enabled) defaults for nix.conf.

We should definitely look more at Hercules effects, looks like there are lots of cool ideas there! Would be great to offer some level of compatibility, even.

Concerning state, I think that can actually be pretty nice! For instance, say you want to track build time regressions, having a simple state API would be cool :)

@pnmadelaine pnmadelaine changed the title Tighten the action's sandbox Tighten the actions' sandbox Feb 23, 2024
@pnmadelaine pnmadelaine merged commit 68a5f09 into main Feb 23, 2024
4 checks passed
@pnmadelaine pnmadelaine deleted the pnm/clearenv branch February 23, 2024 11:05
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.

3 participants