Converts storage and secure area to asynchronous APIs #856

sorotokin · 2025-01-28T02:16:09Z

Moves to new Storage API and propagates asynchronous ("suspend") function signatures through the existing code.

We will need to define concurrency model for DocumentStore/Document/Credential APIs in future PRs.

Also adjusts unit tests to use new APIs

davidz25 · 2025-01-28T03:40:21Z

identity/src/commonMain/kotlin/com/android/identity/document/DocumentStore.kt

+ * obtain a [DocumentStore] lock by running the code that accesses these APIs inside
+ * [DocumentStore.withLock] (or a convenience shortcut [Document.withLock]). Note that
+ * there is a single lock for all documents and credentials in a [DocumentStore]
+ * instance and only a single coroutine can hold the lock at the same time.


I have to look at this in more detail but my gut feeling is that it's not appropriate to ask API callers to obtain a lock before calling an API. And it's also not very common to have this requirement. I don't think we should require this, it's a bit of an anti-pattern.

The ordinary way this is done - and the way I thought we'd do it when you mentioned a DocumentStore-wide lock - is that the API entry-points the app calls will take the lock. Is there a specific reason we can't do that instead?

I don't disagree with the general gist of it, but there is a lot of nuance here. Let's move this to the chat.

OK, removed the locking altogether for now. We'll need to figure out how to do it later.

davidz25

Did a read-through. Looks great and thanks for all your hard work on this! Comments included but my only real question I have is that we're not entirely consistent with asynchronous object creation...

In some cases the constructors are still public and for the Credential hierarchy you expect the user to call the constructor and then a suspend function after that. I think in general we should move to suspend create() on the Companion object and in the case of multiple constructors (as e.g. Credential), they should be suspend createFoo() and suspend createBar() functions on the Companion object, I think. It would be good if that were made more consistent...

davidz25 · 2025-01-29T12:28:26Z

...y-android-csa/src/main/java/com/android/identity/android/securearea/cloud/CloudSecureArea.kt

+        private const val METADATA_PREFIX = "@"
+
+        /**
+         * Creates an instance of AndroidKeystoreSecureArea.


ITYM CloudSecureArea here.

davidz25 · 2025-01-29T12:33:09Z

...oid/src/main/java/com/android/identity/android/mdoc/deviceretrieval/DeviceRetrievalHelper.kt

@@ -444,6 +447,8 @@ class DeviceRetrievalHelper internal constructor(
     * The [Listener.onError] callback can be called at any time - for
     * example - if the remote verifier disconnects without using session termination or if the
     * underlying transport encounters an unrecoverable error.
+     *
+     * TODO: consider redoing this API through flows.


In fact, DeviceRetrievalHelper is now replaced by PresentmentModel and MdocTransport which both use flows. The idea is to deprecate all of identity-android.

OK, removed that TODO

davidz25 · 2025-01-29T12:34:49Z

...oid/src/main/java/com/android/identity/android/mdoc/deviceretrieval/DeviceRetrievalHelper.kt

@@ -454,7 +459,7 @@ class DeviceRetrievalHelper internal constructor(
         *
         * @param eReaderKey the ephemeral reader key.
         */
-        fun onEReaderKeyReceived(eReaderKey: EcPublicKey)
+        suspend fun onEReaderKeyReceived(eReaderKey: EcPublicKey)


I'm not convinced we need to make these suspend. What's the rationale?

If this is going away, I'll put it back the way it was. I'll just need to add runBlocking in one place.

Yeah, it's going to be deprecated and will be moved to multipaz-legacy library with the rest of identity-android and identity-android-legacy. We'll keep using it in wallet, other downstreams may keep using it, so it needs to keep working.

I think runBlocking before invoking this will be fine. The listener implementations are not supposed to be doing any blocking I/O anyway and I don't think any of them do.

davidz25 · 2025-01-29T12:47:27Z

.../src/androidMain/kotlin/com/android/identity/android/securearea/AndroidKeystoreSecureArea.kt

+         */
+        suspend fun create(context: Context, storage: Storage): AndroidKeystoreSecureArea {
+            return AndroidKeystoreSecureArea(context, storage.getTable(tableSpec))
+        }


A separate create() function is needed b/c constructors cannot be suspend?

Yes, storage.getTable(tableSpec) is asynchronous

davidz25 · 2025-01-29T12:52:41Z

.../src/androidMain/kotlin/com/android/identity/android/securearea/AndroidKeystoreSecureArea.kt

@@ -102,13 +105,15 @@ import java.io.ByteArrayInputStream
 * what the device supports.
 *
 * This implementation works only on Android and requires API level 24 or later.
+ *
+ * Use [AndroidKeystoreSecureArea.create] to create an instance of this class.


Can we make the constructor private or protected? Seems like that would force users to use create()...

Yes, this is an oversight.

davidz25 · 2025-01-29T13:01:12Z

identity/src/commonMain/kotlin/com/android/identity/securearea/SecureArea.kt

     */
-    fun createKey(alias: String, createKeySettings: CreateKeySettings)
+    suspend fun createKey(alias: String?, createKeySettings: CreateKeySettings): String


Actually we want this to return KeyInfo in the future.... maybe add alias to KeyInfo and return KeyInfo now? I think that's only a few lines of code...

davidz25 · 2025-01-29T13:04:57Z

identity/src/commonMain/kotlin/com/android/identity/securearea/SecureAreaRepository.kt

+ * requested URL and configure the instance as needed. Key to the map is Secure Area
+ * identifier prefix of the Secure Area identifier up until the '?' character.
+ * @param factoryFunc a function to create a new Secure Area with the requested identifier.
+ *


This comment needs both formatting changes (two empty lines, "can" should start with a capital) and also references now non-existant factoryFunc

davidz25 · 2025-01-29T13:11:19Z

identity/src/commonMain/kotlin/com/android/identity/securearea/SecureAreaRepository.kt

+                builder.block()
+                builder.build()
+            })
+        }


This looks like a non-standard builder pattern... do we even need a builder-ish thing here?

This is pretty standard Kotlin DSL... And what it does is trickier than it seems:

// This builds SecureAreaRepository *synchronously*, can be called in static initializers val r = SecureAreaRepository.build { // *asynchronous code* that does actual building add(...) addImplementation(...) }

OK, just checking, I'm pretty rusty when it comes to DSLs in Kotlin. Fine with me!

davidz25 · 2025-01-29T13:15:46Z

identity-mdoc/src/commonMain/kotlin/com/android/identity/mdoc/credential/MdocCredential.kt

     * @param docType the docType of the credential
+     *
+     * [SecureAreaBoundCredential.generateKey] must be called before using this object.


So you have this requirement b/c constructors can't be suspend. How about making the constructor private/protected and have a static suspend create() instead?

Yes, this is the same issue as in SecureAreaBoundCredential. I agree that we want something like this, but I think these APIs probably should be designed for concurrency/locking from grounds up.

davidz25 · 2025-01-29T13:16:48Z

identity/src/commonMain/kotlin/com/android/identity/credential/SecureAreaBoundCredential.kt

@@ -38,38 +38,46 @@ open class SecureAreaBoundCredential : Credential {
    /**
     * Constructs a new [SecureAreaBoundCredential].
     *
+     * [generateKey] providing [CreateKeySettings] must be called before using this object.


Same as with MdocCredential, make the constructor private and have a static suspend create() that plays the role of a suspend constructor.

davidz25

As discussed, LGTM to merge now and we can work on addressing concurrency/locking and API for DocumentStore, Document, and Credential in separate PRs. Cc @kdeus

kdeus · 2025-01-30T02:28:29Z

As discussed, LGTM to merge now and we can work on addressing concurrency/locking and API for DocumentStore, Document, and Credential in separate PRs. Cc @kdeus

I've done a few spot checks, not a thorough review.
+1 to merge now, fix later.

Signed-off-by: Peter Sorotokin <[email protected]>

sorotokin requested a review from kdeus January 28, 2025 02:16

davidz25 reviewed Jan 28, 2025

View reviewed changes

sorotokin force-pushed the migrate-storage branch 5 times, most recently from e2769df to 46a0dbf Compare January 29, 2025 05:11

davidz25 reviewed Jan 29, 2025

View reviewed changes

sorotokin force-pushed the migrate-storage branch from 46a0dbf to 793d828 Compare January 29, 2025 19:49

davidz25 approved these changes Jan 30, 2025

View reviewed changes

kdeus approved these changes Jan 30, 2025

View reviewed changes

Converts storage and secure area to new asynchronous APIs.

2ec71e5

Signed-off-by: Peter Sorotokin <[email protected]>

sorotokin force-pushed the migrate-storage branch from 793d828 to 2ec71e5 Compare January 30, 2025 05:33

sorotokin merged commit bb9ea05 into main Jan 30, 2025
5 checks passed

sorotokin deleted the migrate-storage branch January 30, 2025 06:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Converts storage and secure area to asynchronous APIs #856

Converts storage and secure area to asynchronous APIs #856

sorotokin commented Jan 28, 2025 •

edited

Loading

davidz25 Jan 28, 2025

sorotokin Jan 28, 2025

sorotokin Jan 28, 2025

davidz25 left a comment

davidz25 Jan 29, 2025

sorotokin Jan 29, 2025

davidz25 Jan 29, 2025

sorotokin Jan 29, 2025

davidz25 Jan 29, 2025

sorotokin Jan 29, 2025

davidz25 Jan 29, 2025

davidz25 Jan 29, 2025

sorotokin Jan 29, 2025

davidz25 Jan 29, 2025

sorotokin Jan 29, 2025

davidz25 Jan 29, 2025

sorotokin Jan 29, 2025

davidz25 Jan 29, 2025

davidz25 Jan 29, 2025

sorotokin Jan 29, 2025

davidz25 Jan 29, 2025

davidz25 Jan 29, 2025

sorotokin Jan 29, 2025

davidz25 Jan 29, 2025

davidz25 left a comment

kdeus commented Jan 30, 2025

Converts storage and secure area to asynchronous APIs #856

Converts storage and secure area to asynchronous APIs #856

Conversation

sorotokin commented Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidz25 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidz25 left a comment

Choose a reason for hiding this comment

kdeus commented Jan 30, 2025

sorotokin commented Jan 28, 2025 •

edited

Loading