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,15 @@ 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, ""));

// Validate noProxyAddresses patterns before applying — reject deployment early on invalid entries
List<String> invalidEntries = ProxyUtils.validateNoProxyAddresses(newNoProxyAddresses);
if (!invalidEntries.isEmpty()) {
throw new ComponentConfigurationValidationException(
"Invalid noProxyAddresses entries: " + invalidEntries
+ ". Supported formats: '*.domain.com', '.domain.com', 'domain.com', or IP addresses.");
}

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 @@ -252,7 +252,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
83 changes: 83 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,85 @@ public static String getNoProxyEnvVarValue(DeviceConfiguration deviceConfigurati
return "localhost";
}

/**
* <p>Validates noProxyAddresses entries at config-load time.</p>
*
* <p>Rejects entries that are clearly invalid (empty after trim, contain spaces, or use
* unsupported wildcard positions). Valid patterns are:</p>
* <ul>
* <li><code>*.domain.com</code> — wildcard subdomain prefix</li>
* <li><code>.domain.com</code> — leading-dot (equivalent to *.)</li>
* <li><code>domain.com</code> — exact hostname</li>
* <li><code>127.0.0.1</code> — exact IP</li>
* </ul>
*
* @param noProxyAddresses comma-separated noProxy string from config
* @return list of invalid entries (empty if all 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()) {
invalid.add(entry);
continue;
}
// Reject wildcards in unsupported positions (only leading *. is valid)
if (trimmed.contains("*") && !trimmed.startsWith("*.")) {
invalid.add(entry);
continue;
}
// Reject entries with spaces (likely typo)
if (trimmed.contains(" ")) {
invalid.add(entry);
}
}
return invalid;
}

/**
* <p>Checks whether an endpoint matches a NO_PROXY pattern using standard glob-style matching.</p>
*
* <p>Supports the standard NO_PROXY conventions:</p>
* <ul>
* <li><code>*.domain.com</code> — matches any subdomain (e.g. foo.domain.com)</li>
* <li><code>.domain.com</code> — same as *.domain.com (leading-dot convention)</li>
* <li><code>domain.com</code> — exact match only</li>
* </ul>
*
* <p>Falls back gracefully on invalid patterns — logs a warning and returns false (no match),
* preventing a bad noProxyAddresses entry from crashing the process.</p>
*
* @param endpoint the hostname to check (e.g. "data-ats.iot.us-west-2.amazonaws.com")
* @param pattern the NO_PROXY pattern to match against
* @return true if the endpoint matches the pattern
*/
public static boolean noProxyMatches(String endpoint, String pattern) {
String trimmed = pattern.trim();
if (Utils.isEmpty(trimmed)) {
return false;
}

String regex;
if (trimmed.startsWith("*.")) {
// *.domain.com → match any prefix ending with .domain.com
regex = ".*" + Pattern.quote(trimmed.substring(1));
} else if (trimmed.startsWith(".")) {
// .domain.com → same as *.domain.com per NO_PROXY convention
regex = ".*" + Pattern.quote(trimmed);
} else {
// Exact hostname match — quote all special chars
regex = Pattern.quote(trimmed);
}

try {
return endpoint.matches(regex);
} catch (PatternSyntaxException e) {

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.

Problem: An exception is being logged incorrectly.

Fix: Set the original exception as the cause of the log statement. Otherwise, you will lose the stack trace and message of the original exception, which will make it difficult to analyze the event that caused the exception.
Learn more

Suggested remediation:
Pass the caught exceptions to the logs to retain the original exception message and the stack trace.

@@ -484,3 +484,3 @@
         } catch (PatternSyntaxException e) {
-            logger.warn(&#34;Invalid noProxyAddress pattern &#39;{}&#39;, skipping: {}&#34;, pattern, e.getMessage());
+            logger.warn(&#34;Invalid noProxyAddress pattern &#39;{}&#39;, skipping: {}&#34;, pattern, e.getMessage(), e);
             return false;

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.

Problem: An exception is being logged incorrectly.

Fix: Set the original exception as the cause of the log statement. Otherwise, you will lose the stack trace and message of the original exception, which will make it difficult to analyze the event that caused the exception.
Learn more

Suggested remediation:
Pass the caught exceptions to the logs to retain the original exception message and the stack trace.

@@ -523,3 +523,3 @@
         } catch (PatternSyntaxException e) {
-            logger.warn(&#34;Invalid noProxyAddress pattern &#39;{}&#39;, skipping: {}&#34;, pattern, e.getMessage());
+            logger.warn(&#34;Invalid noProxyAddress pattern &#39;{}&#39;, skipping: {}&#34;, pattern, e.getMessage(), e);
             return false;

logger.warn("Invalid noProxyAddress pattern '{}', skipping: {}", pattern, e.getMessage());
return false;
}
}
}
66 changes: 66 additions & 0 deletions src/test/java/com/aws/greengrass/util/ProxyUtilsTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

import com.aws.greengrass.deployment.DeviceConfiguration;
import com.aws.greengrass.testcommons.testutilities.GGExtension;

import java.util.List;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.Mock;
Expand All @@ -16,7 +18,9 @@
import software.amazon.awssdk.crt.io.TlsContextOptions;

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 +153,66 @@ void testGetNoProxyEnvVarValue_noProxyConfigured() {
assertEquals("", ProxyUtils.getNoProxyEnvVarValue(deviceConfiguration));
}

@Test
void testNoProxyMatches_wildcardPrefix() {
assertTrue(ProxyUtils.noProxyMatches("foo.sts.amazon.com", "*.sts.amazon.com"));
assertTrue(ProxyUtils.noProxyMatches("bar.baz.sts.amazon.com", "*.sts.amazon.com"));
assertFalse(ProxyUtils.noProxyMatches("sts.amazon.com", "*.sts.amazon.com"));
assertFalse(ProxyUtils.noProxyMatches("notsts.amazon.com", "*.sts.amazon.com"));
}

@Test
void testNoProxyMatches_leadingDot() {
// Leading dot is equivalent to *. per NO_PROXY convention
assertTrue(ProxyUtils.noProxyMatches("foo.sts.amazon.com", ".sts.amazon.com"));
assertTrue(ProxyUtils.noProxyMatches("bar.baz.sts.amazon.com", ".sts.amazon.com"));
assertFalse(ProxyUtils.noProxyMatches("sts.amazon.com", ".sts.amazon.com"));
}

@Test
void testNoProxyMatches_exactMatch() {
assertTrue(ProxyUtils.noProxyMatches("sts.amazon.com", "sts.amazon.com"));
assertFalse(ProxyUtils.noProxyMatches("foo.sts.amazon.com", "sts.amazon.com"));
assertFalse(ProxyUtils.noProxyMatches("sts.amazon.com.evil.com", "sts.amazon.com"));
}

@Test
void testNoProxyMatches_invalidPatternDoesNotThrow() {
// Previously this would throw PatternSyntaxException and crash Nucleus
assertFalse(ProxyUtils.noProxyMatches("anything.com", "[invalid"));
assertFalse(ProxyUtils.noProxyMatches("anything.com", ""));
assertFalse(ProxyUtils.noProxyMatches("anything.com", " "));
}

@Test
void testNoProxyMatches_dotsAreNotRegexWildcards() {
// Dots in patterns must be literal, not regex "any char"
assertFalse(ProxyUtils.noProxyMatches("stsXamazonXcom", "sts.amazon.com"));
assertTrue(ProxyUtils.noProxyMatches("sts.amazon.com", "sts.amazon.com"));
}

@Test
void testValidateNoProxyAddresses_validPatterns() {
// All valid — should return empty list
assertTrue(ProxyUtils.validateNoProxyAddresses("*.sts.amazon.com,.domain.com,exact.com,127.0.0.1").isEmpty());
assertTrue(ProxyUtils.validateNoProxyAddresses("").isEmpty());
assertTrue(ProxyUtils.validateNoProxyAddresses(null).isEmpty());
}

@Test
void testValidateNoProxyAddresses_invalidPatterns() {
// Wildcard in wrong position
List<String> invalid = ProxyUtils.validateNoProxyAddresses("foo.*.com,*bar.com,valid.com");
assertEquals(2, invalid.size());
assertTrue(invalid.contains("foo.*.com"));
assertTrue(invalid.contains("*bar.com"));
}

@Test
void testValidateNoProxyAddresses_spacesRejected() {
List<String> invalid = ProxyUtils.validateNoProxyAddresses("has space.com,valid.com");
assertEquals(1, invalid.size());
assertTrue(invalid.contains("has space.com"));
}

}
Loading