From d62db9f08e3e081b0b6584904d31f92ce6db273c Mon Sep 17 00:00:00 2001 From: Ethan Donowitz <8703826+ethowitz@users.noreply.github.com> Date: Thu, 12 May 2022 13:19:43 -0400 Subject: [PATCH] feat: support setting JWK for Tokenserver OAuth verification (#1307) --- docker-compose.e2e.mysql.yaml | 85 +++++++++-------- docker-compose.e2e.spanner.yaml | 87 ++++++++++-------- syncstorage/src/tokenserver/auth/oauth.rs | 101 ++++++++++++++------- syncstorage/src/tokenserver/auth/verify.py | 4 +- syncstorage/src/tokenserver/mod.rs | 2 +- syncstorage/src/tokenserver/settings.rs | 17 ++++ 6 files changed, 182 insertions(+), 114 deletions(-) diff --git a/docker-compose.e2e.mysql.yaml b/docker-compose.e2e.mysql.yaml index 2f256d93..8ed81498 100644 --- 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' + " diff --git a/docker-compose.e2e.spanner.yaml b/docker-compose.e2e.spanner.yaml index d7dca5e5..13fcdf40 100644 --- 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' + " diff --git a/syncstorage/src/tokenserver/auth/oauth.rs b/syncstorage/src/tokenserver/auth/oauth.rs index 78f2e335..052d1500 100644 --- 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, 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 { let inner: Py = Python::with_gil::<_, Result, 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 = 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 { - 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())? + } } } diff --git a/syncstorage/src/tokenserver/auth/verify.py b/syncstorage/src/tokenserver/auth/verify.py index ff32a51f..44d374f1 100644 --- 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: diff --git a/syncstorage/src/tokenserver/mod.rs b/syncstorage/src/tokenserver/mod.rs index 1a2a4b85..872c3036 100644 --- 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 { 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( diff --git a/syncstorage/src/tokenserver/settings.rs b/syncstorage/src/tokenserver/settings.rs index aa5459ac..981f8d69 100644 --- 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, /// 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(),