From ce018645651d3dc23835007ced40802ca9344d8c Mon Sep 17 00:00:00 2001 From: Gabriel-Trintinalia Date: Thu, 23 Jan 2025 13:53:37 +1100 Subject: [PATCH] Move p2p host validation to CLI validation Signed-off-by: Gabriel-Trintinalia --- .../org/hyperledger/besu/cli/BesuCommand.java | 24 ++-- .../besu/cli/options/P2PDiscoveryOptions.java | 50 +++++++ .../hyperledger/besu/cli/BesuCommandTest.java | 107 -------------- .../besu/cli/CommandTestAbstract.java | 4 +- .../cli/options/P2PDiscoveryOptionsTest.java | 133 ++++++++++++++++++ 5 files changed, 196 insertions(+), 122 deletions(-) create mode 100644 besu/src/test/java/org/hyperledger/besu/cli/options/P2PDiscoveryOptionsTest.java diff --git a/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java b/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java index 8ee35458b2f..579f71ce30e 100644 --- a/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java +++ b/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java @@ -212,10 +212,8 @@ import java.io.InputStream; import java.io.InputStreamReader; import java.math.BigInteger; -import java.net.SocketException; import java.net.URI; import java.net.URL; -import java.net.UnknownHostException; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.attribute.GroupPrincipal; @@ -1433,7 +1431,7 @@ private void configureNativeLibs() { private void validateOptions() { issueOptionWarnings(); - validateP2PInterface(p2PDiscoveryOptions.p2pInterface); + validateP2POptions(); validateMiningParams(); validateNatParams(); validateNetStatsParams(); @@ -1488,20 +1486,18 @@ private void validateMiningParams() { commandLine, genesisConfigOptionsSupplier.get(), isMergeEnabled(), logger); } + private void validateP2POptions() { + p2PDiscoveryOptions.validate(commandLine, getNetworkInterfaceChecker()); + } + /** - * Validates P2P interface IP address/host name. Visible for testing. + * Returns a network interface checker that can be used to validate P2P options. * - * @param p2pInterface IP Address/host name + * @return A {@link P2PDiscoveryOptions.NetworkInterfaceChecker} that checks if a network + * interface is available. */ - protected void validateP2PInterface(final String p2pInterface) { - final String failMessage = "The provided --p2p-interface is not available: " + p2pInterface; - try { - if (!NetworkUtility.isNetworkInterfaceAvailable(p2pInterface)) { - throw new ParameterException(commandLine, failMessage); - } - } catch (final UnknownHostException | SocketException e) { - throw new ParameterException(commandLine, failMessage, e); - } + protected P2PDiscoveryOptions.NetworkInterfaceChecker getNetworkInterfaceChecker() { + return NetworkUtility::isNetworkInterfaceAvailable; } private void validateGraphQlOptions() { diff --git a/besu/src/main/java/org/hyperledger/besu/cli/options/P2PDiscoveryOptions.java b/besu/src/main/java/org/hyperledger/besu/cli/options/P2PDiscoveryOptions.java index 798d246b52b..0a190d880e3 100644 --- a/besu/src/main/java/org/hyperledger/besu/cli/options/P2PDiscoveryOptions.java +++ b/besu/src/main/java/org/hyperledger/besu/cli/options/P2PDiscoveryOptions.java @@ -25,12 +25,15 @@ import org.hyperledger.besu.util.number.Percentage; import java.net.InetAddress; +import java.net.SocketException; +import java.net.UnknownHostException; import java.util.ArrayList; import java.util.Collection; import java.util.List; import java.util.Optional; import java.util.stream.Collectors; +import com.google.common.net.InetAddresses; import org.apache.commons.net.util.SubnetUtils; import org.apache.tuweni.bytes.Bytes; import picocli.CommandLine; @@ -38,6 +41,22 @@ /** Command line options for configuring P2P discovery on the node. */ public class P2PDiscoveryOptions implements CLIOptions { + /** Functional interface for checking if a network interface is available. */ + @FunctionalInterface + public interface NetworkInterfaceChecker { + + /** + * Checks if the provided network interface is available. + * + * @param p2pInterface The network interface to check. + * @return True if the network interface is available, false otherwise. + * @throws UnknownHostException If the host is unknown. + * @throws SocketException If there is an error with the socket. + */ + boolean isNetworkInterfaceAvailable(final String p2pInterface) + throws UnknownHostException, SocketException; + } + /** Default constructor */ public P2PDiscoveryOptions() {} @@ -217,6 +236,37 @@ public P2PDiscoveryConfiguration toDomainObject() { discoveryDnsUrl); } + /** + * Validates the provided P2P discovery options. + * + * @param commandLine The command line object used for parsing and error reporting. + * @param networkInterfaceChecker The checker used to validate the network interface. + */ + public void validate( + final CommandLine commandLine, final NetworkInterfaceChecker networkInterfaceChecker) { + validateP2PHost(commandLine); + validateP2PInterface(commandLine, networkInterfaceChecker); + } + + private void validateP2PInterface( + final CommandLine commandLine, final NetworkInterfaceChecker networkInterfaceChecker) { + final String failMessage = "The provided --p2p-interface is not available: " + p2pInterface; + try { + if (!networkInterfaceChecker.isNetworkInterfaceAvailable(p2pInterface)) { + throw new CommandLine.ParameterException(commandLine, failMessage); + } + } catch (final UnknownHostException | SocketException e) { + throw new CommandLine.ParameterException(commandLine, failMessage, e); + } + } + + private void validateP2PHost(final CommandLine commandLine) { + final String failMessage = "The provided --p2p-host is invalid: " + p2pHost; + if (!InetAddresses.isInetAddress(p2pHost)) { + throw new CommandLine.ParameterException(commandLine, failMessage); + } + } + @Override public List getCLIOptions() { return CommandLineUtils.getCLIOptions(this, new P2PDiscoveryOptions()); diff --git a/besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java b/besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java index 70cb4d507c4..e4a65d4d7f9 100644 --- a/besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java +++ b/besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java @@ -1011,113 +1011,6 @@ public void callingWithBannedNodeidsOptionWithInvalidValuesMustDisplayError() { assertThat(commandErrorOutput.toString(UTF_8)).startsWith(expectedErrorOutputStart); } - @Test - public void p2pHostAndPortOptionsAreRespectedAndNotLeaked() { - - final String host = "1.2.3.4"; - final int port = 1234; - parseCommand("--p2p-host", host, "--p2p-port", String.valueOf(port)); - - verify(mockRunnerBuilder).p2pAdvertisedHost(stringArgumentCaptor.capture()); - verify(mockRunnerBuilder).p2pListenPort(intArgumentCaptor.capture()); - verify(mockRunnerBuilder).metricsConfiguration(metricsConfigArgumentCaptor.capture()); - verify(mockRunnerBuilder).jsonRpcConfiguration(jsonRpcConfigArgumentCaptor.capture()); - verify(mockRunnerBuilder).build(); - - assertThat(stringArgumentCaptor.getValue()).isEqualTo(host); - assertThat(intArgumentCaptor.getValue()).isEqualTo(port); - - assertThat(commandOutput.toString(UTF_8)).isEmpty(); - assertThat(commandErrorOutput.toString(UTF_8)).isEmpty(); - - // all other port values remain default - assertThat(metricsConfigArgumentCaptor.getValue().getPort()).isEqualTo(9545); - assertThat(metricsConfigArgumentCaptor.getValue().getPushPort()).isEqualTo(9001); - assertThat(jsonRpcConfigArgumentCaptor.getValue().getPort()).isEqualTo(8545); - - // all other host values remain default - final String defaultHost = "127.0.0.1"; - assertThat(metricsConfigArgumentCaptor.getValue().getHost()).isEqualTo(defaultHost); - assertThat(metricsConfigArgumentCaptor.getValue().getPushHost()).isEqualTo(defaultHost); - assertThat(jsonRpcConfigArgumentCaptor.getValue().getHost()).isEqualTo(defaultHost); - } - - @Test - public void p2pHostAndPortOptionsAreRespectedAndNotLeakedWithMetricsEnabled() { - - final String host = "1.2.3.4"; - final int port = 1234; - parseCommand("--p2p-host", host, "--p2p-port", String.valueOf(port), "--metrics-enabled"); - - verify(mockRunnerBuilder).p2pAdvertisedHost(stringArgumentCaptor.capture()); - verify(mockRunnerBuilder).p2pListenPort(intArgumentCaptor.capture()); - verify(mockRunnerBuilder).metricsConfiguration(metricsConfigArgumentCaptor.capture()); - verify(mockRunnerBuilder).jsonRpcConfiguration(jsonRpcConfigArgumentCaptor.capture()); - verify(mockRunnerBuilder).build(); - - assertThat(stringArgumentCaptor.getValue()).isEqualTo(host); - assertThat(intArgumentCaptor.getValue()).isEqualTo(port); - - assertThat(commandOutput.toString(UTF_8)).isEmpty(); - assertThat(commandErrorOutput.toString(UTF_8)).isEmpty(); - - // all other port values remain default - assertThat(metricsConfigArgumentCaptor.getValue().getPort()).isEqualTo(9545); - assertThat(metricsConfigArgumentCaptor.getValue().getPushPort()).isEqualTo(9001); - assertThat(jsonRpcConfigArgumentCaptor.getValue().getPort()).isEqualTo(8545); - - // all other host values remain default - final String defaultHost = "127.0.0.1"; - assertThat(metricsConfigArgumentCaptor.getValue().getHost()).isEqualTo(defaultHost); - assertThat(metricsConfigArgumentCaptor.getValue().getPushHost()).isEqualTo(defaultHost); - assertThat(jsonRpcConfigArgumentCaptor.getValue().getHost()).isEqualTo(defaultHost); - } - - @Test - public void p2pInterfaceOptionIsRespected() { - - final String ip = "1.2.3.4"; - parseCommand("--p2p-interface", ip); - - assertThat(commandOutput.toString(UTF_8)).isEmpty(); - assertThat(commandErrorOutput.toString(UTF_8)).isEmpty(); - - verify(mockRunnerBuilder).p2pListenInterface(stringArgumentCaptor.capture()); - verify(mockRunnerBuilder).build(); - - assertThat(stringArgumentCaptor.getValue()).isEqualTo(ip); - } - - @Test - public void p2pHostMayBeLocalhost() { - - final String host = "localhost"; - parseCommand("--p2p-host", host); - - verify(mockRunnerBuilder).p2pAdvertisedHost(stringArgumentCaptor.capture()); - verify(mockRunnerBuilder).build(); - - assertThat(stringArgumentCaptor.getValue()).isEqualTo(host); - - assertThat(commandOutput.toString(UTF_8)).isEmpty(); - assertThat(commandErrorOutput.toString(UTF_8)).isEmpty(); - } - - @Test - public void p2pHostMayBeIPv6() { - - final String host = "2600:DB8::8545"; - parseCommand("--p2p-host", host); - - verify(mockRunnerBuilder).p2pAdvertisedHost(stringArgumentCaptor.capture()); - verify(mockRunnerBuilder).build(); - - assertThat(stringArgumentCaptor.getValue()).isEqualTo(host); - - assertThat(commandOutput.toString(UTF_8)).isEmpty(); - assertThat(commandErrorOutput.toString(UTF_8)).isEmpty(); - } - @Test public void maxpeersOptionMustBeUsed() { diff --git a/besu/src/test/java/org/hyperledger/besu/cli/CommandTestAbstract.java b/besu/src/test/java/org/hyperledger/besu/cli/CommandTestAbstract.java index 78f782d5e55..e6fdfa095ce 100644 --- a/besu/src/test/java/org/hyperledger/besu/cli/CommandTestAbstract.java +++ b/besu/src/test/java/org/hyperledger/besu/cli/CommandTestAbstract.java @@ -38,6 +38,7 @@ import org.hyperledger.besu.cli.options.EthstatsOptions; import org.hyperledger.besu.cli.options.MiningOptions; import org.hyperledger.besu.cli.options.NetworkingOptions; +import org.hyperledger.besu.cli.options.P2PDiscoveryOptions; import org.hyperledger.besu.cli.options.SynchronizerOptions; import org.hyperledger.besu.cli.options.TransactionPoolOptions; import org.hyperledger.besu.cli.options.storage.DataStorageOptions; @@ -568,8 +569,9 @@ public static class TestBesuCommand extends BesuCommand { } @Override - protected void validateP2PInterface(final String p2pInterface) { + protected P2PDiscoveryOptions.NetworkInterfaceChecker getNetworkInterfaceChecker() { // For testing, don't actually query for networking interfaces to validate this option + return (networkInterface) -> true; } @Override diff --git a/besu/src/test/java/org/hyperledger/besu/cli/options/P2PDiscoveryOptionsTest.java b/besu/src/test/java/org/hyperledger/besu/cli/options/P2PDiscoveryOptionsTest.java new file mode 100644 index 00000000000..7105e76214b --- /dev/null +++ b/besu/src/test/java/org/hyperledger/besu/cli/options/P2PDiscoveryOptionsTest.java @@ -0,0 +1,133 @@ +/* + * Copyright contributors to Besu. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + * + * SPDX-License-Identifier: Apache-2.0 + */ +package org.hyperledger.besu.cli.options; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.verify; + +import org.hyperledger.besu.cli.CommandTestAbstract; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) +public class P2PDiscoveryOptionsTest extends CommandTestAbstract { + + @Test + public void p2pHostAndPortOptionsAreRespectedAndNotLeakedWithMetricsEnabled() { + + final String host = "1.2.3.4"; + final int port = 1234; + parseCommand("--p2p-host", host, "--p2p-port", String.valueOf(port), "--metrics-enabled"); + + verify(mockRunnerBuilder).p2pAdvertisedHost(stringArgumentCaptor.capture()); + verify(mockRunnerBuilder).p2pListenPort(intArgumentCaptor.capture()); + verify(mockRunnerBuilder).metricsConfiguration(metricsConfigArgumentCaptor.capture()); + verify(mockRunnerBuilder).jsonRpcConfiguration(jsonRpcConfigArgumentCaptor.capture()); + verify(mockRunnerBuilder).build(); + + assertThat(stringArgumentCaptor.getValue()).isEqualTo(host); + assertThat(intArgumentCaptor.getValue()).isEqualTo(port); + + assertThat(commandOutput.toString(UTF_8)).isEmpty(); + assertThat(commandErrorOutput.toString(UTF_8)).isEmpty(); + + // all other port values remain default + assertThat(metricsConfigArgumentCaptor.getValue().getPort()).isEqualTo(9545); + assertThat(metricsConfigArgumentCaptor.getValue().getPushPort()).isEqualTo(9001); + assertThat(jsonRpcConfigArgumentCaptor.getValue().getPort()).isEqualTo(8545); + + // all other host values remain default + final String defaultHost = "127.0.0.1"; + assertThat(metricsConfigArgumentCaptor.getValue().getHost()).isEqualTo(defaultHost); + assertThat(metricsConfigArgumentCaptor.getValue().getPushHost()).isEqualTo(defaultHost); + assertThat(jsonRpcConfigArgumentCaptor.getValue().getHost()).isEqualTo(defaultHost); + } + + @Test + public void p2pInterfaceOptionIsRespected() { + + final String ip = "1.2.3.4"; + parseCommand("--p2p-interface", ip); + + assertThat(commandOutput.toString(UTF_8)).isEmpty(); + assertThat(commandErrorOutput.toString(UTF_8)).isEmpty(); + + verify(mockRunnerBuilder).p2pListenInterface(stringArgumentCaptor.capture()); + verify(mockRunnerBuilder).build(); + + assertThat(stringArgumentCaptor.getValue()).isEqualTo(ip); + } + + @ParameterizedTest + @ValueSource(strings = {"localhost", " localhost.localdomain", "invalid-host"}) + public void p2pHostMustBeAnIPAddress(final String host) { + parseCommand("--p2p-host", host); + + assertThat(commandOutput.toString(UTF_8)).isEmpty(); + String errorMessage = "The provided --p2p-host is invalid: " + host; + assertThat(commandErrorOutput.toString(UTF_8)).contains(errorMessage); + } + + @Test + public void p2pHostMayBeIPv6() { + + final String host = "2600:DB8::8545"; + parseCommand("--p2p-host", host); + + verify(mockRunnerBuilder).p2pAdvertisedHost(stringArgumentCaptor.capture()); + verify(mockRunnerBuilder).build(); + + assertThat(stringArgumentCaptor.getValue()).isEqualTo(host); + + assertThat(commandOutput.toString(UTF_8)).isEmpty(); + assertThat(commandErrorOutput.toString(UTF_8)).isEmpty(); + } + + @Test + public void p2pHostAndPortOptionsAreRespectedAndNotLeaked() { + + final String host = "1.2.3.4"; + final int port = 1234; + parseCommand("--p2p-host", host, "--p2p-port", String.valueOf(port)); + + verify(mockRunnerBuilder).p2pAdvertisedHost(stringArgumentCaptor.capture()); + verify(mockRunnerBuilder).p2pListenPort(intArgumentCaptor.capture()); + verify(mockRunnerBuilder).metricsConfiguration(metricsConfigArgumentCaptor.capture()); + verify(mockRunnerBuilder).jsonRpcConfiguration(jsonRpcConfigArgumentCaptor.capture()); + verify(mockRunnerBuilder).build(); + + assertThat(stringArgumentCaptor.getValue()).isEqualTo(host); + assertThat(intArgumentCaptor.getValue()).isEqualTo(port); + + assertThat(commandOutput.toString(UTF_8)).isEmpty(); + assertThat(commandErrorOutput.toString(UTF_8)).isEmpty(); + + // all other port values remain default + assertThat(metricsConfigArgumentCaptor.getValue().getPort()).isEqualTo(9545); + assertThat(metricsConfigArgumentCaptor.getValue().getPushPort()).isEqualTo(9001); + assertThat(jsonRpcConfigArgumentCaptor.getValue().getPort()).isEqualTo(8545); + + // all other host values remain default + final String defaultHost = "127.0.0.1"; + assertThat(metricsConfigArgumentCaptor.getValue().getHost()).isEqualTo(defaultHost); + assertThat(metricsConfigArgumentCaptor.getValue().getPushHost()).isEqualTo(defaultHost); + assertThat(jsonRpcConfigArgumentCaptor.getValue().getHost()).isEqualTo(defaultHost); + } +}