Whitelist rustc dependencies #48456

mark-i-m · Feb 23, 2018

As per https://internals.rust-lang.org/t/rustc-dependency-policy/6537/8?u=nikomatsakis

cc @alexcrichton @nikomatsakis

rust-highfive · Feb 23, 2018

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

mark-i-m · Feb 23, 2018

Hopefully, travis fails because I haven't added any files to the whitelist yet! 😛

mark-i-m · Feb 23, 2018

Hmmm... It seems that it only runs stage0 tidy? How do I get my tidy to work...

nikomatsakis · Feb 23, 2018

@mark-i-m travis...isn't failing?

nikomatsakis · Feb 23, 2018

nikomatsakis reviewed Feb 23, 2018

View changes

Any idea why travis seems happy?

nikomatsakis · Feb 23, 2018

src/tools/tidy/src/deps.rs

        saw_dir = true;
        let dir = t!(dir);

        // skip our exceptions
-        for exception in EXCEPTIONS {
-            if dir.path()
+        if EXCEPTIONS.iter().any(|exception| {


nikomatsakis · Feb 23, 2018

src/tools/tidy/src/deps.rs

+             }| {
+                WHITELIST
+                    .iter()
+                    .all(|&(wname, wversion)| name != wname || version != wversion)


seems like it would be nice if we could report what needs to be added to the whitelist

mark-i-m · Feb 24, 2018

Ok, it should fail now 🤞

mark-i-m · Feb 24, 2018

MAKE IT FAILgit statusgit status

Lol, that's what I get for using !!!! in a git commit message...

mark-i-m · Feb 24, 2018

Hmm... it looks like paths are incorrect...

mark-i-m · Feb 24, 2018

It looks like a spurious failure... I just want to know what path the Cargo.toml for the argument of tidy is at.

mark-i-m · Feb 25, 2018

So currently, I'm having path failures. I'm looking for a Cargo.toml in /checkout/src/Cargo.toml and there isn't one... does anyone know the correct place to find it?

[00:05:02] * 534 error codes
[00:05:02] * highest error code: E0908
[00:05:02] * 175 features
[00:05:02] Getting metadata from "/checkout/src/Cargo.toml"
[00:05:02] thread 'main' panicked at 'Unable to run `cargo metadata`: Os { code: 2, kind: NotFound, message: "No such file or directory" }', libcore/result.rs:945:5
[00:05:02] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:05:02] 
[00:05:02] 
[00:05:02] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "--no-vendor" "--quiet"
[00:05:02] expected success, got: exit code: 101
[00:05:02] 
[00:05:02] 
[00:05:02] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:05:02] Build completed unsuccessfully in 0:01:57
[00:05:02] Makefile:74: recipe for target 'tidy' failed
[00:05:02] make: *** [tidy] Error 1

alexcrichton · Feb 26, 2018

@mark-i-m that looks like it's perhaps failing to spawn cargo rather than failing to find Cargo.toml? By default I believe cargo isn't in PATH so rustbuild may need to pass the path to cargo down to the tidy script.

mark-i-m · Feb 26, 2018

Ok, it seems to be working 🎉

There are currently two variables which need to be hardcoded into tidy, though:

The set of crates whose dependencies we want to check (currently those corresponding to the compiler)
The whitelist itself

So we have a choice:

It is conceivable that we want to have different whitelists for rustc, libstd, tooling, books, etc... In that case, maybe those variables shouldn't be hardcoded into tidy but passed in from somewhere else.
The simple solution for now is to just hardcode into tidy the whitelist for rustc and always check the 'librustc*' and 'libsyntax*' crates.

alexcrichton · Feb 26, 2018

alexcrichton reviewed Feb 26, 2018

View changes

Looking good! I think it's fine to hardcode the crates to check and whitelist in the code itself. For the crates to check you can probably just probe the entire dependency graph for dependencies from crates.io starting from rustc and rustc_trans.

alexcrichton · Feb 26, 2018

src/tools/tidy/src/deps.rs

+    #[allow(dead_code)] manifest_path: String,
+}
+
+// Not used, but needed to not confuse serde :P


Hm if these aren't used, are they needed? I think serde should ignore unused fields by default?

Hmm... When I tried that it was giving me deserialization errors. I will take another look.

alexcrichton · Feb 26, 2018

src/tools/tidy/src/deps.rs

+
+// Whitelist of crates rustc is allowed to depend on. Avoid adding to the list if possible.
+static WHITELIST: &'static [(&'static str, &'static str)] = &[
+//    ("advapi32-sys", "0.2.0"),


I think it's probably ok to only check the name of the dependency and not worry about the version for now, upgrading across major versions and such tends to not bring in too large of a change at least!

I argue that it's important for security purposes to whitelist exact versions and to vet those versions before before upgrading in any way.

Oh sure yeah, but I think we'll want to consider this further down the road. If we were to lock down all the versions of everything it'd cause I think picking up deps from crates.io to be too painful (or otherwise routine updates)

I think this is part of the downside of having a super-awesome dependency system: it becomes easy to have too many dependencies. Argue that that makes code quality, security, and build times all worse.

Sure but I think it's also best to do this incrementally rather than all at once, can this switch to just verifying the names through a whitelist?

I can if you would like, but I would really like not to. I feel like adding a bit of friction to adding and updating dependencies would be healthy; in this case, you would need to update WHITELIST. We can later have an RFC to potentially add more restrictions to PRs that change WHITELIST... I see that as an incremental approach as well (we add a mechanism, then add policy through an RFC).

mark-i-m · Feb 27, 2018

@alexcrichton Ok, so I am now only checking the dependencies of rustc and rustc_trans. (I will uncomment the whitelist when the build fails). I designed the Crate type so that we can easily turn on/off version checking, as you choose.

alexcrichton · Feb 27, 2018

alexcrichton reviewed Feb 27, 2018

View changes

alexcrichton · Feb 27, 2018

src/tools/tidy/src/deps.rs

+    resolve: Resolve,
+
+    // Not used, but needed to not confuse serde :P
+    #[allow(dead_code)] packages: Vec<Package>,


Could this get removed? If not, how come?

alexcrichton · Feb 27, 2018

alexcrichton reviewed Feb 27, 2018

View changes

alexcrichton · Feb 27, 2018

src/tools/tidy/src/deps.rs

+        unapproved.append(&mut bad);
+    }
+
+    // Remove duplicates


Perhaps this could be passed around as &mut BTreeSet<Crate<'a>>?

alexcrichton · Feb 27, 2018

Can this please remove the version checking as well? That happens anyway when Cargo.lock changes and is reviewed. The purpose of this feature is to prevent accidental inclusion of extra crates from crates.io or otherwise alert ourselves to new dependencies.

mark-i-m · Feb 27, 2018

@alexcrichton done 👍

Let me know what you think :)

mark-i-m · Feb 27, 2018

I will uncomment the whitelist itself when the build fails...

mark-i-m · Feb 27, 2018

Ok, I uncommented the whitelist.

alexcrichton · Feb 27, 2018

Looks good to me!

I think a few more deps may need to be added though?

mark-i-m · Feb 27, 2018

Oh, hmm... I wonder how that happened 😖

Fixed 👍

alexcrichton · Feb 28, 2018

Looks like another crate is missing? Although I don't know how core got in there...

mark-i-m · Feb 28, 2018

Hmm... different versions of a crate may have different dependencies... I think this should be fixed now.

Once again, I will wait for build failure and then enable the whitelist.

mark-i-m · Mar 5, 2018

@alexcrichton Hmm... I tried rebasing on the latest master, but I didn't get any conflicts locally. Do you know what's going on here?

alexcrichton · Mar 5, 2018

Ah yeah sometimes bors is not so good at merges...

@bors: r+

bors · Mar 5, 2018

📌 Commit e5d2920 has been approved by alexcrichton

bors · Mar 6, 2018

⌛️ Testing commit e5d2920 with merge 1733a61...

bors · Mar 6, 2018

💔 Test failed - status-appveyor

alexcrichton · Mar 6, 2018

All dist jobs passed, merging manually

rust-highfive assigned alexcrichton Feb 23, 2018

rust-highfive added the S-waiting-on-review label Feb 23, 2018

kennytm added S-waiting-on-author and removed S-waiting-on-review labels Feb 25, 2018

mark-i-m changed the title ~~[WIP] Whitelist rustc dependencies~~ Whitelist rustc dependencies Feb 27, 2018

mark-i-m force-pushed the mark-i-m:whitelist branch from 878e4d5 to 24e929d Feb 28, 2018

mark-i-m force-pushed the mark-i-m:whitelist branch from 2bc27d3 to e5d2920 Mar 5, 2018

bors added S-waiting-on-bors and removed S-waiting-on-author labels Mar 5, 2018

bors added S-waiting-on-review and removed S-waiting-on-bors labels Mar 6, 2018

alexcrichton merged commit e5d2920 into rust-lang:master Mar 6, 2018
1 of 2 checks passed

1 of 2 checks passed

homu Test failed
Details

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

mark-i-m deleted the mark-i-m:whitelist branch Nov 14, 2018

rust-lang/rust

Join GitHub today

Whitelist rustc dependencies #48456

Conversation

mark-i-m commented Feb 23, 2018

rust-highfive assigned alexcrichton Feb 23, 2018

This comment has been minimized.

rust-highfive commented Feb 23, 2018

rust-highfive added the S-waiting-on-review label Feb 23, 2018

This comment has been minimized.

mark-i-m commented Feb 23, 2018

This comment has been minimized.

mark-i-m commented Feb 23, 2018

This comment has been minimized.

nikomatsakis commented Feb 23, 2018

nikomatsakis reviewed Feb 23, 2018 View changes

nikomatsakis left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mark-i-m commented Feb 24, 2018

This comment has been minimized.

mark-i-m commented Feb 24, 2018

This comment has been minimized.

mark-i-m commented Feb 24, 2018

This comment has been minimized.

mark-i-m commented Feb 24, 2018

This comment has been minimized.

mark-i-m commented Feb 25, 2018

kennytm added S-waiting-on-author and removed S-waiting-on-review labels Feb 25, 2018

This comment has been minimized.

alexcrichton commented Feb 26, 2018

This comment has been minimized.

mark-i-m commented Feb 26, 2018

alexcrichton reviewed Feb 26, 2018 View changes

alexcrichton left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mark-i-m commented Feb 27, 2018

alexcrichton reviewed Feb 27, 2018 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Feb 27, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton commented Feb 27, 2018

This comment has been minimized.

mark-i-m commented Feb 27, 2018

This comment has been minimized.

mark-i-m commented Feb 27, 2018

This comment has been minimized.

mark-i-m commented Feb 27, 2018

mark-i-m changed the title [WIP] Whitelist rustc dependencies Whitelist rustc dependencies Feb 27, 2018

This comment has been minimized.

alexcrichton commented Feb 27, 2018

This comment has been minimized.

mark-i-m commented Feb 27, 2018

This comment has been minimized.

alexcrichton commented Feb 28, 2018

This comment has been minimized.

mark-i-m commented Feb 28, 2018

mark-i-m force-pushed the mark-i-m:whitelist branch from 878e4d5 to 24e929d Feb 28, 2018

mark-i-m added some commits Feb 23, 2018

mark-i-m force-pushed the mark-i-m:whitelist branch from 2bc27d3 to e5d2920 Mar 5, 2018

This comment has been minimized.

mark-i-m commented Mar 5, 2018

This comment has been minimized.

alexcrichton commented Mar 5, 2018

This comment has been minimized.

bors commented Mar 5, 2018

bors added S-waiting-on-bors and removed S-waiting-on-author labels Mar 5, 2018

This comment has been minimized.

nikomatsakis reviewed Feb 23, 2018

View changes

alexcrichton reviewed Feb 26, 2018

View changes

alexcrichton reviewed Feb 27, 2018

View changes

alexcrichton reviewed Feb 27, 2018

View changes

mark-i-m force-pushed the mark-i-m:whitelist branch from `878e4d5` to `24e929d` Feb 28, 2018

mark-i-m force-pushed the mark-i-m:whitelist branch from `2bc27d3` to `e5d2920` Mar 5, 2018

alexcrichton merged commit `e5d2920` into rust-lang:master Mar 6, 2018
1 of 2 checks passed