Skip to content

Commit

Permalink
feat: cherry-pick: change DAB node delete transaction sign requirement (
Browse files Browse the repository at this point in the history 
#16990)

Signed-off-by: Iris Simon <iris.simon@swirldslabs.com>
  • Loading branch information
@iwsimon
iwsimon authored Dec 9, 2024
1 parent 17d44e3 commit a67cef4
Show file tree
Hide file tree
Showing 14 changed files with 350 additions and 40 deletions.
6 changes: 3 additions & 3 deletions hapi/hedera-protobufs/services/node_delete.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@ option java_multiple_files = true;
/**
* A transaction body to delete a node from the network address book.
*
* This transaction body SHALL be considered a "privileged transaction".
*
* - A `NodeDeleteTransactionBody` MUST be signed by the governing council.
* - A `NodeDeleteTransactionBody` MUST be signed by one of those keys:
* adminKey, treasure account (2) key, systemAdmin(50) key, or
* addressBookAdmin(55) key.
* - Upon success, the address book entry SHALL enter a "pending delete"
* state.
* - All address book entries pending deletion SHALL be removed from the
Expand Down
23 changes: 20 additions & 3 deletions ...rc/main/java/com/hedera/node/app/service/addressbook/impl/handlers/NodeDeleteHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package com.hedera.node.app.service.addressbook.impl.handlers;

import static com.hedera.hapi.node.base.ResponseCodeEnum.INVALID_ADMIN_KEY;
import static com.hedera.hapi.node.base.ResponseCodeEnum.INVALID_NODE_ID;
import static com.hedera.hapi.node.base.ResponseCodeEnum.NODE_DELETED;
import static com.hedera.node.app.service.addressbook.AddressBookHelper.checkDABEnabled;
Expand All @@ -28,13 +29,15 @@
import com.hedera.hapi.node.base.SubType;
import com.hedera.hapi.node.state.addressbook.Node;
import com.hedera.hapi.node.transaction.TransactionBody;
import com.hedera.node.app.service.addressbook.ReadableNodeStore;
import com.hedera.node.app.service.addressbook.impl.WritableNodeStore;
import com.hedera.node.app.spi.fees.FeeContext;
import com.hedera.node.app.spi.fees.Fees;
import com.hedera.node.app.spi.workflows.HandleContext;
import com.hedera.node.app.spi.workflows.PreCheckException;
import com.hedera.node.app.spi.workflows.PreHandleContext;
import com.hedera.node.app.spi.workflows.TransactionHandler;
import com.hedera.node.config.data.AccountsConfig;
import edu.umd.cs.findbugs.annotations.NonNull;
import javax.inject.Inject;
import javax.inject.Singleton;
Expand All @@ -51,15 +54,29 @@ public NodeDeleteHandler() {}
@Override
public void pureChecks(@NonNull final TransactionBody txn) throws PreCheckException {
requireNonNull(txn);
final NodeDeleteTransactionBody transactionBody = txn.nodeDeleteOrThrow();
final long nodeId = transactionBody.nodeId();
final var op = txn.nodeDeleteOrThrow();
final long nodeId = op.nodeId();

validateFalsePreCheck(nodeId < 0, INVALID_NODE_ID);
}

@Override
public void preHandle(@NonNull final PreHandleContext context) throws PreCheckException {
// Empty method
final var op = context.body().nodeDeleteOrThrow();
final var accountConfig = context.configuration().getConfigData(AccountsConfig.class);
final var nodeStore = context.createStore(ReadableNodeStore.class);
final var payerNum = context.payer().accountNum();

final var existingNode = nodeStore.get(op.nodeId());
validateFalsePreCheck(existingNode == null, INVALID_NODE_ID);
validateFalsePreCheck(existingNode.deleted(), NODE_DELETED);

// if payer is not one of the system admin, treasury or address book admin, check the admin key signature
if (payerNum != accountConfig.treasury()
&& payerNum != accountConfig.systemAdmin()
&& payerNum != accountConfig.addressBookAdmin()) {
context.requireKeyOrThrow(existingNode.adminKey(), INVALID_ADMIN_KEY);
}
}

/**
Expand Down
2 changes: 1 addition & 1 deletion ...rc/main/java/com/hedera/node/app/service/addressbook/impl/handlers/NodeUpdateHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public NodeUpdateHandler(@NonNull final AddressBookValidator addressBookValidato
@Override
public void pureChecks(@NonNull final TransactionBody txn) throws PreCheckException {
requireNonNull(txn);
final var op = txn.nodeUpdate();
final var op = txn.nodeUpdateOrThrow();
validateFalsePreCheck(op.nodeId() < 0, INVALID_NODE_ID);
if (op.hasGossipCaCertificate()) {
validateFalsePreCheck(op.gossipCaCertificate().equals(Bytes.EMPTY), INVALID_GOSSIP_CA_CERTIFICATE);
Expand Down
83 changes: 83 additions & 0 deletions ...l/src/test/java/com/hedera/node/app/service/addressbook/impl/test/CertificatePemTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package com.hedera.node.app.service.addressbook.impl.test;

import static com.hedera.hapi.node.base.ResponseCodeEnum.INVALID_GOSSIP_CA_CERTIFICATE;
import static com.hedera.node.app.service.addressbook.AddressBookHelper.loadResourceFile;
import static com.hedera.node.app.service.addressbook.AddressBookHelper.readCertificatePemFile;
import static com.hedera.node.app.service.addressbook.AddressBookHelper.writeCertificatePemFile;
Expand All @@ -29,11 +30,16 @@
import com.hedera.hapi.node.base.ResponseCodeEnum;
import com.hedera.node.app.spi.workflows.PreCheckException;
import com.hedera.pbj.runtime.io.buffer.Bytes;
import edu.umd.cs.findbugs.annotations.NonNull;
import java.io.ByteArrayInputStream;
import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.security.cert.CertificateException;
import java.security.cert.CertificateFactory;
import java.security.cert.X509Certificate;
import java.util.Arrays;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;

Expand Down Expand Up @@ -84,4 +90,81 @@ void invalidBytesInPemCannotRead() throws IOException {
final var msg = assertThrows(PreCheckException.class, () -> validateX509Certificate(Bytes.wrap("anyString")));
assertEquals(ResponseCodeEnum.INVALID_GOSSIP_CA_CERTIFICATE, msg.responseCode());
}

@Test
void badX509CertificateFailedOnReadAndValidation() throws IOException {
final var pemFileName = "badX509.pem";
final var pemFilePath = loadResourceFile(pemFileName);
final var exception = assertThrows(IOException.class, () -> readCertificatePemFile(pemFilePath));
assertEquals("problem parsing cert: java.io.IOException: unknown tag 13 encountered", exception.getMessage());

final byte[] certBytes = Files.readAllBytes(pemFilePath);
final var msg = assertThrows(PreCheckException.class, () -> validateX509Certificate(Bytes.wrap(certBytes)));
assertEquals(ResponseCodeEnum.INVALID_GOSSIP_CA_CERTIFICATE, msg.responseCode());
final var getmsg = assertThrows(PreCheckException.class, () -> getX509Certificate(Bytes.wrap(certBytes)));
assertEquals(ResponseCodeEnum.INVALID_GOSSIP_CA_CERTIFICATE, getmsg.responseCode());
}

@Test
void goodX509CertificateSuccessOnReadAndValidation() throws IOException, CertificateException {
final var pemFileName = "goodX509.pem";
final var pemFilePath = loadResourceFile(pemFileName);
final var cert = readCertificatePemFile(pemFilePath);

assertEquals("SHA384withRSA", cert.getSigAlgName());
assertEquals("X.509", cert.getType());
assertDoesNotThrow(() -> getX509Certificate(Bytes.wrap(cert.getEncoded())));
assertDoesNotThrow(() -> validateX509Certificate(Bytes.wrap(cert.getEncoded())));

final byte[] certBytes = Files.readAllBytes(pemFilePath);
assertDoesNotThrow(() -> getX509Certificate(Bytes.wrap(certBytes)));
}

@Test
void getSameCertificateFromPemEncodingAndPemBytes() throws IOException, CertificateException, PreCheckException {
final var pemFileName = "goodX509.pem";
final var pemFilePath = loadResourceFile(pemFileName);
final var cert = readCertificatePemFile(pemFilePath);

assertEquals("SHA384withRSA", cert.getSigAlgName());
assertEquals("X.509", cert.getType());
final var encodingCert = getX509Certificate(Bytes.wrap(cert.getEncoded()));

final byte[] certBytes = Files.readAllBytes(pemFilePath);
final var bytesCert = getX509Certificate(Bytes.wrap(certBytes));

assertEquals(encodingCert, bytesCert);
}

@Test
void goodCertificateBecomeBad() throws IOException {
final var goodPem = "goodX509.pem";
final var goodFilePath = loadResourceFile(goodPem);
final byte[] certBytes = Files.readAllBytes(goodFilePath);
final var genPemPath = Path.of(tmpDir.getPath() + "/generated.pem");
writeCertificatePemFile(genPemPath, certBytes);
final byte[] genAllBytes = Files.readAllBytes(genPemPath);
final byte[] genBytes = Arrays.copyOf(genAllBytes, genAllBytes.length - 1);

final var badPem = "badX509.pem";
final var badFilePath = loadResourceFile(badPem);
final byte[] badBytes = Files.readAllBytes(badFilePath);
assertArrayEquals(genBytes, badBytes);
}

/**
* Parse X509Certificate bytes and get the X509Certificate object.
* @param certBytes the Bytes to validate
* @throws PreCheckException if the certificate is invalid
*/
private static X509Certificate getX509Certificate(@NonNull Bytes certBytes) throws PreCheckException {
X509Certificate cert;
try {
cert = (X509Certificate) CertificateFactory.getInstance("X.509")
.generateCertificate(new ByteArrayInputStream(certBytes.toByteArray()));
} catch (final CertificateException e) {
throw new PreCheckException(INVALID_GOSSIP_CA_CERTIFICATE);
}
return cert;
}
}
9 changes: 9 additions & 0 deletions .../java/com/hedera/node/app/service/addressbook/impl/test/handlers/AddressBookTestBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,12 @@
import com.hedera.hapi.node.state.addressbook.Node;
import com.hedera.hapi.node.state.common.EntityNumber;
import com.hedera.hapi.node.state.primitives.ProtoBytes;
import com.hedera.hapi.node.state.token.Account;
import com.hedera.node.app.service.addressbook.ReadableNodeStore;
import com.hedera.node.app.service.addressbook.impl.ReadableNodeStoreImpl;
import com.hedera.node.app.service.addressbook.impl.WritableNodeStore;
import com.hedera.node.app.service.addressbook.impl.schemas.V053AddressBookSchema;
import com.hedera.node.app.service.token.ReadableAccountStore;
import com.hedera.node.app.spi.metrics.StoreMetricsService;
import com.hedera.node.config.testfixtures.HederaTestConfigBuilder;
import com.hedera.pbj.runtime.io.buffer.Bytes;
Expand Down Expand Up @@ -278,6 +280,13 @@ protected Node createNode() {
.build();
}

protected Key mockPayerLookup(Key key, AccountID contextPayerId, ReadableAccountStore accountStore) {
final var account = mock(Account.class);
given(account.key()).willReturn(key);
given(accountStore.getAccountById(contextPayerId)).willReturn(account);
return key;
}

public static List<X509Certificate> generateX509Certificates(final int n) {
final var randomAddressBook = RandomAddressBookBuilder.create(new Random())
.withSize(n)
Expand Down
91 changes: 88 additions & 3 deletions ...ava/com/hedera/node/app/service/addressbook/impl/test/handlers/NodeDeleteHandlerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,14 @@

package com.hedera.node.app.service.addressbook.impl.test.handlers;

import static com.hedera.hapi.node.base.ResponseCodeEnum.INVALID_ADMIN_KEY;
import static com.hedera.hapi.node.base.ResponseCodeEnum.INVALID_NODE_ID;
import static com.hedera.node.app.service.addressbook.impl.schemas.V053AddressBookSchema.NODES_KEY;
import static com.hedera.node.app.spi.fixtures.Assertions.assertThrowsPreCheck;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatCode;
import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
import static org.assertj.core.api.AssertionsForClassTypes.catchThrowable;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyLong;
Expand All @@ -31,11 +32,14 @@
import static org.mockito.Mockito.mock;

import com.hedera.hapi.node.addressbook.NodeDeleteTransactionBody;
import com.hedera.hapi.node.base.AccountID;
import com.hedera.hapi.node.base.Key;
import com.hedera.hapi.node.base.ResponseCodeEnum;
import com.hedera.hapi.node.base.TransactionID;
import com.hedera.hapi.node.state.addressbook.Node;
import com.hedera.hapi.node.state.common.EntityNumber;
import com.hedera.hapi.node.transaction.TransactionBody;
import com.hedera.node.app.service.addressbook.ReadableNodeStore;
import com.hedera.node.app.service.addressbook.impl.ReadableNodeStoreImpl;
import com.hedera.node.app.service.addressbook.impl.WritableNodeStore;
import com.hedera.node.app.service.addressbook.impl.handlers.NodeDeleteHandler;
Expand All @@ -44,6 +48,7 @@
import com.hedera.node.app.spi.fees.FeeCalculatorFactory;
import com.hedera.node.app.spi.fees.FeeContext;
import com.hedera.node.app.spi.fees.Fees;
import com.hedera.node.app.spi.fixtures.workflows.FakePreHandleContext;
import com.hedera.node.app.spi.metrics.StoreMetricsService;
import com.hedera.node.app.spi.store.StoreFactory;
import com.hedera.node.app.spi.workflows.HandleContext;
Expand Down Expand Up @@ -219,8 +224,57 @@ void noFileKeys() {
}

@Test
void preHandleDoesNothing() {
assertDoesNotThrow(() -> subject.preHandle(mock(PreHandleContext.class)));
void preHandleWorksWhenExistingAdminKeyValid() throws PreCheckException {
givenValidNodeWithAdminKey(anotherKey);
refreshStoresWithCurrentNodeInReadable();

final var txn = newDeleteTxnWithNodeId(nodeId.number());
final var context = setupPreHandlePayerKey(txn, accountId, key);
subject.preHandle(context);
assertThat(txn).isEqualTo(context.body());
assertThat(context.payerKey()).isEqualTo(key);
assertThat(context.requiredNonPayerKeys()).contains(anotherKey);
}

@Test
void preHandleFailedWhenAdminKeyInValid() throws PreCheckException {
givenValidNodeWithAdminKey(invalidKey);
refreshStoresWithCurrentNodeInReadable();
final var txn = newDeleteTxnWithNodeId(nodeId.number());
final var context = setupPreHandlePayerKey(txn, accountId, anotherKey);
assertThrowsPreCheck(() -> subject.preHandle(context), INVALID_ADMIN_KEY);
}

@Test
void preHandleWorksWhenTreasureSign() throws PreCheckException {
final var txn = newDeleteTxn();
final var context = setupPreHandlePayerKey(txn, payerId, anotherKey);
subject.preHandle(context);
assertThat(txn).isEqualTo(context.body());
assertThat(context.payerKey()).isEqualTo(anotherKey);
assertThat(context.requiredNonPayerKeys()).isEmpty();
}

@Test
void preHandleWorksWhenSysAdminSign() throws PreCheckException {
final var accountID = AccountID.newBuilder().accountNum(50).build();
final var txn = newDeleteTxnWithPayerId(accountID);
final var context = setupPreHandlePayerKey(txn, accountID, anotherKey);
subject.preHandle(context);
assertThat(txn).isEqualTo(context.body());
assertThat(context.payerKey()).isEqualTo(anotherKey);
assertThat(context.requiredNonPayerKeys()).isEmpty();
}

@Test
void preHandleWorksWhenAddressBookAdminSign() throws PreCheckException {
final var accountID = AccountID.newBuilder().accountNum(55).build();
final var txn = newDeleteTxnWithPayerId(accountID);
final var context = setupPreHandlePayerKey(txn, accountID, anotherKey);
subject.preHandle(context);
assertThat(txn).isEqualTo(context.body());
assertThat(context.payerKey()).isEqualTo(anotherKey);
assertThat(context.requiredNonPayerKeys()).isEmpty();
}

private TransactionBody newDeleteTxn() {
Expand All @@ -232,6 +286,37 @@ private TransactionBody newDeleteTxn() {
.build();
}

private TransactionBody newDeleteTxnWithPayerId(AccountID accountID) {
final var txnId = TransactionID.newBuilder().accountID(accountID).build();
final var deleteFileBuilder = NodeDeleteTransactionBody.newBuilder().nodeId(WELL_KNOWN_NODE_ID);
return TransactionBody.newBuilder()
.transactionID(txnId)
.nodeDelete(deleteFileBuilder.build())
.build();
}

private TransactionBody newDeleteTxnWithNodeId(long nodeId) {
final var txnId = TransactionID.newBuilder().accountID(accountId).build();
final var deleteFileBuilder = NodeDeleteTransactionBody.newBuilder().nodeId(nodeId);
return TransactionBody.newBuilder()
.transactionID(txnId)
.nodeDelete(deleteFileBuilder.build())
.build();
}

private PreHandleContext setupPreHandlePayerKey(TransactionBody txn, AccountID contextPayerId, Key key)
throws PreCheckException {
final var config = HederaTestConfigBuilder.create()
.withValue("accounts.treasury", 2)
.withValue("accounts.systemAdmin", 50)
.withValue("accounts.addressBookAdmin", 55)
.getOrCreateConfig();
mockPayerLookup(key, contextPayerId, accountStore);
final var context = new FakePreHandleContext(accountStore, txn, config);
context.registerStore(ReadableNodeStore.class, readableStore);
return context;
}

private static void assertFailsWith(final Runnable something, final ResponseCodeEnum status) {
assertThatThrownBy(something::run)
.isInstanceOf(HandleException.class)
Expand Down
Loading

0 comments on commit a67cef4

Please sign in to comment.