Skip to content

Add secure_db_abstraction example (std-only)#171

Merged
DemesneGH merged 2 commits intoapache:mainfrom
DemesneGH:main
Mar 4, 2025
Merged

Add secure_db_abstraction example (std-only)#171
DemesneGH merged 2 commits intoapache:mainfrom
DemesneGH:main

Conversation

@DemesneGH
Copy link
Contributor

  • Add secure_db_abstraction example and test script
  • Reduce the parallel building job for STD CI tests, which is the workaround for LTO error

@DemesneGH
Copy link
Contributor Author

@ivila Could you help to review this PR? thanks!

@ivila
Copy link
Contributor

ivila commented Feb 27, 2025

Why don't we make it no-std, I think it can be easily achieved by:

  1. std::collections::{HashMap, HashSet} => hashbrown::{HashSet, HashMap}
  2. std::sync::RwLock => spin::RwLock
  3. For other imports, just import from core or alloc directly.

@DemesneGH
Copy link
Contributor Author

Why don't we make it no-std, I think it can be easily achieved by:

  1. std::collections::{HashMap, HashSet} => hashbrown::{HashSet, HashMap}
  2. std::sync::RwLock => spin::RwLock
  3. For other imports, just import from core or alloc directly.

I did a quick test, and the HashMap and RwLock can be replaced with hashbrown and spin.
However, bincode and anyhow with no-std need some code change:

I suggest we review and merge the std version first, and then update to the no-std version in a separate PR. This way we can highlight the differences between the two and leave a record for the std version.

@ivila
Copy link
Contributor

ivila commented Feb 28, 2025

Why don't we make it no-std, I think it can be easily achieved by:

  1. std::collections::{HashMap, HashSet} => hashbrown::{HashSet, HashMap}
  2. std::sync::RwLock => spin::RwLock
  3. For other imports, just import from core or alloc directly.

I did a quick test, and the HashMap and RwLock can be replaced with hashbrown and spin. However, bincode and anyhow with no-std need some code change:

I suggest we review and merge the std version first, and then update to the no-std version in a separate PR. This way we can highlight the differences between the two and leave a record for the std version.

I didn't look into bincode and anyhow in detail—my mistake. In that case, I agree with your opinion; let's start with a std-only version first.

@DemesneGH
Copy link
Contributor Author

let's start with a std-only version first.

@ivila Yep please help review this and provide any comments or the Reviewed-By tag. Thanks!


Ok(mut object) => {
object.close_and_delete()?;
std::mem::forget(object);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we need to call std::mem::forget here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comes from legacy example code secure_storage-rs and related explanation is: https://github.com/apache/incubator-teaclave-trustzone-sdk/blob/main/optee-utee/src/object.rs#L1087-L1089

Comment on lines 29 to 39
match PersistentObject::create(
ObjectStorageConstants::Private,
obj_id,
obj_data_flag,
None,
data,
) {
Err(e) => {
bail!("[-] {:?}: failed to create object: {:?}", &obj_id, e);
}
Ok(_) => {
return Ok(());
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just use map_err

      PersistentObject::create(
          ObjectStorageConstants::Private,
          obj_id,
          obj_data_flag,
          None,
          data,
      ).map_err(|e| bail!("[-] {:?}: failed to create object: {:?}", &obj_id, e))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

self.delete(&key)?;
}
self.key_list.clear();
Ok(())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the key_list delete the key in the same step as the storage? What if the deletion fails midway through processing the key_list?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.key_list.clear(); is redundant because in delete() we have self.key_list.remove(key);. So removed the self.key_list.clear();

@ivila
Copy link
Contributor

ivila commented Feb 28, 2025

Sorry I forgot the submit the CR comments last night😂

@DemesneGH DemesneGH force-pushed the main branch 2 times, most recently from 974be0a to d373503 Compare February 28, 2025 13:29
@ivila
Copy link
Contributor

ivila commented Mar 4, 2025

Sorry for the late reply, kind of busy in these days.

Reviewed-by: Zehui Chen <ivila@apache.org>

Add reference implementation for TA that simplifies interaction
with secure storage. It provides basic methods for database
operations, including `get()`, `put()`, `delete_entries()`, and
`list_entries()`, making it easier for developers to store and
retrieve data based on Rust Type constraints.

The example is std-only for now.

Signed-off-by: Yuan Zhuang <yuanz@apache.org>
Reviewed-by: Zehui Chen <ivila@apache.org>
The following error sometimes occurs during the build process:
"error: failed to get bitcode from object file for LTO (could not
find requested section)".
This issue is often observed when building with multiple parallel
jobs. As a temporary workaround, the number of parallel jobs has
been reduced to mitigate the error.

Signed-off-by: Yuan Zhuang <yuanz@apache.org>
Reviewed-by: Zehui Chen <ivila@apache.org>
@DemesneGH
Copy link
Contributor Author

Seems one issue exists in setup scripts which leads to CI error. Merging this PR and will open a new PR for fix.

@DemesneGH DemesneGH merged commit ece214b into apache:main Mar 4, 2025
1 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants