mozilla-services / syncstorage-rs

Sync Storage server in Rust
Mozilla Public License 2.0
973 stars 49 forks source link

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

Closed tarikeshaq closed 8 months ago

tarikeshaq commented 9 months ago

Description

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

  1. 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
  2. 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:

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

Issue(s)

Closes SYNC-3528.

tarikeshaq commented 9 months ago

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

ethowitz commented 9 months ago

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

tarikeshaq commented 9 months ago

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

jrconlin commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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