Merge "Revert^2 "[rkpd_client] Add Error type to rkpd_client"" into main am: 5c96cefe87

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

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

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