From 849cfe4347e913dfc05ed10cb3db4cad1f729be5 Mon Sep 17 00:00:00 2001 From: Alice Wang Date: Fri, 10 Nov 2023 12:43:36 +0000 Subject: Revert^2 "[rkpd_client] Add Error type to rkpd_client" This reverts commit f84c46c3b3cad3ea4908ab44c361f637c7fcdb09. Reason for revert: Reland the original cl aosp/2821995 with an adjustment about the Timeout error type in order to maintain the original ResponseCode. Test: atest RkpdAppIntegrationTests Bug: 310139666 Change-Id: Id4ee05eb616c125f9d28b25f4668ca3071ccb26c --- keystore2/src/error.rs | 76 ++++++++++++++++++++ keystore2/src/remote_provisioning.rs | 3 +- keystore2/src/rkpd_client.rs | 132 ++++++++++++++++------------------- keystore2/src/security_level.rs | 11 ++- 4 files changed, 146 insertions(+), 76 deletions(-) diff --git a/keystore2/src/error.rs b/keystore2/src/error.rs index 1a048b6b..ac5ba4c3 100644 --- a/keystore2/src/error.rs +++ b/keystore2/src/error.rs @@ -27,7 +27,9 @@ //! 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, @@ -66,6 +68,49 @@ 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::RetryableTimeout => Error::Rc(ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR), + 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., @@ -409,4 +454,35 @@ 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::RetryableTimeout, ResponseCode::OUT_OF_KEYS_TRANSIENT_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 3f7833ed..14c61fb0 100644 --- a/keystore2/src/remote_provisioning.rs +++ b/keystore2/src/remote_provisioning.rs @@ -31,6 +31,7 @@ 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; @@ -102,7 +103,7 @@ impl RemProvState { Err(e) => { if self.is_rkp_only() { log::error!("Error occurred: {:?}", e); - return Err(e); + return Err(wrapped_rkpd_error_to_ks_error(&e)).context(format!("{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 fe641506..d8a5276c 100644 --- a/keystore2/src/rkpd_client.rs +++ b/keystore2/src/rkpd_client.rs @@ -14,7 +14,6 @@ //! 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, @@ -24,8 +23,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; @@ -41,6 +40,44 @@ 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, + + /// Retryable timeout when waiting for a callback. + #[error("Retryable timeout when waiting for a callback")] + RetryableTimeout, + + /// 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 { @@ -87,17 +124,17 @@ impl IGetRegistrationCallback for GetRegistrationCallback { fn onCancel(&self) -> binder::Result<()> { log::warn!("IGetRegistrationCallback cancelled"); self.registration_tx.send( - Err(Error::Rc(ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR)) + Err(Error::RequestCancelled) .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::Rc(ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR)).context( - source_location_msg!("GetRegistrationCallback failed: {:?}", description), - )); + self.registration_tx.send( + Err(Error::GetRegistrationFailed) + .context(source_location_msg!("GetRegistrationCallback failed: {:?}", description)), + ); Ok(()) } } @@ -105,7 +142,8 @@ 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 = - map_binder_status_code(binder::get_interface("remote_provisioning")) + binder::get_interface("remote_provisioning") + .map_err(Error::from) .context(source_location_msg!("Trying to connect to IRemoteProvisioning service."))?; let (tx, rx) = oneshot::channel(); @@ -116,8 +154,7 @@ async fn get_rkpd_registration(rpc_name: &str) -> Result Err(Error::Rc(ResponseCode::SYSTEM_ERROR)) - .context(source_location_msg!("Waiting for RKPD: {:?}", e)), + Err(e) => Err(Error::Timeout).context(source_location_msg!("Waiting for RKPD: {:?}", e)), Ok(v) => v.unwrap(), } } @@ -148,28 +185,13 @@ impl IGetKeyCallback for GetKeyCallback { fn onCancel(&self) -> binder::Result<()> { log::warn!("IGetKeyCallback cancelled"); self.key_tx.send( - Err(Error::Rc(ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR)) - .context(source_location_msg!("GetKeyCallback cancelled.")), + Err(Error::RequestCancelled).context(source_location_msg!("GetKeyCallback cancelled.")), ); Ok(()) } fn onError(&self, error: GetKeyErrorCode, description: &str) -> binder::Result<()> { log::error!("IGetKeyCallback failed: {description}"); - 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!( + self.key_tx.send(Err(Error::GetKeyFailed(error)).context(source_location_msg!( "GetKeyCallback failed: {:?} {:?}", error, description @@ -195,7 +217,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::Rc(ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR)) + Err(Error::RetryableTimeout) .context(source_location_msg!("Waiting for RKPD key timed out: {:?}", e)) } Ok(v) => v.unwrap(), @@ -234,9 +256,9 @@ impl IStoreUpgradedKeyCallback for StoreUpgradedKeyCallback { } fn onError(&self, error: &str) -> binder::Result<()> { - log::error!("IGetRegistrationCallback failed: {error}"); + log::error!("IStoreUpgradedKeyCallback failed: {error}"); self.completer.send( - Err(Error::Rc(ResponseCode::SYSTEM_ERROR)) + Err(Error::StoreUpgradedKeyFailed) .context(source_location_msg!("Failed to store upgraded key: {:?}", error)), ); Ok(()) @@ -256,7 +278,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::Rc(ResponseCode::SYSTEM_ERROR)) + Err(e) => Err(Error::Timeout) .context(source_location_msg!("Waiting for RKPD to complete storing key: {:?}", e)), Ok(v) => v.unwrap(), } @@ -291,7 +313,6 @@ 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}; @@ -415,10 +436,7 @@ mod tests { assert!(cb.onCancel().is_ok()); let result = tokio_rt().block_on(rx).unwrap(); - assert_eq!( - result.unwrap_err().downcast::().unwrap(), - Error::Rc(ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR) - ); + assert_eq!(result.unwrap_err().downcast::().unwrap(), Error::RequestCancelled); } #[test] @@ -428,10 +446,7 @@ mod tests { assert!(cb.onError("error").is_ok()); let result = tokio_rt().block_on(rx).unwrap(); - assert_eq!( - result.unwrap_err().downcast::().unwrap(), - Error::Rc(ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR) - ); + assert_eq!(result.unwrap_err().downcast::().unwrap(), Error::GetRegistrationFailed); } #[test] @@ -453,29 +468,11 @@ mod tests { assert!(cb.onCancel().is_ok()); let result = tokio_rt().block_on(rx).unwrap(); - assert_eq!( - result.unwrap_err().downcast::().unwrap(), - Error::Rc(ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR) - ); + assert_eq!(result.unwrap_err().downcast::().unwrap(), Error::RequestCancelled); } #[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); @@ -484,7 +481,7 @@ mod tests { let result = tokio_rt().block_on(rx).unwrap(); assert_eq!( result.unwrap_err().downcast::().unwrap(), - Error::Rc(error_mapping[&get_key_error]), + Error::GetKeyFailed(get_key_error), ); } } @@ -505,10 +502,7 @@ 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::Rc(ResponseCode::SYSTEM_ERROR) - ); + assert_eq!(result.unwrap_err().downcast::().unwrap(), Error::StoreUpgradedKeyFailed); } #[test] @@ -532,10 +526,7 @@ 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::Rc(ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR) - ); + assert_eq!(result.unwrap_err().downcast::().unwrap(), Error::RetryableTimeout); } #[test] @@ -560,10 +551,7 @@ mod tests { &[], &[], )); - assert_eq!( - result.unwrap_err().downcast::().unwrap(), - Error::Rc(ResponseCode::SYSTEM_ERROR) - ); + assert_eq!(result.unwrap_err().downcast::().unwrap(), Error::Timeout); } #[test] diff --git a/keystore2/src/security_level.rs b/keystore2/src/security_level.rs index 830fbe11..f2b332b0 100644 --- a/keystore2/src/security_level.rs +++ b/keystore2/src/security_level.rs @@ -19,7 +19,9 @@ 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, Error, ErrorCode}; +use crate::error::{ + self, map_km_error, map_or_log_err, wrapped_rkpd_error_to_ks_error, Error, ErrorCode, +}; use crate::globals::{ get_remotely_provisioned_component_name, DB, ENFORCEMENTS, LEGACY_IMPORTER, SUPER_KEY, }; @@ -900,8 +902,11 @@ impl KeystoreSecurityLevel { f, |upgraded_blob| { let _wp = wd::watch_millis("Calling store_rkpd_attestation_key()", 500); - store_rkpd_attestation_key(&rpc_name, key_blob, upgraded_blob) - .context(ks_err!("Failed store_rkpd_attestation_key().")) + 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(()) + } }, ) .context(ks_err!()) -- cgit v1.2.3