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 Any account can appear and should be activated in domain #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

client/tests/integration/auto_register_account/on_transfer.rs

data_model/src/account.rs

core/src/smartcontracts/isi/mod.rs

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

client/tests/integration/auto_register_account/on_transfer.rs

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

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 · 2024-04-23T07:16:00Z

Updated, see this comparison. The changes are as follows:

remove the 2nd commit: Auto-register destination account on transfer
fix pytest account structure
move iroha_sample_params directory to under the root
initialize and load SampleParams in static memory at the first call
document to alert parse_alias() is for testing only
provide (Account/Asset)Id::from_alias() syntax
remove imports under features

Then, now rebasing

.github/workflows/iroha2-dev-pr.yml

s8sato · 2024-04-24T05:49:20Z

Some discussions are now outside the scope of this PR, so they will be folded once and tracked in #4426

s8sato

Updated according to this policy. Then, now rebasing

test_samples/src/lib.rs

client/tests/integration/upgrade.rs

client/tests/integration/smartcontracts/executor_with_admin/src/lib.rs

core/src/sumeragi/main_loop.rs

smart_contract/src/lib.rs

genesis/src/lib.rs

test_samples/src/lib.rs

client/benches/tps/utils.rs

Erigara · 2024-05-08T07:07:42Z

client_cli/pytests/common/consts.py

@@ -15,11 +15,11 @@ class Stderr(Enum):
    Enum for standard error messages.
    """

-    CANNOT_BE_EMPTY = "cannot be empty\n\nFor more information, try '--help'.\n"
+    EMPTY = "mpty"


"mpty" was the intersection of various Empty error messages and was considered enough to identify the cause, but I revised the messages to contain "Empty"

s8sato

Your feedbacks applied. See diff

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

s8sato · 2024-05-09T00:16:05Z

configs/swarm/genesis.json

+        "Transfer": {
+          "AssetDefinition": {
+            "source_id": "ed01204164BF554923ECE1FD412D241036D863A6AE430476C898248B8237D77534CFC4@genesis",
+            "object": "rose#wonderland",
+            "destination_id": "ed0120CE7FA46C9DCE7EA4B125E2E36BDB63EA33073E7590AC92816AE1E861B7048B03@wonderland"
          }
        }
      },
      {
        "Transfer": {
          "Domain": {
-            "source_id": "genesis@genesis",
+            "source_id": "ed01204164BF554923ECE1FD412D241036D863A6AE430476C898248B8237D77534CFC4@genesis",
            "object": "wonderland",
-            "destination_id": "alice@wonderland"
+            "destination_id": "ed0120CE7FA46C9DCE7EA4B125E2E36BDB63EA33073E7590AC92816AE1E861B7048B03@wonderland"


Here genesis.json has a literal genesis public key, which means kagami genesis requires some input of the genesis public key. That's why I made kagami genesis read GENESIS_PUBLIC_KEY environment variable

s8sato · 2024-05-09T00:17:33Z

scripts/tests/consistency.sh

+        GENESIS_PUBLIC_KEY=ed01204164BF554923ECE1FD412D241036D863A6AE430476C898248B8237D77534CFC4 cargo run --release --bin kagami -- genesis --executor-path-in-genesis ./executor.wasm | diff - configs/swarm/genesis.json || {
+            echo 'Please re-generate the default genesis with `GENESIS_PUBLIC_KEY=ed01204164BF554923ECE1FD412D241036D863A6AE430476C898248B8237D77534CFC4 cargo run --release --bin kagami -- genesis --executor-path-in-genesis ./executor.wasm > configs/swarm/genesis.json`'
+            echo 'The assumption here is that the authority of the default genesis transaction is `test_samples::SAMPLE_GENESIS_ACCOUNT_ID`'


As long as genesis.json is not generated on demand but tracked in git, there is always the problem of which genesis public key should be specified. I've introduced an assumption that the authority of the default genesis transaction is test_samples::SAMPLE_GENESIS_ACCOUNT_ID, which is consistent with the preset GENESIS_PUBLIC_KEY in swarm, which is known to be obtained by kagami crypto --seed "Irohagenesis"

So SAMPLE_GENESIS_ACCOUNT_ID is deeply involved with the default genesis, but its prefix is not DEFAULT but SAMPLE because it may be used elsewhere e.g. a synthetic genesis

s8sato · 2024-05-09T00:20:21Z

core/src/smartcontracts/isi/domain.rs

-            if account_id == *iroha_genesis::GENESIS_ACCOUNT_ID {
+            if *account_id.domain_id() == *iroha_genesis::GENESIS_DOMAIN_ID {
                return Err(InstructionExecutionError::InvariantViolation(
-                    "Not allowed to register `genesis@genesis` account".to_owned(),
+                    "Not allowed to register account in genesis domain".to_owned(),


Not genesis account but genesis domain of the registration target now causes transaction rejection

s8sato · 2024-05-09T00:21:49Z

core/src/tx.rs

-        if *iroha_genesis::GENESIS_ACCOUNT_ID == *tx.authority() {
+        if *iroha_genesis::GENESIS_DOMAIN_ID == *tx.authority().domain_id() {
            return Err(AcceptTransactionFail::UnexpectedGenesisAccountSignature);


Not genesis account but genesis domain of the authority now causes transaction rejection

BREAKING CHANGE: - change `AccountId` from "name@domain" to "multihash@domain" - make account signatory single - make transaction signature single - remove query `FindAccountsByName` closes issue hyperledger#2085 Signed-off-by: Shunkichi Sato <49983831+s8sato@users.noreply.github.com>

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

client/tests/integration/auto_register_account/on_transfer.rs Outdated Show resolved Hide resolved

data_model/src/account.rs Show resolved Hide resolved

core/src/smartcontracts/isi/mod.rs Outdated Show resolved Hide resolved

data_model/src/query/predicate.rs Outdated Show resolved Hide resolved

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

configs/sample_params/src/alias.rs Outdated Show resolved Hide resolved

configs/sample_params/Cargo.toml Outdated Show resolved Hide resolved

client/tests/integration/auto_register_account/on_transfer.rs Outdated Show resolved Hide resolved

0x009922 reviewed Apr 10, 2024

View reviewed changes

s8sato mentioned this pull request Apr 12, 2024

Any account can appear and should be activated in domain #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 mentioned this pull request Apr 18, 2024

feat: improve config debug-ability #4456

Open

4 tasks

s8sato force-pushed the feat/2085/1 branch from af55ee4 to fd5a898 Compare April 23, 2024 07:05

s8sato force-pushed the feat/2085/1 branch 2 times, most recently from c74f371 to 2d2463a Compare April 24, 2024 05:17

mversic reviewed Apr 24, 2024

View reviewed changes

.github/workflows/iroha2-dev-pr.yml Outdated Show resolved Hide resolved

s8sato force-pushed the feat/2085/1 branch 2 times, most recently from 05f1314 to d08dfcb Compare April 28, 2024 05:15

s8sato commented Apr 28, 2024

View reviewed changes

s8sato force-pushed the feat/2085/1 branch 2 times, most recently from 2b8776d to 2a397a7 Compare April 28, 2024 06:38

s8sato force-pushed the feat/2085/1 branch from 2a397a7 to 2e6916f Compare May 6, 2024 14:49

s8sato changed the title ~~[feature] #2085: Authenticate personal accounts by ID~~ feat!: authenticate personal accounts by ID May 6, 2024

s8sato force-pushed the feat/2085/1 branch 2 times, most recently from 103ffc4 to f90e2ee Compare May 7, 2024 14:32

0x009922 reviewed May 8, 2024

View reviewed changes

genesis/src/lib.rs Outdated Show resolved Hide resolved

test_samples/src/lib.rs Outdated Show resolved Hide resolved

test_samples/src/lib.rs Outdated Show resolved Hide resolved

Erigara reviewed May 8, 2024

View reviewed changes

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

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

Open

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

github-actions bot commented Apr 4, 2024

coveralls commented Apr 5, 2024 •

edited

s8sato commented Apr 23, 2024

s8sato commented Apr 24, 2024

s8sato left a comment •

edited

Erigara May 8, 2024

s8sato May 9, 2024

s8sato left a comment

s8sato May 9, 2024

s8sato May 9, 2024 •

edited

s8sato May 9, 2024

s8sato May 9, 2024

s8sato May 9, 2024

feat!: authenticate personal accounts by ID #4411

Are you sure you want to change the base?

feat!: authenticate personal accounts by ID #4411

Conversation

s8sato commented Apr 4, 2024 • edited

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

coveralls commented Apr 5, 2024 • edited

Pull Request Test Coverage Report for Build 8595871540

Details

💛 - Coveralls

s8sato commented Apr 23, 2024

s8sato commented Apr 24, 2024

s8sato left a comment • edited

Choose a reason for hiding this comment

Erigara May 8, 2024

Choose a reason for hiding this comment

s8sato May 9, 2024

Choose a reason for hiding this comment

s8sato left a comment

Choose a reason for hiding this comment

s8sato May 9, 2024

Choose a reason for hiding this comment

s8sato May 9, 2024 • edited

Choose a reason for hiding this comment

s8sato May 9, 2024

Choose a reason for hiding this comment

s8sato May 9, 2024

Choose a reason for hiding this comment

s8sato May 9, 2024

Choose a reason for hiding this comment

s8sato commented Apr 4, 2024 •

edited

coveralls commented Apr 5, 2024 •

edited

s8sato left a comment •

edited

s8sato May 9, 2024 •

edited