Sargon OS Boot fixes #212

micbakos-rdx · 2024-09-10T10:59:09Z

Sargon Rust changes

A new function called derive_wallet in sargon os. This method can be used when restoring a profile from only a seed phrase and some accounts retrieved from GW.
A new error is introduced SecureStorageAccessError. This is used specifically in Android were the secure storage driver may fail to read from or write to secure storage key which is secured under biometrics. Currently mnemonics fall under this category. This error can be caused by several states of the system. Since I didn't want to make this Android specific, I introduced SecureStorageAccessErrorKind which will mostly be used for Android. Most of these kinds are system related and two are related to the user cancelling the authorisation. Although, a suggestion, would be for iOS to return SecureStorageAccessError(errorKind: .userCancelled), maybe when authorisation is cancelled since this seems to be the only possible error on iOS. Whatever the decision is, it will not make any difference.

Sargon Android changes

SargonOsManager: Holds a reference to sargon os. Boots sargon when initialised. Implemented as a singleton.
AndroidProfileStateChangeDriver: Holds the state of the current ProfileState no need for the host to do that.
Fixed encrypt method to first encrypt and then encode to base64 and not the other way around. This was a bug.
Fixed biometric requests when the system opens the pattern or pin screens and not the biometrics screen. These screens are usually full screen, causing the app to pause and stop. Before this was buggy because it did not account for that case.
Inreoduced KeySpec. reset. Please take a look at its doc to understand what it does and what problems it fixes especially on Android API <= 30

codecov · 2024-09-10T11:08:28Z

Codecov Report

Attention: Patch coverage is 93.71429% with 11 lines in your changes missing coverage. Please review.

Project coverage is 96.6%. Comparing base (424a98f) to head (f903fe3).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...argon/src/system/sargon_os/profile_state_holder.rs	83.3%	2 Missing ⚠️
crates/sargon/src/system/sargon_os/sargon_os.rs	94.5%	2 Missing ⚠️
...ain/java/com/radixdlt/sargon/os/SargonOsManager.kt	87.5%	0 Missing and 2 partials ⚠️
...on/src/profile/v100/header/profile_id_uniffi_fn.rs	80.0%	1 Missing ⚠️
...ent/secure_storage_client/secure_storage_client.rs	90.0%	1 Missing ⚠️
...ecure_storage_driver/support/secure_storage_key.rs	95.4%	1 Missing ⚠️
...s/sargon/src/system/sargon_os/sargon_os_profile.rs	92.8%	1 Missing ⚠️
...com/radixdlt/sargon/os/storage/EncryptionHelper.kt	92.3%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #212     +/-   ##
=======================================
- Coverage   97.0%   96.6%   -0.4%     
=======================================
  Files        941     684    -257     
  Lines      14863   12611   -2252     
  Branches      66      70      +4     
=======================================
- Hits       14426   12193   -2233     
+ Misses       430     408     -22     
- Partials       7      10      +3

Flag	Coverage Δ
rust	`96.5% <94.3%> (+<0.1%)`	⬆️
swift	`?`

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

Files with missing lines	Coverage Δ
crates/sargon/src/core/error/common_error.rs	`100.0% <ø> (ø)`
...sargon/src/profile/logic/account/create_account.rs	`87.8% <ø> (ø)`
...sargon/src/profile/logic/account/query_accounts.rs	`100.0% <100.0%> (ø)`
...argon/src/profile/logic/gateway/current_gateway.rs	`100.0% <100.0%> (ø)`
...rates/sargon/src/profile/v100/header/profile_id.rs	`100.0% <ø> (+11.1%)`	⬆️
...c/profile/v100/networks/network/profile_network.rs	`100.0% <100.0%> (ø)`
...rgon/src/profile/v100/networks/profile_networks.rs	`100.0% <100.0%> (ø)`
crates/sargon/src/profile/v100/profile.rs	`99.1% <100.0%> (+<0.1%)`	⬆️
...rates/sargon/src/profile/v100/profile_uniffi_fn.rs	`100.0% <100.0%> (ø)`
crates/sargon/src/system/drivers/drivers.rs	`100.0% <ø> (ø)`
... and 14 more

... and 261 files with indirect coverage changes

CyonAlexRDX · 2024-09-10T11:09:13Z

crates/sargon/src/system/sargon_os/sargon_os.rs

@@ -151,6 +151,38 @@ impl SargonOS {
        Ok(())
    }

+    pub async fn derive_wallet(


Why "derive"? This is not deriving anything..?

Why "wallet"?

Can we name it something "profile with accounts" ?

Wanted to keep the wallet name consistent with the other "entry" methods like

new_wallet

import_wallet

delete_wallet

Maybe new_wallet_with_derived_bdfs ?

Oh I had forgot we had functions called import_wallet.

Right I think my idea is that we loosely define "wallet = (Profile, (BDFS) Mnemonic).

Maybe new_profile_with_restored_accounts?

Btw! Are we asserting that the SecureStorage does in fact contain the device factor source?

If not, we should right?

Unfortunately that is impossible. I wanted to do the same, but in order to do that I need another biometric prompt. The same should apply to import wallet.

Why is that?

If we need to ask for biometrics only once, we need to pass the mnemonic to ffi. I am not sure if we want to do that. If we ever did that, then sargon os could check the validity and store it. Thus having total control over it.

Otherwise we let the host store the mnemonic, as it used to, but now this method will need to read the mnemonic again, asking for biometrics for a second time.

Right, I think we should do any of those too.

We are already passing the mnemonic across FFI in the form of entropy. So I think best pass a PrivateHierarchicalDeterministicFactorSource from Host, that way we know user has intact "wallet"

Or well, we know if we also check that the accounts are controlled by that DeviceFactorSource

I am in favour of passing the mnemonic over FFI, unless there are some specific concerns. Can't see any in particular.

We are already passing the mnemonic across FFI in the form of entropy.

You are right @CyonAlexRDX I forgot about that. Will make the changes.

@CyonAlexRDX I have implemented the logic to

pass thePrivateHierarchicalDeterministicFactorSource into the function

Validate the accounts public keys

Ensure that secure storage succeeds saving the mnemonic
84c7af6

Please let me know if the execution is valid

CyonAlexRDX · 2024-09-10T11:33:03Z

crates/sargon/src/profile/v100/profile.rs

+
+        let mainnet_network = match maybe_accounts {
+            None => ProfileNetwork::new_empty_on(NetworkID::Mainnet),
+            Some(accounts) => ProfileNetwork::new_with_accounts(


Should we not assert that all accounts are controlled by the BDFS? Maybe not, if accounts passed is a mix of Ledger accounts has software.

Hmmm

Wanted to ask about that. We are currently only checking if the accounts are not on mainnet.

Issue is that this probably seems like a very generic purpose API, while it should be used only when recovering with a Seed Phrase which by definition can be only BDFS currently. It is meant to replace this Host logic.

crates/sargon/src/system/sargon_os/sargon_os.rs

...n-android/src/main/java/com/radixdlt/sargon/os/driver/AndroidBiometricAuthorizationDriver.kt

jvm/sargon-android/src/main/java/com/radixdlt/sargon/os/driver/AndroidStorageDriver.kt

jvm/sargon-android/src/main/java/com/radixdlt/sargon/os/storage/KeySpec.kt

crates/sargon/src/profile/v100/profile.rs

examples/android/src/main/java/com/radixdlt/sargon/android/ExampleApplication.kt

giannis-rdx · 2024-09-11T08:13:26Z

...n-android/src/main/java/com/radixdlt/sargon/os/driver/AndroidBiometricAuthorizationDriver.kt

-                    biometricsResultsChannel.send(result)
-                }
+        biometricRequestsChannel
+            .receiveAsFlow()


why you define the biometricRequestsChannel and biometricsResultsChannel as Channel and then receive the as Flow?
Why not to define them as Flows?

I wanted to implement a logic of two cold flows. Sargon os can ask for biometrics at any time. So I wanted to solve 2 problems

No activity is yet STARTED in order to handle biometrics. Remember in order to ask for biometrics, we need a reference to a FragmentActivity. In order for the biometrics dialog to show up, we need the activity to be at least started. So I wanted to have a suspending function waiting for an activity to either register and start or return from background. This is why biometricRequestsChannel exists. It waits for the request to be handled by one activity.

The biometricsResultsChannel is waiting for the USER to respond. The values of the channel will be emitted by another function.

Sajjon · 2024-09-11T11:40:25Z

crates/sargon/src/system/sargon_os/sargon_os.rs

+                    .map(|c| c.transaction_signing.public_key)
+                    .map_err(|_| CommonError::EntitiesNotDerivedByFactorSource)
+            })
+            .try_collect()?;


cool, I did not know about try_collect, we should embrace it!

I think I'd prefer let ..: Vec<T> = ....try_collect()? be rewritten as let .. = ....try_collect::<Vec<_>>()?

where try_collect::<Vec<_>>() is verbatim, the _ usage tells rust to fill it in (which it can do based on .map(|c| c.transaction_signing.public_key).

That is the code style I've mostly used elsewhere in Sargon - the advantage is that for long chains of functional calls one can control the return type at the end of the chain, which one typically does by adding yet another map that makes it a bit easier to maintain IMO.

I like doing this too, but this does not compile. I cannot find a sane way to make this compile.

An example of this working is this:

.try_collect::<_, Vec<_>, _>()?;

Does this look more readable?

Ah it is generic over many things. Right, yeah I prefer that actually

Sajjon · 2024-09-11T11:41:07Z

crates/sargon/src/system/sargon_os/sargon_os.rs

+
+        if !hd_factor_source
+            .mnemonic_with_passphrase
+            .validate_public_keys(hd_keys)


nice! so now we assert that all factor instances are indeed controlled by the specified mnemonic

CyonAlexRDX

LGTM,I like the change to PrivateHDFS.

Left a minor remark about code style

sergiupuhalschi-rdx · 2024-09-11T11:46:27Z

jvm/sargon-android/src/test/java/com/radixdlt/sargon/TransactionManifestTest.kt

                ),
                AccountLockerClaimableResource.Fungible(
                    resourceAddress = ResourceAddress.init("resource_rdx1tknxxxxxxxxxradxrdxxxxxxxxx009923554798xxxxxxxxxradxrd"),
                    amount = 1500.toDecimal192()
                ),
                AccountLockerClaimableResource.NonFungible(
                    resourceAddress = ResourceAddress.init("resource_rdx1n2ekdd2m0jsxjt9wasmu3p49twy2yfalpaa6wf08md46sk8dfmldnd"),
-                    ids = listOf(NonFungibleLocalId.stringId("foobar"))
+                    numberOfItems = 1.toULong()


Sorry about this, it escaped me while working on the account locker. It's good that we have the kotlin-test workflow fixed now and it shouldn't happen anymore.

Thanks @sergiupuhalschi-rdx wanted to ask you about this

sergiupuhalschi-rdx

Great job! It's impressive how you found what was the issue with the Keystore secret key generation and fixed it 🏅

GhenadieVP

LGTM! Amazing work 🔥!

giannis-rdx

🦅

micbakos-rdx · 2024-09-16T10:07:12Z

@GhenadieVP Latest commit 8505a88 addresses deleting old ephemeral profile and associated mnemonic when booting again.

…mutated profile

* wip * wip * fix test * wip * fix ios tests * test * update * wip

* wip * fix * fix * wip * wip * wip

micbakos-rdx added Rust 🦀 Changes in Rust Sargon Kotlin 🤖 Changes in Kotlin Sargon UniFFI 🦄 Changes in UniFFI exported APIs labels Sep 10, 2024

micbakos-rdx requested review from CyonAlexRDX, GhenadieVP, giannis-rdx and sergiupuhalschi-rdx September 10, 2024 10:59

micbakos-rdx self-assigned this Sep 10, 2024

micbakos-rdx mentioned this pull request Sep 10, 2024

Sargon boot radixdlt/babylon-wallet-android#1191

Merged

1 task

CyonAlexRDX reviewed Sep 10, 2024

View reviewed changes

GhenadieVP reviewed Sep 10, 2024

View reviewed changes

giannis-rdx reviewed Sep 11, 2024

View reviewed changes

micbakos-rdx requested review from CyonAlexRDX, GhenadieVP and giannis-rdx September 11, 2024 11:30

Sajjon reviewed Sep 11, 2024

View reviewed changes

CyonAlexRDX approved these changes Sep 11, 2024

View reviewed changes

sergiupuhalschi-rdx reviewed Sep 11, 2024

View reviewed changes

sergiupuhalschi-rdx approved these changes Sep 11, 2024

View reviewed changes

GhenadieVP approved these changes Sep 11, 2024

View reviewed changes

giannis-rdx approved these changes Sep 11, 2024

View reviewed changes

micbakos-rdx force-pushed the fixes branch from 6a0fe60 to 8f19600 Compare September 12, 2024 07:25

micbakos-rdx force-pushed the fixes branch from fb39a50 to 7258cb4 Compare September 27, 2024 12:06

micbakos-rdx added 3 commits September 30, 2024 15:00

Replace kotlin base64 with android base 64 and mock it in unit tests

54e834c

Make profileStateChanges a shared flow

1a26596

Remove planting of debug tree

c74384f

micbakos-rdx and others added 26 commits September 30, 2024 15:00

Remove checks before asking for biometrics

a54e521

Suspend request for biometrics until activity is at least started

64fe07c

Report secure storage access error

f1c6ebe

Make read and write methods public

e190154

Make encrypt and decrypt methods public

6c07461

Implement function to remove a list of keyspecs

0879e59

Make generateSecretKey public for now

48c5dca

Add error kind for biometrics failure

7968a8f

Export method to derive profile based on bdfs and some accounts

b16c975

Emit derived profile in profile state change

68e6c7c

Store profile state into android state change driver

b4e5352

Make reset keyspec method to regenerate a new key when fail

fba06db

Included SargonOsManager into sargon android

4089ab3

Make drivers explicit

58c5d22

Replace with new_wallet_with_derived_bdfs

2cfe61d

Change doc

0c0057b

Allow set profile to set one profile when none exists

f7720c3

Change iOS test

9b554cc

Fix android test

4e07965

Fix set_profile to notify ProfileStateChange client about the latest …

ef45de0

…mutated profile

Fix content hint to report correct total amount of personas

43eda31

Create initial profile with no networks

bd365aa

Delete ephemeral profile when booting

ca40ae4

Return error when network does not yet exist

7d6a571

Profile snapshot id payload (#214)

3d8d347

* wip * wip * fix test * wip * fix ios tests * test * update * wip

Bump cargo version

ad4feee

micbakos-rdx force-pushed the fixes branch from 7258cb4 to ad4feee Compare September 30, 2024 12:00

Profile safe concurrent access (#225)

f903fe3

* wip * fix * fix * wip * wip * wip

GhenadieVP merged commit 817779d into main Sep 30, 2024
11 of 12 checks passed

GhenadieVP deleted the fixes branch September 30, 2024 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sargon OS Boot fixes #212

Sargon OS Boot fixes #212

micbakos-rdx commented Sep 10, 2024 •

edited

Loading

codecov bot commented Sep 10, 2024 •

edited

Loading

CyonAlexRDX Sep 10, 2024

micbakos-rdx Sep 10, 2024 •

edited

Loading

CyonAlexRDX Sep 10, 2024

micbakos-rdx Sep 10, 2024

CyonAlexRDX Sep 10, 2024

CyonAlexRDX Sep 10, 2024

GhenadieVP Sep 10, 2024

micbakos-rdx Sep 11, 2024

micbakos-rdx Sep 11, 2024

CyonAlexRDX Sep 10, 2024

micbakos-rdx Sep 10, 2024

GhenadieVP Sep 10, 2024

giannis-rdx Sep 11, 2024

micbakos-rdx Sep 11, 2024

Sajjon Sep 11, 2024

micbakos-rdx Sep 11, 2024

micbakos-rdx Sep 11, 2024

CyonAlexRDX Sep 11, 2024

Sajjon Sep 11, 2024

CyonAlexRDX left a comment

sergiupuhalschi-rdx Sep 11, 2024 •

edited

Loading

micbakos-rdx Sep 11, 2024

sergiupuhalschi-rdx left a comment

GhenadieVP left a comment

giannis-rdx left a comment

micbakos-rdx commented Sep 16, 2024

Sargon OS Boot fixes #212

Sargon OS Boot fixes #212

Conversation

micbakos-rdx commented Sep 10, 2024 • edited Loading

Sargon Rust changes

Sargon Android changes

codecov bot commented Sep 10, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

micbakos-rdx Sep 10, 2024 • edited Loading

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

CyonAlexRDX left a comment

Choose a reason for hiding this comment

sergiupuhalschi-rdx Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergiupuhalschi-rdx left a comment

Choose a reason for hiding this comment

GhenadieVP left a comment

Choose a reason for hiding this comment

giannis-rdx left a comment

Choose a reason for hiding this comment

micbakos-rdx commented Sep 16, 2024

micbakos-rdx commented Sep 10, 2024 •

edited

Loading

codecov bot commented Sep 10, 2024 •

edited

Loading

micbakos-rdx Sep 10, 2024 •

edited

Loading

sergiupuhalschi-rdx Sep 11, 2024 •

edited

Loading