From 28fd374d8677f5cf8dbbb515b7f0132ff46f3f78 Mon Sep 17 00:00:00 2001 From: Mitchell Wang Date: Fri, 17 Jul 2026 00:12:25 +0000 Subject: [PATCH] fix: never crash on invalid noProxyAddresses entries An invalid noProxyAddresses entry (e.g. a glob-style '*.example.com', which is not valid Java regex) threw an uncaught PatternSyntaxException from String.matches() at both MQTT proxy-bypass call sites, killing MQTT connectivity for the device. - Add ProxyUtils.noProxyMatches(): regex semantics preserved, but entries are trimmed before matching (previously a space-padded entry from a comma-space separated list could never match), a null/empty endpoint or blank entry never matches, and an uncompilable entry is logged and treated as a non-match instead of throwing. - Route the MqttClient and StandaloneMqttConnector bypass checks through it. - Add ProxyUtils.validateNoProxyAddresses() and reject deployments early in BootstrapManager when an entry contains whitespace or does not compile, so operators get a clear deployment-time error instead of a runtime crash. Empty segments from stray commas are skipped, matching the runtime matcher's tolerance. A device already carrying an invalid entry can still recover: a deployment with a corrected value passes validation. Wildcard (glob) support is intentionally out of scope for this change and will follow separately. --- .../bootstrap/BootstrapManager.java | 14 +++- .../aws/greengrass/mqttclient/MqttClient.java | 3 +- .../mqttclient/StandaloneMqttConnector.java | 3 +- .../com/aws/greengrass/util/ProxyUtils.java | 66 +++++++++++++++++ .../bootstrap/BootstrapManagerTest.java | 62 ++++++++++++++++ .../aws/greengrass/util/ProxyUtilsTest.java | 74 +++++++++++++++++++ 6 files changed, 219 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/aws/greengrass/deployment/bootstrap/BootstrapManager.java b/src/main/java/com/aws/greengrass/deployment/bootstrap/BootstrapManager.java index ef32f87e22..40fb1bacff 100644 --- a/src/main/java/com/aws/greengrass/deployment/bootstrap/BootstrapManager.java +++ b/src/main/java/com/aws/greengrass/deployment/bootstrap/BootstrapManager.java @@ -24,6 +24,7 @@ import com.aws.greengrass.util.CommitableReader; import com.aws.greengrass.util.CommitableWriter; import com.aws.greengrass.util.DependencyOrder; +import com.aws.greengrass.util.ProxyUtils; import com.aws.greengrass.util.SerializerFactory; import com.aws.greengrass.util.Utils; import com.aws.greengrass.util.platforms.Platform; @@ -262,7 +263,8 @@ private boolean spoolerStorageTypeHasChanged(Map newNucleusParam } private boolean networkProxyHasChanged(Map newNucleusParameters, - DeviceConfiguration currentDeviceConfiguration) { + DeviceConfiguration currentDeviceConfiguration) + throws ComponentConfigurationValidationException { Map newNetworkProxy = (Map) newNucleusParameters.get(DEVICE_NETWORK_PROXY_NAMESPACE); if (newNetworkProxy == null) { @@ -271,6 +273,16 @@ private boolean networkProxyHasChanged(Map newNucleusParameters, // deviceconfig defaults to empty string on null for network proxy parameters so we must do the same String newNoProxyAddresses = Coerce.toString(newNetworkProxy.getOrDefault(DEVICE_PARAM_NO_PROXY_ADDRESSES, "")); + + // Reject the deployment early on entries the runtime matcher cannot process, + // instead of silently never matching them at connection time + List invalidNoProxyEntries = ProxyUtils.validateNoProxyAddresses(newNoProxyAddresses); + if (!invalidNoProxyEntries.isEmpty()) { + throw new ComponentConfigurationValidationException( + "Invalid noProxyAddresses entries: " + invalidNoProxyEntries + + ". Each entry must be a hostname, IP address, or valid pattern without whitespace."); + } + String currentNoProxyAddresses = Coerce.toString(currentDeviceConfiguration.getNoProxyAddresses()); if (Utils.stringHasChanged(newNoProxyAddresses, currentNoProxyAddresses)) { logger.atInfo().kv(DEVICE_PARAM_NO_PROXY_ADDRESSES, newNoProxyAddresses).log(RESTART_REQUIRED_MESSAGE); diff --git a/src/main/java/com/aws/greengrass/mqttclient/MqttClient.java b/src/main/java/com/aws/greengrass/mqttclient/MqttClient.java index 324e758f6b..16aa0c63bc 100644 --- a/src/main/java/com/aws/greengrass/mqttclient/MqttClient.java +++ b/src/main/java/com/aws/greengrass/mqttclient/MqttClient.java @@ -253,7 +253,8 @@ public MqttClient(DeviceConfiguration deviceConfiguration, ScheduledExecutorServ boolean useProxy = true; // Only use the proxy when the endpoint we're connecting to is not in the NoProxyAddress list if (Utils.isNotEmpty(noProxy) && Utils.isNotEmpty(endpoint)) { - useProxy = Arrays.stream(noProxy.split(",")).noneMatch(endpoint::matches); + useProxy = Arrays.stream(noProxy.split(",")) + .noneMatch(pattern -> ProxyUtils.noProxyMatches(endpoint, pattern)); } if (useProxy) { builder.withHttpProxyOptions(httpProxyOptions); diff --git a/src/main/java/com/aws/greengrass/mqttclient/StandaloneMqttConnector.java b/src/main/java/com/aws/greengrass/mqttclient/StandaloneMqttConnector.java index 8d2db33960..3ae2c80707 100644 --- a/src/main/java/com/aws/greengrass/mqttclient/StandaloneMqttConnector.java +++ b/src/main/java/com/aws/greengrass/mqttclient/StandaloneMqttConnector.java @@ -126,7 +126,8 @@ private static void configureProxy(AwsIotMqttConnectionBuilder mqttBuilder, } String noProxy = Coerce.toString(deviceConfiguration.getNoProxyAddresses()); if (Utils.isNotEmpty(noProxy) && Utils.isNotEmpty(endpoint) - && Arrays.stream(noProxy.split(",")).anyMatch(endpoint::matches)) { + && Arrays.stream(noProxy.split(",")) + .anyMatch(pattern -> ProxyUtils.noProxyMatches(endpoint, pattern))) { return; } mqttBuilder.withHttpProxyOptions(httpProxyOptions); diff --git a/src/main/java/com/aws/greengrass/util/ProxyUtils.java b/src/main/java/com/aws/greengrass/util/ProxyUtils.java index e379814212..1a5a976faf 100644 --- a/src/main/java/com/aws/greengrass/util/ProxyUtils.java +++ b/src/main/java/com/aws/greengrass/util/ProxyUtils.java @@ -35,6 +35,8 @@ import java.util.List; import java.util.Set; import java.util.concurrent.atomic.AtomicReference; +import java.util.regex.Pattern; +import java.util.regex.PatternSyntaxException; import java.util.stream.Collectors; import javax.annotation.Nullable; import javax.net.ssl.TrustManager; @@ -442,4 +444,68 @@ public static String getNoProxyEnvVarValue(DeviceConfiguration deviceConfigurati return "localhost"; } + /** + *

Checks whether an endpoint matches a single noProxyAddresses entry without ever throwing.

+ * + *

Entries are Java regular expressions (existing documented behavior) and are trimmed of + * surrounding whitespace before matching. Note this is a small behavior change: previously a + * space-padded entry (e.g. from a comma-space separated list) could never match. A blank entry + * or an entry that does not compile is logged and treated as a non-match, so a bad + * noProxyAddresses value (e.g. a glob-style entry such as *.example.com) can no + * longer crash MQTT connection setup with a {@link PatternSyntaxException}.

+ * + * @param endpoint the endpoint host being connected to; null or empty never matches + * @param pattern a single noProxyAddresses entry + * @return true if the endpoint matches the entry; false for blank or invalid entries + */ + public static boolean noProxyMatches(String endpoint, String pattern) { + String trimmed = pattern == null ? "" : pattern.trim(); + if (Utils.isEmpty(endpoint) || Utils.isEmpty(trimmed)) { + return false; + } + try { + return endpoint.matches(trimmed); + } catch (PatternSyntaxException e) { + logger.atWarn().kv("pattern", pattern).setCause(e) + .log("Ignoring invalid noProxyAddresses entry; it will never match an endpoint"); + return false; + } + } + + /** + *

Validates a comma-separated noProxyAddresses value at config-validation time.

+ * + *

Flags entries that do not compile as a Java regular expression (which the runtime matcher + * would silently never match), and entries with embedded whitespace (which compile but can + * never match a hostname -- almost certainly a typo). Empty segments produced by stray commas + * are skipped rather than flagged, matching the runtime matcher which treats them as harmless + * non-matches.

+ * + * @param noProxyAddresses comma-separated noProxyAddresses value from config + * @return the invalid entries; empty when all entries are valid + */ + public static List validateNoProxyAddresses(String noProxyAddresses) { + List invalid = new ArrayList<>(); + if (Utils.isEmpty(noProxyAddresses)) { + return invalid; + } + for (String entry : noProxyAddresses.split(",")) { + String trimmed = entry.trim(); + if (trimmed.isEmpty()) { + // A stray comma is benign -- skip it rather than fail the deployment + continue; + } + if (trimmed.contains(" ") || trimmed.contains("\t")) { + invalid.add(trimmed); + continue; + } + try { + Pattern.compile(trimmed); + } catch (PatternSyntaxException e) { + invalid.add(trimmed); + } + } + return invalid; + } + } diff --git a/src/test/java/com/aws/greengrass/deployment/bootstrap/BootstrapManagerTest.java b/src/test/java/com/aws/greengrass/deployment/bootstrap/BootstrapManagerTest.java index eaf07a7835..cfeea2adff 100644 --- a/src/test/java/com/aws/greengrass/deployment/bootstrap/BootstrapManagerTest.java +++ b/src/test/java/com/aws/greengrass/deployment/bootstrap/BootstrapManagerTest.java @@ -604,6 +604,68 @@ void GIVEN_run_with_changes_invalid_WHEN_isBootstrapRequired_THEN_return_true() bootstrapManager.isBootstrapRequired(config)); } + @Test + void GIVEN_invalid_noProxyAddresses_WHEN_isBootstrapRequired_THEN_deployment_rejected() { + when(context.get(DeviceConfiguration.class)).thenReturn(deviceConfiguration); + when(kernel.getContext()).thenReturn(context); + + BootstrapManager bootstrapManager = new BootstrapManager(kernel, platform); + Map config = new HashMap() {{ + put(SERVICES_NAMESPACE_TOPIC, new HashMap() {{ + put(DEFAULT_NUCLEUS_COMPONENT_NAME, new HashMap() {{ + put(SERVICE_TYPE_TOPIC_KEY, ComponentType.NUCLEUS.toString()); + put(CONFIGURATION_CONFIG_KEY, new HashMap() {{ + put(DeviceConfiguration.DEVICE_NETWORK_PROXY_NAMESPACE, new HashMap() {{ + put(DeviceConfiguration.DEVICE_PARAM_NO_PROXY_ADDRESSES, "*.sts.amazon.com"); + }}); + }}); + }}); + }}); + }}; + + ComponentConfigurationValidationException e = assertThrows(ComponentConfigurationValidationException.class, + () -> bootstrapManager.isBootstrapRequired(config)); + assertThat(e.getMessage(), stringContainsInOrder("Invalid noProxyAddresses", "*.sts.amazon.com")); + } + + @Test + void GIVEN_valid_noProxyAddresses_with_stray_commas_WHEN_isBootstrapRequired_THEN_validation_passes() + throws ServiceUpdateException, ComponentConfigurationValidationException, ServiceLoadException { + when(context.get(DeviceConfiguration.class)).thenReturn(deviceConfiguration); + when(kernel.getContext()).thenReturn(context); + + GenericExternalService service = mock(GenericExternalService.class); + doReturn(false).when(service).isBootstrapRequired(anyMap()); + when(kernel.locate(DEFAULT_NUCLEUS_COMPONENT_NAME)).thenReturn(service); + when(deviceConfiguration.getSpoolerNamespace().findOrDefault(any(), any())) + .thenReturn(SpoolerStorageType.Memory); + Topics mockMqttTopics = mock(Topics.class); + when(mockMqttTopics.findOrDefault(any(), any())).thenAnswer((c) -> c.getArgument(0)); + when(deviceConfiguration.getMQTTNamespace()).thenReturn(mockMqttTopics); + Map runWith = mockRunWith; + when(deviceConfiguration.getRunWithTopic().toPOJO()).thenReturn(runWith); + + BootstrapManager bootstrapManager = new BootstrapManager(kernel, platform); + Map config = new HashMap() {{ + put(SERVICES_NAMESPACE_TOPIC, new HashMap() {{ + put(DEFAULT_NUCLEUS_COMPONENT_NAME, new HashMap() {{ + put(SERVICE_TYPE_TOPIC_KEY, ComponentType.NUCLEUS.toString()); + put(CONFIGURATION_CONFIG_KEY, new HashMap() {{ + put(DeviceConfiguration.DEVICE_NETWORK_PROXY_NAMESPACE, new HashMap() {{ + // stray commas are benign and must not fail the deployment + put(DeviceConfiguration.DEVICE_PARAM_NO_PROXY_ADDRESSES, + "sts.amazon.com,,data.iot.us-west-2.amazonaws.com,"); + }}); + put(DeviceConfiguration.RUN_WITH_TOPIC, runWith); + }}); + }}); + }}); + }}; + + // noProxyAddresses changed vs current config -> restart required, but no validation exception + assertTrue(bootstrapManager.isBootstrapRequired(config)); + } + @Test void GIVEN_spooler_storage_type_changes_WHEN_isBootstrapRequired_THEN_return_true() throws ServiceUpdateException, ComponentConfigurationValidationException, ServiceLoadException { diff --git a/src/test/java/com/aws/greengrass/util/ProxyUtilsTest.java b/src/test/java/com/aws/greengrass/util/ProxyUtilsTest.java index 09e35674b0..1910e10703 100644 --- a/src/test/java/com/aws/greengrass/util/ProxyUtilsTest.java +++ b/src/test/java/com/aws/greengrass/util/ProxyUtilsTest.java @@ -9,14 +9,21 @@ import com.aws.greengrass.testcommons.testutilities.GGExtension; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.api.extension.ExtensionContext; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import software.amazon.awssdk.crt.http.HttpProxyOptions; import software.amazon.awssdk.crt.io.ClientTlsContext; import software.amazon.awssdk.crt.io.TlsContextOptions; +import java.util.List; +import java.util.regex.PatternSyntaxException; + +import static com.aws.greengrass.testcommons.testutilities.ExceptionLogProtector.ignoreExceptionOfType; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.when; @ExtendWith({GGExtension.class, MockitoExtension.class}) @@ -149,4 +156,71 @@ void testGetNoProxyEnvVarValue_noProxyConfigured() { assertEquals("", ProxyUtils.getNoProxyEnvVarValue(deviceConfiguration)); } + @Test + void testNoProxyMatches_validRegexBehaviorPreserved() { + // Existing documented behavior: entries are Java regular expressions + assertTrue(ProxyUtils.noProxyMatches("sts.amazon.com", "sts.amazon.com")); + assertTrue(ProxyUtils.noProxyMatches("foo.sts.amazon.com", ".*\\.sts\\.amazon\\.com")); + assertFalse(ProxyUtils.noProxyMatches("example.com", "sts.amazon.com")); + } + + @Test + void testNoProxyMatches_invalidPatternDoesNotThrow(ExtensionContext context) { + // The matcher logs the rejected entry -- expected, not a test failure + ignoreExceptionOfType(context, PatternSyntaxException.class); + // Previously an entry like "*.sts.amazon.com" threw PatternSyntaxException + // ("Dangling meta character '*'") and killed MQTT connectivity + assertFalse(ProxyUtils.noProxyMatches("foo.sts.amazon.com", "*.sts.amazon.com")); + assertFalse(ProxyUtils.noProxyMatches("anything.com", "[invalid")); + } + + @Test + void testNoProxyMatches_emptyOrBlankPatternNeverMatches() { + assertFalse(ProxyUtils.noProxyMatches("anything.com", "")); + assertFalse(ProxyUtils.noProxyMatches("anything.com", " ")); + } + + @Test + void testNoProxyMatches_nullOrEmptyEndpointNeverMatchesOrThrows() { + // The method's contract is total crash-safety -- a missing endpoint is a non-match, not an NPE + assertFalse(ProxyUtils.noProxyMatches(null, "sts.amazon.com")); + assertFalse(ProxyUtils.noProxyMatches("", "sts.amazon.com")); + } + + @Test + void testNoProxyMatches_surroundingWhitespaceTrimmed() { + assertTrue(ProxyUtils.noProxyMatches("sts.amazon.com", " sts.amazon.com ")); + } + + @Test + void testValidateNoProxyAddresses_validEntriesAccepted() { + assertTrue(ProxyUtils.validateNoProxyAddresses("sts.amazon.com,.*\\.example\\.com,127.0.0.1").isEmpty()); + assertTrue(ProxyUtils.validateNoProxyAddresses("").isEmpty()); + assertTrue(ProxyUtils.validateNoProxyAddresses(null).isEmpty()); + } + + @Test + void testValidateNoProxyAddresses_emptySegmentsSkippedNotRejected() { + // A stray comma must not fail an otherwise valid deployment + assertTrue(ProxyUtils.validateNoProxyAddresses("a.com,,b.com").isEmpty()); + assertTrue(ProxyUtils.validateNoProxyAddresses(",a.com").isEmpty()); + assertTrue(ProxyUtils.validateNoProxyAddresses("a.com,").isEmpty()); + } + + @Test + void testValidateNoProxyAddresses_uncompilablePatternRejected() { + // These are exactly the entries the runtime matcher cannot process + List invalid = ProxyUtils.validateNoProxyAddresses("*.sts.amazon.com,valid.com,[invalid"); + assertEquals(2, invalid.size()); + assertTrue(invalid.contains("*.sts.amazon.com")); + assertTrue(invalid.contains("[invalid")); + } + + @Test + void testValidateNoProxyAddresses_embeddedWhitespaceRejected() { + List invalid = ProxyUtils.validateNoProxyAddresses("has space.com,valid.com"); + assertEquals(1, invalid.size()); + assertTrue(invalid.contains("has space.com")); + } + }