feat!: authenticate personal accounts by ID #4411

s8sato · 2024-04-04T18:22:53Z

Description

[feature] #2085: Authenticate personal accounts by ID

This is a milestone for the following commit. The inclusion of the public key in the ID allows the ID to match the verifying key of the signature. This makes it possible to authenticate transactions and queries with the ID alone.

api-changes: single signatory account, single signature transaction
api-changes: remove query FindAccountsByName
config-changes: client config account.id changes to account.domain_id
add test_samples utility for tests
different notions of signatories "peer", "genesis", and "alice" in the test network

[feature] #2085: Auto-register destination account on transfer

~~Includes registration of the destination account in the transfer execution. The main use case is described in test auto_register_account::on_transfer::asset_numeric~~

EDIT: left to Target accounts before registration #4426

Linked issue

This is the 1st of the 3-part work of account restructuring:
Opens [suggestion] Dedup public keys in transaction.signature and transaction.payload.authority #4410
Contains suggestions for [suggestion] Remove "genesis account" notion altogether #4409
Closes [BUG] kagami - after creating new genesis.json file we have 2 same public keys per user by default #3511

Benefits

As long as you know the address (account ID), you can send without worrying whether the destination account is already registered on the chain.
However, the current design requires the sender to be authorized to register a new account within the domain. See #4411 (comment)

Drawbacks

Conventional name@domain notation will be temporary lost
- To be restored with Account alias resolution #4372
Multi-signature functionality will be temporary lost, coupled with the queue change
- To be restored and enhanced with Reimplement multi-signature #4373

Checklist

I've read CONTRIBUTING.md
I've used the standard signed-off commit format (or will squash just before merging)
All applicable CI checks pass (or I promised to make them pass later)
I've written unit tests for the code changes
I replied to all comments after code review, marking all implemented changes with thumbs up

github-actions · 2024-04-04T18:23:10Z

@BAStos525

s8sato · 2024-04-04T18:37:39Z

client/tests/integration/auto_register_account/on_transfer.rs

+/// A new account should be automatically registered when being a destination of a transfer of numeric asset e.g. "rose"
+///
+/// # Scenario
+///
+/// 0. alice@wonderland: domain owner
+/// 0. alice -> new carol ... ok (domain owner)
+/// 0. carol -> new dave ... err (permission denied)
+/// 0. grant `CanRegisterAccountInDomain` permission to carol
+/// 0. carol -> new dave ... ok (authorized)
+#[test]
+fn asset_numeric() {


This test tells the main goal of this PR

This transfer permissions concerns me a bit.
As regular user i probably don't care if person i' trying to transfer assets is registered or not.

Can we do the following?
Transfer to any address is always allowed (what about mint and burn?), but to actually make use of this assets (e.g. transfer them to someone else) you have to register an account.
Thus by registering account you claim ownership of this assets.
Since accounts are identified by public key only owner of the private key can register the account.

You have a point, I will consider it. If this discussion is controversial or things get too complicated, we may split the 2nd commit into a separate PR

I think the point is the domain membership screening.
How should a new account apply for and be allowed to join a domain?
In terms of authority, there will be a gap between transferring one's own asset to a new account and registering a new account, especially in "private domain".
So as you pointed out, the admission process should be managed by a different authority than the sender of the first transaction that mentioned the account.

One idea is that each domain has its own admission policy as a trigger that "activates" a new account in response to the account creation event.
Admission policies may vary from domain to domain e.g.

No screening

Wait for approval of the domain owner

Wait for approvals of m of n domain members

In any case, as I implied, I'm going to create a new issue on this and remove the 2nd commit from this PR

Target accounts before registration #4426

Maybe we can preserve our current behavior and to register an account you have to be domain owner or have corresponding permission.

So anyone (with permission) can make transfer/mint/burn assets to certain pub key (even if destination not yet registered).

Then some authority need to register an account to activate it*.

After that owner of corresponding secret key can start using this account (this would be checks that account is registered and transaction signature is correct).

If owner of secret key would try to make a transfer before being regestred transfer would be rejected.

in case of public blockchain could be owner of secret key itself

We seem to be in agreement that there will be two types of account registration, so to speak, provisional and full registration.

Is it your opinion that since the current auto-registration is full registration, it should be weaken to provisional one and the existing registration should be utilized as full registration?

If so, does provisional registration require permission?
If it cannot be executed without the same permission as full registration, then there would be no motive for them to split into two.
Or do we need dedicated permission for provisional registration?

Is it your opinion that since the current auto-registration is full registration, it should be weaken to provisional one and the existing registration should be utilized as full registration?

Yeah, exactly

If so, does provisional registration require permission?

Imo it shouldn't require any additional permissions.

s8sato · 2024-04-04T18:55:13Z

data_model/src/account.rs

    pub struct AccountId {
-        /// [`Account`]'s [`Domain`](`crate::domain::Domain`) id.
+        /// [`Domain`](crate::domain::Domain) that the [`Account`] belongs to.
        pub domain_id: DomainId,
-        /// [`Account`]'s name.
-        pub name: Name,
+        /// Sole signatory of the [`Account`].
+        pub signatory: PublicKey,
    }


With this ID spec, the same signatory could have accounts in multiple domains, but I'm not sure if that's a good idea.
My first thought was to move account.id.domain_id to account.domain_id, but I decided it would not pay for the large impact on the current world and event structure. Opened #4410

My first thought was to move account.id.domain_id to account.domain_id

if we want to disallow accounts with the same public key across different domains, this is the way to go about it. since I see Domains as rather isolated somewhat akin to tenancy in databases. The nice advantage of moving domain_id out of AccountId is that it would reduce memory footprint of iroha.

Yes, I see similar discussions about other entities in #3921 and #4243, so it may be only a matter of time before we are working on it

s8sato · 2024-04-04T18:59:05Z

core/src/smartcontracts/isi/mod.rs

+/// Validate and execute an additional instruction `T` during `Self` execution.
+/// FIXME This means going backwards from execution phase to validation phase, which is contrary to current design principles
+trait BackValidate<T>


This is a kind of compromise because it goes against the current design principle that all validations should be done prior to any execution.
On the other hand, what if during the transfer execution, the account registration executes without any validation?
In that case, transfer could be abused to bypass the validation for account registration.
For example, an attacker could spam a small amount of assets, create a large number of insignificant accounts, and take up memory space.
Account reaping may be effective in such cases, as it is for other public chains

IMO it is not the responsibility of iroha_core to create destination account if it doesn't exist, it is executor's responsibility. I would move all this logic into default executor and iroha_core should plainly reject all instructions that operate on non-existing accounts. In other words, default executor should check whether destination account exists and if not first execute Register<Account> which will go through the regular validation process (since validation is done in executor)

If you do this you wouldn't have the problem you are describing

this will also remove the need to differentiate between full and provisional account registrations as was discussed here because now executor will check whether the authority has the permission to register new account and either make full registration or reject the transaction. No loopholes

@mversic my initial concern was exactly about demanding registration permissions when i.e. making transfer.

Imo we shouldn't require such permissions on transfer.

I see. IMO, in any case iroha_core should reject this transfer if destination doesn't exist. It is up to the executor to create missing destination before doing the transfer. We can make the default executor not require any permissions when creating a provisional account, but it has to create it before executing the transfer

I guess accounts will then have a flag to indicate whether they are provisional or not

@Erigara could you be more specific why the ~~Map<AccountId, Asset>~~ Map<AccountId, Assets> etc. would achieve an equivalent of provisional accounts?
I understand all we have to do to apply e.g. transfer instruction would be modifying the map only.

I'm not sure it is relevant to the following, but
it reminded me of what I called @nodomain approach, which represents being provisional by being under "nodomain".
Why I rejected this approach was because it seemed obviously unreasonable to replace e.g. @wonderland with @nodomain and remember the original @wonderland and later modify @nodomain to @wonderland.
But an evil here might be non-separation of the account identification and the domain membership.
If they were separated, being under "nodomain" (and being provisional) would have been equal to account.domain_id: None.
This approach might rather work well with the shallow objects

@mversic my understanding so far is that core passes the execution of the provisional account registration to executor, who does all-or-nothing validation depending on whether it is permissioned or permissionless.

Before we go any further, I'd like to reach some agreement on whether permission ed/less is characteristic of chain or domain

could you be more specific why the Map<AccountId, Asset> etc. would achieve an equivalent of provisional accounts?

On transfer we would update entry in Map<AccountId, Asset> with such operation we don't need to check if destinationAccountId is registered or not or access any fields of Account.
So in case of when account is not registered this destination AccountId would be kinda dangling.

In that case we still need something indicates if the source account is active or not

If there is no need for provisional registration we can check that source account is registered and it should be enough, shouldn't it?

data_model/src/query/predicate.rs

coveralls · 2024-04-05T03:24:22Z

Pull Request Test Coverage Report for Build 8595871540

Details

378 of 511 (73.97%) changed or added relevant lines in 32 files are covered.
5535 unchanged lines in 97 files lost coverage.
Overall coverage increased (+0.4%) to 57.185%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
client/src/lib.rs	0	1	0.0%
core/src/queue.rs	56	57	98.25%
torii/src/lib.rs	0	1	0.0%
client_cli/src/main.rs	0	2	0.0%
smart_contract/executor/src/default.rs	0	2	0.0%
client/src/config/user.rs	0	3	0.0%
client/src/config/user/boilerplate.rs	0	3	0.0%
core/src/smartcontracts/isi/query.rs	10	13	76.92%
data_model/src/asset.rs	59	62	95.16%
smart_contract/src/lib.rs	0	5	0.0%

Files with Coverage Reduction	New Missed Lines	%
primitives/src/conststr.rs	1	91.14%
crypto/src/hash.rs	1	73.78%
ffi/derive/src/convert.rs	1	84.45%
primitives/src/lib.rs	1	0.0%
core/src/sumeragi/network_topology.rs	1	98.78%
primitives/src/must_use.rs	2	74.29%
crypto/src/signature/bls/mod.rs	2	0.0%
config/src/snapshot.rs	3	78.57%
data_model/derive/src/has_origin.rs	3	95.16%
config/src/kura.rs	3	80.0%

Totals
Change from base Build 7884695009:	0.4%
Covered Lines:	22990
Relevant Lines:	40203

💛 - Coveralls

configs/sample_params/src/alias.rs

configs/sample_params/Cargo.toml

Erigara · 2024-04-09T09:22:19Z

client/tests/integration/auto_register_account/on_transfer.rs

+/// A new account should be automatically registered when being a destination of a transfer of numeric asset e.g. "rose"
+///
+/// # Scenario
+///
+/// 0. alice@wonderland: domain owner
+/// 0. alice -> new carol ... ok (domain owner)
+/// 0. carol -> new dave ... err (permission denied)
+/// 0. grant `CanRegisterAccountInDomain` permission to carol
+/// 0. carol -> new dave ... ok (authorized)
+#[test]
+fn asset_numeric() {


This transfer permissions concerns me a bit.
As regular user i probably don't care if person i' trying to transfer assets is registered or not.

Can we do the following?
Transfer to any address is always allowed (what about mint and burn?), but to actually make use of this assets (e.g. transfer them to someone else) you have to register an account.
Thus by registering account you claim ownership of this assets.
Since accounts are identified by public key only owner of the private key can register the account.

configs/swarm/client.toml

configs/sample_params/src/alias.rs

data_model/src/query/predicate.rs

configs/sample_params/Cargo.toml

core/src/smartcontracts/isi/mod.rs

mversic · 2024-04-15T06:45:08Z

data_model/src/account.rs

    pub struct AccountId {
-        /// [`Account`]'s [`Domain`](`crate::domain::Domain`) id.
+        /// [`Domain`](crate::domain::Domain) that the [`Account`] belongs to.
        pub domain_id: DomainId,
-        /// [`Account`]'s name.
-        pub name: Name,
+        /// Sole signatory of the [`Account`].
+        pub signatory: PublicKey,
    }


My first thought was to move account.id.domain_id to account.domain_id

if we want to disallow accounts with the same public key across different domains, this is the way to go about it. since I see Domains as rather isolated somewhat akin to tenancy in databases. The nice advantage of moving domain_id out of AccountId is that it would reduce memory footprint of iroha.

data_model/src/account.rs

data_model/src/query/predicate.rs

data_model/src/transaction.rs

data_model/src/block.rs

data_model/src/trigger.rs

genesis/src/lib.rs

s8sato

Your feedbacks applied. See diff

fixed genesis account configuration flow @0x009922
please check diff and re-approve @mversic

configs/swarm/genesis.json

scripts/tests/consistency.sh

core/src/smartcontracts/isi/domain.rs

core/src/tx.rs

client_cli/.gitignore

0x009922

Overall no objections, LGTM (except for Kagami UI).

tools/kagami/src/genesis.rs

s8sato · 2024-05-14T14:24:19Z

Made GENESIS_PUBLIC_KEY an arg in kagami. See diff
cargo test -p iroha_core --lib -- snapshot::tests::can_read_snapshot_after_writing seems to be failing since 1606d08

Erigara · 2024-05-14T16:22:32Z

cargo test -p iroha_core --lib -- snapshot::tests::can_read_snapshot_after_writing seems to be failing since 1606d08

Going to fix this issue

BREAKING CHANGE: - change `AccountId` from "name@domain" to "multihash@domain" - make account signatory single - make transaction signature single - remove query `FindAccountsByName` closes issue hyperledger-iroha#2085 Signed-off-by: Shunkichi Sato <[email protected]>

s8sato · 2024-05-15T06:24:27Z

Reverted it to be ignored: cargo test --all-features -p iroha_smart_contract --doc -- parse has been failing even before this PR

github-actions bot added Enhancement New feature or request iroha2-dev The re-implementation of a BFT hyperledger in RUST api-changes Changes in the API for client libraries config-changes Changes in configuration and start up of the Iroha labels Apr 4, 2024

s8sato force-pushed the feat/2085/1 branch from 9d74d00 to e156941 Compare April 4, 2024 18:33

s8sato commented Apr 4, 2024

View reviewed changes

s8sato force-pushed the feat/2085/1 branch from e156941 to a4cd481 Compare April 4, 2024 19:09

s8sato force-pushed the feat/2085/1 branch from a4cd481 to 562e38e Compare April 5, 2024 07:24

Erigara assigned Erigara and s8sato Apr 5, 2024

s8sato force-pushed the feat/2085/1 branch from 562e38e to af55ee4 Compare April 8, 2024 06:59

s8sato marked this pull request as ready for review April 8, 2024 07:47

s8sato requested review from mversic, SamHSmith, outoftardis, Erigara, 0x009922, DCNick3, VAmuzing and Asem-Abdelhady as code owners April 8, 2024 07:47

Erigara reviewed Apr 9, 2024

View reviewed changes

0x009922 reviewed Apr 10, 2024

View reviewed changes

s8sato mentioned this pull request Apr 12, 2024

Target accounts before registration #4426

Open

mversic requested changes Apr 15, 2024

View reviewed changes

mversic self-assigned this Apr 15, 2024

nxsaken changed the base branch from iroha2-dev to main April 16, 2024 08:34

nxsaken requested a review from dima74 as a code owner April 16, 2024 08:34

mversic mentioned this pull request Apr 17, 2024

Forbid Unregister<Account> if's referenced from elsewhere #4329

Closed

mversic previously approved these changes May 8, 2024

View reviewed changes

s8sato dismissed mversic’s stale review via 6f39591 May 9, 2024 00:08

s8sato force-pushed the feat/2085/1 branch 2 times, most recently from 6f39591 to c321539 Compare May 9, 2024 00:10

s8sato commented May 9, 2024

View reviewed changes

configs/swarm/genesis.json Show resolved Hide resolved

scripts/tests/consistency.sh Outdated Show resolved Hide resolved

core/src/smartcontracts/isi/domain.rs Show resolved Hide resolved

core/src/tx.rs Show resolved Hide resolved

s8sato force-pushed the feat/2085/1 branch from c321539 to e77b138 Compare May 9, 2024 05:31

s8sato mentioned this pull request May 9, 2024

Build config/swarm/executor.wasm in CI #4549

Closed

mversic force-pushed the feat/2085/1 branch from e77b138 to 7493bc2 Compare May 14, 2024 05:55

Erigara reviewed May 14, 2024

View reviewed changes

client_cli/.gitignore Outdated Show resolved Hide resolved

Erigara previously approved these changes May 14, 2024

View reviewed changes

0x009922 reviewed May 14, 2024

View reviewed changes

tools/kagami/src/genesis.rs Outdated Show resolved Hide resolved

s8sato dismissed Erigara’s stale review via afb1cbb May 14, 2024 12:37

s8sato force-pushed the feat/2085/1 branch 3 times, most recently from 45189ee to 0eb152b Compare May 14, 2024 14:20

s8sato force-pushed the feat/2085/1 branch from 0eb152b to 1f1daa0 Compare May 15, 2024 04:43

s8sato force-pushed the feat/2085/1 branch from 1f1daa0 to b336eaa Compare May 15, 2024 06:16

mversic approved these changes May 15, 2024

View reviewed changes

Erigara approved these changes May 15, 2024

View reviewed changes

mversic merged commit f12077d into hyperledger-iroha:main May 15, 2024
13 of 16 checks passed

s8sato deleted the feat/2085/1 branch May 15, 2024 12:10

This was referenced May 17, 2024

Illustrate the evolution of ownership over objects during bootstrapping hyperledger-iroha/iroha-2-docs#443

Open

feat!: filtered queries in contracts #4544

Merged

s8sato mentioned this pull request May 29, 2024

Add primary key to account #2085

Closed

nxsaken mentioned this pull request Jul 3, 2024

Comment PR hyperledger-iroha/iroha-2-docs#502

Draft

nxsaken mentioned this pull request Aug 2, 2024

Getting started update and overhaul hyperledger-iroha/iroha-2-docs#504

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: authenticate personal accounts by ID #4411

feat!: authenticate personal accounts by ID #4411

s8sato commented Apr 4, 2024 •

edited

Loading

github-actions bot commented Apr 4, 2024

s8sato Apr 4, 2024

Erigara Apr 9, 2024 •

edited

Loading

s8sato Apr 10, 2024

s8sato Apr 12, 2024 •

edited

Loading

s8sato Apr 12, 2024

Erigara Apr 12, 2024 •

edited

Loading

s8sato Apr 12, 2024 •

edited

Loading

Erigara Apr 15, 2024

s8sato Apr 4, 2024

mversic Apr 15, 2024 •

edited

Loading

s8sato Apr 17, 2024

s8sato Apr 4, 2024

mversic Apr 15, 2024

mversic Apr 15, 2024

Erigara Apr 15, 2024 •

edited

Loading

mversic Apr 15, 2024 •

edited

Loading

s8sato Apr 18, 2024 •

edited

Loading

s8sato Apr 18, 2024

Erigara Apr 22, 2024

s8sato Apr 23, 2024

Erigara Apr 23, 2024

coveralls commented Apr 5, 2024 •

edited

Loading

Erigara Apr 9, 2024 •

edited

Loading

mversic Apr 15, 2024 •

edited

Loading

s8sato left a comment

0x009922 left a comment

s8sato commented May 14, 2024

Erigara commented May 14, 2024

s8sato commented May 15, 2024 •

edited

Loading

feat!: authenticate personal accounts by ID #4411

feat!: authenticate personal accounts by ID #4411

Conversation

s8sato commented Apr 4, 2024 • edited Loading

Description

[feature] #2085: Authenticate personal accounts by ID

[feature] #2085: Auto-register destination account on transfer

Linked issue

Benefits

Drawbacks

Checklist

github-actions bot commented Apr 4, 2024

Choose a reason for hiding this comment

Erigara Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s8sato Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Erigara Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

s8sato Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mversic Apr 15, 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

Erigara Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

mversic Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

s8sato Apr 18, 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

coveralls commented Apr 5, 2024 • edited Loading

Pull Request Test Coverage Report for Build 8595871540

Details

💛 - Coveralls

Erigara Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

mversic Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

s8sato left a comment

Choose a reason for hiding this comment

0x009922 left a comment

Choose a reason for hiding this comment

s8sato commented May 14, 2024

Erigara commented May 14, 2024

s8sato commented May 15, 2024 • edited Loading

s8sato commented Apr 4, 2024 •

edited

Loading

Erigara Apr 9, 2024 •

edited

Loading

s8sato Apr 12, 2024 •

edited

Loading

Erigara Apr 12, 2024 •

edited

Loading

s8sato Apr 12, 2024 •

edited

Loading

mversic Apr 15, 2024 •

edited

Loading

Erigara Apr 15, 2024 •

edited

Loading

mversic Apr 15, 2024 •

edited

Loading

s8sato Apr 18, 2024 •

edited

Loading

coveralls commented Apr 5, 2024 •

edited

Loading

Erigara Apr 9, 2024 •

edited

Loading

mversic Apr 15, 2024 •

edited

Loading

s8sato commented May 15, 2024 •

edited

Loading