feat: Removes pyo3 and derives tokens directly in Rust #1513

tarikeshaq · 2024-01-17T20:48:06Z

Description

We were using pyo3 in two places to call into Python code:

When verifying an FxA oauth access token, calling into PyFxA's verify_token function - to verify an oauth token, which would first try to validate the token directly using a JWK stored in our environment variables, or otherwise it would trigger a /v1/verify/ call on FxA's oauth server, asking FxA to verify for us
When generating a new Token to be used against storage server, we called into tokenlib's make_token and get_derived_secret

This PR replaces both usages of pyo3 with Rust code implemented directly in the tokenserver.

Testing

I've tested a local firefox connected to FxA's stage, and my local environment with tokenserver and syncstorage and verified that:

Tokenserver can verify tokens properly if given the JWK's in its env variables
Tokenserver can send the network request properly to FxA when it cannot verify the token locally (think FxA rotated their JWKs)
Tokenserver can issue tokens, and those tokens can be used correctly for sync against syncstorage

My local firefox syncs successfully with the changes in this PR, however, Out of abundance of caution we should run end-to-end tests and deploy this first on a canary if we plan to take it

TODOs

Add unit tests for both verify and get_token_and_derived_secret
- verify
- token generation
Document the hidden policies here so they're not hidden, eg: how the tokenserver tokens are derived, and how it validates the FxA access token, this was documented in the code for now as doc strings on a public API

Issue(s)

Closes SYNC-3528.

Dockerfile

tarikeshaq · 2024-01-17T20:50:00Z

tokenserver-auth/src/crypto.rs

+impl Crypto for CryptoImpl {
+    type Error = TokenserverError;
+    fn hkdf(&self, secret: &str, salt: Option<&[u8]>, info: &[u8]) -> Result<Vec<u8>, Self::Error> {
+        let hk = Hkdf::<Sha256>::new(salt, secret.as_bytes());
+        let mut okm = [0u8; SHA256_OUTPUT_LEN];
+        hk.expand(info, &mut okm)
+            .map_err(|_| TokenserverError::internal_error())?;
+        Ok(okm.to_vec())
+    }
+
+    fn hmac_sign(&self, key: &[u8], payload: &[u8]) -> Result<Vec<u8>, Self::Error> {
+        let mut mac: Hmac<Sha256> =
+            Hmac::new_from_slice(key).map_err(|_| TokenserverError::internal_error())?;
+        mac.update(payload);
+        Ok(mac.finalize().into_bytes().to_vec())
+    }


This will require a careful review since they're trying to do crypto!

tokenserver-auth/src/lib.rs

tokenserver-auth/src/oauth.rs

Cargo.toml

Cargo.lock

tools/integration_tests/tokenserver/test_e2e.py

tarikeshaq · 2024-01-18T19:30:02Z

Alrighty this should be in a good stage for a first-round review

@jrconlin @pjenvey I know there is a lot going in the autopush world, so no rush getting to this - it is not urgent and I wouldn't want to interrupt your flow over there but I'll keep my eye on the patch for when ya'll get the time

tokenserver-auth/src/lib.rs

tokenserver-auth/src/oauth.rs

tokenserver-auth/src/lib.rs

tokenserver-auth/src/crypto.rs

tokenserver-auth/src/oauth.rs

tokenserver-auth/src/crypto.rs

Cargo.lock

Dockerfile

tokenserver-auth/src/crypto.rs

tokenserver-auth/src/lib.rs

tokenserver-auth/src/oauth.rs

jrconlin

~~Still going through the tests for oauth.rs~~ Done. Nothing looks out of sorts.

ethowitz · 2024-01-30T14:13:22Z

Just stopping by to say this is awesome and I'm glad it's happening 🎉

tarikeshaq · 2024-01-31T16:49:05Z

This is why I love working in the open - Great to see you @ethowitz!!

tokenserver-auth/src/crypto.rs

jrconlin · 2024-02-07T18:47:38Z

tokenserver-auth/src/crypto.rs

+
+impl Crypto for CryptoImpl {
+    type Error = TokenserverError;
+    fn hkdf(&self, secret: &str, salt: Option<&[u8]>, info: &[u8]) -> Result<Vec<u8>, Self::Error> {


Equivalent function for hawk crate takes only a single key vec, but specifies the hash algo (e.g. Key::new(vec![99u8; 32], SHA256) (where the vec! would contain a real key and not a dummy value like that.) It's worth noting that we are using a salt where the hawk crate appears not to?

Yeah the difference is the hawk function we're looking at is generating the Hmac key, which happens (in hmac_sign for us, and unlike hawk we hard-code the algorithm to be sha256, although we could expand the API if the use case comes!)

We end up using a salt here in the HKDF for the per-token secret, using the salt that is randomly generated and inserted into the token. That all said, a lot of this is out of my comfort zone and I tried to keep it true to what the python was doing and not re-invent anything

tokenserver-auth/src/lib.rs

tokenserver-auth/src/oauth.rs

jrconlin · 2024-02-07T20:13:11Z

Part of me would love to put the python replacement stuff behind a feature flag, maybe break the functions out of tokenserver-auth/lib.rs and into either lib-py.rs or lib-rs.rs (same with oauth.rs) and bring them in via a flag? That would let us land this code but keep it out of production use while it gets reviewed.

Fortunately a LOT of this PR is general cleanup.

tarikeshaq · 2024-02-08T20:04:12Z

Thank you @jrconlin!! I like the suggestion of using rust features for this so it's safer (also easier to rollback and canary, etc etc)

I went ahead and moved the python implementation behind a rust feature, and removing that feature will have the Rust implementation

tarikeshaq · 2024-02-08T20:39:00Z

(the ci failures are unrelated and should be fixed in #1518 )

jrconlin

Oh, awesome! You were able to break the python bit into it's own feature.

…oading)

tarikeshaq commented Jan 17, 2024

View reviewed changes

tarikeshaq force-pushed the remove-pyo3 branch 3 times, most recently from e1ed400 to bc2c3cd Compare January 18, 2024 02:44

tarikeshaq commented Jan 18, 2024

View reviewed changes

tools/integration_tests/tokenserver/test_e2e.py Show resolved Hide resolved

tarikeshaq force-pushed the remove-pyo3 branch 4 times, most recently from 60d6409 to f48c0e1 Compare January 18, 2024 18:12

tarikeshaq force-pushed the remove-pyo3 branch from f48c0e1 to f0258d9 Compare January 18, 2024 21:07

tarikeshaq commented Jan 19, 2024

View reviewed changes

tokenserver-auth/src/lib.rs Outdated Show resolved Hide resolved

tokenserver-auth/src/oauth.rs Outdated Show resolved Hide resolved

tarikeshaq force-pushed the remove-pyo3 branch from af70535 to 927f460 Compare January 19, 2024 22:30

tarikeshaq commented Jan 22, 2024

View reviewed changes

tokenserver-auth/src/lib.rs Outdated Show resolved Hide resolved

jrconlin reviewed Jan 22, 2024

View reviewed changes

tokenserver-auth/src/lib.rs Outdated Show resolved Hide resolved

tokenserver-auth/src/lib.rs Outdated Show resolved Hide resolved

tokenserver-auth/src/crypto.rs Show resolved Hide resolved

tarikeshaq force-pushed the remove-pyo3 branch 2 times, most recently from 3e5f129 to 025c2f2 Compare January 25, 2024 03:15

tarikeshaq commented Jan 25, 2024

View reviewed changes

tokenserver-auth/src/oauth.rs Outdated Show resolved Hide resolved

tokenserver-auth/src/crypto.rs Show resolved Hide resolved

Cargo.lock Outdated Show resolved Hide resolved

tarikeshaq mentioned this pull request Jan 26, 2024

bump: hawk, resolving duplicate dependencies and fix advisory #1516

Merged

tarikeshaq force-pushed the remove-pyo3 branch 2 times, most recently from 843ce10 to cafcb90 Compare January 26, 2024 17:19

jrconlin reviewed Jan 26, 2024

View reviewed changes

tarikeshaq requested a review from jrconlin February 1, 2024 18:05

jrconlin reviewed Feb 7, 2024

View reviewed changes

tarikeshaq force-pushed the remove-pyo3 branch 2 times, most recently from d41ece5 to 36eea7f Compare February 8, 2024 18:50

tarikeshaq force-pushed the remove-pyo3 branch from 36eea7f to 9efcaeb Compare February 8, 2024 19:52

tarikeshaq requested a review from jrconlin February 8, 2024 20:04

jrconlin previously approved these changes Feb 9, 2024

View reviewed changes

Tarik Eshaq added 9 commits February 9, 2024 14:15

Removes pyo3 and derives tokens directly in Rust

bfba6e3

Adds tests for JWT verifying

fd601b4

Adds tests for token generation

3028c83

Adds metrics for oauth verify error cases

38e0374

Updates jsonwebtoken to not include default features (including pem l…

587bef8

…oading)

Adds context and logs errors during oauth verify

6fc2c1e

Uses ring for cryptographic rng

2128b6a

Adds back python impl under feature flag

8f85619

Uses one cached http client for reqwest

5f6be6c

tarikeshaq dismissed jrconlin’s stale review via 5f6be6c February 9, 2024 19:24

tarikeshaq force-pushed the remove-pyo3 branch from 9efcaeb to 5f6be6c Compare February 9, 2024 19:24

jrconlin approved these changes Feb 9, 2024

View reviewed changes

tarikeshaq merged commit 1b11684 into mozilla-services:master Feb 12, 2024
8 checks passed

jrconlin mentioned this pull request May 9, 2024

feat: Syncstorage inital sqlite support #1520

Draft

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Removes pyo3 and derives tokens directly in Rust #1513

feat: Removes pyo3 and derives tokens directly in Rust #1513

tarikeshaq commented Jan 17, 2024 •

edited

Loading

tarikeshaq Jan 17, 2024

tarikeshaq commented Jan 18, 2024

jrconlin left a comment •

edited

Loading

ethowitz commented Jan 30, 2024

tarikeshaq commented Jan 31, 2024

jrconlin Feb 7, 2024

tarikeshaq Feb 8, 2024

jrconlin commented Feb 7, 2024

tarikeshaq commented Feb 8, 2024

tarikeshaq commented Feb 8, 2024

jrconlin left a comment

feat: Removes pyo3 and derives tokens directly in Rust #1513

feat: Removes pyo3 and derives tokens directly in Rust #1513

Conversation

tarikeshaq commented Jan 17, 2024 • edited Loading

Description

Testing

TODOs

Issue(s)

tarikeshaq Jan 17, 2024

Choose a reason for hiding this comment

tarikeshaq commented Jan 18, 2024

jrconlin left a comment • edited Loading

Choose a reason for hiding this comment

ethowitz commented Jan 30, 2024

tarikeshaq commented Jan 31, 2024

jrconlin Feb 7, 2024

Choose a reason for hiding this comment

tarikeshaq Feb 8, 2024

Choose a reason for hiding this comment

jrconlin commented Feb 7, 2024

tarikeshaq commented Feb 8, 2024

tarikeshaq commented Feb 8, 2024

jrconlin left a comment

Choose a reason for hiding this comment

tarikeshaq commented Jan 17, 2024 •

edited

Loading

jrconlin left a comment •

edited

Loading