Add authentication throughout sccache-dist, including simple token auth by aidanhs · Pull Request #274 · mozilla/sccache

aidanhs · 2018-08-13T19:46:50Z

This adds

authenticated scheduler<->server communication in its (mostly) final form
the infrastructure for client<->scheduler communication, as well as a basic auth method (token)
job tokens to authenticate whatever you do with a job after the initial job creation

Should be reviewed after #271

aidanhs · 2018-08-16T20:17:41Z

src/bin/sccache-dist/main.rs

-                // TODO: for some reason these don't get called out in specific help if they're omitted
-                .requires_if("overlay", "overlay-build-dir")
-                .requires_if("overlay", "overlay-bwrap-path")
+        .subcommand(SubCommand::with_name("auth")


New utility subcommand of sccache-dist

Note also that scheduler and server move to being configured solely by config files which I found to be more future-proof.

aidanhs · 2018-08-16T20:19:38Z

Rebased and added the docs for this initial authentication (unfortunately it depends on the Windows PR due to some slight overlaps). You can just review the last commit though, as it's all in there.

luser · 2018-08-24T18:36:25Z

Does this need to be rebased atop the landing of #271 ?

luser · 2018-08-24T19:20:27Z

Cargo.toml

 hyper-tls = { version = "0.1", optional = true }
 jobserver = "0.1"
-jsonwebtoken = { version = "4.0", optional = true }
+jsonwebtoken = "4.0.1"


This could remain optional and be listed under dist and dist-server, right?

luser · 2018-08-24T19:22:29Z

docs/Distributed.md

@@ -0,0 +1,212 @@
+= Distributed sccache


The formatting seems a bit off on this. Markdown uses # for headers, not =.

luser · 2018-08-24T19:27:54Z

docs/Distributed.md

+automatically package the compiler in use, while Windows and OSX users will
+need to specify a toolchain for cross-compilation ahead of time.
+
+== Communication


I think this section would be better at the end or in a separate file, as it's less useful to end users than everything else in here.

This doc is actually intended as a deep dive into the technical details of how distributed sccache works and the implications of certain decisions. The user-facing doc will come later, as part of describing how to install and set it up.

luser · 2018-08-24T19:28:41Z

docs/Distributed.md

+follows:
+
+```
+server_auth = { type = "jwt_hs256", secret_key = "YOUR_KEY_HERE" }


It would be great to have examples of full config files for each of the scheduler, client, builder in the docs at some point.

luser · 2018-08-24T19:44:40Z

src/dist/http.rs

+
+    split.next()
+}
+macro_rules! try_jwt_or_401 {


Are these macros just so they can early-return?

luser · 2018-08-24T19:51:12Z

src/config.rs


 pub const HIDDEN_FILE_CONFIG_DATA_VAR: &str = "_SCCACHE_TEST_CONFIG";

+pub const INSECURE_DIST_CLIENT_TOKEN: &str = "dangerously_insecure_client";


This doesn't seem to match the value in the docs. Am I misinterpreting?

This token is the one used when you specify DANGEROUSLY_INSECURE mode in your config file. The exact value is left undocumented in the readme as there's no reason to know it (it's enough for it to be harcoded).

luser · 2018-08-24T19:55:15Z

src/bin/sccache-dist/main.rs

+            Ok(0)
+        },
+        Command::Auth(AuthSubcommand::JwtHS256ServerToken { secret_key, server_id }) => {
+            let header = jwt::Header::new(jwt::Algorithm::HS256);


This feels like it could all get split out into a separate function.

luser · 2018-08-24T19:55:46Z

src/bin/sccache-dist/main.rs

+        },
+
+        Command::Scheduler(scheduler_config::Config { client_auth, server_auth }) => {
+            let check_client_auth: Box<Fn(&str) -> bool + Send + Sync> = match client_auth {


Oops, I meant to put that comment here.

The separate function bit? I'm not clear - what do you think could go in that function?

I was just thinking that extracting the meat of this match arm into a separate function would make this function smaller and easier to follow. It's not a huge concern.

luser · 2018-08-24T19:56:49Z

src/bin/sccache-dist/main.rs

+            let server_id = ServerId(public_addr);
+            let scheduler_auth = match scheduler_auth {
+                server_config::SchedulerAuth::Insecure => {
+                    create_server_token(server_id, &INSECURE_DIST_SERVER_TOKEN)


I think it'd be good to log a warning on server startup for the insecure case.

Good point.

luser · 2018-08-24T20:00:16Z

src/bin/sccache-dist/main.rs

+                (job_id, server_id, auth)
            } else {
-                let msg = format!("Insufficient capacity: {} available servers", servers.len());
+                let msg = format!("Insufficient capacity across {} available servers", num_servers);


This seems unfortunate. If a client tries to request a job that we don't have capacity for, it fails? I'd like to chat about this next time we talk.

luser

Overall this looks good! I'll let you rebase and merge this (if I didn't already grant you commit access to this repo I'll fix that as well).

aidanhs · 2018-08-30T00:10:12Z

I've rebased and backported some conditional compilation cleanup (in light of the additional PRs coming through).

aidanhs · 2018-08-30T02:16:55Z

(I'm not going to go ahead and merge because I think it would be useful to get some eyes on it, since it involved a bit of shuffling)

luser · 2018-08-30T12:19:34Z

src/dist/http.rs

+
+    impl Cfg {
+        pub fn scheduler_listen_addr() -> SocketAddr {
+            let ip_addr = "0.0.0.0".parse().unwrap();


I tend to prefer to write out the Ipv4Addr longhand, like:

sccache/src/server.rs

Line 198 in 1d89944

let addr = SocketAddrV4::new(Ipv4Addr::new(127, 0, 0, 1), port);

aidanhs force-pushed the aphs-dist-sccache-simple-auth branch from 9f2bd00 to 30e5be5 Compare August 16, 2018 20:16

aidanhs commented Aug 16, 2018

View reviewed changes

aidanhs mentioned this pull request Aug 22, 2018

Richer argument handling for compilers #277

Closed

luser reviewed Aug 24, 2018

View reviewed changes

luser approved these changes Aug 24, 2018

View reviewed changes

This was referenced Aug 28, 2018

Use Reqwest to access s3 to follow redirects #279

Merged

Use redis-0.9 for async requests #280

Closed

aidanhs added 2 commits August 29, 2018 22:35

Implement scheduler auth for clients and servers, add job tokens

36741e8

Backport conditional compilation, fix markdown

f104074

aidanhs force-pushed the aphs-dist-sccache-simple-auth branch from 30e5be5 to f104074 Compare August 30, 2018 00:06

luser reviewed Aug 30, 2018

View reviewed changes

luser merged commit 105ab91 into mozilla:master Aug 30, 2018


		pub const HIDDEN_FILE_CONFIG_DATA_VAR: &str = "_SCCACHE_TEST_CONFIG";

		pub const INSECURE_DIST_CLIENT_TOKEN: &str = "dangerously_insecure_client";

Conversation

aidanhs commented Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aidanhs commented Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luser commented Aug 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luser left a comment

Choose a reason for hiding this comment

Uh oh!

aidanhs commented Aug 30, 2018

Uh oh!

aidanhs commented Aug 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aidanhs commented Aug 13, 2018 •

edited

Loading

aidanhs commented Aug 16, 2018 •

edited

Loading