`TransationManifest::securify_unsecurified_entity` & `ProvisionalSecurifiedConfig` #293

CyonAlexRDX · 2024-12-06T14:26:36Z

Preface

This should have been two PRs, sorry about that. It could have made sense for all the changes to be part of the same PR if I also implemented the SecurifyEntitiesManager - which I started doing because I'm bad at not-scope-creeping, but I did not wanna create yet another 6k LOC PR.

So two (somewhat distinct) changes this PR brings is:

TransactionManifest::securify_unsecurified_entity
Adds a provisional: Option<ProvisionalSecurifiedConfig> on UnsecurifiedEntityControl and SecurifiedEntityControl

Additional changes:

Removed authentication_signing: Option<HierarchicalDeterministicFactorInstance> field on UnsecurifiedEntityControl
Added authentication_signing_factor_instance: HierarchicalDeterministicFactorInstance field to SecurifiedEntityControl SecurityStructureOfFactorInstances

`TransactionManifest::securify_unsecurified_entity`

Resurrected impl From<Role> for ScryptoAccessRole (implemented by @0xOmarA ) from before #286 was merged (which temporarily removed this).

`ProvisionalSecurifiedConfig`

Once a user has selected some accounts and/or personas to securify with a new shield, those accounts should now retain the information that user wants to use some shield with them.

But the FactorInstancesProvider might not contain enough factors to securify all those accounts now. And user can always cancel derivation even if we try to derive more factors. **This means we cannot directly save SecurityStructureOfFactorInstances per account. **

So we start with saving the unique shield ID - SecurityStructureID associated with the accounts (or personas). This is the variant ShieldSelected(SecurityStructureID) of the new type ProvisionalSecurifiedConfig.

Then later when we were able to use the FactorInstanceProvider to provide factor instances for the SecurityStructureOfFactorSourceIds - identified by SecurityStructureID of ProvisionalSecurifiedConfig::ShieldSelected - we update the provisional state of all relevant entities in Profile
from: ProvisionalSecurifiedConfig::ShieldSelected
to: ProvisionalSecurifiedConfig::FactorInstancesDerived(SecurityStructureOfFactorInstances).

Later we might ask user to sign a batch of transaction. User might cancel, so we must support reminding user to batch sign these transaction later.

Later, once signed we update the provisional state of all relevant entities in Profile
from: ProvisionalSecurifiedConfig::FactorInstancesDerived
to: ProvisionalSecurifiedConfig::TransactionQueued(SecurityStructureOfFactorInstances, TXID).

I've put assertions on what is destructive / unsupported transitions:
FactorInstancesDerived(SecurityStructureOfFactorInstances) -> ShieldSelected is invalid for now, since those FactorInstance already have been deleted from the FactorInstancesCache so those factor instances would be lost, and we would have "gaps" in the derivation entity index - not the woooorst thing which can happen, but it makes Account Recovery Scan tricker in the future, since we would need to scan a broader range. So we should try to avoid creating such gaps. By "invalid for now" I mean this PR does not support it. But we can probably quite easily support this as a feature, by allowing us to "put instance back into the cache"!

Furthermore transitions from TransactionQueued to FactorInstancesDerived is not directly supported by set_provisional, I've exposed a dedicated method for it called cancel_queued_transaction, making it clear that we should first actually cancel the transaction and removed it from the queue, and then call account.entity_security_state.cancel_queued_transaction(..).

codecov · 2024-12-06T14:39:11Z

Codecov Report

Attention: Patch coverage is 95.68106% with 13 lines in your changes missing coverage. Please review.

Project coverage is 93.3%. Comparing base (0c5278b) to head (3ba71d3).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...a/secured_entity_control/secured_entity_control.rs	90.0%	4 Missing ⚠️
..._structures/matrices/matrix_of_factor_instances.rs	94.8%	2 Missing ⚠️
.../sargon/src/system/sargon_os/sargon_os_accounts.rs	0.0%	2 Missing ⚠️
crates/sargon/src/types/samples/persona_samples.rs	95.5%	2 Missing ⚠️
...100/entity_security_state/entity_security_state.rs	94.7%	1 Missing ⚠️
...l/manifest_building/manifests_access_controller.rs	93.7%	1 Missing ⚠️
...low_level/transaction_hashes/transaction_hashes.rs	0.0%	1 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##            main    #293    +/-   ##
======================================
  Coverage   93.2%   93.3%            
======================================
  Files       1104    1109     +5     
  Lines      23303   23528   +225     
  Branches      79      79            
======================================
+ Hits       21735   21955   +220     
- Misses      1553    1558     +5     
  Partials      15      15

Flag	Coverage Δ
kotlin	`97.1% <ø> (ø)`
rust	`92.8% <95.6%> (+<0.1%)`	⬆️
swift	`94.8% <100.0%> (-0.1%)`	⬇️

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

Files with missing lines	Coverage Δ
...pple/Sources/Sargon/Protocols/EntityProtocol.swift	`100.0% <100.0%> (ø)`
crates/sargon/src/core/error/common_error.rs	`93.3% <ø> (ø)`
...ates/sargon/src/core/types/signatures/signature.rs	`88.5% <100.0%> (+1.4%)`	⬆️
...e/logic/profile_network/profile_network_details.rs	`98.7% <100.0%> (+1.4%)`	⬆️
...or_instance_level/role_into_scrypto_access_rule.rs	`100.0% <100.0%> (ø)`
..._factors/security_structure_of_factor_instances.rs	`100.0% <100.0%> (ø)`
...factors/security_structure_of_factor_source_ids.rs	`100.0% <ø> (+13.3%)`	⬆️
.../sargon/src/profile/v100/entity/account/account.rs	`96.5% <100.0%> (+1.7%)`	⬆️
...rgon/src/profile/v100/entity/has_security_state.rs	`100.0% <100.0%> (ø)`
.../sargon/src/profile/v100/entity/persona/persona.rs	`99.3% <100.0%> (+<0.1%)`	⬆️
... and 24 more

... and 1 file with indirect coverage changes

…stances

README.md

CyonAlexRDX · 2024-12-16T14:59:28Z

crates/sargon-uniffi/src/profile/mfa/secured_entity_control/secured_entity_control.rs

+
+    /// A provisional new security structure configuration which user
+    /// is about to change to
+    pub provisional: Option<ProvisionalSecurifiedConfig>,


Alternatives to provisional:

draft - discarded since it sounds like it is not "saved" or otherwise incomplete.

pending - make it sound like it has been submitted to the network and waiting for a tx complete

user_intention - some version of this might be ok.

Definition of provisional: "arranged or existing for the present, possibly to be changed later."

Temporal?

You mean temporary I think? Hmm temporary relates more to time, whereas I'm going for a term which relates more to step in a sequence of steps, which I feel is a little bit more precise. And "temporary" also has a meaning that after some time it is no longer relevant, whereas what is actually happening here is that it holds the state which will be the future state, so not at all temporary, just that we are note quite there yet.

I asked ChatGPT about the nuances of "temporal" vs "temporary"

crates/sargon-uniffi/src/profile/v100/entity_security_state/provisional_securified_config.rs

CyonAlexRDX · 2024-12-16T15:01:47Z

.../security_structures/security_structure_of_factors/security_structure_of_factor_instances.rs

Moved, and added authentication_signing_factor_instance: HierarchicalDeterministicFactorInstance,

also added new ctor with validation!

CyonAlexRDX · 2024-12-16T15:02:20Z

...security_structures/security_structure_of_factors/security_structure_of_factor_source_ids.rs

-        security_structure_id: SecurityStructureID,
-        matrix_of_factors: MatrixOfFactorInstances,
-    ) -> Self {
-        Self {


was not throwing, is now!

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

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

CyonAlexRDX · 2024-12-16T15:48:57Z

crates/sargon/fixtures/transaction/create_access_controller_for_account.rtm

+                Array<Enum>(
+                    Enum<0u8>(
+                        Enum<2u8>(
+                            2u8,


this 2 is threshold

CyonAlexRDX · 2024-12-16T15:52:17Z

crates/sargon/fixtures/transaction/create_access_controller_for_account.rtm

+;
+CREATE_ACCESS_CONTROLLER
+    Bucket("bucket1")
+    Tuple(


@0xOmarA the RuleSet uses position (index) to determine which AccessRule is primary, recovery and confirmation? Because The values of the Enum seems to be the same?

Enum<2u8>( Enum<1u8>( Array<Enum>(

for all three?

It does indeed use the position to determine that since these are just arguments to the function.

CyonAlexRDX · 2024-12-16T15:56:02Z

crates/sargon/src/profile/mfa/security_structures/matrices/matrix_of_factor_instances.rs

@@ -105,12 +173,17 @@ impl MnemonicWithPassphrase {
 impl MatrixOfFactorInstances {
    fn sample_from_matrix_of_sources(
        matrix_of_sources: MatrixOfFactorSources,
+        entity_kind: CAP26EntityKind,


we now have stricter validation so that we do not accidentally use Account factorinstances for Persona or vice versa.

CyonAlexRDX · 2024-12-16T15:56:27Z

...curity_structures/roles/factor_levels/factor_instance_level/role_into_scrypto_access_rule.rs

+use crate::prelude::*;
+
+impl<const ROLE: u8> From<RoleWithFactorInstances<ROLE>> for ScryptoAccessRule {
+    fn from(value: RoleWithFactorInstances<ROLE>) -> Self {


small upgrade of impl we got from @0xOmarA some months ago.

CyonAlexRDX · 2024-12-16T15:56:52Z

...curity_structures/roles/factor_levels/factor_instance_level/role_into_scrypto_access_rule.rs

+
+pub const MINUTES_PER_DAY: u32 = 24 * 60;
+
+impl From<SecurityStructureOfFactorInstances> for ScryptoRecoveryProposal {


this PR does not make use of this From for ScryptoRecoveryProposal

GhenadieVP

Nice work! Left some minor-ish comments.

GhenadieVP · 2024-12-17T07:48:32Z

...rofile/mfa/security_structures/security_structures/security_structure_of_factor_instances.rs

+    /// Non-optional since we can replace it with a new one for entities
+    /// we have recovered during Onboarding Account Recovery Scan for securified
+    /// entities
+    pub authentication_signing_factor_instance:


Need to update the docs for the SecurityStructureOfFactorInstances as it is no more accurate.
@micbakos-rdx FYI, this will be used for ROLA signing.

GhenadieVP · 2024-12-17T07:54:10Z

crates/sargon-uniffi/src/profile/v100/entity_security_state/unsecured_entity_control.rs

-    /// The factor instance which can be used for ROLA.
-    pub authentication_signing: Option<HierarchicalDeterministicFactorInstance>,
+    /// The provisional security structure configuration
+    pub provisional: Option<ProvisionalSecurifiedConfig>,


maybe provisional_securified_configu

GhenadieVP · 2024-12-17T07:56:32Z

crates/sargon/src/profile/logic/profile_network/profile_network_details.rs

-            }
-            _ => false, // TODO change that
-        };
+        let has_auth_signing_key = persona.is_securified();


what does has_auth_signing_key mean? That it literally as a dedicated signing key or that there is some key for signing?

AuthorizedPersonaDetailed struct contains that field. Not sure we use it anywhere in hosts, but I did not wanna change that. And the assignment let has_auth_signing_key = persona.is_securified(); is correct. ALL Securified Entties has an auth signing factor. No Unsecurified has one.

@CyonAlexRDX We discussed using transaction_signing factor instance when an entity is not securified. Doesn't that mean that has_auth_signing_key will always be true? Or does has_auth_signing_key mean like a true ROLA key?

does has_auth_signing_key mean like a true ROLA key?

YES. auth signing KeyKind/Instance == true ROLA

so rather has_auth_signing_key will always be false for unsecurified entities. But unsecurified entities can ofc proof-ownership (which it does using transaction_signing.)

I'm not sure this field is at all relevant. I think it might have been used by iOS behind DEBUG flag. So we should not put too much importance on this field.

Thanks Alex, indeed it was used to disable the button that created the rola key for unsecurified entities (under debug ofc). Indeed unneeded.

GhenadieVP · 2024-12-17T08:01:16Z

crates/sargon/src/profile/mfa/secured_entity_control/secured_entity_control.rs

+                // We have already consumed FactorInstances from the FactorInstancesCache.
+                // We currently do not have a way of putting these back. We don't want to 
+                // create gaps if we can. Wallet Features might change, to REQUIRE us to
+                // allow this, if so we can change this - allowing transition from 
+                // `FactorInstancesDerived` to Non-TransactionQueued state.
+                Err(
+                CommonError::SecurityEntityControlCannotChangeProvisionalAlreadyDerivedInstances,


This can happen quite easily if user updates a shield a second time before applying it. So most likely we will need to handle this, let's discuss in design session how to do it.

GhenadieVP · 2024-12-17T08:07:25Z

...curity_structures/roles/factor_levels/factor_instance_level/role_into_scrypto_access_rule.rs

+    }
+}
+
+pub const MINUTES_PER_DAY: u32 = 24 * 60;


We do have a constants file

constants which have a "natural home" - should be put in a natural home I think. That is most Rusty I think. The constants file seems to contain mostly constants relating to GW?

well I agree, at the same time MINUTES_PER_DAY is very generic. Anyway, this is minor, we'll migrate in case there will be other components using this.

right, ok agreed, this should be moved, since it is common relating to time!

.../security_structures/security_structure_of_factors/security_structure_of_factor_instances.rs

GhenadieVP · 2024-12-17T08:18:01Z

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

@@ -294,7 +294,7 @@ impl Profile {
            .items()
            .into_iter()
            .map(Into::<AccountOrPersona>::into)
-            .map(|e| (e.clone(), e.unique_factor_instances()))
+            .map(|e| (e.clone(), e.unique_tx_signing_factor_instances()))


Why not all instances including authentication_signing?

good catch, this should have been unique_all_factor_instances, thx, will change

micbakos-rdx · 2024-12-17T08:55:14Z

...rofile/mfa/security_structures/security_structures/security_structure_of_factor_instances.rs

+
+    /// The authentication signing factor instance which is used to sign
+    /// proof of ownership - aka "True Rola Key". User can select which FactorSource
+    /// to use during Shield Building, but typically most users will user the


Typo in will user

micbakos-rdx · 2024-12-17T09:01:44Z

crates/sargon/src/profile/logic/profile_network/profile_network_details.rs

-            }
-            _ => false, // TODO change that
-        };
+        let has_auth_signing_key = persona.is_securified();


@CyonAlexRDX We discussed using transaction_signing factor instance when an entity is not securified. Doesn't that mean that has_auth_signing_key will always be true? Or does has_auth_signing_key mean like a true ROLA key?

Reintroduce From<RoleWithFactorInstances<ROLE>> for ScryptoAccessRule

c91f8f6

impl From<MatrixOfFactorInstances> for ScryptoRuleSet

2d3259e

CyonAlexRDX changed the title ~~Reintroduce From<RoleWithFactorInstances<ROLE>> for ScryptoAccessRule~~ [WIP] Into (Scrypto) AccessRule & RuleSet for MatrixOfFactorInstances Dec 10, 2024

CyonAlexRDX and others added 9 commits December 12, 2024 14:30

Merge branch 'main' into scrypto_access_rules_for_matrix_of_factor_in…

502ba59

…stances

bump Scrypto and RET

7eb9e2d

[no ci] WIP

4f39117

[no ci] WIP

bf57732

[no ci] wip

ed7f78d

create_access_controller

9f7790d

[no ci] WIP

332c6a9

[no ci] wip

9cded45

merge

8faaf3e

CyonAlexRDX force-pushed the scrypto_access_rules_for_matrix_of_factor_instances branch from 670f899 to 8faaf3e Compare December 16, 2024 09:07

Sajjon and others added 5 commits December 16, 2024 10:45

[no ci] WIP

5efcfff

[no ci] wip

20fccd0

[no ci] WIP

f2ba3fa

[no ci] WIP

642c9cc

update readme

d68c597

CyonAlexRDX marked this pull request as ready for review December 16, 2024 14:01

CyonAlexRDX changed the title ~~[WIP] Into (Scrypto) AccessRule & RuleSet for MatrixOfFactorInstances~~ TransationManifest: create_access_controller & ProvisionalSecurifiedConfig Dec 16, 2024

CyonAlexRDX changed the title ~~TransationManifest: create_access_controller & ProvisionalSecurifiedConfig~~ TransationManifest: securify_unsecurified_entity & ProvisionalSecurifiedConfig Dec 16, 2024

CyonAlexRDX changed the title ~~TransationManifest: securify_unsecurified_entity & ProvisionalSecurifiedConfig~~ TransationManifest::securify_unsecurified_entity & ProvisionalSecurifiedConfig Dec 16, 2024

CyonAlexRDX requested review from GhenadieVP, micbakos-rdx and danvleju-rdx December 16, 2024 14:55