Merge branch 'master' into refactor/remove-unnecessary-syncserverrequest-impl

2026-05-12 08:06:47 +02:00 · 2022-05-13 16:11:48 -04:00 · 2022-05-13 16:11:48 -04:00 · 2c089a0d7a
commit 2c089a0d7a
parent a21073d0f3 d62db9f08e
6 changed files with 182 additions and 114 deletions
--- a/docker-compose.e2e.mysql.yaml
+++ b/docker-compose.e2e.mysql.yaml
@ -1,41 +1,48 @@
 version: '3'
 services:
-    sync-db:
-    tokenserver-db:
-    syncstorage-rs:
-        depends_on:
-          - sync-db
-          - tokenserver-db
-        # TODO: either syncstorage-rs should retry the db connection
-        # itself a few times or should include a wait-for-it.sh script
-        # inside its docker that would do this for us. Same (probably
-        # the latter solution) for server-syncstorage below
-        entrypoint: >
-          /bin/sh -c "
-            sleep 15;
-            /app/bin/syncstorage;
-          "
-    e2e-tests:
-        depends_on:
-          - mock-fxa-server
-          - syncstorage-rs
-        image: app:build
-        privileged: true
-        user: root
-        environment:
-          MOCK_FXA_SERVER_URL: http://mock-fxa-server:6000
-          SYNC_HOST: 0.0.0.0
-          SYNC_MASTER_SECRET: secret0
-          SYNC_DATABASE_URL: mysql://test:test@sync-db:3306/syncstorage
-          SYNC_TOKENSERVER__DATABASE_URL: mysql://test:test@tokenserver-db:3306/tokenserver
-          SYNC_TOKENSERVER__ENABLED: "true"
-          SYNC_TOKENSERVER__FXA_BROWSERID_AUDIENCE: "https://token.stage.mozaws.net/"
-          SYNC_TOKENSERVER__FXA_BROWSERID_ISSUER: "api-accounts.stage.mozaws.net"
-          SYNC_TOKENSERVER__FXA_EMAIL_DOMAIN: api-accounts.stage.mozaws.net
-          SYNC_TOKENSERVER__FXA_METRICS_HASH_SECRET: secret0
-          SYNC_TOKENSERVER__RUN_MIGRATIONS: "true"
-          TOKENSERVER_HOST: http://localhost:8000
-        entrypoint: >
-          /bin/sh -c "
-            sleep 28; python3 /app/tools/integration_tests/run.py 'http://localhost:8000#secret0'
-          "
+  sync-db:
+  tokenserver-db:
+  syncstorage-rs:
+    depends_on:
+      - sync-db
+      - tokenserver-db
+    # TODO: either syncstorage-rs should retry the db connection
+    # itself a few times or should include a wait-for-it.sh script
+    # inside its docker that would do this for us. Same (probably
+    # the latter solution) for server-syncstorage below
+    entrypoint: >
+      /bin/sh -c "
+        sleep 15;
+        /app/bin/syncstorage;
+      "
+  e2e-tests:
+    depends_on:
+      - mock-fxa-server
+      - syncstorage-rs
+    image: app:build
+    privileged: true
+    user: root
+    environment:
+      MOCK_FXA_SERVER_URL: http://mock-fxa-server:6000
+      SYNC_HOST: 0.0.0.0
+      SYNC_MASTER_SECRET: secret0
+      SYNC_DATABASE_URL: mysql://test:test@sync-db:3306/syncstorage
+      SYNC_TOKENSERVER__DATABASE_URL: mysql://test:test@tokenserver-db:3306/tokenserver
+      SYNC_TOKENSERVER__ENABLED: "true"
+      SYNC_TOKENSERVER__FXA_BROWSERID_AUDIENCE: "https://token.stage.mozaws.net/"
+      SYNC_TOKENSERVER__FXA_BROWSERID_ISSUER: "api-accounts.stage.mozaws.net"
+      SYNC_TOKENSERVER__FXA_EMAIL_DOMAIN: api-accounts.stage.mozaws.net
+      SYNC_TOKENSERVER__FXA_METRICS_HASH_SECRET: secret0
+      SYNC_TOKENSERVER__RUN_MIGRATIONS: "true"
+      SYNC_TOKENSERVER__FXA_OAUTH_JWK__KTY: "RSA"
+      SYNC_TOKENSERVER__FXA_OAUTH_JWK__ALG: "RS256"
+      SYNC_TOKENSERVER__FXA_OAUTH_JWK__KID: "20190730-15e473fd"
+      SYNC_TOKENSERVER__FXA_OAUTH_JWK__FXA_CREATED_AT: "1564502400"
+      SYNC_TOKENSERVER__FXA_OAUTH_JWK__USE: "sig"
+      SYNC_TOKENSERVER__FXA_OAUTH_JWK__N: "15OpVGC7ws_SlU0gRbRh1Iwo8_gR8ElX2CDnbN5blKyXLg-ll0ogktoDXc-tDvTabRTxi7AXU0wWQ247odhHT47y5uz0GASYXdfPponynQ_xR9CpNn1eEL1gvDhQN9rfPIzfncl8FUi9V4WMd5f600QC81yDw9dX-Z8gdkru0aDaoEKF9-wU2TqrCNcQdiJCX9BISotjz_9cmGwKXFEekQNJWBeRQxH2bUmgwUK0HaqwW9WbYOs-zstNXXWFsgK9fbDQqQeGehXLZM4Cy5Mgl_iuSvnT3rLzPo2BmlxMLUvRqBx3_v8BTtwmNGA0v9O0FJS_mnDq0Iue0Dz8BssQCQ"
+      SYNC_TOKENSERVER__FXA_OAUTH_JWK__E: "AQAB"
+      TOKENSERVER_HOST: http://localhost:8000
+    entrypoint: >
+      /bin/sh -c "
+        sleep 28; python3 /app/tools/integration_tests/run.py 'http://localhost:8000#secret0'
+      "
--- a/docker-compose.e2e.spanner.yaml
+++ b/docker-compose.e2e.spanner.yaml
@ -1,42 +1,49 @@
 version: '3'
 services:
-    sync-db:
-    sync-db-setup:
-    tokenserver-db:
-    syncstorage-rs:
-        depends_on:
-          - sync-db-setup
-        # TODO: either syncstorage-rs should retry the db connection
-        # itself a few times or should include a wait-for-it.sh script
-        # inside its docker that would do this for us. Same (probably
-        # the latter solution) for server-syncstorage below
-        entrypoint: >
-          /bin/sh -c "
-            sleep 15;
-            /app/bin/syncstorage;
-          "
-    e2e-tests:
-        depends_on:
-          - mock-fxa-server
-          - syncstorage-rs
-        image: app:build
-        privileged: true
-        user: root
-        environment:
-          MOCK_FXA_SERVER_URL: http://mock-fxa-server:6000
-          SYNC_HOST: 0.0.0.0
-          SYNC_MASTER_SECRET: secret0
-          SYNC_DATABASE_URL: spanner://projects/test-project/instances/test-instance/databases/test-database
-          SYNC_SPANNER_EMULATOR_HOST: sync-db:9010
-          SYNC_TOKENSERVER__DATABASE_URL: mysql://test:test@tokenserver-db:3306/tokenserver
-          SYNC_TOKENSERVER__ENABLED: "true"
-          SYNC_TOKENSERVER__FXA_BROWSERID_AUDIENCE: "https://token.stage.mozaws.net/"
-          SYNC_TOKENSERVER__FXA_BROWSERID_ISSUER: "api-accounts.stage.mozaws.net"
-          SYNC_TOKENSERVER__FXA_EMAIL_DOMAIN: api-accounts.stage.mozaws.net
-          SYNC_TOKENSERVER__FXA_METRICS_HASH_SECRET: secret0
-          SYNC_TOKENSERVER__RUN_MIGRATIONS: "true"
-          TOKENSERVER_HOST: http://localhost:8000
-        entrypoint: >
-          /bin/sh -c "
-            sleep 28; python3 /app/tools/integration_tests/run.py 'http://localhost:8000#secret0'
-          "
+  sync-db:
+  sync-db-setup:
+  tokenserver-db:
+  syncstorage-rs:
+    depends_on:
+      - sync-db-setup
+    # TODO: either syncstorage-rs should retry the db connection
+    # itself a few times or should include a wait-for-it.sh script
+    # inside its docker that would do this for us. Same (probably
+    # the latter solution) for server-syncstorage below
+    entrypoint: >
+      /bin/sh -c "
+        sleep 15;
+        /app/bin/syncstorage;
+      "
+  e2e-tests:
+    depends_on:
+      - mock-fxa-server
+      - syncstorage-rs
+    image: app:build
+    privileged: true
+    user: root
+    environment:
+      MOCK_FXA_SERVER_URL: http://mock-fxa-server:6000
+      SYNC_HOST: 0.0.0.0
+      SYNC_MASTER_SECRET: secret0
+      SYNC_DATABASE_URL: spanner://projects/test-project/instances/test-instance/databases/test-database
+      SYNC_SPANNER_EMULATOR_HOST: sync-db:9010
+      SYNC_TOKENSERVER__DATABASE_URL: mysql://test:test@tokenserver-db:3306/tokenserver
+      SYNC_TOKENSERVER__ENABLED: "true"
+      SYNC_TOKENSERVER__FXA_BROWSERID_AUDIENCE: "https://token.stage.mozaws.net/"
+      SYNC_TOKENSERVER__FXA_BROWSERID_ISSUER: "api-accounts.stage.mozaws.net"
+      SYNC_TOKENSERVER__FXA_EMAIL_DOMAIN: api-accounts.stage.mozaws.net
+      SYNC_TOKENSERVER__FXA_METRICS_HASH_SECRET: secret0
+      SYNC_TOKENSERVER__RUN_MIGRATIONS: "true"
+      SYNC_TOKENSERVER__FXA_OAUTH_JWK__KTY: "RSA"
+      SYNC_TOKENSERVER__FXA_OAUTH_JWK__ALG: "RS256"
+      SYNC_TOKENSERVER__FXA_OAUTH_JWK__KID: "20190730-15e473fd"
+      SYNC_TOKENSERVER__FXA_OAUTH_JWK__FXA_CREATED_AT: "1564502400"
+      SYNC_TOKENSERVER__FXA_OAUTH_JWK__USE: "sig"
+      SYNC_TOKENSERVER__FXA_OAUTH_JWK__N: "15OpVGC7ws_SlU0gRbRh1Iwo8_gR8ElX2CDnbN5blKyXLg-ll0ogktoDXc-tDvTabRTxi7AXU0wWQ247odhHT47y5uz0GASYXdfPponynQ_xR9CpNn1eEL1gvDhQN9rfPIzfncl8FUi9V4WMd5f600QC81yDw9dX-Z8gdkru0aDaoEKF9-wU2TqrCNcQdiJCX9BISotjz_9cmGwKXFEekQNJWBeRQxH2bUmgwUK0HaqwW9WbYOs-zstNXXWFsgK9fbDQqQeGehXLZM4Cy5Mgl_iuSvnT3rLzPo2BmlxMLUvRqBx3_v8BTtwmNGA0v9O0FJS_mnDq0Iue0Dz8BssQCQ"
+      SYNC_TOKENSERVER__FXA_OAUTH_JWK__E: "AQAB"
+      TOKENSERVER_HOST: http://localhost:8000
+    entrypoint: >
+      /bin/sh -c "
+        sleep 28; python3 /app/tools/integration_tests/run.py 'http://localhost:8000#secret0'
+      "
--- a/syncstorage/src/tokenserver/auth/oauth.rs
+++ b/syncstorage/src/tokenserver/auth/oauth.rs
@ -4,6 +4,7 @@ use futures::TryFutureExt;
 use pyo3::{
    prelude::{Py, PyAny, PyErr, PyModule, Python},
    types::{IntoPyDict, PyString},
+    IntoPy,
 };
 use serde::{Deserialize, Serialize};
 use serde_json;
@ -26,25 +27,48 @@ pub struct VerifyOutput {

 /// The verifier used to verify OAuth tokens.
 #[derive(Clone)]
-pub struct RemoteVerifier {
+pub struct Verifier {
    // Note that we do not need to use an Arc here, since Py is already a reference-counted
    // pointer
    inner: Py<PyAny>,
    timeout: u64,
+    jwk_is_cached: bool,
 }

-impl RemoteVerifier {
+impl Verifier {
    const FILENAME: &'static str = "verify.py";
 }

-impl TryFrom<&Settings> for RemoteVerifier {
+impl TryFrom<&Settings> for Verifier {
    type Error = Error;

    fn try_from(settings: &Settings) -> Result<Self, Error> {
        let inner: Py<PyAny> = Python::with_gil::<_, Result<Py<PyAny>, PyErr>>(|py| {
            let code = include_str!("verify.py");
            let module = PyModule::from_code(py, code, Self::FILENAME, Self::FILENAME)?;
-            let kwargs = [("server_url", &settings.fxa_oauth_server_url)].into_py_dict(py);
+            let kwargs = {
+                let dict = [("server_url", &settings.fxa_oauth_server_url)].into_py_dict(py);
+                let jwks = settings
+                    .fxa_oauth_jwk
+                    .as_ref()
+                    .map(|jwk| {
+                        let dict = [
+                            ("kty", &jwk.kty),
+                            ("alg", &jwk.alg),
+                            ("kid", &jwk.kid),
+                            ("use", &jwk.use_of_key),
+                            ("n", &jwk.n),
+                            ("e", &jwk.e),
+                        ]
+                        .into_py_dict(py);
+                        dict.set_item("fxa-createdAt", jwk.fxa_created_at).unwrap();
+
+                        [dict]
+                    })
+                    .into_py(py);
+                dict.set_item("jwks", jwks).unwrap();
+                dict
+            };
            let object: Py<PyAny> = module
                .getattr("FxaOAuthClient")?
                .call((), Some(kwargs))
@ -61,20 +85,23 @@ impl TryFrom<&Settings> for RemoteVerifier {
        Ok(Self {
            inner,
            timeout: settings.fxa_oauth_request_timeout,
+            jwk_is_cached: settings.fxa_oauth_jwk.is_some(),
        })
    }
 }

 #[async_trait]
-impl VerifyToken for RemoteVerifier {
+impl VerifyToken for Verifier {
    type Output = VerifyOutput;

    /// Verifies an OAuth token. Returns `VerifyOutput` for valid tokens and a `TokenserverError`
    /// for invalid tokens.
    async fn verify(&self, token: String) -> Result<VerifyOutput, TokenserverError> {
-        let verifier = self.clone();
-
-        let fut = task::spawn_blocking(move || {
+        // We don't want to move `self` into the body of the closure here because we'd need to
+        // clone it. Cloning it is only necessary if we need to verify the token remotely via FxA,
+        // since that would require passing `self` to a separate thread. Passing &Self to a closure
+        // gives us the flexibility to clone only when necessary.
+        let verify_inner = |verifier: &Self| {
            let maybe_verify_output_string = Python::with_gil(|py| {
                let client = verifier.inner.as_ref(py);
                // `client.verify_token(token)`
@ -112,31 +139,41 @@ impl VerifyToken for RemoteVerifier {
                    ..TokenserverError::invalid_credentials("Unauthorized")
                }),
            }
-        })
-        .map_err(|err| {
-            let context = if err.is_cancelled() {
-                "Tokenserver threadpool operation cancelled"
-            } else if err.is_panic() {
-                "Tokenserver threadpool operation panicked"
-            } else {
-                "Tokenserver threadpool operation failed for unknown reason"
-            };
+        };

-            TokenserverError {
-                context: context.to_owned(),
-                ..TokenserverError::internal_error()
-            }
-        });
+        if self.jwk_is_cached {
+            verify_inner(self)
+        } else {
+            let verifier = self.clone();

-        // The PyFxA OAuth client does not offer a way to set a request timeout, so we set one here
-        // by timing out the future if the verification process blocks this thread for longer
-        // than the specified number of seconds.
-        time::timeout(Duration::from_secs(self.timeout), fut)
-            .await
-            .map_err(|_| TokenserverError {
-                context: "OAuth verification timeout".to_owned(),
-                ..TokenserverError::resource_unavailable()
-            })?
-            .map_err(|_| TokenserverError::resource_unavailable())?
+            // If the JWK is not cached, PyFxA will make a request to FxA to retrieve it, blocking
+            // this thread. To improve performance, we make the request on a thread in a threadpool
+            // specifically used for blocking operations.
+            let fut = task::spawn_blocking(move || verify_inner(&verifier)).map_err(|err| {
+                let context = if err.is_cancelled() {
+                    "Tokenserver threadpool operation cancelled"
+                } else if err.is_panic() {
+                    "Tokenserver threadpool operation panicked"
+                } else {
+                    "Tokenserver threadpool operation failed for unknown reason"
+                };
+
+                TokenserverError {
+                    context: context.to_owned(),
+                    ..TokenserverError::internal_error()
+                }
+            });
+
+            // The PyFxA OAuth client does not offer a way to set a request timeout, so we set one here
+            // by timing out the future if the verification process blocks this thread for longer
+            // than the specified number of seconds.
+            time::timeout(Duration::from_secs(self.timeout), fut)
+                .await
+                .map_err(|_| TokenserverError {
+                    context: "OAuth verification timeout".to_owned(),
+                    ..TokenserverError::resource_unavailable()
+                })?
+                .map_err(|_| TokenserverError::resource_unavailable())?
+        }
    }
 }
--- a/syncstorage/src/tokenserver/auth/verify.py
+++ b/syncstorage/src/tokenserver/auth/verify.py
@ -6,8 +6,8 @@ DEFAULT_OAUTH_SCOPE = 'https://identity.mozilla.com/apps/oldsync'


 class FxaOAuthClient:
-    def __init__(self, server_url=None):
-        self._client = Client(server_url=server_url)
+    def __init__(self, server_url=None, jwks=None):
+        self._client = Client(server_url=server_url, jwks=jwks)

    def verify_token(self, token):
        try:
--- a/syncstorage/src/tokenserver/mod.rs
+++ b/syncstorage/src/tokenserver/mod.rs
@ -41,7 +41,7 @@ pub struct ServerState {
 impl ServerState {
    pub fn from_settings(settings: &Settings, metrics: StatsdClient) -> Result<Self, ApiError> {
        let oauth_verifier = Box::new(
-            oauth::RemoteVerifier::try_from(settings)
+            oauth::Verifier::try_from(settings)
                .expect("failed to create Tokenserver OAuth verifier"),
        );
        let browserid_verifier = Box::new(
--- a/syncstorage/src/tokenserver/settings.rs
+++ b/syncstorage/src/tokenserver/settings.rs
@ -27,6 +27,10 @@ pub struct Settings {
    pub fxa_oauth_server_url: String,
    /// The timeout to be used when making requests to the FxA OAuth verification server.
    pub fxa_oauth_request_timeout: u64,
+    /// The JWK to be used to verify OAuth tokens. Passing a JWK to the PyFxA Python library
+    /// prevents it from making an external API call to FxA to get the JWK, yielding substantial
+    /// performance benefits.
+    pub fxa_oauth_jwk: Option<Jwk>,
    /// The issuer expected in the BrowserID verification response.
    pub fxa_browserid_issuer: String,
    /// The audience to be sent to the FxA BrowserID verification server.
@ -50,6 +54,18 @@ pub struct Settings {
    pub run_migrations: bool,
 }

+#[derive(Clone, Debug, Deserialize)]
+pub struct Jwk {
+    pub kty: String,
+    pub alg: String,
+    pub kid: String,
+    pub fxa_created_at: u64,
+    #[serde(rename = "use")]
+    pub use_of_key: String,
+    pub n: String,
+    pub e: String,
+}
+
 impl Default for Settings {
    fn default() -> Settings {
        Settings {
@ -62,6 +78,7 @@ impl Default for Settings {
            fxa_metrics_hash_secret: "secret".to_owned(),
            fxa_oauth_server_url: "https://oauth.stage.mozaws.net".to_owned(),
            fxa_oauth_request_timeout: 10,
+            fxa_oauth_jwk: None,
            fxa_browserid_audience: "https://token.stage.mozaws.net".to_owned(),
            fxa_browserid_issuer: "api-accounts.stage.mozaws.net".to_owned(),
            fxa_browserid_server_url: "https://verifier.stage.mozaws.net/v2".to_owned(),