feat: implement hawk authentication #20

philbooth · 2018-08-29T16:52:35Z

Fixes #19.

I didn't quite manage to get this finished today, mostly because I wasted a few hours writing parsing logic for the Hawk header, only to eventually RTFM and notice the hawk crate already implements that via the FromStr trait. Sigh.

Anyway, I wanted to post this as a WIP so that @rfk can hopefully cast his eye over the crypto logic while I'm away. As you'll see Ryan, I've left your excellent (and long) comment in place for now, interjected with my attempts to implement each section of it. There are a couple of places where I'm not sure I understood you correctly, so I'll call those out in the inline comments.

If I have time after I've packed my bag tomorrow, I might see if I can wire it into the request mechanism and write some tests for it. If I do get to that point I'll comment here and remove the WIP. But, being realistic, I probably won't get to that point.

See you all next week! 😄

philbooth · 2018-08-29T16:59:30Z

src/auth.rs

+            &mut token_secret,
+        )?;
+
+        //   * Use `token_secret` as the secret key for calculating the Hawk request MAC


I wasn't sure about the significance of calculating the Hawk request MAC here. Does it mean something more than the direct comparison of mac and token_secret I'm doing below?

There's more to it. The "id" field from the header and the "token_secret" from this calculation form a hawk "credential" which can be used for signing requests:

https://github.com/taskcluster/rust-hawk/blob/master/hawk/src/credentials.rs#L28

So you now need to take a hmac over the contents of the Authorization header using token_secret as the key, and compare that to the provided mac field in the header.

I'm not sure what the abstractions are like in the rust hawk library. You may be able to do the validation by passing "token_secret" as the key into this validate_header function:

https://github.com/taskcluster/rust-hawk/blob/master/hawk/src/request.rs#L131

Or maybe as the key for this lower-level Mac impl:

https://github.com/taskcluster/rust-hawk/blob/master/hawk/src/mac.rs#L23

Either way, it should do the constant-time-compare part for you for free.

philbooth · 2018-08-29T17:02:24Z

src/auth.rs

+        //       'userid': 42,
+        //       'expires': 1329875384.073159
+        //       'salt': '1c033f'
+        //     }


Is this a comprehensive definition or can other properties be present?

Other properties can and will be present but you're welcome to ignore them. The current set are at:

https://github.com/mozilla-services/tokenserver/blob/master/tokenserver/views.py#L403

Plus "salt" and "expires" which get added automagically.

You may find the "node" property useful as an extra sanity-check, it should match the hostname of the current machine.

I notice the linked code has a uid property but your comment had userid. I'm presuming that was just a typo in the comment, am changing to uid here.

philbooth · 2018-08-29T17:55:10Z

src/auth.rs

+
+        //   * Check that the "expires" timestamp is not in the past.
+        let now = Utc::now().timestamp_millis();
+        if (payload.expires.round() as i64) < now {


Whoops, just realised that this is all kinds of wrong, I need to multiply so that I'm not comparing apples to oranges.

rfk

Nice work @philbooth! The logic here looks solid through until the end, where you need to do the extra hawk-mac-calculation step.

If it helps, I generated some test data using a local syncserver instance.

Master secret: "Ted Koppel is a robot" (as ascii bytes)

Authorization header: Hawk nonce="uVkprSQ=", mac="ZZUcPtn7CU1kb2fclI4mkjjGOPEEKQf2NwWPA62hk7A=", id="eyJub2RlIjogImh0dHA6Ly9sb2NhbGhvc3Q6NTAwMCIsICJ1aWQiOiAxLCAiZXhwaXJlcyI6IDE1MzU1ODAyMDIsICJmeGFfdWlkIjogImYyNjMwNzZlZDhhNTcyMmE1YmE0YzJhMWQwN2IwZjMwIiwgInNhbHQiOiAiM2JkODk2IiwgImRldmljZV9pZCI6ICIxOTFiMjJiNjliY2JhZGZiYWE3NDYyZWYzOTQxMDI2NCJ9cUipB9hpg83VuYfwFk-S120QSp8mzrBROcq9vxzlha0=", ts="1535579912"

Validated token data: {'node': 'http://localhost:5000', 'uid': 1, 'expires': 1535580202, 'fxa_uid': 'f263076ed8a5722a5ba4c2a1d07b0f30', 'salt': '3bd896', 'device_id': '191b22b69bcbadfbaa7462ef39410264'}

rfk · 2018-08-29T21:29:59Z

src/auth.rs

+        //       'userid': 42,
+        //       'expires': 1329875384.073159
+        //       'salt': '1c033f'
+        //     }


Other properties can and will be present but you're welcome to ignore them. The current set are at:

https://github.com/mozilla-services/tokenserver/blob/master/tokenserver/views.py#L403

Plus "salt" and "expires" which get added automagically.

You may find the "node" property useful as an extra sanity-check, it should match the hostname of the current machine.

rfk · 2018-08-29T21:37:43Z

src/auth.rs

+        let mut check: Hmac<Sha256> = Hmac::new_varkey(&signing_secret)?;
+        check.input(&payload);
+        let expected_signature = check.result().code();
+        if !constant_time_compare(signature, &expected_signature) {


Don't implement this yourself; check.result() has a method for doing the constant-time compare for you in a safe way, IIUC from the docs this should be something like: check.result().verify(&expected_signature)?

rfk · 2018-08-29T21:47:53Z

src/auth.rs

+            &mut token_secret,
+        )?;
+
+        //   * Use `token_secret` as the secret key for calculating the Hawk request MAC


There's more to it. The "id" field from the header and the "token_secret" from this calculation form a hawk "credential" which can be used for signing requests:

https://github.com/taskcluster/rust-hawk/blob/master/hawk/src/credentials.rs#L28

So you now need to take a hmac over the contents of the Authorization header using token_secret as the key, and compare that to the provided mac field in the header.

I'm not sure what the abstractions are like in the rust hawk library. You may be able to do the validation by passing "token_secret" as the key into this validate_header function:

https://github.com/taskcluster/rust-hawk/blob/master/hawk/src/request.rs#L131

Or maybe as the key for this lower-level Mac impl:

https://github.com/taskcluster/rust-hawk/blob/master/hawk/src/mac.rs#L23

Either way, it should do the constant-time-compare part for you for free.

rfk · 2018-08-29T21:49:04Z

src/auth.rs

+    }
+}
+
+fn constant_time_compare(actual: &[u8], expected: &[u8]) -> bool {


You shouldn't need to do this yourself, but there's also a create for it btw ;-)

https://docs.rs/constant_time_eq/0.1.3/constant_time_eq/

rfk · 2018-08-29T21:50:48Z

src/auth.rs

+    type Result = AuthResult<HawkPayload>;
+
+    fn from_request(request: &HttpRequest<S>, settings: &Self::Config) -> Self::Result {
+        for header in request.headers().get_all("authorization") {


I would be surprised and alarmed if we got a request with more than one authorization header, and we should error out rather than letting it through if one of them happens to be valid.

rfk · 2018-08-29T21:51:20Z

src/settings.rs

 #[derive(Debug, Deserialize)]
 pub struct Settings {
    pub debug: bool,
    pub port: u16,
+    pub master_secret: Vec<u8>,


FWIW, it seems like this could have a clearer name like "master_token_secret" or something.

philbooth · 2018-09-05T17:30:55Z

If it helps, I generated some test data using a local syncserver instance.

Thanks for the test data @rfk, I started to write a test case with it today but I'm still having a bit of trouble getting it to work.

From reading the docs (both the protocol spec and the hawk-rs docs), I have the impression that I need some request data in order to validate the mac from the header: method, host, port and path. Obviously I can just read those from the actix request object during real execution, but what are they for your test data?

I tried a few things using host and port from the node property and some variations including uid as the path, but without success... what am I missing?

rfk · 2018-09-06T02:00:59Z

I need some request data in order to validate the mac from the header [...]
what are they for your test data

Oh yeah, right, sorry. I didn't write these down at the time, so I generated some fresh ones:

REQUEST:  GET http://localhost:5000/storage/1.5/1/storage/col2
AUTHZ:    Hawk nonce="omxLZWE=", mac="+1oGdzqpxYndK5ejQLdnZpXgGSt/IlxNh5MvcR6j7t4=", id="eyJub2RlIjogImh0dHA6Ly9sb2NhbGhvc3Q6NTAwMCIsICJ1aWQiOiAxLCAiZXhwaXJlcyI6IDE1MzYxOTkyNzQsICJmeGFfdWlkIjogIjMxOWI5OGY5OTYxZmYxZGJkZDA3MzEzY2Q2YmE5MjVhIiwgInNhbHQiOiAiYjAyNjBlIiwgImRldmljZV9pZCI6ICJjMDlkMjZmYWYyYjQ5YWI2NGEyODgyOTA3MjA2ZDBiNSJ96drmQ_KNFOe7U3g1D8ZX5-he2Bv2aRvKZzBPrCjHKO4=", ts="1536198980"
DATA:     {'node': 'http://localhost:5000', 'uid': 1, 'expires': 1536199274, 'fxa_uid': '319b98f9961ff1dbdd07313cd6ba925a', 'salt': 'b0260e', 'device_id': 'c09d26faf2b49ab64a2882907206d0b5'}

REQUEST:  POST http://localhost:5000/storage/1.5/1/storage/col2?batch=MTUzNjE5ODk3NjkyMQ==&commit=true
AUTHZ:    Hawk nonce="1d4mRs0=", mac="xRVjP7607eZUWCBxJKwTo1CsLcNf4TZwUUNrLPUqkdQ=", id="eyJub2RlIjogImh0dHA6Ly9sb2NhbGhvc3Q6NTAwMCIsICJ1aWQiOiAxLCAiZXhwaXJlcyI6IDE1MzYxOTkyNzQsICJmeGFfdWlkIjogIjMxOWI5OGY5OTYxZmYxZGJkZDA3MzEzY2Q2YmE5MjVhIiwgInNhbHQiOiAiYjAyNjBlIiwgImRldmljZV9pZCI6ICJjMDlkMjZmYWYyYjQ5YWI2NGEyODgyOTA3MjA2ZDBiNSJ96drmQ_KNFOe7U3g1D8ZX5-he2Bv2aRvKZzBPrCjHKO4=", ts="1536198978"
DATA:     {'node': 'http://localhost:5000', 'uid': 1, 'expires': 1536199274, 'fxa_uid': '319b98f9961ff1dbdd07313cd6ba925a', 'salt': 'b0260e', 'device_id': 'c09d26faf2b49ab64a2882907206d0b5'}

philbooth · 2018-09-06T14:23:11Z

Hey @rfk, sorry to keep bugging you about this, would you mind casting a fresh eye over my logic here to see if you can spot where I'm going wrong?

My problem is that I'm still failing to validate the mac with that new test data you gave me. It feels like I might be deriving token_secret incorrectly (line 88-ish), but I'm not sure how.

If you look in src/auth/test.rs, you'll see 2 test cases based on your data. Then over in src/auth/mod.rs, towards the bottom of HawkPayload::new, there are 3 separately commented attempts to verify the mac. The first one (line 102-ish) is the code that I'm aiming for, using validate_header. That fails rather opaquely, so the second attempt (line 110-ish) pulls out the underlying Mac stuff and prints the respective byte arrays to the console. Then the third attempt (line 134-ish) was just debugging to make sure that different crypto libs produce the exact same result as hawk/ring do (and as you'd expect, they are bitwise identical).

So that leaves me thinking that the key must be wrong. I did try messing about with it a bit, but not with any great confidence that it would produce a better result. Any ideas?

philbooth · 2018-09-06T14:26:10Z

So that leaves me thinking that the key must be wrong.

(well either that, or I've got your test data wrong somehow...)

rfk · 2018-09-07T02:12:27Z

src/auth/mod.rs

+        hkdf.expand(
+            format!("services.mozilla.com/tokenlib/v1/derive/{}", id).as_bytes(),
+            &mut token_secret,
+        )?;


I missed it in my initial description, but re-reading the python tokenlib source code, I believe that token_secret here needs to be b64urlsafe encoded before using it to verify the mac below. I'm having a bit of trouble building the project right now, but if I can get that working I'll put an encode step in here to check.

I believe it is supposed to be base64encoded.

https://github.com/mozilla-services/go-syncstorage/blob/master/token/token.go#L101

🎉 Woop woop! It's validating successfully now, thanks for the help with this!

rfk · 2018-09-07T02:16:04Z

Cargo.lock

@@ -1301,6 +1403,18 @@ dependencies = [
 "quick-error 1.2.2 (registry+https://github.com/rust-lang/crates.io-index)",
 ]

+[[package]]
+name = "ring"
+version = "0.8.1"


Both travis-ci and I are getting build failures like:

/home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/ring-0.12.1/crypto/curve25519/curve25519.c:4693: multiple definition of `GFp_x25519_public_from_private' /home/travis/build/mozilla-services/syncstorage-rs/target/debug/deps/libring-ae014aa1941bdf5c.rlib(curve25519.o):/home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/ring-0.8.1/crypto/curve25519/curve25519.c:4712: first defined here

Could be because this is introducing a second version of ring, and both versions can't be compiled together into a single binary?

Could be because this is introducing a second version of ring...

Looks like it, yep. Thanks for the tip, I'll make sure I sort it before de-wipping this.

mostlygeek · 2018-09-07T03:51:03Z

src/auth/mod.rs

+        let payload: HawkPayload = serde_json::from_slice(payload)?;
+        println!("{:?}", payload);
+
+        // Check payload is not expired


FYI in this commit I made go-syncstorage accept an environment configurable timestamp skew. In reality some client clocks are off by months and every sync request they sent was 403'd ... I wound up setting this to 1 year just so they could sync.

👍 Thanks for the tip, will do the same here!

philbooth · 2018-09-07T14:50:10Z

Okay, I think this is close to being reviewable now, there's just one last thing that I'm unsure about.

As @rfk pointed out in #20 (comment), the hawk crate depends on a different version of ring to actix. Although that doesn't seem to affect my build on MacOS, it causes linker errors about conflicting symbols in other environments.

Initially I just forked hawk to make it use the same version of ring as actix, and that allows the CI build to succeed. But thinking it through, is that setting us up for a maintenance nightmare as we risk the same thing happening again every time we run cargo update? @bbangert & @pjenvey, is there some clever Rust/cargo magic you guys know of for managing this kind of situation differently?

An alternative, which I've fleshed out in a separate branch, would be to ditch the hawk crate and replace it with some internal logic. That doesn't mean implementing all the crypto ourselves of course, it just means doing a thin layer of parsing and validation on top of external crypto libs that I was already using anyway. Concretely, here's the diff from this PR to what that looks like:

https://github.com/philbooth/syncstorage-rs/compare/hawk-authentication...philbooth:hawk-authentication-internal?expand=1

The exact same tests pass for both branches, so it seems like a low-risk proposal to me (but I'm a hawk/crypto novice so I realise that doesn't count for much).

bbangert · 2018-09-07T22:55:12Z

The hawk Rust crate is maintained by a Mozillian on the Taskcluster team, perhaps we can just ask @djmitche to upgrade the ring dependency? Or does actix use an older version?

djmitche · 2018-09-08T03:22:34Z

I don't do much rust these days, so I can try but I'd also be happy to merge a PR (sadly). The code hasn't been touched for many Rust releases so any updates would be fine too.

…

On Fri, Sep 7, 2018, 6:55 PM Ben Bangert ***@***.***> wrote: The hawk Rust crate is maintained by a Mozillian on the Taskcluster team, perhaps we can just ask @djmitche <https://github.com/djmitche> to upgrade the ring dependency? Or does actix use an older version? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABwAcOhvjXuuIMDvoBsTBhSbpW-d6oEks5uYvlQgaJpZM4WR8Y_> .

bbangert · 2018-09-08T04:24:53Z

@djmitche Thanks! The application-services group is working on more Rust nowadays, so we'll likely need to use the hawk crate in multiple Rust projects going forward. Would it make sense to move the hawk crate to mozilla-services so we can iterate on it, or do you have the time to merge/release new iterations as we submit PR's?

philbooth · 2018-09-08T12:54:05Z

...perhaps we can just ask @djmitche to upgrade the ring dependency?

Well yep, of course. My question was more about whether there's a mechanism that works longer term than that. I don't know, some kind of name-mangling magic that lets different versions of the lib co-exist in the same binary? If not that's cool, it just seems annoying that we have to synchronise the sub-dependencies of two unrelated external dependencies every time we want to run cargo update.

...I'd also be happy to merge a PR...

Ref: taskcluster/rust-hawk#10

djmitche · 2018-09-08T14:24:17Z

Let's keep it under the taskcluster org for now, since that's where it started. I'd like to stay involved -- I might just learn something! If it turns out I'm slowing down progress, we can either move it or I can add collaborators to the repo.

bbangert · 2018-09-08T16:13:24Z

Well yep, of course. My question was more about whether there's a mechanism that works longer term than that. I don't know, some kind of name-mangling magic that lets different versions of the lib co-exist in the same binary? If not that's cool, it just seems annoying that we have to synchronise the sub-dependencies of two unrelated external dependencies every time we want to run cargo update.

It depends, sometimes the crate will work just fine depending on what constructs we need to pass it. If we need to pass it something impl a trait from one version of the dep, and the crate uses a different version we'll end up with a difficult to understand error. If we just call it with more basic types though, it should be fine that it uses one version of ring, while we use another elsewhere.... as long as they don't both try and bind to different versions of say, openssl.

An alternative approach would be to see if it makes sense to change the API of the rust-hawk crate more substantially, such that the user must pass in an object impl a crypto trait for the operations hawk needs. This way the user can choose what ring/crypto libs are used, and the hawk crate can avoid being bound to any particular crypto dep version. We could add this as a lower-level API with feature flags, so that we could turn off the ring dep in rust-hawk and pass in our own object.

philbooth · 2018-09-09T08:19:25Z

It depends, sometimes the crate will work just fine depending on what constructs we need to pass it. If we need to pass it something impl a trait from one version of the dep, and the crate uses a different version we'll end up with a difficult to understand error. If we just call it with more basic types though, it should be fine that it uses one version of ring, while we use another elsewhere.... as long as they don't both try and bind to different versions of say, openssl.

Fwiw, just to be clear, that's not what we saw in CI. This repo and hawk were compiling against the exact same version of ring, and on the actix side no ring objects are passed between us. Indeed compilation itself was fine, there were no errors during that phase of the build.

It was only when the linker kicked in that it complained about multiple definitions of symbols that come directly from C code in the ring lib (example). Looking at the GitHub stats, there's more C than Rust in the ring source code (and more assembler than C).

I played around with the dependencies a bit and even for minor rev differences, the linker fails. But it never fails on my machine, which is what led to my question because if it's possible for the build to succeed on MacOS, it must be doing some kind of magic to address those symbol clashes.

An alternative approach would be to see if it makes sense to change the API of the rust-hawk crate more substantially, such that the user must pass in an object impl a crypto trait for the operations hawk needs. This way the user can choose what ring/crypto libs are used, and the hawk crate can avoid being bound to any particular crypto dep version. We could add this as a lower-level API with feature flags, so that we could turn off the ring dep in rust-hawk and pass in our own object.

I quite like the sound of this, although I haven't looked at the hawk crate to see how pervasive any coupling with ring is. Obviously we can kick this decision down the road for a bit now but if it comes up again soon I'll take a look into it.

thomcc · 2018-09-09T19:42:34Z

Possibly worth noting that this would be very useful for the rust-hawk client lib too.

In different places in mozilla/application-services we're using https://github.com/eoger/rust-hawk/tree/use-openssl and https://github.com/eoger/rust-hawk/tree/use-ring-latest. We planned on doing something like that (exposing a trait) before upstreaming these, but never got around to it...

djmitche · 2018-09-09T19:46:16Z

I'd love to see those upstreamed. I didn't totally understand hawk at the time I was writing rust-hawk (and anyway I think it's gotten several major revisions since then).

rfk

Looking good @philbooth! I'm r- here due to incorrect application of clock skew, but otherwise this looks good to me!

rfk · 2018-09-10T01:17:23Z

src/auth/mod.rs

+        if request.validate_header(
+            &header,
+            &Key::new(token_secret.as_bytes(), &ring::digest::SHA256),
+            Duration::weeks(52),


I think we should add a comment to explain what this weeks(52) is doing here, because is could easily raise eyebrows.

rfk · 2018-09-10T01:24:07Z

src/auth/mod.rs

+            request.uri().port().ok_or(AuthError)?,
+            settings,
+            // Allow a generous amount of client skew for expiry
+            Utc::now().timestamp() as u64 + YEAR,


This expiry date applies to the expires timestamp that is decoded from inside the Hawk id JSON payload, right? That timestamp is generated server-side by the tokenserver (and is covered by the HMAC signature check in extract_and_validate) so we do not need to allow for client-side clock skew in that check. We should assume that the servers have well-synchronized clocks.

As written, this would allow tokenserver tokens to be valid for up to a year longer than intended, which we definitely don't want.

(The timestamp from the hawk header is client-generated and we can allow some generous clock skew on that one).

Oh, my bad. I only added this after the suggestion in #20 (comment).

philbooth · 2018-09-10T18:05:23Z

(just in case anyone looks at this PR overnight and wonders what's going on with it, I'm still trying to make the server tests pass with authentication turned on)

philbooth · 2018-09-11T15:17:04Z

The test failures appear to be related to the actix-web upgrade, so I'm waiting to see what comes out of actix/actix-web#508 before spending any more time on this.

philbooth · 2018-09-11T19:09:10Z

The test failures appear to be related to the actix-web upgrade...

Problem will be fixed in actix-web 0.7.7, so I should have something to review here tomorrow.

philbooth · 2018-09-12T12:24:45Z

This is, at long last, ready for proper review (I hope). Sorry it took so long! 😕

I've wired the authentication into the handlers, but haven't gone any further than that in terms of checking uid from the unpacked payload because I know @bbangert is making changes in src/handlers.rs and I wanted to minimise conflicts for whichever of us loses the merge race. But it does mean that the server tests are now sending authenticated requests to the server, as an extra layer of testing on top of the more unity tests I've added in src/auth/test.rs.

I notice the Settings stuff is starting to get a bit unwieldy with different test modules needing to manually create instances and I do have some thoughts on that, but I opted not to do it in this PR so it doesn't get in the way of the substantive change. (and also my proposal is probably contentious, so I don't want to derail this discussion)

@bbangert @pjenvey @rfk r?

philbooth · 2018-09-12T12:29:21Z

src/server/test.rs

+        key: Key::new(token_secret.as_bytes(), &ring::digest::SHA256),
+    };
+    let header = request.make_header(&credentials).unwrap();
+    format!("Hawk {}", header)


I opted for a manual implementation in the tests here, rather than leaning on code in HawkPayload, because I didn't want the possibility of a bug in production code leaking into the tests and preventing them from picking up on it.

philbooth · 2018-09-12T12:31:20Z

src/settings.rs

+    pub master_token_secret: Vec<u8>,
+}
+
+impl Default for Settings {


Default is required for the implementation of FromRequest on HawkPayload.

The FromRequest Config's a little odd, it always gets the from_request() cfg via Config::default() (so HawkPayloads from_request will always get this bad master_token_secret returned here). I guess it's more of a hardcoded (at build time) configuration

What we should do instead is put Settings (or some subset of it) in the actix_web state and from_request can get it from req.state()

philbooth · 2018-09-12T12:45:53Z

Also, side-note, the hawk crate includes some logging that shows up when authenticating requests (including in the tests). I've opened taskcluster/rust-hawk#12 to investigate the possibility of turning it off.

pjenvey · 2018-09-12T17:27:40Z

src/auth/mod.rs

+        expiry: u64,
+    ) -> AuthResult<HawkPayload> {
+        let decoded_id = base64::decode_config(id, base64::URL_SAFE)?;
+        let payload_length = decoded_id.len() - 32;


could decoded_id be < 32 here?

I was about to say "no", but then I realised I really mean "not if it's a valid header". But of course we still need to fail gracefully for invalid input so I'll add a length check before this step.

bbangert

Looks good to me!

pjenvey

^

rfk

LGTM (modulo @pjenvey's comments that is)

rfk · 2018-09-13T00:38:48Z

src/auth/mod.rs

+            b"services.mozilla.com/tokenlib/v1/signing",
+            None,
+            master_secret,
+        )?;


In a future refactor, you could consider pre-calculating this and caching it somewhere globally. The overhead will be pretty minimal in practice though so NBD.

We're going to need to implement a custom Deserialize for this property on Settings anyway, so could do it in there.

philbooth · 2018-09-13T05:41:52Z

Yeah, so re: the Settings stuff, I noticed that too and it's part of what I mentioned in #20 (comment). Putting it in the request state is a fine suggestion and I'm happy to do that, but I also had another idea which I'll expand on now to see what you think.

Over in fxa-email-service we have a kind of cascade of configs that let us commit different defaults for local dev, tests, CI etc to the repo, without forcing the consuming code to complicate itself with the correct initialisation of Settings:

config.merge(File::with_name("config/default"))?;

let current_env = match env::var("FXA_EMAIL_ENV") {
    Ok(env) => env,
    _ => String::from("dev"),
};
config.merge(File::with_name(&format!("config/{}", current_env)).required(false))?;
config.set_default("env", "dev")?;

config.merge(File::with_name("config/local").required(false))?;
let env = Environment::with_prefix("fxa_email");
config.merge(env.separator("_"))?;

If we did this it would mean that we just need to update those default configs when new settings get added, rather than what I had to do in this PR which was go and make changes in the db tests as well as in the auth tests and in that implementation of the Default trait. So the tests can stay a bit cleaner, but also it means that implementation of Default could be made useful.

Instead of hard-coded default values, it could invoke something like the above code and let the Settings struct concern itself with loading the correct settings for each environment. Then we wouldn't need to include Settings in the request state and it would keep things a bit more separated (I think).

I was going to propose all this in a separate PR, but I'm also happy to do it in this one if you think it's a pre-requisite for landing this stuff.

Thoughts?

philbooth · 2018-09-13T06:04:56Z

Thoughts?

But of course, the thing I didn't consider there is that the linked code returns a Result and the Default trait doesn't, so it would only work if we allowed for the possibility of panics. I guess that means it's a non-starter for the Default trait, so I'll do the request thing.

But fwiw, I'm still interested in whether the above proposal is of interest for separating the test code from it's dependency on Settings (and providing a sensible default value for SYNC_DATABASE_URL for local dev).

philbooth · 2018-09-13T09:03:25Z

I've made a change in d3995ee to put master_token_secret in ServerState. Note that I opted to wrap it in an Arc, to avoid calling clone() on the entire array each time ServerState is instantiated.

Fwiw I also tried giving ServerState an explicit lifetime and borrowing master_token_secret as a slice instead, but borrowck complained about a conflicting lifetime requirement on the App instance (which requires 'static). Lifetimes are hard. 🤔

(although if you guys can suggest a way to make it work, I'm keen to learn!)

r?

philbooth · 2018-09-13T09:07:02Z

src/handlers.rs

-    pub db: Box<Db>,
-}
+use db::{params, util::ms_since_epoch, DbError};
+use server::ServerState;


Now that ServerState is referenced in a few different places, it seemed to belong more in the server module than in handlers.

`FromRequest::Config` uses the `Default` trait to create dependencies, which is no good for us because we don't want to use a default value for the master token secret. Instead, this change pushes the master secret into the `ServerState` struct, where we can set it after reading from environment variables or a config file. One slight downside to this is that I had to wrap it in an `Arc`, to avoid cloning a fresh copy of the master secret in every instance of `ServerState`. I did try giving `ServerState` an explicit lifetime and borrowing the master secret as a slice, but I couldn't get that to play nicely with the requirement for a static lifetime on the `App` instance.

pjenvey · 2018-09-13T20:12:40Z

The Settings per environ setup sounds good Phil (you can nuke the Settings Default when adding it)

Arc looks good here too. It's not worth setting up lifetimes for data like this setup once (or in the case of actix_web State, at least infrequently) -- the clones or the like just don't matter

philbooth commented Aug 29, 2018

View reviewed changes

rfk reviewed Aug 29, 2018

View reviewed changes

rfk reviewed Sep 7, 2018

View reviewed changes

mostlygeek reviewed Sep 7, 2018

View reviewed changes

philbooth changed the title ~~WIP: hawk authentication~~ feat: implement hawk authentication Sep 7, 2018

rfk suggested changes Sep 10, 2018

View reviewed changes

chore: upgrade actix-web to 0.7.7

df4febe

philbooth commented Sep 12, 2018

View reviewed changes

pjenvey reviewed Sep 12, 2018

View reviewed changes

bbangert previously approved these changes Sep 12, 2018

View reviewed changes

feat: implement hawk authentication

6234a6b

philbooth dismissed bbangert’s stale review via 6234a6b September 12, 2018 19:20

bbangert previously approved these changes Sep 12, 2018

View reviewed changes

pjenvey requested changes Sep 13, 2018

View reviewed changes

rfk previously approved these changes Sep 13, 2018

View reviewed changes

philbooth dismissed stale reviews from rfk and bbangert via e29ce8e September 13, 2018 08:25

philbooth commented Sep 13, 2018

View reviewed changes

philbooth mentioned this pull request Sep 13, 2018

Implement a custom deserializer for Settings::master_token_secret #26

Closed

pjenvey approved these changes Sep 13, 2018

View reviewed changes

pjenvey merged commit 6bb4d32 into mozilla-services:master Sep 13, 2018

philbooth deleted the hawk-authentication branch September 14, 2018 08:31

feat: implement hawk authentication #20

feat: implement hawk authentication #20

Conversation

philbooth commented Aug 29, 2018 • edited Loading

philbooth Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfk left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philbooth commented Sep 5, 2018

rfk commented Sep 6, 2018

philbooth commented Sep 6, 2018 • edited Loading

philbooth commented Sep 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philbooth commented Sep 7, 2018

bbangert commented Sep 7, 2018

djmitche commented Sep 8, 2018 via email

bbangert commented Sep 8, 2018

philbooth commented Sep 8, 2018

djmitche commented Sep 8, 2018

bbangert commented Sep 8, 2018

philbooth commented Sep 9, 2018

thomcc commented Sep 9, 2018

djmitche commented Sep 9, 2018 via email

rfk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philbooth commented Sep 10, 2018

philbooth commented Sep 11, 2018

philbooth commented Sep 11, 2018

philbooth commented Sep 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philbooth commented Sep 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbangert left a comment

Choose a reason for hiding this comment

pjenvey left a comment

Choose a reason for hiding this comment

rfk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philbooth commented Sep 13, 2018 • edited Loading

philbooth commented Sep 13, 2018

philbooth commented Sep 13, 2018 • edited Loading

Choose a reason for hiding this comment

pjenvey commented Sep 13, 2018

philbooth commented Aug 29, 2018 •

edited

Loading

philbooth Aug 29, 2018 •

edited

Loading

rfk left a comment •

edited

Loading

philbooth commented Sep 6, 2018 •

edited

Loading

philbooth commented Sep 13, 2018 •

edited

Loading

philbooth commented Sep 13, 2018 •

edited

Loading