Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sargon os v1 android #196

Merged
merged 5 commits into from
Aug 7, 2024
Merged

Sargon os v1 android #196

merged 5 commits into from
Aug 7, 2024

Conversation

micbakos-rdx
Copy link
Contributor

@micbakos-rdx micbakos-rdx commented Aug 6, 2024

Introduces all the implementations of drivers needed to boot sargon os on Android.

More details about the drivers

  • AndroidProfileChangeDriver: Exposes a shared flow for the host app to subscribe to.
  • AndroidFileSystemDriver: Allows read/write to files stored in app internal storage in ${context.filesDir}/sargon/
  • AndroidEventBusDriver: exposes a similar flow as AndroidProfileChangeDriver
  • AndroidLoggingDriver: uses Timber to print logs. Logs can be enabled or not when instantiating the Bios.
  • AndroidHostInfoDriver: implements all the logic that retrieves host information and also reads stored HostId.
  • AndroidEntropyProviderDriver: provides entropy by using android's SecureRandom
  • AndroidStorageDriver: is a driver that provides access to key/value storage and encrypted or not.
  • AndroidNetworkingDriver: is the driver that makes the network requests by accepting an OkHttpClient.
  • AndroidBiometricAuthorizationDriver uses BiometricsHandler to ask for biometrics.

AndroidStorageDriver

This driver took the most time to develop. Even though sargon exposes an API to read/write to a key/value storage, the contents of the android storage are usually differently stored, like different schema or different data types. For example
* profile snapshot is encrypted but kept as a string
* HostId used to be stored as DeviceInfo so some migration logic should be done. Also host Id used to be stored in SharedPreferences and sargon os also provides a test that migrates shared preferences to Datastore<Preferences>.
* MnemonicWithPassphrase has different serialization logic.

BiometricsHandler

This is an android specific class which is responsible for asking for biometrics when secure storage needs to read/write data. This is a key difference between iOS and Android. On Android we are not using a storage that manages its own encryption, but we simply encrypt the data and store them to a file as we would store data in any other file that are not encrypted. In order to encrypt securely we use Android's Keystore System. This keystore manages the encryption outside of the app's sandbox. Some data are tied to keys that not need authorization (like profile snapshot), but other, like mnemonic, need authorisation. We are responsible to chose when and were to ask for biometrics before decrypt/encrypt a mnemonic. So in order to bridge the difference between iOS and Android, whenever a secure storage key requires authorisation, sargon uses BiometricsHandler in order to handle exactly that.

In order to ask for biometrics on Android we need to have access to a FragmentActivity. That means that such an activity must have been created in order for us to request biometrics. Since sargon os is booted in application level, were no activity has yet been created, we need to ensure that in some cases sargon may ask the host for biometrics before the first activity is registered. Currently this is possible when the app is booting sargon os for the first time when no profile is available in storage. Immediately sargon creates an ephemeral profile with an ephemeral mnemonic that needs to be stored. This whole operation is quicker that the first activity created. So sargon os needs to suspend until some activity is available in order to handle the biometrics request. Also this handler needs to handle the case were an activity might stop (moved to background) or a new activity is created on top (Current android wallet has only one activity that handles all the screens, but I tried to make it not rely on that assumption). In order to solve this problem BiometricsHandler should be a singleton injected to any activity that can handle biometrics. Such activity should call register() in their respective onCreate. Inside this class we have two channels communicating together.

  • biometricRequestsChannel which suspends until an activity is subscribed to biometrics and becomes Lifecycle.State.ON_CREATE.
  • biometricsResultsChannel which waits until the user provides the biometrics, or the system fails for some reason to ask for biometrics (maybe the user did not enrol yet)
    These two are combined together in order to expose a simple function suspend askForBiometrics(): Result<Unit> which can be called from sargon.

Android tests

I tried to mock as many classes I could, classes that touch the android system. But there are some tests that would be more reliable to be tested under instrumentation tests. These tests cover all the drivers that need to communicate with the system. In order to run such tests we need a device or an emulator. Such tests run with ./gradlew sargon-android:connectedAndroidTest. Our CI does not facilitate yet a connection to any device. A good approach here is to make our CI pipeline run these tests some times every day on main branch by connecting to firebase test lab. This can be done in a separate PR.

Changes in sargon

In sargon I had to do the following changes, in order to successfully boot the sargon os.

  1. One of them is Implement HostInfo struct #189 . This separated the HostInfo from HostId and makes it possible for android to provide reliably all the data needed.
  2. Sargon os used to operate on the following logic: When booting, ask the storage on what profile id is the active profile id and then if such id exists, load that profile from storage. This is due to the fact that the iOS host used to store multiple profiles but mark what profile is "in use" due to the old backup logic. On android the host only stored the currently used profile. So in order to bridge the gap, sargon os needs to load the currently used profile without any knowledge on what is perceived as active. Android will just load a profile if exists, and iOS needs to perform this active profile id logic in its drivers.

Host integration.

I played around a bit by booting sargon os in the sample application. The host needs to do the following:

  • In application scope boot the bios. Take a look at example app's SargonOsManager. This works a bit similarly with how android host loads the profile. This will ease the migration of ProfileRepository and sargon os.
  • In each activity that can handle biometrics (currently on Android wallet's MainActivity) we need to add the following:
@AndroidEntryPoint
class MainActivity : FragmentActivity() {
    @Inject
    lateinit var biometricsHandler: BiometricsHandler
    
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)

        biometricsHandler.register(this)
        // ...
    }
}

And that's all about it.

@micbakos-rdx micbakos-rdx added Coverage OK ☂️ ✅ False Negatives in Code Coverage by Tarpaulin Rust 🦀 Changes in Rust Sargon Kotlin 🤖 Changes in Kotlin Sargon UniFFI 🦄 Changes in UniFFI exported APIs labels Aug 6, 2024
Copy link

codecov bot commented Aug 6, 2024

Codecov Report

Attention: Patch coverage is 93.51351% with 12 lines in your changes missing coverage. Please review.

Project coverage is 97.5%. Comparing base (498f627) to head (676c1f7).

Files Patch % Lines
...torage/key/DeviceFactorSourceMnemonicKeyMapping.kt 86.9% 2 Missing and 1 partial ⚠️
...radixdlt/sargon/os/storage/key/HostIdKeyMapping.kt 90.9% 2 Missing and 1 partial ⚠️
...ixdlt/sargon/os/storage/key/ByteArrayKeyMapping.kt 95.0% 0 Missing and 2 partials ⚠️
...ain/java/com/radixdlt/sargon/os/storage/KeySpec.kt 87.5% 1 Missing ⚠️
...ava/com/radixdlt/sargon/os/storage/StorageUtils.kt 95.6% 0 Missing and 1 partial ⚠️
...sargon/os/storage/key/ProfileSnapshotKeyMapping.kt 94.4% 0 Missing and 1 partial ⚠️
...ent/secure_storage_client/secure_storage_client.rs 87.5% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff               @@
##           sargon_os_v1    #196     +/-   ##
==============================================
+ Coverage          97.3%   97.5%   +0.1%     
==============================================
  Files               907     910      +3     
  Lines             14458   14537     +79     
  Branches             38      63     +25     
==============================================
+ Hits              14079   14176     +97     
+ Misses              373     353     -20     
- Partials              6       8      +2     
Flag Coverage Δ
kotlin 98.6% <93.5%> (+2.0%) ⬆️
rust 97.0% <92.8%> (+<0.1%) ⬆️
swift 99.3% <ø> (-0.2%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
.../java/com/radixdlt/sargon/extensions/BagOfBytes.kt 100.0% <100.0%> (ø)
.../java/com/radixdlt/sargon/extensions/Decimal192.kt 98.3% <ø> (-0.3%) ⬇️
...ava/com/radixdlt/sargon/extensions/FactorSource.kt 100.0% <100.0%> (+16.6%) ⬆️
...main/java/com/radixdlt/sargon/extensions/Result.kt 100.0% <100.0%> (ø)
...com/radixdlt/sargon/extensions/UnsafeStorageKey.kt 100.0% <100.0%> (ø)
...rgon/os/radixconnect/RadixConnectSessionStorage.kt 100.0% <100.0%> (ø)
src/profile/supporting_types/host_id_uniffi_fn.rs 100.0% <100.0%> (ø)
...ecure_storage_driver/support/secure_storage_key.rs 100.0% <ø> (ø)
src/system/sargon_os/sargon_os.rs 97.3% <100.0%> (+1.2%) ⬆️
src/system/sargon_os/sargon_os_accounts.rs 83.2% <ø> (ø)
... and 8 more

... and 7 files with indirect coverage changes

Copy link
Contributor

@CyonAlexRDX CyonAlexRDX left a comment

Choose a reason for hiding this comment

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

Nice, did not look into Kotlin code, just read description of PR and Rust changes, look good!

We should probably do a PR into this PR source branch with updates to Swift Sargon / iOS example app! We should decide who does that, if you want wanna do it @micbakos-rdx ? Should be straightforward I think.

@micbakos-rdx
Copy link
Contributor Author

Nice, did not look into Kotlin code, just read description of PR and Rust changes, look good!

We should probably do a PR into this PR source branch with updates to Swift Sargon / iOS example app! We should decide who does that, if you want wanna do it @micbakos-rdx ? Should be straightforward I think.

Happy to do that myself. I need to add more tests which downgrade the coverage. These tests are a bit difficult to emulate. Also I would like us to decide on some things related to booting in today's design session, before jumping into the ios implementation.

@micbakos-rdx micbakos-rdx force-pushed the sargon_os_v1_android branch 2 times, most recently from 322f61b to a1a7841 Compare August 7, 2024 11:59
@micbakos-rdx micbakos-rdx marked this pull request as ready for review August 7, 2024 12:41
@micbakos-rdx micbakos-rdx merged commit 32b07fb into sargon_os_v1 Aug 7, 2024
10 checks passed
@micbakos-rdx micbakos-rdx deleted the sargon_os_v1_android branch August 7, 2024 13:16
Copy link
Contributor

@sergiupuhalschi-rdx sergiupuhalschi-rdx left a comment

Choose a reason for hiding this comment

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

@micbakos-rdx Outstanding work! Very well thought out and structured.

boot()
}

fun boot() = applicationScope.launch {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is called during init but it's also public, is it meant to be called again at a later point?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to split this into a few different modules, something like: StorageModule (with everything related to DataStore / SharedPreferences), NetworkingModule (with OkHttp stuff), etc

Copy link
Contributor

Choose a reason for hiding this comment

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

SecureStorageDriver and UnsafeStorageDriver could be implemented by 2 separate classes for better separation of concerns, or maybe there are some advantages to keep them together?

GhenadieVP added a commit that referenced this pull request Aug 12, 2024
* Rust: `SargonOS` &`Drivers` more Profile Logic | Swift: Shared State  (working example app)! (#131)

Introducing SargonOS

* MFA: Sec Structure of Factor <Source | ID | Instance>s (#150)

MFA Batch 1: SecurityStructureOfFactorSources and MatrixOfFactorSoures and also on FactorSourceID and FactorInstance level. Also add some new FactorSourceKinds at least a POC version of them or stubbed out ones.

* Sargon OS undo Profile JSON format change (#177)

Revert Profile JSON format changes in DeviceInfo

* Merge 'main' into 'sargon_os'

* wip

* Implement HostInfo struct (#189)

* Implement HostInfo struct

* Store host id in secure storage

* Fix doc

* Separate id and date from host info

* Implement HostOS that will encapsulate the different values and constants of each host os

* Expose uniffi method for creating device info from host info

* wip

* Sargon os v1 android (#196)

* Implement sargon os drivers

* Boot sargon os in example app

* Remove active profile id

* Add more unit tests

* Fix apple tests

* pr review

* version bump

---------

Co-authored-by: Alexander Cyon <116169792+CyonAlexRDX@users.noreply.github.com>
Co-authored-by: micbakos-rdx <michael.bakogiannis@rdx.works>
Co-authored-by: micbakos-rdx <125959264+micbakos-rdx@users.noreply.github.com>
micbakos-rdx added a commit that referenced this pull request Aug 14, 2024
* Rust: `SargonOS` &`Drivers` more Profile Logic | Swift: Shared State  (working example app)! (#131)

Introducing SargonOS

* MFA: Sec Structure of Factor <Source | ID | Instance>s (#150)

MFA Batch 1: SecurityStructureOfFactorSources and MatrixOfFactorSoures and also on FactorSourceID and FactorInstance level. Also add some new FactorSourceKinds at least a POC version of them or stubbed out ones.

* Sargon OS undo Profile JSON format change (#177)

Revert Profile JSON format changes in DeviceInfo

* Merge 'main' into 'sargon_os'

* Implement HostInfo struct (#189)

* Implement HostInfo struct

* Store host id in secure storage

* Fix doc

* Separate id and date from host info

* Implement HostOS that will encapsulate the different values and constants of each host os

* Expose uniffi method for creating device info from host info

* wip

* Sargon os v1 android (#196)

* Implement sargon os drivers

* Boot sargon os in example app

* Remove active profile id

* Add more unit tests

* Fix apple tests

* Change profile holder to keep profile state instead of profile

* Driver emits profile states instead of profile

* Implement new and delete wallet functions

* Remove unused comment

* Implement import functionality

* Propagate CRUD operations on profile to the profile state change driver.

* Remove unnecessary comment

* Fix swift tests

* Fix doc

* Prioritize bdfs save and then save profile in new wallet

* Implement retry read mechanism for profile snapshot.

This currently exists on Android codebase and needs revisiting.

* Bump version

---------

Co-authored-by: Alexander Cyon <116169792+CyonAlexRDX@users.noreply.github.com>
Co-authored-by: Ghenadie Vasiliev-Pusca <ghenadie.vasiliev-pusca@rdx.works>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Coverage OK ☂️ ✅ False Negatives in Code Coverage by Tarpaulin Kotlin 🤖 Changes in Kotlin Sargon Rust 🦀 Changes in Rust Sargon UniFFI 🦄 Changes in UniFFI exported APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants