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

feat: update security parameters when updating the addressbook #1901

Merged

Conversation

matteriben
Copy link
Contributor

Description:

Update security parameters when updating the addressbook

Fixes #1900

@matteriben matteriben requested review from a team as code owners July 16, 2024 06:25
@matteriben matteriben self-assigned this Jul 16, 2024
@matteriben matteriben force-pushed the 01900-update-security-parameters-when-updating-the-addressbook branch 4 times, most recently from 7caa747 to 6b606d7 Compare July 16, 2024 18:12
@thenswan
Copy link
Contributor

Hey @matteriben, thank you for your contribution here once again 🙂
Can you please explain use-case(s) for this feature more in detail? It would be great if you provide some examples of how you are planning to use it.

@matteriben
Copy link
Contributor Author

matteriben commented Jul 17, 2024

Hey @matteriben, thank you for your contribution here once again 🙂 Can you please explain use-case(s) for this feature more in detail? It would be great if you provide some examples of how you are planning to use it.

I'd like to test using TLS with a network other than {mainnet, testnet, previewnet}. To do so, I need a way to provide the certHash in addition to the address string and accountId when configuring the client for my network.

The test case provides an example where the client is configured from an addressbook constructed for some alternative network.

        try (Client client = Client.forNetwork(Map.of())) {

            // reconfigure client network from addressbook (add new nodes)
            client.setNetworkFromAddressBook(NodeAddressBook.fromProtobuf(com.hedera.hashgraph.sdk.proto.NodeAddressBook.newBuilder()
                .addNodeAddress(nodeAddress(10001, new byte[] {10, 0, 0, 1}, "10001", new byte[] {1, 0, 1}))
                .addNodeAddress(nodeAddress(10002, new byte[] {10, 0, 0, 2}, "10002", new byte[] {1, 0, 2}))
                .build()));

            // verify security parameters in client
            assertThat(nodeAddress.apply(10001).certHash).isEqualTo(ByteString.copyFrom(new byte[]{1, 0, 1}));
            assertThat(nodeAddress.apply(10001).publicKey).isEqualTo("10001");
            assertThat(nodeAddress.apply(10002).certHash).isEqualTo(ByteString.copyFrom(new byte[]{1, 0, 2}));
            assertThat(nodeAddress.apply(10002).publicKey).isEqualTo("10002");

            // ...
        }

@matteriben
Copy link
Contributor Author

matteriben commented Jul 18, 2024

[This concern has been addressed]

@thenswan This change may be problematic as it stands currently since it overwrites certHash (needed for TLS), and it appears that hedera-service may not provide certHash meaning with this change updating the client with the current addressbook could effectively remove certHash (by overwriting with nothing).

@matteriben matteriben force-pushed the 01900-update-security-parameters-when-updating-the-addressbook branch 3 times, most recently from 76aec80 to 5e50482 Compare July 18, 2024 01:58
@matteriben
Copy link
Contributor Author

@thenswan I updated this PR and so that the old certHash will be used in case the new addressbook is missing certHash, which currently appears to be the case when retrieving the addressbook from hedera-service directly.

I also updated the test to verify this case.

@thenswan
Copy link
Contributor

@matteriben thank you for provided description and example. I reviewed everything and have a question:
Can you please take a look at this example that I made some time ago and let me know if this will suit your needs (or how your use-case is different from that)?

@matteriben
Copy link
Contributor Author

matteriben commented Jul 22, 2024

@matteriben thank you for provided description and example. I reviewed everything and have a question: Can you please take a look at this example that I made some time ago and let me know if this will suit your needs (or how your use-case is different from that)?

@thenswan issue #1569 is related, but not the same issue. In issue #1569 the primary concern is configure the client to use a specific node which exists in the predefined address book. Having the ability to load a custom address book (including security parameters) would solve both issue #1569 and issue #1900.

PR #1570 demonstrates how to load a predefined address book then narrow the network to a single node. This is sufficient for issue #1569 but not issue #1900 because issue #1569 is dealing with nodes that exist in one of the predefined networks, where issue #1900 is dealing with a custom network that is not predefined.

@matteriben matteriben force-pushed the 01900-update-security-parameters-when-updating-the-addressbook branch 4 times, most recently from 189042e to ebccf32 Compare July 25, 2024 06:30
@matteriben matteriben force-pushed the 01900-update-security-parameters-when-updating-the-addressbook branch from ebccf32 to 196ce32 Compare July 25, 2024 14:36
Signed-off-by: Matt Riben <matt.riben@swirldslabs.com>
@matteriben matteriben force-pushed the 01900-update-security-parameters-when-updating-the-addressbook branch from 196ce32 to 6caca18 Compare July 25, 2024 14:48
Copy link

sonarcloud bot commented Jul 25, 2024

@0xivanov 0xivanov added this to the v2.40.0 milestone Sep 9, 2024
@0xivanov 0xivanov added the enhancement New feature or request label Sep 10, 2024
Signed-off-by: Ivan Ivanov <ivanivanov.ii726@gmail.com>
Signed-off-by: Ivan Ivanov <ivanivanov.ii726@gmail.com>
@0xivanov 0xivanov requested a review from a team as a code owner September 10, 2024 07:56
Signed-off-by: Ivan Ivanov <ivanivanov.ii726@gmail.com>
Copy link

sonarcloud bot commented Sep 10, 2024

Copy link
Contributor

@0xivanov 0xivanov left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@agadzhalov agadzhalov left a comment

Choose a reason for hiding this comment

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

LGTM

@0xivanov 0xivanov merged commit c21c03a into main Sep 17, 2024
12 checks passed
@0xivanov 0xivanov deleted the 01900-update-security-parameters-when-updating-the-addressbook branch September 17, 2024 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
4 participants