Merge "Revert "[rkpd_client] Add Error type to rkpd_client"" into main am: 0b140f418a

Original change: https://android-review.googlesource.com/c/platform/system/security/+/2824755

Change-Id: I8b7db0cd664c3d8c0bba5b4a9e9cb3840c2bdef2
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

4 files changed, 76 insertions, 140 deletions

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<RkpdError> 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::<RkpdError>() {
-        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<StatusCode> 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<T> {
@@ -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<binder::Strong<dyn IRegistration>> {
     let remote_provisioning: Strong<dyn IRemoteProvisioning> =
-        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<binder::Strong<dyn IReg
         .context(source_location_msg!("Trying to get registration."))?;
 
     match timeout(RKPD_TIMEOUT, rx).await {
-        Err(e) => 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::<Error>().unwrap(), Error::RequestCancelled);
+        assert_eq!(
+            result.unwrap_err().downcast::<Error>().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::<Error>().unwrap(), Error::GetRegistrationFailed);
+        assert_eq!(
+            result.unwrap_err().downcast::<Error>().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::<Error>().unwrap(), Error::RequestCancelled);
+        assert_eq!(
+            result.unwrap_err().downcast::<Error>().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::<Error>().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::<Error>().unwrap(), Error::StoreUpgradedKeyFailed);
+        assert_eq!(
+            result.unwrap_err().downcast::<Error>().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(&registration, 0));
-        assert_eq!(result.unwrap_err().downcast::<Error>().unwrap(), Error::Timeout);
+        assert_eq!(
+            result.unwrap_err().downcast::<Error>().unwrap(),
+            Error::Rc(ResponseCode::OUT_OF_KEYS_TRANSIENT_ERROR)
+        );
     }
 
     #[test]
@@ -547,7 +560,10 @@ mod tests {
             &[],
             &[],
         ));
-        assert_eq!(result.unwrap_err().downcast::<Error>().unwrap(), Error::Timeout);
+        assert_eq!(
+            result.unwrap_err().downcast::<Error>().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!())