From 5e221bf21902d69810a3db3bfeaf5894f02d2ec0 Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Tue, 9 Jul 2019 13:37:18 -0600 Subject: [PATCH] Make config_api more robust (#4980) * Make config_api more robust * Add test and update store instruction --- install/src/command.rs | 1 + programs/config_api/src/config_instruction.rs | 3 +- programs/config_api/src/config_processor.rs | 76 +++++++++++++++---- 3 files changed, 65 insertions(+), 15 deletions(-) diff --git a/install/src/command.rs b/install/src/command.rs index fa1a113a28..d17f515c0b 100644 --- a/install/src/command.rs +++ b/install/src/command.rs @@ -226,6 +226,7 @@ fn store_update_manifest( let signers = [from_keypair, update_manifest_keypair]; let instruction = config_instruction::store::( &update_manifest_keypair.pubkey(), + true, // update_manifest_keypair is signer vec![], // additional keys update_manifest, ); diff --git a/programs/config_api/src/config_instruction.rs b/programs/config_api/src/config_instruction.rs index 34cba51e55..faeeafb83b 100644 --- a/programs/config_api/src/config_instruction.rs +++ b/programs/config_api/src/config_instruction.rs @@ -44,10 +44,11 @@ pub fn create_account( /// Store new data in a configuration account pub fn store( config_account_pubkey: &Pubkey, + is_config_signer: bool, keys: Vec<(Pubkey, bool)>, data: &T, ) -> Instruction { - let mut account_metas = vec![AccountMeta::new(*config_account_pubkey, true)]; + let mut account_metas = vec![AccountMeta::new(*config_account_pubkey, is_config_signer)]; for (signer_pubkey, _) in keys.iter().filter(|(_, is_signer)| *is_signer) { account_metas.push(AccountMeta::new(*signer_pubkey, true)); } diff --git a/programs/config_api/src/config_processor.rs b/programs/config_api/src/config_processor.rs index ea99790a51..3a7e5e46ac 100644 --- a/programs/config_api/src/config_processor.rs +++ b/programs/config_api/src/config_processor.rs @@ -12,25 +12,30 @@ pub fn process_instruction( keyed_accounts: &mut [KeyedAccount], data: &[u8], ) -> Result<(), InstructionError> { - if keyed_accounts[0].signer_key().is_none() { - error!("account[0].signer_key().is_none()"); - Err(InstructionError::MissingRequiredSignature)?; - } - - let key_list: ConfigKeys = deserialize(data).unwrap(); + let key_list: ConfigKeys = deserialize(data).map_err(|err| { + error!("Invalid ConfigKeys data: {:?} {:?}", data, err); + InstructionError::InvalidInstructionData + })?; + let mut counter = 0; for (i, (signer, _)) in key_list .keys .iter() .filter(|(_, is_signer)| *is_signer) .enumerate() { + counter += 1; let account_index = i + 1; - let signer_account = keyed_accounts[account_index].signer_key(); + let signer_account = keyed_accounts.get(account_index); if signer_account.is_none() { - error!("account[{:?}].signer_key().is_none()", account_index); + error!("account {:?} is not in account list", signer); Err(InstructionError::MissingRequiredSignature)?; } - if signer_account.unwrap() != signer { + let signer_key = signer_account.unwrap().signer_key(); + if signer_key.is_none() { + error!("account {:?} signer_key().is_none()", signer); + Err(InstructionError::MissingRequiredSignature)?; + } + if signer_key.unwrap() != signer { error!( "account[{:?}].signer_key() does not match Config data)", account_index @@ -38,6 +43,13 @@ pub fn process_instruction( Err(InstructionError::MissingRequiredSignature)?; } } + if counter == 0 { + // If Config data does not specify any signers, Config account keypair must be a signer + if keyed_accounts[0].signer_key().is_none() { + error!("account[0].signer_key().is_none()"); + Err(InstructionError::MissingRequiredSignature)?; + } + } if keyed_accounts[0].account.data.len() < data.len() { error!("instruction data too large"); @@ -138,7 +150,7 @@ mod tests { let my_config = MyConfig::new(42); - let instruction = config_instruction::store(&config_pubkey, keys.clone(), &my_config); + let instruction = config_instruction::store(&config_pubkey, true, keys.clone(), &my_config); let message = Message::new_with_payer(vec![instruction], Some(&mint_keypair.pubkey())); bank_client @@ -166,7 +178,7 @@ mod tests { let my_config = MyConfig::new(42); - let mut instruction = config_instruction::store(&config_pubkey, vec![], &my_config); + let mut instruction = config_instruction::store(&config_pubkey, true, vec![], &my_config); instruction.data = vec![0; 123]; // <-- Replace data with a vector that's too large let message = Message::new(vec![instruction]); bank_client @@ -188,7 +200,8 @@ mod tests { let transfer_instruction = system_instruction::transfer(&system_pubkey, &Pubkey::new_rand(), 42); let my_config = MyConfig::new(42); - let mut store_instruction = config_instruction::store(&config_pubkey, vec![], &my_config); + let mut store_instruction = + config_instruction::store(&config_pubkey, true, vec![], &my_config); store_instruction.accounts[0].is_signer = false; // <----- not a signer let message = Message::new(vec![transfer_instruction, store_instruction]); @@ -215,7 +228,7 @@ mod tests { let my_config = MyConfig::new(42); - let instruction = config_instruction::store(&config_pubkey, keys.clone(), &my_config); + let instruction = config_instruction::store(&config_pubkey, true, keys.clone(), &my_config); let message = Message::new_with_payer(vec![instruction], Some(&mint_keypair.pubkey())); bank_client @@ -239,6 +252,41 @@ mod tests { ); } + #[test] + fn test_process_store_without_config_signer() { + solana_logger::setup(); + let (bank, mint_keypair) = create_bank(10_000); + let pubkey = Pubkey::new_rand(); + let signer0 = Keypair::new(); + let keys = vec![(pubkey, false), (signer0.pubkey(), true)]; + let (bank_client, config_keypair) = + create_config_account(bank, &mint_keypair, keys.clone()); + let config_pubkey = config_keypair.pubkey(); + + let my_config = MyConfig::new(42); + + let instruction = + config_instruction::store(&config_pubkey, false, keys.clone(), &my_config); + let message = Message::new_with_payer(vec![instruction], Some(&mint_keypair.pubkey())); + + bank_client + .send_message(&[&mint_keypair, &signer0], message) + .unwrap(); + + let config_account_data = bank_client + .get_account_data(&config_pubkey) + .unwrap() + .unwrap(); + let meta_length = ConfigKeys::serialized_size(keys.clone()); + let meta_data: ConfigKeys = deserialize(&config_account_data[0..meta_length]).unwrap(); + assert_eq!(meta_data.keys, keys); + let config_account_data = &config_account_data[meta_length..config_account_data.len()]; + assert_eq!( + my_config, + MyConfig::deserialize(&config_account_data).unwrap() + ); + } + #[test] fn test_process_store_with_bad_additional_signer() { solana_logger::setup(); @@ -253,7 +301,7 @@ mod tests { let my_config = MyConfig::new(42); // Config-data pubkey doesn't match signer - let instruction = config_instruction::store(&config_pubkey, keys.clone(), &my_config); + let instruction = config_instruction::store(&config_pubkey, true, keys.clone(), &my_config); let mut message = Message::new_with_payer(vec![instruction.clone()], Some(&mint_keypair.pubkey())); message.account_keys[2] = signer1.pubkey();