Skip to content

/crater

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

Prevent external network traffic for builds and tests #336

Merged
merged 5 commits into from Oct 7, 2018

Conversation

Reviewers

@pietroalbini pietroalbini

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@AndrewSpeed @pietroalbini
@AndrewSpeed
Copy link
Contributor

AndrewSpeed commented Oct 7, 2018

This PR resolves #281 by disabling docker networking for instances of run_cargo where tests are being run, as this could be abused by crates under experiment.
@pietroalbini

pietroalbini requested changes Oct 7, 2018
View changes

Copy link
Member

pietroalbini left a comment

Thanks for the PR!

I left two comments for a few minor things you need to fix, and you'll need to run rustfmt over the codebase to fix some formatting issues. This looks really good otherwise!
src/ex_run.rs Outdated
@@ -67,6 +67,7 @@ fn build(
CargoState::Locked,
quiet,
false,
false,

This comment has been minimized.

Sign in to view
Copy link
@pietroalbini

pietroalbini Oct 7, 2018

Member

Networking should also be disabled here, since build scripts are run during the build phase.
src/docker.rs Outdated
@@ -102,6 +109,10 @@ impl<'a> ContainerBuilder<'a> {
args.push(limit.to_string());
}

if self.networking_disabled {
args.push("--network none".into());

This comment has been minimized.

Sign in to view
Copy link
@pietroalbini

pietroalbini Oct 7, 2018

Member

This should be split in two push calls (one for each word).

AndrewSpeed added some commits Oct 7, 2018

@AndrewSpeed
PR feedback 
Disables networking when building the experiment and splits the networking disable command into two push calls
a86ffc9
@AndrewSpeed
cargo fmt
Loading status checks…
c6db624
@AndrewSpeed

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

AndrewSpeed commented Oct 7, 2018

👍 Thanks for the feedback, I've addressed the points you raised.

Would you mind if I added the cargo fmt bit to CONTRIBUTING.md?
@pietroalbini

This comment has been minimized.

Sign in to view
Copy link
Member

pietroalbini commented Oct 7, 2018

Thanks for the feedback, I've addressed the points you raised.

I'll test the changes locally and get back to you on the PR soon.

Would you mind if I added the cargo fmt bit to CONTRIBUTING.md?

That would be awesome! If you write that you could mention that the CI requires both rustfmt and clippy to pass on stable, maybe with the commands to install them with rustup and run them.
@pietroalbini

pietroalbini approved these changes Oct 7, 2018
View changes

Copy link
Member

pietroalbini left a comment

This works great, thanks!

@pietroalbini pietroalbini merged commit c8c7933 into rust-lang-nursery:master Oct 7, 2018
2 checks passed

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@AndrewSpeed AndrewSpeed deleted the AndrewSpeed:prevent-external-network-traffic-for-builds-and-tests branch Oct 7, 2018

@AndrewSpeed AndrewSpeed referenced this pull request Oct 7, 2018

Merged

Update contributing docs with instructions on running rustfmt and clippy #337

@tarcieri tarcieri referenced this pull request Jan 24, 2019

Open

Build-time sandboxing #29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.