From 012b2aae3a1a6a90ec19dcafaa549cd96c3a9413 Mon Sep 17 00:00:00 2001 From: Serhii Volovyk Date: Thu, 26 Oct 2023 17:14:39 +0300 Subject: [PATCH] remove oidc providers check from signing nodes (#334) * Remove OIDC providers check from signing nodes --- DEPLOY.md | 15 --------- infra/main.tf | 12 ++----- infra/modules/signer/main.tf | 9 ----- infra/modules/signer/variables.tf | 4 --- infra/partner/main.tf | 12 ++----- infra/partner/variables.tf | 4 --- infra/terraform-dev.tfvars | 1 - infra/variables.tf | 4 --- integration-tests/src/env/containers.rs | 10 ------ integration-tests/src/env/local.rs | 10 ------ mpc-recovery/src/leader_node/mod.rs | 6 ++-- mpc-recovery/src/lib.rs | 31 +---------------- mpc-recovery/src/oauth.rs | 45 +++++++++++++++++++++---- mpc-recovery/src/sign_node/mod.rs | 9 ++--- 14 files changed, 48 insertions(+), 124 deletions(-) diff --git a/DEPLOY.md b/DEPLOY.md index 7de471b89..f20d37dd2 100644 --- a/DEPLOY.md +++ b/DEPLOY.md @@ -63,20 +63,6 @@ You also need to grab the AES cipher key that was printed after `Cipher 0:`; it Save it to GCP Secret Manager under the name of your choosing (e.g. `mpc-recovery-cipher-prod`). This name will be referred to as `GCP_CIPHER_SECRET_ID`. -### OIDC Providers -Create one more secret secret under the name of your choosing (e.g. `mpc-recovery-oidc-providers-prod`). This name will be referred to as `GCP_OIDC_PROVIDERS_SECRET_ID`. - -Populate it with whatever your Pagoda point of contact told you to use, it should look similar to below: - -``` -[ - { - issuer = "https://securetoken.google.com/near-fastauth-prod", - audience = "near-fastauth-prod" - } -] -``` - ## Building Docker Image Build the mpc-recovery docker image from this folder and make sure to tag it for convenience: @@ -92,7 +78,6 @@ Go to `infra/partner` and copy `template.tfvars` as `prod.tfvars`. Edit `prod.tf * Set `env` to `prod` * Set `project` to `` * Set `node_id` to whatever your point of contact with Pagoda has given you (ask them if they did not). It is very important you use this specific ID for your node's configuration -* Set `oidc_providers_secret_id` to `` * Set `cipher_key_secret_id` to `` * Set `sk_share_secret_id` to `` diff --git a/infra/main.tf b/infra/main.tf index c0030a38f..57a0b69a5 100644 --- a/infra/main.tf +++ b/infra/main.tf @@ -86,12 +86,6 @@ resource "google_secret_manager_secret_iam_member" "secret_share_secret_access" member = "serviceAccount:${google_service_account.service_account.email}" } -resource "google_secret_manager_secret_iam_member" "oidc_providers_secret_access" { - secret_id = var.oidc_providers_secret_id - role = "roles/secretmanager.secretAccessor" - member = "serviceAccount:${google_service_account.service_account.email}" -} - resource "google_secret_manager_secret_iam_member" "account_creator_secret_access" { secret_id = var.account_creator_sk_secret_id role = "roles/secretmanager.secretAccessor" @@ -120,16 +114,14 @@ module "signer" { node_id = count.index - oidc_providers_secret_id = var.oidc_providers_secret_id - cipher_key_secret_id = var.signer_configs[count.index].cipher_key_secret_id - sk_share_secret_id = var.signer_configs[count.index].sk_share_secret_id + cipher_key_secret_id = var.signer_configs[count.index].cipher_key_secret_id + sk_share_secret_id = var.signer_configs[count.index].sk_share_secret_id jwt_signature_pk_url = var.jwt_signature_pk_url depends_on = [ google_secret_manager_secret_iam_member.cipher_key_secret_access, google_secret_manager_secret_iam_member.secret_share_secret_access, - google_secret_manager_secret_iam_member.oidc_providers_secret_access ] } diff --git a/infra/modules/signer/main.tf b/infra/modules/signer/main.tf index de1b6660e..e22c36158 100644 --- a/infra/modules/signer/main.tf +++ b/infra/modules/signer/main.tf @@ -49,15 +49,6 @@ resource "google_cloud_run_v2_service" "signer" { } } } - env { - name = "OIDC_PROVIDERS" - value_source { - secret_key_ref { - secret = var.oidc_providers_secret_id - version = "latest" - } - } - } env { name = "MPC_RECOVERY_JWT_SIGNATURE_PK_URL" value = var.jwt_signature_pk_url diff --git a/infra/modules/signer/variables.tf b/infra/modules/signer/variables.tf index f4093963d..5c7e2670d 100644 --- a/infra/modules/signer/variables.tf +++ b/infra/modules/signer/variables.tf @@ -29,10 +29,6 @@ variable "sk_share_secret_id" { type = string } -variable "oidc_providers_secret_id" { - type = string -} - variable "jwt_signature_pk_url" { type = string } diff --git a/infra/partner/main.tf b/infra/partner/main.tf index 4f366585d..f42377d78 100644 --- a/infra/partner/main.tf +++ b/infra/partner/main.tf @@ -62,12 +62,6 @@ resource "google_secret_manager_secret_iam_member" "secret_share_secret_access" member = "serviceAccount:${google_service_account.service_account.email}" } -resource "google_secret_manager_secret_iam_member" "oidc_providers_secret_access" { - secret_id = var.oidc_providers_secret_id - role = "roles/secretmanager.secretAccessor" - member = "serviceAccount:${google_service_account.service_account.email}" -} - /* * Create a partner signer node */ @@ -83,15 +77,13 @@ module "signer" { node_id = var.node_id - cipher_key_secret_id = var.cipher_key_secret_id - sk_share_secret_id = var.sk_share_secret_id - oidc_providers_secret_id = var.oidc_providers_secret_id + cipher_key_secret_id = var.cipher_key_secret_id + sk_share_secret_id = var.sk_share_secret_id jwt_signature_pk_url = var.jwt_signature_pk_url depends_on = [ google_secret_manager_secret_iam_member.cipher_key_secret_access, google_secret_manager_secret_iam_member.secret_share_secret_access, - google_secret_manager_secret_iam_member.oidc_providers_secret_access ] } diff --git a/infra/partner/variables.tf b/infra/partner/variables.tf index d314e83b1..8eaee70c4 100644 --- a/infra/partner/variables.tf +++ b/infra/partner/variables.tf @@ -29,10 +29,6 @@ variable "sk_share_secret_id" { type = string } -variable "oidc_providers_secret_id" { - type = string -} - variable "jwt_signature_pk_url" { type = string } diff --git a/infra/terraform-dev.tfvars b/infra/terraform-dev.tfvars index f0899093b..f22843b8a 100644 --- a/infra/terraform-dev.tfvars +++ b/infra/terraform-dev.tfvars @@ -4,7 +4,6 @@ docker_image = "us-east1-docker.pkg.dev/pagoda-discovery-platform-dev/mpc-recove account_creator_id = "mpc-recovery-dev-creator.testnet" account_creator_sk_secret_id = "mpc-recovery-account-creator-sk-dev" -oidc_providers_secret_id = "mpc-allowed-oidc-providers-dev" fast_auth_partners_secret_id = "mpc-fast-auth-partners-dev" signer_configs = [ { diff --git a/infra/variables.tf b/infra/variables.tf index fc2aeff87..9d5a8bae4 100644 --- a/infra/variables.tf +++ b/infra/variables.tf @@ -39,10 +39,6 @@ variable "account_creator_sk_secret_id" { type = string } -variable "oidc_providers_secret_id" { - type = string -} - variable "fast_auth_partners_secret_id" { type = string } diff --git a/integration-tests/src/env/containers.rs b/integration-tests/src/env/containers.rs index 29ad7827f..b03f9b08f 100644 --- a/integration-tests/src/env/containers.rs +++ b/integration-tests/src/env/containers.rs @@ -524,16 +524,6 @@ impl SignerNode<'_> { web_port: Self::CONTAINER_PORT, sk_share: Some(serde_json::to_string(&sk_share)?), cipher_key: Some(hex::encode(cipher_key)), - oidc_providers_filepath: None, - oidc_providers: Some( - serde_json::json!([ - { - "issuer": ctx.issuer, - "audience": ctx.audience_id, - }, - ]) - .to_string(), - ), gcp_project_id: ctx.gcp_project_id.clone(), gcp_datastore_url: Some(ctx.datastore.address.clone()), jwt_signature_pk_url: ctx.oidc_provider.jwt_pk_url.clone(), diff --git a/integration-tests/src/env/local.rs b/integration-tests/src/env/local.rs index 57c5ac7c1..acef7eeb4 100644 --- a/integration-tests/src/env/local.rs +++ b/integration-tests/src/env/local.rs @@ -37,16 +37,6 @@ impl SignerNode { web_port, sk_share: Some(serde_json::to_string(&sk_share)?), cipher_key: Some(hex::encode(cipher_key)), - oidc_providers_filepath: None, - oidc_providers: Some( - serde_json::json!([ - { - "issuer": ctx.issuer, - "audience": ctx.audience_id, - }, - ]) - .to_string(), - ), gcp_project_id: ctx.gcp_project_id.clone(), gcp_datastore_url: Some(ctx.datastore.local_address.clone()), jwt_signature_pk_url: ctx.oidc_provider.jwt_pk_local_url.clone(), diff --git a/mpc-recovery/src/leader_node/mod.rs b/mpc-recovery/src/leader_node/mod.rs index 87e0cb924..e7f8f7726 100644 --- a/mpc-recovery/src/leader_node/mod.rs +++ b/mpc-recovery/src/leader_node/mod.rs @@ -322,7 +322,7 @@ async fn process_user_credentials( ) -> Result { verify_oidc_token( &request.oidc_token, - &state.partners.oidc_providers(), + Some(&state.partners.oidc_providers()), &state.reqwest_client, &state.jwt_signature_pk_url, ) @@ -354,7 +354,7 @@ async fn process_new_account( let new_user_account_id = request.near_account_id; let oidc_token_claims = verify_oidc_token( &request.oidc_token, - &state.partners.oidc_providers(), + Some(&state.partners.oidc_providers()), &state.reqwest_client, &state.jwt_signature_pk_url, ) @@ -497,7 +497,7 @@ async fn process_sign( // Check OIDC token verify_oidc_token( &request.oidc_token, - &state.partners.oidc_providers(), + Some(&state.partners.oidc_providers()), &state.reqwest_client, &state.jwt_signature_pk_url, ) diff --git a/mpc-recovery/src/lib.rs b/mpc-recovery/src/lib.rs index 7ed1815aa..28c4b3ba3 100644 --- a/mpc-recovery/src/lib.rs +++ b/mpc-recovery/src/lib.rs @@ -18,7 +18,7 @@ use near_crypto::{InMemorySigner, SecretKey}; use near_fetch::signer::KeyRotatingSigner; use near_primitives::types::AccountId; -use crate::firewall::allowed::{OidcProviderList, PartnerList}; +use crate::firewall::allowed::PartnerList; use crate::gcp::GcpService; use crate::sign_node::migration; @@ -136,12 +136,6 @@ pub enum Cli { /// The web port for this server #[arg(long, env("MPC_RECOVERY_WEB_PORT"))] web_port: u16, - /// JSON list of related items to be used to verify OIDC tokens. - #[arg(long, env("OIDC_PROVIDERS"))] - oidc_providers: Option, - /// Filepath to a JSON list of related items to be used to verify OIDC tokens. - #[arg(long, value_parser, env("OIDC_PROVIDERS_FILEPATH"))] - oidc_providers_filepath: Option, /// GCP project ID #[arg(long, env("MPC_RECOVERY_GCP_PROJECT_ID"))] gcp_project_id: String, @@ -251,8 +245,6 @@ pub async fn run(cmd: Cli) -> anyhow::Result<()> { sk_share, cipher_key, web_port, - oidc_providers, - oidc_providers_filepath, gcp_project_id, gcp_datastore_url, jwt_signature_pk_url, @@ -268,16 +260,6 @@ pub async fn run(cmd: Cli) -> anyhow::Result<()> { .global(); let gcp_service = GcpService::new(env.clone(), gcp_project_id, gcp_datastore_url).await?; - let oidc_providers = OidcProviderList { - entries: load_entries( - &gcp_service, - &env, - node_id.to_string().as_str(), - oidc_providers, - oidc_providers_filepath, - ) - .await?, - }; let cipher_key = load_cipher_key(&gcp_service, &env, node_id, cipher_key).await?; let cipher_key = hex::decode(cipher_key)?; let cipher_key = GenericArray::::clone_from_slice(&cipher_key); @@ -294,7 +276,6 @@ pub async fn run(cmd: Cli) -> anyhow::Result<()> { node_key: sk_share, cipher, port: web_port, - oidc_providers, jwt_signature_pk_url, }; run_sign_node(config).await; @@ -502,8 +483,6 @@ impl Cli { web_port, cipher_key, sk_share, - oidc_providers, - oidc_providers_filepath, gcp_project_id, gcp_datastore_url, jwt_signature_pk_url, @@ -530,14 +509,6 @@ impl Cli { buf.push("--sk-share".to_string()); buf.push(share); } - if let Some(providers) = oidc_providers { - buf.push("--oidc-providers".to_string()); - buf.push(providers); - } - if let Some(providers_filepath) = oidc_providers_filepath { - buf.push("--oidc-providers-filepath".to_string()); - buf.push(providers_filepath.to_str().unwrap().to_string()); - } if let Some(gcp_datastore_url) = gcp_datastore_url { buf.push("--gcp-datastore-url".to_string()); buf.push(gcp_datastore_url); diff --git a/mpc-recovery/src/oauth.rs b/mpc-recovery/src/oauth.rs index e50ec2a9d..d69a1b93a 100644 --- a/mpc-recovery/src/oauth.rs +++ b/mpc-recovery/src/oauth.rs @@ -11,7 +11,7 @@ use crate::sign_node::oidc::OidcToken; // Firebase: https://firebase.google.com/docs/auth/admin/verify-id-tokens#verify_id_tokens_using_a_third-party_jwt_library pub async fn verify_oidc_token( token: &OidcToken, - oidc_providers: &OidcProviderList, + oidc_providers: Option<&OidcProviderList>, client: &reqwest::Client, jwt_signature_pk_url: &str, ) -> anyhow::Result { @@ -41,7 +41,7 @@ pub async fn verify_oidc_token( fn validate_jwt( token: &OidcToken, public_key: &[u8], - oidc_providers: &OidcProviderList, + oidc_providers: Option<&OidcProviderList>, ) -> anyhow::Result { tracing::info!( oidc_token = format!("{:.5}...", token), @@ -57,8 +57,12 @@ fn validate_jwt( .. } = &claims; - if !oidc_providers.contains(issuer, audience) { - anyhow::bail!("UnauthorizedTokenIssuerOrAudience: iss={issuer}, aud={audience}"); + // If no OIDC providers are specified in the allowlist, we allow any issuer and audience. + // Should be used in signing nodes only. + if let Some(oidc_providers) = oidc_providers { + if !oidc_providers.contains(issuer, audience) { + anyhow::bail!("UnauthorizedTokenIssuerOrAudience: iss={issuer}, aud={audience}"); + } } tracing::info!( @@ -145,11 +149,11 @@ mod tests { }; // Valid token and claims - validate_jwt(&token, &public_key_der, &oidc_providers).unwrap(); + validate_jwt(&token, &public_key_der, Some(&oidc_providers)).unwrap(); // Invalid public key let (invalid_public_key, _invalid_private_key) = get_rsa_pem_key_pair(); - match validate_jwt(&token, &invalid_public_key, &oidc_providers) { + match validate_jwt(&token, &invalid_public_key, Some(&oidc_providers)) { Ok(_) => panic!("Token validation should fail"), Err(e) => assert_eq!(e.to_string(), "InvalidSignature"), } @@ -169,12 +173,39 @@ mod tests { Ok(t) => OidcToken::new(t.as_str()), Err(e) => panic!("Failed to encode token: {}", e), }; - match validate_jwt(&token, &public_key_der, &oidc_providers) { + match validate_jwt(&token, &public_key_der, Some(&oidc_providers)) { Ok(_) => panic!("Token validation should fail on invalid issuer or audience"), Err(e) => assert_eq!(e.to_string(), "UnauthorizedTokenIssuerOrAudience: iss=unauthorized_issuer, aud=unauthorized_audience", "{:?}", e), } } + #[test] + fn test_validate_jwt_without_oidc() { + let (private_key_der, public_key_der): (Vec, Vec) = get_rsa_pem_key_pair(); + + let my_claims = IdTokenClaims { + iss: "test_issuer".to_string(), + sub: "test_subject".to_string(), + aud: "test_audience".to_string(), + exp: (Utc::now() + Duration::hours(1)).timestamp() as usize, + }; + + let token = match encode( + &Header::new(Algorithm::RS256), + &my_claims, + &EncodingKey::from_rsa_pem(&private_key_der).unwrap(), + ) { + Ok(t) => OidcToken::new(t.as_str()), + Err(e) => panic!("Failed to encode token: {}", e), + }; + + // Valid token and claims + match validate_jwt(&token, &public_key_der, None) { + Ok(_) => (), + Err(e) => panic!("Token validation should succeed: {}", e), + } + } + pub fn get_rsa_pem_key_pair() -> (Vec, Vec) { let mut rng = OsRng; let bits: usize = 2048; diff --git a/mpc-recovery/src/sign_node/mod.rs b/mpc-recovery/src/sign_node/mod.rs index 7f892a85a..d003da440 100644 --- a/mpc-recovery/src/sign_node/mod.rs +++ b/mpc-recovery/src/sign_node/mod.rs @@ -2,7 +2,6 @@ use self::aggregate_signer::{NodeInfo, Reveal, SignedCommitment, SigningState}; use self::oidc::OidcDigest; use self::user_credentials::EncryptedUserCredentials; use crate::error::{MpcError, SignNodeError}; -use crate::firewall::allowed::OidcProviderList; use crate::gcp::GcpService; use crate::msg::{AcceptNodePublicKeysRequest, PublicKeyNodeRequest, SignNodeRequest}; use crate::oauth::verify_oidc_token; @@ -42,7 +41,6 @@ pub struct Config { pub node_key: ExpandedKeyPair, pub cipher: Aes256Gcm, pub port: u16, - pub oidc_providers: OidcProviderList, pub jwt_signature_pk_url: String, } @@ -54,7 +52,6 @@ pub async fn run(config: Config) { node_key, cipher, port, - oidc_providers, jwt_signature_pk_url, } = config; let our_index = usize::try_from(our_index).expect("This index is way to big"); @@ -71,7 +68,6 @@ pub async fn run(config: Config) { cipher, signing_state: SigningState::new(), node_info: NodeInfo::new(our_index, pk_set.map(|set| set.public_keys)), - oidc_providers, jwt_signature_pk_url, }); @@ -107,7 +103,6 @@ struct SignNodeState { cipher: Aes256Gcm, signing_state: SigningState, node_info: NodeInfo, - oidc_providers: OidcProviderList, jwt_signature_pk_url: String, } @@ -218,7 +213,7 @@ async fn process_commit( // Check OIDC Token let oidc_token_claims = verify_oidc_token( &request.oidc_token, - &state.oidc_providers, + None, &state.reqwest_client, &state.jwt_signature_pk_url, ) @@ -398,7 +393,7 @@ async fn process_public_key( // Check OIDC Token let oidc_token_claims = verify_oidc_token( &request.oidc_token, - &state.oidc_providers, + None, &state.reqwest_client, &state.jwt_signature_pk_url, )