Stop reporting DNS noise#317
Conversation
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| private static void process(String hostname, InetAddress[] inetAddresses) { |
There was a problem hiding this comment.
Private method process(String, InetAddress[]) is too vague; give it a descriptive name (e.g., handleDnsLookupResults or handleDnsRecord) and add Javadoc describing its responsibilities.
Details
✨ AI Reasoning
A private helper named 'process' was added to handle DNS lookup results. Its single-word name is vague given the method performs multiple distinct tasks: recording stats, consuming and mutating pending hostname state, applying outbound-blocking rules, converting InetAddress objects, and running SSRF and stored-SSRF detections. Without a descriptive name or Javadoc, future maintainers will need to read the implementation to understand which responsibilities it encapsulates and when/how it should be used.
🔧 How do I fix it?
Use descriptive verb-noun function names, add docstrings explaining the function's purpose, or provide meaningful return type hints.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
|
|
||
| // Flush pending hostnames on every context change to prevent the store from | ||
| // growing unboundedly when a thread is reused across multiple requests. | ||
| PendingHostnamesStore.clear(); |
There was a problem hiding this comment.
we stopped clearing explicitly, now we use lru cache with 5000 entries limit to avoid memory leaks (see PendingHostnamesStore)
stopped clearing explicitly because now we store hostnames to ports globally (we used to store it thread local)
| } | ||
|
|
||
| // Block if the hostname is in the blocked domains list | ||
| if (ServiceConfigStore.shouldBlockOutgoingRequest(hostname)) { |
There was a problem hiding this comment.
Shouldn't this be tied to the URLCollector somehow?
…l site Private IP literal hostnames with no pending port (resolver bootstrap noise, network capability probing, etc.) flood the outbound-connections dashboard - skip recording those specifically. Outbound domain blocking and SSRF detection stay fully unconditional either way. Known-port HTTP client calls now record their dashboard hit directly at the call site (URLCollector), matching every other Zen agent (Go, Node, Ruby, .NET, PHP) and restoring the original architecture from #264. DNS-resolution-time recording is now only a fallback for hostnames that never reached an instrumented HTTP client (JDBC, raw sockets, etc.) - which is also the only place the private-IP noise gate applies.
Mishenevd
force-pushed
the
fix/private-ip-dns-noise-and-thread-hop
branch
from
353995b to
303a687
Compare
| private DNSRecordCollector() {} | ||
| private static final Logger logger = LogManager.getLogger(DNSRecordCollector.class); | ||
| private static final String OPERATION_NAME = "java.net.InetAddress.getAllByName"; | ||
|
|
There was a problem hiding this comment.
OPERATION_NAME constant was added but the same string literal is still used in registerCall. Use OPERATION_NAME in registerCall to avoid duplicated literals and simplify maintenance.
Details
✨ AI Reasoning
A constant OPERATION_NAME was added and later used in SSRFDetector and StoredSSRFDetector calls, but the call to StatisticsStore.registerCall still uses the literal string "java.net.InetAddress.getAllByName". This duplicates the same string in two places and was introduced by the current diff (the added OPERATION_NAME constant at the top of the file). Using the constant in the registerCall call would be a simpler, less error-prone, and consistent approach. This change is purely readability/maintenance (no behavior change) and is fixable within this PR.
🔧 How do I fix it?
Rewrite the snippet in the simpler, behavior-equivalent form: return a boolean expression directly instead of if cond return true else return false, avoid using lists when they are guaranteed to contain one element, etc.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| // Record the hit here, at the real call site, where the port is always known - | ||
| // instead of waiting for the DNS lookup, which can't distinguish this call from | ||
| // unrelated infra noise resolving the same hostname. | ||
| HostnamesStore.incrementHits(url.getHost(), port); |
There was a problem hiding this comment.
URLCollector.report now both adds to PendingHostnamesStore and records HostnamesStore.incrementHits, mixing responsibilities and adding an unexpected side-effect; consider renaming or splitting into clearer methods.
Details
✨ AI Reasoning
A public method named report in a collector class now both records a pending hostname/port and also increments the HostnamesStore hit counter. The method's name doesn't make these distinct side-effects obvious, and the change increased its responsibilities (state mutation of two different stores). This makes the function's purpose less self-evident to callers and maintainers.
🔧 How do I fix it?
Use descriptive verb-noun function names, add docstrings explaining the function's purpose, or provide meaningful return type hints.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| // We still need to report a hit to the hostname for outbound domain blocking | ||
| // URLCollector already recorded known-port hits. This is the fallback for hostnames | ||
| // it never saw (JDBC, raw sockets, ...) - skip only private IP literals, infra noise. | ||
| if (ports.isEmpty() && !IsPrivateIP.isPrivateIp(hostname)) { |
There was a problem hiding this comment.
DNSRecordCollector.report's behavior now conditionally skips recording hostname hits for private IPs when ports are absent, increasing mixed responsibilities and making the method's purpose less clear; consider renaming or extracting the fallback hit-recording into a well-named function.
Details
✨ AI Reasoning
The DNSRecordCollector.report method's role became less obvious: it now conditionally records a fallback HostnamesStore hit only when no pending ports exist and the hostname is not a private IP. The method already handled multiple concerns (port consumption, blocking, SSRF detection); the added conditional logic increases cognitive load and makes 'report' a vague name for these responsibilities.
🔧 How do I fix it?
Use descriptive verb-noun function names, add docstrings explaining the function's purpose, or provide meaningful return type hints.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Mishenevd
changed the title
3 participants
Minimal fix for the Minze Health flood (customer's on okhttp3, not WebClient) - split off from the Spring client work, see #312.
PendingHostnamesStorewasThreadLocal, so async clients (OkHttpenqueue()) could lose the pending port on a thread hop, making a real request look like noise. Now a global bounded-LRU map, carrying captured context along.enqueue()case (fails without the fix, passes with it).Spring WebClient/Netty instrumentation stays in #312.
Summary by Aikido
⚡ Enhancements
🐛 Bugfixes
More info