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

FIP: Support mix of Accounts & Personas #306

Merged

Conversation

CyonAlexRDX
Copy link
Contributor

@CyonAlexRDX CyonAlexRDX commented Dec 17, 2024

FactorInstancesProvider - FIP - does not support providing instances for a mix of DerivationPresets - meaning we cannot ask it for instance for accounts and personas mixed.

Today one would need to do "two passes" to the FIP => bad UX since if the cache could not fully satisfied any requests => two passes to the KeysCollector (once per pass to the FIP).

What we want is a single pass to the KeysCollector in case of not enough instances in cache.

Thus what we need is to support multiple derivation presets queried.

Changes

  • FIP now supports providing instances for a mix of accounts and personas
  • FIP now derives ROLA keys for accounts and personas for new Factors and for securifying accounts If cache could not satisfy request. Also upgraded the NextDerivationEntityIndexProfileAnalysingAssigner to analyse profile and return next derivation index for
    ROLA factors.
  • SecurityStructureOfFactor{Source/SourceIds} now contain an authentication_signing_factor field
  • SecurityShieldBuilders (Sargon and Sargon-UniFFI) now exposes methods for setting Auth Signing factor
  • Auto Shield Builder now supports ROLA key: auto assign a Device FactorSource as authentication_signing factor.
  • Split single const CACHE_FILLING_QUANTITY into one const per preset (they still have the same value: 30)

Important

This PR does NOT validate the FactorSourceKind used for Authentication Signing Factor. So the SecurityShieldBuilder need to validate that. A tiny PR by anyone fixing this post getting this merged is welcome! See owner row in table https://radixdlt.atlassian.net/wiki/spaces/AT/pages/3758063620/MFA+Rules+for+Factors+and+Security+Shields#Factor-Type-Usage-Rules

@CyonAlexRDX CyonAlexRDX changed the title Ac/mix persona and accounts in factor instances provider Ac/and factor instances provider Dec 17, 2024
@CyonAlexRDX CyonAlexRDX changed the title Ac/and factor instances provider [WIP] FIP: Support mix of Accounts & Personas Dec 17, 2024
Copy link

codecov bot commented Dec 18, 2024

Codecov Report

Attention: Patch coverage is 93.69863% with 46 lines in your changes missing coverage. Please review.

Project coverage is 93.3%. Comparing base (c4ed26a) to head (ee84209).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ces_provider/provider/factor_instances_provider.rs 92.6% 12 Missing ⚠️
.../sargon/src/system/sargon_os/sargon_os_accounts.rs 85.4% 9 Missing ⚠️
...rgon/src/profile/v100/entity/has_security_state.rs 72.2% 5 Missing ⚠️
...provider/factor_instances_cache/keyed_instances.rs 0.0% 4 Missing ⚠️
...r/factor_instances_cache/factor_instances_cache.rs 98.1% 2 Missing ⚠️
...come/internal_factor_instances_provider_outcome.rs 94.7% 2 Missing ⚠️
...pters/securify_entity_factor_instances_provider.rs 83.3% 2 Missing ⚠️
...actor_instance_level/role_with_factor_instances.rs 33.3% 2 Missing ⚠️
...c/profile/v100/networks/network/profile_network.rs 84.6% 2 Missing ⚠️
...rgon/src/profile/v100/networks/profile_networks.rs 50.0% 2 Missing ⚠️
... and 3 more
Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff           @@
##            main    #306    +/-   ##
======================================
  Coverage   93.3%   93.3%            
======================================
  Files       1110    1111     +1     
  Lines      23595   24043   +448     
  Branches      79      79            
======================================
+ Hits       22025   22445   +420     
- Misses      1555    1583    +28     
  Partials      15      15            
Flag Coverage Δ
kotlin 97.1% <ø> (ø)
rust 92.8% <93.6%> (+<0.1%) ⬆️
swift 94.8% <ø> (ø)

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 93.3% <ø> (ø)
...n/src/core/types/keys/key_agreement/private_key.rs 100.0% <ø> (ø)
...ances_provider/agnostic_paths/derivation_preset.rs 100.0% <100.0%> (ø)
...ces_provider/agnostic_paths/index_agnostic_path.rs 98.6% <100.0%> (+<0.1%) ⬆️
...or_instances_provider/agnostic_paths/quantities.rs 100.0% <100.0%> (ø)
...ivation_entity_index_profile_analyzing_assigner.rs 100.0% <100.0%> (ø)
..._derivation_entity_index_with_ephemeral_offsets.rs 100.0% <ø> (ø)
...vider/outcome/factor_instances_provider_outcome.rs 100.0% <100.0%> (ø)
...al_factor_instances_provider_outcome_for_factor.rs 100.0% <100.0%> (ø)
...rovider/provider/provider_adopters/cache_filler.rs 97.4% <100.0%> (+1.7%) ⬆️
... and 44 more

... and 1 file with indirect coverage changes

/// unsecurified entities. For already securified entities we might not
/// need to change the ROLA key.
fn mfa_for_entities(
addresses_of_entities: &IndexSet<AddressOfAccountOrPersona>,
Copy link
Contributor

Choose a reason for hiding this comment

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

In future when we support updating of Securified Entities we probably wanna change:

fn mfa_for_entities(
    addresses_of_entities: &IndexSet<AddressOfAccountOrPersona>,
    include_rola_key_for_each_entity: bool
) -> IdentifiedVecOf<Self>

into:

struct DeriveInstancesForEntityRequest {
    entity_address: AddressOfAccountOrPersona,
   need_new_rola: bool
}

fn mfa_for_entities(
    requests: &IndexSet<DeriveInstancesForEntityRequest>,
) -> IdentifiedVecOf<Self>

/// When applying a security shield to an account, initiates keys collection
/// When applying a security shield to accounts and personas mixed, initiates keys collection
/// for account MFA
SecurifyingAccountsAndPersonas,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@micbakos-rdx we will automatically select this as DerivationPurpose if we are securifying both account and personas mixed - but as we have talked about in the past, host probably maps all keys to the same copy.

@@ -137,136 +154,220 @@ impl FactorInstancesProvider {

async fn derive_more_and_cache(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

upgraded to be PER DerivationPreset and then PER FactorSource

@@ -5,15 +5,19 @@ use RoleKind::*;
/// A tiny holder of factors for each Role.
/// Used by the AutomaticShieldBuilder to keep track of which factors are assigned to which role.
#[derive(Clone, Debug, PartialEq, Eq)]
pub(super) struct ProtoMatrix {
pub(super) struct ProtoShield {
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 could act as a "state snapshot" of a SecurityShieldBuilder if we want. I.e. a UniFFI version of this. But needs name added though. and maybe ID/metadata fields?

Copy link
Contributor

@GhenadieVP GhenadieVP left a comment

Choose a reason for hiding this comment

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

LGTM! Nice work!

Left some minor comments.

@@ -23,6 +23,10 @@ pub enum DerivationPurpose {
/// for identity MFA
SecurifyingPersona,

/// When applying a security shield to accounts and personas mixed, initiates keys collection
/// for account MFA
SecurifyingAccountsAndPersonas,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the other two purposes above? Does SecurifyingAccountsAndPersonas mean necessarily and, or it can happen to have only accounts or personas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes if you only security account or only security personas those other are used.

But host will most likely map all variants to same Crowdin key anyway...

Copy link
Contributor

Choose a reason for hiding this comment

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

In the host API, it will send a list of AccountOrPersona right? and then in Sargon you map to specific purpose based on the input, I think I've seen this logic in this PR.

@@ -135,6 +140,17 @@ impl SecurityShieldBuilder {
self.set(|builder| builder.set_name(&name));
}

pub fn set_authentication_signing_factor(
Copy link
Contributor

Choose a reason for hiding this comment

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

For other factors we seem to follow add/remove, any reason to not do the same 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.

Add / remove makes sense when we support multiple factors. This is always one single

@CyonAlexRDX CyonAlexRDX merged commit 87a2400 into main Dec 20, 2024
13 checks passed
@CyonAlexRDX CyonAlexRDX deleted the ac/mix_persona_and_accounts_in_factor_instances_provider branch December 20, 2024 12:54
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.

3 participants