From 2f454f24a7a947b43c92f6c01d4741daa5e850a5 Mon Sep 17 00:00:00 2001 From: Iris Simon Date: Thu, 7 Nov 2024 17:16:26 -0500 Subject: [PATCH 1/2] NodeUpdate needs only admin key to sign Signed-off-by: Iris Simon --- .../services/address_book_service.proto | 26 ++++++++++--------- .../services/node_update.proto | 7 +++-- .../impl/handlers/NodeUpdateHandler.java | 2 +- .../app/authorization/PrivilegesVerifier.java | 2 +- .../authorization/PrivilegesVerifierTest.java | 9 ------- .../node/config/data/ApiPermissionConfig.java | 2 +- .../bdd/suites/hip869/NodeUpdateTest.java | 6 +++-- .../hip869/UpdateAccountEnabledTest.java | 14 +++++----- 8 files changed, 30 insertions(+), 38 deletions(-) diff --git a/hapi/hedera-protobufs/services/address_book_service.proto b/hapi/hedera-protobufs/services/address_book_service.proto index bcabbd5c4858..81639bbc1afa 100644 --- a/hapi/hedera-protobufs/services/address_book_service.proto +++ b/hapi/hedera-protobufs/services/address_book_service.proto @@ -33,7 +33,7 @@ import "transaction.proto"; * but each node operator may update their own operational attributes without * additional approval, reducing overhead for routine operations. * - * All operations are `privileged operations` and require governing council + * Most operations are `privileged operations` and require governing council * approval. * * ### For a node creation transaction. @@ -91,18 +91,20 @@ import "transaction.proto"; * - If the transaction changes the value of the "node account" the * node operator MUST _also_ sign this transaction with the active `key` * for the account to be assigned as the new "node account". - * - The node operator MUST deliver the signed transaction to the Hedera - * council representative. - * - The Hedera council representative SHALL arrange for council members to - * review and sign the transaction. - * - Once sufficient council members have signed the transaction, the - * Hedera council representative SHALL submit the transaction to the - * network. + * - The node operator SHALL submit the transaction to the + * network. Hedera council approval SHALL NOT be sought for this + * transaction + * - If the Hedera council representative creates the transaction + * - The Hedera council representative SHALL arrange for council members + * to review and sign the transaction. + * - Once sufficient council members have signed the transaction, the + * Hedera council representative SHALL submit the transaction to the + * network. * - Upon receipt of a valid and signed node update transaction the network * software SHALL - * - Validate the threshold signature for the Hedera governing council - * - Validate the signature of the active `key` for the account to be - * assigned as the "node account". + * - If the transaction is signed by the active `key` for the node account + * - Validate the signature of the active `key` for the account assigned + * as the "node account". * - If the transaction modifies the value of the "node account", * - Validate the signature of the _new_ `key` for the account to be * assigned as the new "node account". @@ -147,7 +149,7 @@ service AddressBookService { * This transaction, once complete, SHALL modify the identified consensus * node state as requested. *

- * Hedera governing council authorization is REQUIRED for this transaction. + * This transaction is authorized by the node operator */ rpc updateNode (proto.Transaction) returns (proto.TransactionResponse); } diff --git a/hapi/hedera-protobufs/services/node_update.proto b/hapi/hedera-protobufs/services/node_update.proto index 112464dd66b3..7a1e807a7196 100644 --- a/hapi/hedera-protobufs/services/node_update.proto +++ b/hapi/hedera-protobufs/services/node_update.proto @@ -28,10 +28,9 @@ import "basic_types.proto"; /** * Transaction body to modify address book node attributes. * - * This transaction body SHALL be considered a "privileged transaction". - * - * - This transaction MUST be signed by the governing council and - * the active `admin_key` for the node. + * - This transaction SHALL enable the node operator, as identified by the + * `admin_key`, to modify operational attributes of the node. + * - This transaction MUST be signed by the active `admin_key` for the node. * - If this transaction sets a new value for the `admin_key`, then both the * current `admin_key`, and the new `admin_key` MUST sign this transaction. * - This transaction SHALL NOT change any field that is not set (is null) in diff --git a/hedera-node/hedera-addressbook-service-impl/src/main/java/com/hedera/node/app/service/addressbook/impl/handlers/NodeUpdateHandler.java b/hedera-node/hedera-addressbook-service-impl/src/main/java/com/hedera/node/app/service/addressbook/impl/handlers/NodeUpdateHandler.java index 15f239a3679f..f174aa68dcf8 100644 --- a/hedera-node/hedera-addressbook-service-impl/src/main/java/com/hedera/node/app/service/addressbook/impl/handlers/NodeUpdateHandler.java +++ b/hedera-node/hedera-addressbook-service-impl/src/main/java/com/hedera/node/app/service/addressbook/impl/handlers/NodeUpdateHandler.java @@ -65,7 +65,7 @@ public NodeUpdateHandler(@NonNull final AddressBookValidator addressBookValidato @Override public void pureChecks(@NonNull final TransactionBody txn) throws PreCheckException { requireNonNull(txn); - final var op = txn.nodeUpdateOrThrow(); + final var op = txn.nodeUpdate(); validateFalsePreCheck(op.nodeId() < 0, INVALID_NODE_ID); if (op.hasGossipCaCertificate()) { validateFalsePreCheck(op.gossipCaCertificate().equals(Bytes.EMPTY), INVALID_GOSSIP_CA_CERTIFICATE); diff --git a/hedera-node/hedera-app/src/main/java/com/hedera/node/app/authorization/PrivilegesVerifier.java b/hedera-node/hedera-app/src/main/java/com/hedera/node/app/authorization/PrivilegesVerifier.java index 7485e679d6f6..97972864aefd 100644 --- a/hedera-node/hedera-app/src/main/java/com/hedera/node/app/authorization/PrivilegesVerifier.java +++ b/hedera-node/hedera-app/src/main/java/com/hedera/node/app/authorization/PrivilegesVerifier.java @@ -91,7 +91,7 @@ public SystemPrivilege hasPrivileges( txBody.fileDeleteOrThrow().fileIDOrThrow().fileNum()); case CRYPTO_DELETE -> checkEntityDelete( txBody.cryptoDeleteOrThrow().deleteAccountIDOrThrow().accountNumOrThrow()); - case NODE_CREATE, NODE_UPDATE, NODE_DELETE -> checkNodeChange(payerId); + case NODE_CREATE, NODE_DELETE -> checkNodeChange(payerId); default -> SystemPrivilege.UNNECESSARY; }; } diff --git a/hedera-node/hedera-app/src/test/java/com/hedera/node/app/authorization/PrivilegesVerifierTest.java b/hedera-node/hedera-app/src/test/java/com/hedera/node/app/authorization/PrivilegesVerifierTest.java index bbd494c03cf7..efeb3cf9c0f0 100644 --- a/hedera-node/hedera-app/src/test/java/com/hedera/node/app/authorization/PrivilegesVerifierTest.java +++ b/hedera-node/hedera-app/src/test/java/com/hedera/node/app/authorization/PrivilegesVerifierTest.java @@ -46,7 +46,6 @@ import com.hederahashgraph.api.proto.java.FreezeTransactionBody; import com.hederahashgraph.api.proto.java.NodeCreateTransactionBody; import com.hederahashgraph.api.proto.java.NodeDeleteTransactionBody; -import com.hederahashgraph.api.proto.java.NodeUpdateTransactionBody; import com.hederahashgraph.api.proto.java.SignedTransaction; import com.hederahashgraph.api.proto.java.SystemDeleteTransactionBody; import com.hederahashgraph.api.proto.java.SystemUndeleteTransactionBody; @@ -369,14 +368,6 @@ void addressBookAdminCanCreate() throws InvalidProtocolBufferException { assertEquals(SystemOpAuthorization.AUTHORIZED, subject.authForTestCase(accessor(txn))); } - @Test - void addressBookAdminCanUpdate() throws InvalidProtocolBufferException { - // given: - var txn = addressBookAdminTxn().setNodeUpdate(NodeUpdateTransactionBody.getDefaultInstance()); - // expect: - assertEquals(SystemOpAuthorization.AUTHORIZED, subject.authForTestCase(accessor(txn))); - } - @Test void addressBookAdminCanDelete() throws InvalidProtocolBufferException { // given: diff --git a/hedera-node/hedera-config/src/main/java/com/hedera/node/config/data/ApiPermissionConfig.java b/hedera-node/hedera-config/src/main/java/com/hedera/node/config/data/ApiPermissionConfig.java index 706665cfa4bc..ac40d8ac3ea6 100644 --- a/hedera-node/hedera-config/src/main/java/com/hedera/node/config/data/ApiPermissionConfig.java +++ b/hedera-node/hedera-config/src/main/java/com/hedera/node/config/data/ApiPermissionConfig.java @@ -261,7 +261,7 @@ public record ApiPermissionConfig( @ConfigProperty(defaultValue = "0-*") PermissionedAccountsRange tokenCancelAirdrop, @ConfigProperty(defaultValue = "0-*") PermissionedAccountsRange tokenClaimAirdrop, @ConfigProperty(defaultValue = "2-55") PermissionedAccountsRange createNode, - @ConfigProperty(defaultValue = "2-55") PermissionedAccountsRange updateNode, + @ConfigProperty(defaultValue = "0-*") PermissionedAccountsRange updateNode, @ConfigProperty(defaultValue = "2-55") PermissionedAccountsRange deleteNode, @ConfigProperty(defaultValue = "0-0") PermissionedAccountsRange tssMessage, @ConfigProperty(defaultValue = "0-0") PermissionedAccountsRange tssVote, diff --git a/hedera-node/test-clients/src/main/java/com/hedera/services/bdd/suites/hip869/NodeUpdateTest.java b/hedera-node/test-clients/src/main/java/com/hedera/services/bdd/suites/hip869/NodeUpdateTest.java index 7c7ef3178114..e2e9cfe2f60b 100644 --- a/hedera-node/test-clients/src/main/java/com/hedera/services/bdd/suites/hip869/NodeUpdateTest.java +++ b/hedera-node/test-clients/src/main/java/com/hedera/services/bdd/suites/hip869/NodeUpdateTest.java @@ -375,14 +375,16 @@ final Stream signedByCouncilNotAdminKeyFail() throws CertificateEnc } @HapiTest - final Stream signedByCouncilAndAdminKeySuccess() throws CertificateEncodingException { + final Stream signedByAdminKeySuccess() throws CertificateEncodingException { return hapiTest( newKeyNamed("adminKey"), + cryptoCreate("payer").balance(10_000_000_000L), nodeCreate("testNode") .adminKey("adminKey") .gossipCaCertificate(gossipCertificates.getFirst().getEncoded()), nodeUpdate("testNode") - .signedBy(ADDRESS_BOOK_CONTROL, "adminKey") + .payingWith("payer") + .signedBy("payer", "adminKey") .description("updated description") .via("successUpdate"), getTxnRecord("successUpdate").logged()); diff --git a/hedera-node/test-clients/src/main/java/com/hedera/services/bdd/suites/hip869/UpdateAccountEnabledTest.java b/hedera-node/test-clients/src/main/java/com/hedera/services/bdd/suites/hip869/UpdateAccountEnabledTest.java index 87bdec4c864e..61b15adfc89e 100644 --- a/hedera-node/test-clients/src/main/java/com/hedera/services/bdd/suites/hip869/UpdateAccountEnabledTest.java +++ b/hedera-node/test-clients/src/main/java/com/hedera/services/bdd/suites/hip869/UpdateAccountEnabledTest.java @@ -27,7 +27,6 @@ import static com.hedera.services.bdd.spec.utilops.EmbeddedVerbs.viewNode; import static com.hedera.services.bdd.spec.utilops.UtilVerbs.newKeyNamed; import static com.hedera.services.bdd.spec.utilops.UtilVerbs.validateChargedUsdWithin; -import static com.hedera.services.bdd.suites.HapiSuite.DEFAULT_PAYER; import static com.hedera.services.bdd.suites.HapiSuite.ONE_HBAR; import static com.hedera.services.bdd.suites.hip869.NodeCreateTest.generateX509Certificates; import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.INVALID_NODE_ACCOUNT_ID; @@ -101,17 +100,16 @@ final Stream validateFees() throws CertificateEncodingException { // Submit to a different node so ingest check is skipped nodeUpdate("node100") .setNode("0.0.5") - .signedBy(DEFAULT_PAYER) + .payingWith("payer") .accountId("0.0.1000") .fee(ONE_HBAR) .hasKnownStatus(INVALID_SIGNATURE) .via("failedUpdate"), getTxnRecord("failedUpdate").logged(), - // The fee is not charged here because the payer is privileged - validateChargedUsdWithin("failedUpdate", 0.0, 3.0), + // The fee is charged here because the payer is not privileged + validateChargedUsdWithin("failedUpdate", 0.001, 3.0), nodeUpdate("node100") .adminKey("testKey") - .signedBy(DEFAULT_PAYER, "testKey") .accountId("0.0.1000") .fee(ONE_HBAR) .via("updateNode"), @@ -122,12 +120,12 @@ final Stream validateFees() throws CertificateEncodingException { // Submit with several signatures and the price should increase nodeUpdate("node100") .setNode("0.0.5") - .signedBy(DEFAULT_PAYER, "payer", "payer", "randomAccount", "testKey") + .payingWith("payer") + .signedBy("payer", "payer", "randomAccount", "testKey") .accountId("0.0.1000") .fee(ONE_HBAR) .via("failedUpdateMultipleSigs"), - // The fee is not charged here because the payer is privileged - validateChargedUsdWithin("failedUpdateMultipleSigs", 0.0, 3.0)); + validateChargedUsdWithin("failedUpdateMultipleSigs", 0.0011276316, 3.0)); } @EmbeddedHapiTest(NEEDS_STATE_ACCESS) From ec6b18a5f2640d6180f19acb9cab888ffc1d7946 Mon Sep 17 00:00:00 2001 From: Iris Simon Date: Thu, 7 Nov 2024 17:57:16 -0500 Subject: [PATCH 2/2] Review comments Signed-off-by: Iris Simon --- .../services/address_book_service.proto | 32 ++++++------------- 1 file changed, 10 insertions(+), 22 deletions(-) diff --git a/hapi/hedera-protobufs/services/address_book_service.proto b/hapi/hedera-protobufs/services/address_book_service.proto index 81639bbc1afa..c9cd3231ebe0 100644 --- a/hapi/hedera-protobufs/services/address_book_service.proto +++ b/hapi/hedera-protobufs/services/address_book_service.proto @@ -83,31 +83,19 @@ import "transaction.proto"; * be active in the network following this upgrade. * * ### For a node update transaction. - * - The node operator or Hedera council representative SHALL create an - * `updateNode` transaction. - * - If the node operator creates the transaction - * - The node operator MUST sign this transaction with the active `key` - * for the account assigned as the current "node account". - * - If the transaction changes the value of the "node account" the - * node operator MUST _also_ sign this transaction with the active `key` - * for the account to be assigned as the new "node account". - * - The node operator SHALL submit the transaction to the - * network. Hedera council approval SHALL NOT be sought for this - * transaction - * - If the Hedera council representative creates the transaction - * - The Hedera council representative SHALL arrange for council members - * to review and sign the transaction. - * - Once sufficient council members have signed the transaction, the - * Hedera council representative SHALL submit the transaction to the - * network. + * - The node operator SHALL create an `updateNode` transaction. + * - The node operator MUST sign this transaction with the active `key` + * for the account assigned as the current "node account". + * - The node operator SHALL submit the transaction to the + * network. Hedera council approval SHALL NOT be sought for this + * transaction * - Upon receipt of a valid and signed node update transaction the network * software SHALL - * - If the transaction is signed by the active `key` for the node account - * - Validate the signature of the active `key` for the account assigned - * as the "node account". * - If the transaction modifies the value of the "node account", - * - Validate the signature of the _new_ `key` for the account to be - * assigned as the new "node account". + * - Validate the signature of the active `key` for the account + * assigned as the _current_ "node account". + * - Validate the signature of the active `key` for the account to be + * assigned as the _new_ "node account". * - Modify the node information held in network state with the changes * requested in the update transaction. The node changes SHALL NOT be * applied to network configuration, and SHALL NOT affect network