From f84c46c3b3cad3ea4908ab44c361f637c7fcdb09 Mon Sep 17 00:00:00 2001 From: Aashna Jena Date: Fri, 10 Nov 2023 12:13:22 +0000 Subject: Revert "[rkpd_client] Add Error type to rkpd_client" This reverts commit 2dbabf3b7232781d61faba97865a061b3bae5fd9. Reason for revert: DroidMonitor revert for b/310139666 Bug: 310139666 Change-Id: I1213940cc4e3112038c1cc66f5a218a9378d6b0f --- keystore2/src/error.rs | 74 -------------------- keystore2/src/remote_provisioning.rs | 3 +- keystore2/src/rkpd_client.rs | 128 ++++++++++++++++++++--------------- keystore2/src/security_level.rs | 11 +-- 4 files changed, 76 insertions(+), 140 deletions(-) (limited to 'keystore2') diff --git a/keystore2/src/error.rs b/keystore2/src/error.rs index 8797e232..1a048b6b 100644 --- a/keystore2/src/error.rs +++ b/keystore2/src/error.rs @@ -27,9 +27,7 @@ //! Keystore functions should use `anyhow::Result` to return error conditions, and context should //! be added every time an error is forwarded. -use crate::rkpd_client::Error as RkpdError; pub use android_hardware_security_keymint::aidl::android::hardware::security::keymint::ErrorCode::ErrorCode; -use android_security_rkp_aidl::aidl::android::security::rkp::IGetKeyCallback::ErrorCode::ErrorCode as GetKeyErrorCode; pub use android_system_keystore2::aidl::android::system::keystore2::ResponseCode::ResponseCode; use android_system_keystore2::binder::{ ExceptionCode, Result as BinderResult, Status as BinderStatus, StatusCode, @@ -68,48 +66,6 @@ impl Error { } } -impl From for Error { - fn from(e: RkpdError) -> Self { - match e { - RkpdError::RequestCancelled | RkpdError::GetRegistrationFailed => { - Error::Rc(ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR) - } - RkpdError::GetKeyFailed(e) => { - let response_code = match e { - GetKeyErrorCode::ERROR_UNKNOWN => ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR, - GetKeyErrorCode::ERROR_PERMANENT => ResponseCode::OUT_OF_KEYS_PERMANENT_ERROR, - GetKeyErrorCode::ERROR_PENDING_INTERNET_CONNECTIVITY => { - ResponseCode::OUT_OF_KEYS_PENDING_INTERNET_CONNECTIVITY - } - GetKeyErrorCode::ERROR_REQUIRES_SECURITY_PATCH => { - ResponseCode::OUT_OF_KEYS_REQUIRES_SYSTEM_UPGRADE - } - _ => { - log::error!("Unexpected get key error from rkpd: {e:?}"); - ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR - } - }; - Error::Rc(response_code) - } - RkpdError::StoreUpgradedKeyFailed | RkpdError::Timeout => { - Error::Rc(ResponseCode::SYSTEM_ERROR) - } - RkpdError::BinderTransaction(s) => Error::BinderTransaction(s), - } - } -} - -/// Maps an `rkpd_client::Error` that is wrapped with an `anyhow::Error` to a keystore2 `Error`. -pub fn wrapped_rkpd_error_to_ks_error(e: &anyhow::Error) -> Error { - match e.downcast_ref::() { - Some(e) => Error::from(*e), - None => { - log::error!("Failed to downcast the anyhow::Error to rkpd_client::Error: {e:?}"); - Error::Rc(ResponseCode::SYSTEM_ERROR) - } - } -} - /// Helper function to map the binder status we get from calls into KeyMint /// to a Keystore Error. We don't create an anyhow error here to make /// it easier to evaluate KeyMint errors, which we must do in some cases, e.g., @@ -453,34 +409,4 @@ pub mod tests { expected_error_string ); } - - #[test] - fn rkpd_error_is_in_sync_with_response_code() { - let error_mapping = [ - (RkpdError::RequestCancelled, ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR), - (RkpdError::GetRegistrationFailed, ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR), - ( - RkpdError::GetKeyFailed(GetKeyErrorCode::ERROR_UNKNOWN), - ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR, - ), - ( - RkpdError::GetKeyFailed(GetKeyErrorCode::ERROR_PERMANENT), - ResponseCode::OUT_OF_KEYS_PERMANENT_ERROR, - ), - ( - RkpdError::GetKeyFailed(GetKeyErrorCode::ERROR_PENDING_INTERNET_CONNECTIVITY), - ResponseCode::OUT_OF_KEYS_PENDING_INTERNET_CONNECTIVITY, - ), - ( - RkpdError::GetKeyFailed(GetKeyErrorCode::ERROR_REQUIRES_SECURITY_PATCH), - ResponseCode::OUT_OF_KEYS_REQUIRES_SYSTEM_UPGRADE, - ), - (RkpdError::StoreUpgradedKeyFailed, ResponseCode::SYSTEM_ERROR), - (RkpdError::Timeout, ResponseCode::SYSTEM_ERROR), - ]; - for (rkpd_error, expected_response_code) in error_mapping { - let e: Error = rkpd_error.into(); - assert_eq!(e, Error::Rc(expected_response_code)); - } - } } // mod tests diff --git a/keystore2/src/remote_provisioning.rs b/keystore2/src/remote_provisioning.rs index 14c61fb0..3f7833ed 100644 --- a/keystore2/src/remote_provisioning.rs +++ b/keystore2/src/remote_provisioning.rs @@ -31,7 +31,6 @@ use anyhow::{Context, Result}; use keystore2_crypto::parse_subject_from_certificate; use crate::database::Uuid; -use crate::error::wrapped_rkpd_error_to_ks_error; use crate::globals::get_remotely_provisioned_component_name; use crate::ks_err; use crate::metrics_store::log_rkp_error_stats; @@ -103,7 +102,7 @@ impl RemProvState { Err(e) => { if self.is_rkp_only() { log::error!("Error occurred: {:?}", e); - return Err(wrapped_rkpd_error_to_ks_error(&e)).context(format!("{e:?}")); + return Err(e); } log::warn!("Error occurred: {:?}", e); log_rkp_error_stats( diff --git a/keystore2/src/rkpd_client.rs b/keystore2/src/rkpd_client.rs index 98688382..fe641506 100644 --- a/keystore2/src/rkpd_client.rs +++ b/keystore2/src/rkpd_client.rs @@ -14,6 +14,7 @@ //! Helper wrapper around RKPD interface. +use crate::error::{map_binder_status_code, Error, ResponseCode}; use android_security_rkp_aidl::aidl::android::security::rkp::{ IGetKeyCallback::BnGetKeyCallback, IGetKeyCallback::ErrorCode::ErrorCode as GetKeyErrorCode, IGetKeyCallback::IGetKeyCallback, IGetRegistrationCallback::BnGetRegistrationCallback, @@ -23,8 +24,8 @@ use android_security_rkp_aidl::aidl::android::security::rkp::{ IStoreUpgradedKeyCallback::IStoreUpgradedKeyCallback, RemotelyProvisionedKey::RemotelyProvisionedKey, }; +use android_security_rkp_aidl::binder::{BinderFeatures, Interface, Strong}; use anyhow::{Context, Result}; -use binder::{BinderFeatures, Interface, StatusCode, Strong}; use message_macro::source_location_msg; use std::sync::Mutex; use std::time::Duration; @@ -40,40 +41,6 @@ fn tokio_rt() -> tokio::runtime::Runtime { tokio::runtime::Builder::new_current_thread().enable_all().build().unwrap() } -/// Errors occurred during the interaction with RKPD. -#[derive(Debug, Clone, Copy, thiserror::Error, PartialEq, Eq)] -pub enum Error { - /// An RKPD request gets cancelled. - #[error("An RKPD request gets cancelled")] - RequestCancelled, - - /// Failed to get registration. - #[error("Failed to get registration")] - GetRegistrationFailed, - - /// Failed to get key. - #[error("Failed to get key: {0:?}")] - GetKeyFailed(GetKeyErrorCode), - - /// Failed to store upgraded key. - #[error("Failed to store upgraded key")] - StoreUpgradedKeyFailed, - - /// Timeout when waiting for a callback. - #[error("Timeout when waiting for a callback")] - Timeout, - - /// Wraps a Binder status code. - #[error("Binder transaction error {0:?}")] - BinderTransaction(StatusCode), -} - -impl From for Error { - fn from(s: StatusCode) -> Self { - Self::BinderTransaction(s) - } -} - /// Thread-safe channel for sending a value once and only once. If a value has /// already been send, subsequent calls to send will noop. struct SafeSender { @@ -120,17 +87,17 @@ impl IGetRegistrationCallback for GetRegistrationCallback { fn onCancel(&self) -> binder::Result<()> { log::warn!("IGetRegistrationCallback cancelled"); self.registration_tx.send( - Err(Error::RequestCancelled) + Err(Error::Rc(ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR)) .context(source_location_msg!("GetRegistrationCallback cancelled.")), ); Ok(()) } fn onError(&self, description: &str) -> binder::Result<()> { log::error!("IGetRegistrationCallback failed: '{description}'"); - self.registration_tx.send( - Err(Error::GetRegistrationFailed) - .context(source_location_msg!("GetRegistrationCallback failed: {:?}", description)), - ); + self.registration_tx + .send(Err(Error::Rc(ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR)).context( + source_location_msg!("GetRegistrationCallback failed: {:?}", description), + )); Ok(()) } } @@ -138,8 +105,7 @@ impl IGetRegistrationCallback for GetRegistrationCallback { /// Make a new connection to a IRegistration service. async fn get_rkpd_registration(rpc_name: &str) -> Result> { let remote_provisioning: Strong = - binder::get_interface("remote_provisioning") - .map_err(Error::from) + map_binder_status_code(binder::get_interface("remote_provisioning")) .context(source_location_msg!("Trying to connect to IRemoteProvisioning service."))?; let (tx, rx) = oneshot::channel(); @@ -150,7 +116,8 @@ async fn get_rkpd_registration(rpc_name: &str) -> Result Err(Error::Timeout).context(source_location_msg!("Waiting for RKPD: {:?}", e)), + Err(e) => Err(Error::Rc(ResponseCode::SYSTEM_ERROR)) + .context(source_location_msg!("Waiting for RKPD: {:?}", e)), Ok(v) => v.unwrap(), } } @@ -181,13 +148,28 @@ impl IGetKeyCallback for GetKeyCallback { fn onCancel(&self) -> binder::Result<()> { log::warn!("IGetKeyCallback cancelled"); self.key_tx.send( - Err(Error::RequestCancelled).context(source_location_msg!("GetKeyCallback cancelled.")), + Err(Error::Rc(ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR)) + .context(source_location_msg!("GetKeyCallback cancelled.")), ); Ok(()) } fn onError(&self, error: GetKeyErrorCode, description: &str) -> binder::Result<()> { log::error!("IGetKeyCallback failed: {description}"); - self.key_tx.send(Err(Error::GetKeyFailed(error)).context(source_location_msg!( + let rc = match error { + GetKeyErrorCode::ERROR_UNKNOWN => ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR, + GetKeyErrorCode::ERROR_PERMANENT => ResponseCode::OUT_OF_KEYS_PERMANENT_ERROR, + GetKeyErrorCode::ERROR_PENDING_INTERNET_CONNECTIVITY => { + ResponseCode::OUT_OF_KEYS_PENDING_INTERNET_CONNECTIVITY + } + GetKeyErrorCode::ERROR_REQUIRES_SECURITY_PATCH => { + ResponseCode::OUT_OF_KEYS_REQUIRES_SYSTEM_UPGRADE + } + _ => { + log::error!("Unexpected error from rkpd: {error:?}"); + ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR + } + }; + self.key_tx.send(Err(Error::Rc(rc)).context(source_location_msg!( "GetKeyCallback failed: {:?} {:?}", error, description @@ -213,7 +195,7 @@ async fn get_rkpd_attestation_key_from_registration_async( if let Err(e) = registration.cancelGetKey(&cb) { log::error!("IRegistration::cancelGetKey failed: {:?}", e); } - Err(Error::Timeout) + Err(Error::Rc(ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR)) .context(source_location_msg!("Waiting for RKPD key timed out: {:?}", e)) } Ok(v) => v.unwrap(), @@ -252,9 +234,9 @@ impl IStoreUpgradedKeyCallback for StoreUpgradedKeyCallback { } fn onError(&self, error: &str) -> binder::Result<()> { - log::error!("IStoreUpgradedKeyCallback failed: {error}"); + log::error!("IGetRegistrationCallback failed: {error}"); self.completer.send( - Err(Error::StoreUpgradedKeyFailed) + Err(Error::Rc(ResponseCode::SYSTEM_ERROR)) .context(source_location_msg!("Failed to store upgraded key: {:?}", error)), ); Ok(()) @@ -274,7 +256,7 @@ async fn store_rkpd_attestation_key_with_registration_async( .context(source_location_msg!("Failed to store upgraded blob with RKPD."))?; match timeout(RKPD_TIMEOUT, rx).await { - Err(e) => Err(Error::Timeout) + Err(e) => Err(Error::Rc(ResponseCode::SYSTEM_ERROR)) .context(source_location_msg!("Waiting for RKPD to complete storing key: {:?}", e)), Ok(v) => v.unwrap(), } @@ -309,6 +291,7 @@ pub fn store_rkpd_attestation_key( mod tests { use super::*; use android_security_rkp_aidl::aidl::android::security::rkp::IRegistration::BnRegistration; + use std::collections::HashMap; use std::sync::atomic::{AtomicU32, Ordering}; use std::sync::{Arc, Mutex}; @@ -432,7 +415,10 @@ mod tests { assert!(cb.onCancel().is_ok()); let result = tokio_rt().block_on(rx).unwrap(); - assert_eq!(result.unwrap_err().downcast::().unwrap(), Error::RequestCancelled); + assert_eq!( + result.unwrap_err().downcast::().unwrap(), + Error::Rc(ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR) + ); } #[test] @@ -442,7 +428,10 @@ mod tests { assert!(cb.onError("error").is_ok()); let result = tokio_rt().block_on(rx).unwrap(); - assert_eq!(result.unwrap_err().downcast::().unwrap(), Error::GetRegistrationFailed); + assert_eq!( + result.unwrap_err().downcast::().unwrap(), + Error::Rc(ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR) + ); } #[test] @@ -464,11 +453,29 @@ mod tests { assert!(cb.onCancel().is_ok()); let result = tokio_rt().block_on(rx).unwrap(); - assert_eq!(result.unwrap_err().downcast::().unwrap(), Error::RequestCancelled); + assert_eq!( + result.unwrap_err().downcast::().unwrap(), + Error::Rc(ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR) + ); } #[test] fn test_get_key_cb_error() { + let error_mapping = HashMap::from([ + (GetKeyErrorCode::ERROR_UNKNOWN, ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR), + (GetKeyErrorCode::ERROR_PERMANENT, ResponseCode::OUT_OF_KEYS_PERMANENT_ERROR), + ( + GetKeyErrorCode::ERROR_PENDING_INTERNET_CONNECTIVITY, + ResponseCode::OUT_OF_KEYS_PENDING_INTERNET_CONNECTIVITY, + ), + ( + GetKeyErrorCode::ERROR_REQUIRES_SECURITY_PATCH, + ResponseCode::OUT_OF_KEYS_REQUIRES_SYSTEM_UPGRADE, + ), + ]); + + // Loop over the generated list of enum values to better ensure this test stays in + // sync with the AIDL. for get_key_error in GetKeyErrorCode::enum_values() { let (tx, rx) = oneshot::channel(); let cb = GetKeyCallback::new_native_binder(tx); @@ -477,7 +484,7 @@ mod tests { let result = tokio_rt().block_on(rx).unwrap(); assert_eq!( result.unwrap_err().downcast::().unwrap(), - Error::GetKeyFailed(get_key_error), + Error::Rc(error_mapping[&get_key_error]), ); } } @@ -498,7 +505,10 @@ mod tests { assert!(cb.onError("oh no! it failed").is_ok()); let result = tokio_rt().block_on(rx).unwrap(); - assert_eq!(result.unwrap_err().downcast::().unwrap(), Error::StoreUpgradedKeyFailed); + assert_eq!( + result.unwrap_err().downcast::().unwrap(), + Error::Rc(ResponseCode::SYSTEM_ERROR) + ); } #[test] @@ -522,7 +532,10 @@ mod tests { let result = tokio_rt().block_on(get_rkpd_attestation_key_from_registration_async(®istration, 0)); - assert_eq!(result.unwrap_err().downcast::().unwrap(), Error::Timeout); + assert_eq!( + result.unwrap_err().downcast::().unwrap(), + Error::Rc(ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR) + ); } #[test] @@ -547,7 +560,10 @@ mod tests { &[], &[], )); - assert_eq!(result.unwrap_err().downcast::().unwrap(), Error::Timeout); + assert_eq!( + result.unwrap_err().downcast::().unwrap(), + Error::Rc(ResponseCode::SYSTEM_ERROR) + ); } #[test] diff --git a/keystore2/src/security_level.rs b/keystore2/src/security_level.rs index f2b332b0..830fbe11 100644 --- a/keystore2/src/security_level.rs +++ b/keystore2/src/security_level.rs @@ -19,9 +19,7 @@ use crate::audit_log::{ log_key_deleted, log_key_generated, log_key_imported, log_key_integrity_violation, }; use crate::database::{BlobInfo, CertificateInfo, KeyIdGuard}; -use crate::error::{ - self, map_km_error, map_or_log_err, wrapped_rkpd_error_to_ks_error, Error, ErrorCode, -}; +use crate::error::{self, map_km_error, map_or_log_err, Error, ErrorCode}; use crate::globals::{ get_remotely_provisioned_component_name, DB, ENFORCEMENTS, LEGACY_IMPORTER, SUPER_KEY, }; @@ -902,11 +900,8 @@ impl KeystoreSecurityLevel { f, |upgraded_blob| { let _wp = wd::watch_millis("Calling store_rkpd_attestation_key()", 500); - if let Err(e) = store_rkpd_attestation_key(&rpc_name, key_blob, upgraded_blob) { - Err(wrapped_rkpd_error_to_ks_error(&e)).context(format!("{e:?}")) - } else { - Ok(()) - } + store_rkpd_attestation_key(&rpc_name, key_blob, upgraded_blob) + .context(ks_err!("Failed store_rkpd_attestation_key().")) }, ) .context(ks_err!()) -- cgit v1.2.3