Add active WAF rules version to crash telemetry payload#11954
Add active WAF rules version to crash telemetry payload#11954jandro996 wants to merge 3 commits into
Conversation
- ConfigManager: patch a single .cfg entry in place (updateCrashConfigEntry) without a fresh Config snapshot or a new shutdown hook - WAFStatsReporter.setRulesVersion: funnel the WAF ruleset version into the crash config on every load/RC update - CrashUploader: emit waf_rules_version in the telemetry payload and error-tracking ddtags when present - Migrate WAFStatsReporterSpecification.groovy to JUnit5 WAFStatsReporterTest.java
- ConfigManager: synchronize writeConfigToPath to match updateCrashConfigEntry, closing a race window on the cached cfg file - ConfigManager: delete the orphaned temp file when writing to it fails, not just when the atomic rename fails - ConfigManagerTest: add coverage for the no-op paths (no cached cfg, null key/value) and reset the cached static field in @AfterEach - WAFStatsReporterTest: reset the cached static field in @AfterEach for test isolation
jandro996
added
tag: ai generated
|
@codex review |
jandro996
added
tag: ai generated
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f280128e09
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public static synchronized void writeConfigToPath(File scriptFile, String... additionalEntries) { | ||
| String cfgFileName = getBaseName(scriptFile) + PID_PREFIX + PidHelper.getPid() + ".cfg"; | ||
| File cfgFile = new File(scriptFile.getParentFile(), cfgFileName); | ||
| ConfigManager.cfgFile = cfgFile; |
There was a problem hiding this comment.
writeConfigToPath is used for both the crash uploader and the OOME notifier; in the normal initializer path initializeCrashUploader(...) runs before initializeOOMENotifier(...), whose OOMENotifierScriptInitializer.initialize calls writeConfigToPath again. Caching a single cfgFile here means the later OOME config (dd_oome_notifier_pid*.cfg) replaces the crash uploader config path, so WAFStatsReporter.setRulesVersion patches the OOME cfg and the upload_crash script still reads a crash cfg without waf_rules_version.
Useful? React with 👍 / 👎.
| File cfgFile = ConfigManager.cfgFile; | ||
| if (cfgFile == null || key == null || value == null) { | ||
| return; |
There was a problem hiding this comment.
On Java 9+ the crashtracking initializer is scheduled asynchronously before AppSec starts, so AppSec can call setRulesVersion while this cached file is still null. Returning here permanently drops the bundled ruleset version; when writeConfigToPath later creates the crash uploader cfg it has no way to add it, and unless another remote-config update arrives crash telemetry omits the active WAF version.
Useful? React with 👍 / 👎.
| if (!tmpFile.renameTo(cfgFile)) { | ||
| LOGGER.debug("Failed to atomically replace config file: {}", cfgFile); | ||
| tmpFile.delete(); // best-effort cleanup; failure is acceptable here |
There was a problem hiding this comment.
When crash tracking runs on Windows, this path attempts to rename a temp file over an already-existing .cfg. File.renameTo does not reliably replace an existing target there, so rules-version updates can leave the original cfg unchanged and delete the patched temp file, causing Windows crash uploads to miss waf_rules_version.
Useful? React with 👍 / 👎.
|
🎯 Code Coverage (details) 🔗 Commit SHA: 53143f8 | Docs | Datadog PR Page | Give us feedback! |
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
- Stop the OOME notifier's .cfg write from clobbering the crash-uploader's cached path so waf_rules_version reaches the right file - Buffer updateCrashConfigEntry calls until the .cfg file is cached, so the initial bundled WAF ruleset version isn't lost if it arrives before the deferred Java 9+ crash-tracking init finishes - Fall back to delete+rename when replacing the .cfg file on Windows, where File.renameTo does not reliably replace an existing target
1 participant
What Does This Do
ConfigManagergainsupdateCrashConfigEntry(String key, String value): patches a single entry of the already-written.cfgfile in place — reads all existingkey=valuelines (including onesreadConfigdoesn't parse, likeagent/hs_err/javacore_path), replaces/inserts the target key, and writes atomically via a temp file +renameTo. UnlikewriteConfigToFile, it does not take a freshConfigsnapshot and does not register a new shutdown hook, since it can be called repeatedly (once per Remote Config update).ConfigManagercaches theFilewritten bywriteConfigToPathin avolatilestatic field soupdateCrashConfigEntrycan locate it later — the.cfgpath can't be recomputed elsewhere (HotSpot default temp dir vs J9-Xdump:toolcustom path differ).writeConfigToPathandupdateCrashConfigEntryare bothsynchronizedon the class monitor to avoid the initial write and a near-immediate Remote Config update racing on the same file.StoredConfig(nested class) and itsBuildergain a nullablewafRulesVersionfield;ConfigManager.readConfig's switch adds acase "waf_rules_version".WAFStatsReporter.setRulesVersion(String)now also callsConfigManager.updateCrashConfigEntry("waf_rules_version", rulesetVersion)— this is the single mutation point for the WAF ruleset version, covering both the initial bundled-ruleset load and every subsequent Remote Config push.CrashUploaderemitswaf_rules_versionin the crash telemetry payload and error-tracking tags whenstoredConfig.wafRulesVersionis present.dd-java-agent/appsec/build.gradleadds a dependency ondd-java-agent:agent-crashtracking(needed forWAFStatsReporterto call intoConfigManager).WAFStatsReporterSpecification.groovyto JUnit 5WAFStatsReporterTest.java.Motivation
Correlate JVM crashes in production with the WAF ruleset version that was active at the time of the crash. Spin-off of APPSEC-62784 (SIGSEGV in
ddwaf_run).Additional Notes
None.
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: APPSEC-68788
Note: Once your PR is ready to merge, add it to the merge queue by commenting
/merge./merge -ccancels the queue request./merge -f --reason "reason"skips all merge queue checks; please use this judiciously, as some checks do not run at the PR-level. For more information, see this doc.