Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -262,7 +263,8 @@ private boolean spoolerStorageTypeHasChanged(Map<String, Object> newNucleusParam
}

private boolean networkProxyHasChanged(Map<String, Object> newNucleusParameters,
DeviceConfiguration currentDeviceConfiguration) {
DeviceConfiguration currentDeviceConfiguration)
throws ComponentConfigurationValidationException {
Map<String, Object> newNetworkProxy =
(Map<String, Object>) newNucleusParameters.get(DEVICE_NETWORK_PROXY_NAMESPACE);
if (newNetworkProxy == null) {
Expand All @@ -271,6 +273,16 @@ private boolean networkProxyHasChanged(Map<String, Object> 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<String> 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);
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/com/aws/greengrass/mqttclient/MqttClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
66 changes: 66 additions & 0 deletions src/main/java/com/aws/greengrass/util/ProxyUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -442,4 +444,68 @@ public static String getNoProxyEnvVarValue(DeviceConfiguration deviceConfigurati
return "localhost";
}

/**
* <p>Checks whether an endpoint matches a single noProxyAddresses entry without ever throwing.</p>
*
* <p>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 <code>*.example.com</code>) can no
* longer crash MQTT connection setup with a {@link PatternSyntaxException}.</p>
*
* @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;
}
}

/**
* <p>Validates a comma-separated noProxyAddresses value at config-validation time.</p>
*
* <p>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.</p>
*
* @param noProxyAddresses comma-separated noProxyAddresses value from config
* @return the invalid entries; empty when all entries are valid
*/
public static List<String> validateNoProxyAddresses(String noProxyAddresses) {
List<String> 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);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.

When using regular expressions (regex) in Java, it's best practice to first compile your regex pattern separately instead of using pattern matching methods directly. By compiling first, you can reuse the same Pattern instance repeatedly without having to recompile.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Agreed in principle, but leaving as-is for this PR: this path only runs during MQTT connection setup against a small, comma-separated config list, so recompilation cost is negligible, and caching compiled Patterns would add static state for little gain. A follow-up PR (glob wildcard support for noProxyAddresses) restructures this exact code around a shared entry-translation helper — that's the right place to revisit pattern reuse if it matters.

} catch (PatternSyntaxException e) {
invalid.add(trimmed);
}
}
return invalid;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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<String, Object> config = new HashMap<String, Object>() {{
put(SERVICES_NAMESPACE_TOPIC, new HashMap<String, Object>() {{
put(DEFAULT_NUCLEUS_COMPONENT_NAME, new HashMap<String, Object>() {{
put(SERVICE_TYPE_TOPIC_KEY, ComponentType.NUCLEUS.toString());
put(CONFIGURATION_CONFIG_KEY, new HashMap<String, Object>() {{
put(DeviceConfiguration.DEVICE_NETWORK_PROXY_NAMESPACE, new HashMap<String, Object>() {{
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<String, Object> runWith = mockRunWith;
when(deviceConfiguration.getRunWithTopic().toPOJO()).thenReturn(runWith);

BootstrapManager bootstrapManager = new BootstrapManager(kernel, platform);
Map<String, Object> config = new HashMap<String, Object>() {{
put(SERVICES_NAMESPACE_TOPIC, new HashMap<String, Object>() {{
put(DEFAULT_NUCLEUS_COMPONENT_NAME, new HashMap<String, Object>() {{
put(SERVICE_TYPE_TOPIC_KEY, ComponentType.NUCLEUS.toString());
put(CONFIGURATION_CONFIG_KEY, new HashMap<String, Object>() {{
put(DeviceConfiguration.DEVICE_NETWORK_PROXY_NAMESPACE, new HashMap<String, Object>() {{
// 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 {
Expand Down
74 changes: 74 additions & 0 deletions src/test/java/com/aws/greengrass/util/ProxyUtilsTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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})
Expand Down Expand Up @@ -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<String> 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<String> invalid = ProxyUtils.validateNoProxyAddresses("has space.com,valid.com");
assertEquals(1, invalid.size());
assertTrue(invalid.contains("has space.com"));
}

}
Loading