Skip to content

Commit 91aba31

Browse files
manuel-alvarez-alvarezmanuel-alvarez-alvarez
committed
Fix NPE in AppSecConfigServiceImpl
1 parent 6168591 commit 91aba31

File tree

3 files changed

+149
-46
lines changed

3 files changed

+149
-46
lines changed

‎dd-java-agent/appsec/src/main/java/com/datadog/appsec/config/AppSecConfigServiceImpl.java‎

Lines changed: 82 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@
2121
import static datadog.remoteconfig.Capabilities.CAPABILITY_ASM_TRUSTED_IPS;
2222
import static datadog.remoteconfig.Capabilities.CAPABILITY_ASM_USER_BLOCKING;
2323
import static datadog.remoteconfig.Capabilities.CAPABILITY_ENDPOINT_FINGERPRINT;
24+
import static datadog.trace.api.ConfigOrigin.DEFAULT;
25+
import static datadog.trace.api.ConfigOrigin.LOCAL_STABLE_CONFIG;
2426

2527
import com.datadog.appsec.AppSecModule;
2628
import com.datadog.appsec.AppSecSystem;
@@ -45,19 +47,21 @@
4547
import datadog.remoteconfig.state.ConfigKey;
4648
import datadog.remoteconfig.state.ProductListener;
4749
import datadog.trace.api.Config;
50+
import datadog.trace.api.ConfigOrigin;
4851
import datadog.trace.api.ProductActivation;
4952
import datadog.trace.api.UserIdCollectionMode;
5053
import datadog.trace.api.telemetry.LogCollector;
5154
import datadog.trace.api.telemetry.WafMetricCollector;
5255
import java.io.ByteArrayInputStream;
56+
import java.io.File;
5357
import java.io.FileInputStream;
5458
import java.io.FileNotFoundException;
5559
import java.io.IOException;
5660
import java.io.InputStream;
61+
import java.nio.file.Paths;
5762
import java.util.ArrayList;
5863
import java.util.Collections;
5964
import java.util.HashMap;
60-
import java.util.HashSet;
6165
import java.util.List;
6266
import java.util.Map;
6367
import java.util.Set;
@@ -71,12 +75,12 @@ public class AppSecConfigServiceImpl implements AppSecConfigService {
7175
private static final Logger log = LoggerFactory.getLogger(AppSecConfigServiceImpl.class);
7276

7377
private static final String DEFAULT_CONFIG_LOCATION = "default_config.json";
78+
private static final String DEFAULT_WAF_CONFIG_RULE = "DEFAULT_WAF_CONFIG";
7479

7580
private final ConfigurationPoller configurationPoller;
7681
private WafBuilder wafBuilder;
7782

78-
private MergedAsmFeatures mergedAsmFeatures;
79-
private volatile boolean initialized;
83+
private final MergedAsmFeatures mergedAsmFeatures = new MergedAsmFeatures();
8084

8185
private final ConcurrentHashMap<String, SubconfigListener> subconfigListeners =
8286
new ConcurrentHashMap<>();
@@ -95,10 +99,9 @@ public class AppSecConfigServiceImpl implements AppSecConfigService {
9599
.build()
96100
.adapter(Types.newParameterizedType(Map.class, String.class, Object.class));
97101

98-
private boolean hasUserWafConfig;
99-
private boolean defaultConfigActivated;
100-
private final Set<String> usedDDWafConfigKeys = new HashSet<>();
101-
private final String DEFAULT_WAF_CONFIG_RULE = "DEFAULT_WAF_CONFIG";
102+
private ConfigOrigin wafConfigOrigin;
103+
private final Set<String> usedDDWafConfigKeys =
104+
Collections.newSetFromMap(new ConcurrentHashMap<>());
102105
private String currentRuleVersion;
103106
private List<AppSecModule> modulesToUpdateVersionIn;
104107

@@ -113,13 +116,14 @@ public AppSecConfigServiceImpl(
113116
if (tracerConfig.isAppSecWafMetrics()) {
114117
traceSegmentPostProcessors.add(statsReporter);
115118
}
119+
wafConfigOrigin = hasUserWafConfig(tracerConfig) ? LOCAL_STABLE_CONFIG : DEFAULT;
116120
}
117121

118122
private void subscribeConfigurationPoller() {
119123
// see also close() method
120124
subscribeAsmFeatures();
121125

122-
if (!hasUserWafConfig) {
126+
if (wafConfigOrigin != LOCAL_STABLE_CONFIG) {
123127
subscribeRulesAndData();
124128
} else {
125129
log.debug("Will not subscribe to ASM, ASM_DD and ASM_DATA (AppSec custom rules in use)");
@@ -173,16 +177,10 @@ private class AppSecConfigChangesListener implements ProductListener {
173177
@Override
174178
public void accept(ConfigKey configKey, byte[] content, PollingRateHinter pollingRateHinter)
175179
throws IOException {
176-
if (!initialized) {
177-
throw new IllegalStateException();
178-
}
180+
maybeApplyDefaultConfig();
179181

180182
if (content == null) {
181-
try {
182-
wafBuilder.removeConfig(configKey.toString());
183-
} catch (UnclassifiedWafException e) {
184-
throw new RuntimeException(e);
185-
}
183+
remove(configKey, pollingRateHinter);
186184
} else {
187185
Map<String, Object> contentMap =
188186
ADAPTER.fromJson(Okio.buffer(Okio.source(new ByteArrayInputStream(content))));
@@ -197,7 +195,11 @@ public void accept(ConfigKey configKey, byte[] content, PollingRateHinter pollin
197195
@Override
198196
public void remove(ConfigKey configKey, PollingRateHinter pollingRateHinter)
199197
throws IOException {
200-
accept(configKey, null, pollingRateHinter);
198+
try {
199+
wafBuilder.removeConfig(configKey.toString());
200+
} catch (UnclassifiedWafException e) {
201+
throw new RuntimeException(e);
202+
}
201203
}
202204

203205
@Override
@@ -206,28 +208,48 @@ public void commit(PollingRateHinter pollingRateHinter) {
206208
}
207209
}
208210

209-
private class AppSecConfigChangesDDListener extends AppSecConfigChangesListener {
211+
private class AppSecConfigChangesDDListener implements ProductListener {
210212
@Override
211213
public void accept(ConfigKey configKey, byte[] content, PollingRateHinter pollingRateHinter)
212214
throws IOException {
213-
if (defaultConfigActivated) { // if we get any config, remove the default one
214-
log.debug("Removing default config");
215+
if (content == null) {
216+
remove(configKey, pollingRateHinter);
217+
} else {
218+
if (usedDDWafConfigKeys.remove(DEFAULT_WAF_CONFIG_RULE)) {
219+
log.debug("Removing default config");
220+
try {
221+
wafBuilder.removeConfig(DEFAULT_WAF_CONFIG_RULE);
222+
} catch (UnclassifiedWafException e) {
223+
throw new RuntimeException("Error removing default WAF config", e);
224+
}
225+
}
226+
Map<String, Object> contentMap =
227+
ADAPTER.fromJson(Okio.buffer(Okio.source(new ByteArrayInputStream(content))));
215228
try {
216-
wafBuilder.removeConfig(DEFAULT_WAF_CONFIG_RULE);
217-
} catch (UnclassifiedWafException e) {
229+
final String key = configKey.toString();
230+
handleWafUpdateResultReport(key, contentMap);
231+
usedDDWafConfigKeys.add(key);
232+
} catch (AppSecModule.AppSecModuleActivationException e) {
218233
throw new RuntimeException(e);
219234
}
220-
defaultConfigActivated = false;
221235
}
222-
super.accept(configKey, content, pollingRateHinter);
223-
usedDDWafConfigKeys.add(configKey.toString());
224236
}
225237

226238
@Override
227239
public void remove(ConfigKey configKey, PollingRateHinter pollingRateHinter)
228240
throws IOException {
229-
super.remove(configKey, pollingRateHinter);
230-
usedDDWafConfigKeys.remove(configKey.toString());
241+
try {
242+
final String key = configKey.toString();
243+
wafBuilder.removeConfig(key);
244+
usedDDWafConfigKeys.remove(key);
245+
} catch (UnclassifiedWafException e) {
246+
throw new RuntimeException(e);
247+
}
248+
}
249+
250+
@Override
251+
public void commit(PollingRateHinter pollingRateHinter) {
252+
// no action needed
231253
}
232254
}
233255

@@ -252,7 +274,8 @@ private void handleWafUpdateResultReport(String configKey, Map<String, Object> r
252274
if (wafDiagnostics.rulesetVersion != null
253275
&& !wafDiagnostics.rulesetVersion.isEmpty()
254276
&& !wafDiagnostics.rules.getLoaded().isEmpty()
255-
&& (!defaultConfigActivated || currentRuleVersion == null)) {
277+
&& (!usedDDWafConfigKeys.contains(DEFAULT_CONFIG_LOCATION)
278+
|| currentRuleVersion == null)) {
256279
currentRuleVersion = wafDiagnostics.rulesetVersion;
257280
statsReporter.setRulesVersion(currentRuleVersion);
258281
if (modulesToUpdateVersionIn != null) {
@@ -282,13 +305,7 @@ private void subscribeAsmFeatures() {
282305
Product.ASM_FEATURES,
283306
AppSecFeaturesDeserializer.INSTANCE,
284307
(configKey, newConfig, hinter) -> {
285-
if (!hasUserWafConfig && !defaultConfigActivated) {
286-
// features activated in runtime
287-
init();
288-
}
289-
if (!initialized) {
290-
throw new IllegalStateException();
291-
}
308+
maybeApplyDefaultConfig();
292309
if (newConfig == null) {
293310
mergedAsmFeatures.removeConfig(configKey);
294311
} else {
@@ -305,10 +322,7 @@ private void subscribeAsmFeatures() {
305322

306323
private void distributeSubConfigurations(
307324
String key, AppSecModuleConfigurer.Reconfiguration reconfiguration) {
308-
if (usedDDWafConfigKeys.isEmpty() && !defaultConfigActivated && !hasUserWafConfig) {
309-
// no config left in the WAF builder, add the default config
310-
init();
311-
}
325+
maybeApplyDefaultConfig();
312326
for (Map.Entry<String, SubconfigListener> entry : subconfigListeners.entrySet()) {
313327
SubconfigListener listener = entry.getValue();
314328
try {
@@ -320,43 +334,56 @@ private void distributeSubConfigurations(
320334
}
321335
}
322336

337+
private void maybeApplyDefaultConfig() {
338+
if (!usedDDWafConfigKeys.isEmpty()) {
339+
return;
340+
}
341+
// any error here means that we are not able to fetch the default/user config so we print a
342+
// message and stop receiving RC messages as the installation is broken
343+
try {
344+
init();
345+
} catch (Exception e) {
346+
log.error("Error applying default AppSec configuration; AppSec won´t work properly", e);
347+
close();
348+
}
349+
}
350+
323351
@Override
324352
public void init() {
325353
Map<String, Object> wafConfig;
326-
hasUserWafConfig = false;
327354
try {
328355
wafConfig = loadUserWafConfig(tracerConfig);
356+
wafConfigOrigin = LOCAL_STABLE_CONFIG;
329357
} catch (Exception e) {
330358
log.error("Error loading user-provided config", e);
331359
throw new AbortStartupException("Error loading user-provided config", e);
332360
}
333361
if (wafConfig == null) {
334362
try {
335363
wafConfig = loadDefaultWafConfig();
336-
defaultConfigActivated = true;
364+
wafConfigOrigin = DEFAULT;
337365
} catch (IOException e) {
338366
log.error("Error loading default config", e);
339367
throw new AbortStartupException("Error loading default config", e);
340368
}
341-
} else {
342-
hasUserWafConfig = true;
343369
}
344-
this.mergedAsmFeatures = new MergedAsmFeatures();
345-
this.initialized = true;
370+
mergedAsmFeatures.clear();
371+
usedDDWafConfigKeys.clear();
346372

347373
if (wafConfig.isEmpty()) {
348374
throw new IllegalStateException("Expected default waf config to be available");
349375
}
350376
try {
351377
handleWafUpdateResultReport(DEFAULT_WAF_CONFIG_RULE, wafConfig);
378+
usedDDWafConfigKeys.add(DEFAULT_WAF_CONFIG_RULE);
352379
} catch (AppSecModule.AppSecModuleActivationException e) {
353380
throw new RuntimeException(e);
354381
}
355382
}
356383

357384
public void maybeSubscribeConfigPolling() {
358385
if (this.configurationPoller != null) {
359-
if (hasUserWafConfig
386+
if (wafConfigOrigin == LOCAL_STABLE_CONFIG
360387
&& tracerConfig.getAppSecActivation() == ProductActivation.FULLY_ENABLED) {
361388
log.info(
362389
"AppSec will not use remote config because "
@@ -437,6 +464,15 @@ private static Map<String, Object> loadDefaultWafConfig() throws IOException {
437464
}
438465
}
439466

467+
private static boolean hasUserWafConfig(Config tracerConfig) {
468+
String filename = tracerConfig.getAppSecRulesFile();
469+
if (filename == null) {
470+
return false;
471+
}
472+
File file = Paths.get(filename).toFile();
473+
return file.exists() && file.isFile() && file.canRead();
474+
}
475+
440476
private static Map<String, Object> loadUserWafConfig(Config tracerConfig) throws IOException {
441477
log.debug("Loading user waf config");
442478
String filename = tracerConfig.getAppSecRulesFile();

‎dd-java-agent/appsec/src/main/java/com/datadog/appsec/config/MergedAsmFeatures.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,4 +49,9 @@ private void mergeAutoUserInstrum(
4949
}
5050
target.autoUserInstrum = newValue;
5151
}
52+
53+
public void clear() {
54+
mergedData = null;
55+
configs.clear();
56+
}
5257
}

‎dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/config/AppSecConfigServiceImplSpecification.groovy‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -759,6 +759,68 @@ class AppSecConfigServiceImplSpecification extends DDSpecification {
759759
p.toFile().delete()
760760
}
761761

762+
// https://github.com/DataDog/dd-trace-java/issues/9159
763+
void 'test initialization issues while applying remote config'() {
764+
setup:
765+
final key = new ParsedConfigKey('Test', '1234', 1, 'ASM_DD', 'ID')
766+
final service = new AppSecConfigServiceImpl(config, poller, reconf)
767+
config.getAppSecActivation() >> ProductActivation.ENABLED_INACTIVE
768+
769+
when:
770+
service.maybeSubscribeConfigPolling()
771+
772+
then:
773+
1 * poller.addListener(Product.ASM_DD, _) >> {
774+
listeners.savedWafDataChangesListener = it[1]
775+
}
776+
777+
when:
778+
listeners.savedWafDataChangesListener.accept(key, '''{"rules_override": [{"rules_target": [{"rule_id": "foo"}], "enabled": false}]}'''.getBytes(), NOOP)
779+
780+
then:
781+
noExceptionThrown()
782+
}
783+
784+
void 'test default config is applied when RC removes all ASM_DD configs'() {
785+
setup:
786+
final key = new ParsedConfigKey('Test', '1234', 1, 'ASM_DD', 'ID')
787+
final service = new AppSecConfigServiceImpl(config, poller, reconf)
788+
config.getAppSecActivation() >> ProductActivation.ENABLED_INACTIVE
789+
790+
when:
791+
service.maybeSubscribeConfigPolling()
792+
793+
then:
794+
1 * poller.addListener(Product.ASM_DD, _) >> {
795+
listeners.savedWafDataChangesListener = it[1]
796+
}
797+
1 * poller.addListener(Product.ASM_FEATURES, _, _) >> {
798+
listeners.savedFeaturesDeserializer = it[1]
799+
listeners.savedFeaturesListener = it[2]
800+
}
801+
service.usedDDWafConfigKeys.empty // lazy loaded
802+
803+
when:
804+
listeners.savedFeaturesListener.accept('asm_features conf',
805+
listeners.savedFeaturesDeserializer.deserialize('{"asm":{"enabled": true}}'.bytes),
806+
NOOP)
807+
808+
then:
809+
service.usedDDWafConfigKeys.toList() == [AppSecConfigServiceImpl.DEFAULT_WAF_CONFIG_RULE]
810+
811+
when:
812+
listeners.savedWafDataChangesListener.accept(key, '''{"rules_override": [{"rules_target": [{"rule_id": "foo"}], "enabled": false}]}'''.getBytes(), NOOP)
813+
814+
then:
815+
service.usedDDWafConfigKeys.toList() == [key.toString()]
816+
817+
when:
818+
listeners.savedWafDataChangesListener.remove(key, NOOP)
819+
820+
then:
821+
service.usedDDWafConfigKeys.empty
822+
}
823+
762824
private static AppSecFeatures autoUserInstrum(String mode) {
763825
return new AppSecFeatures().tap { features ->
764826
features.autoUserInstrum = new AppSecFeatures.AutoUserInstrum().tap { instrum ->

0 commit comments

Comments
 (0)