Skip to content

Commit c228fda

Browse files
manuel-alvarez-alvarezmanuel-alvarez-alvarez
committed
Fix NPE in AppSecConfigServiceImpl
1 parent 8486a2b commit c228fda

File tree

3 files changed

+148
-46
lines changed

3 files changed

+148
-46
lines changed

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

Lines changed: 81 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,7 @@ 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+
&& (wafConfigOrigin != DEFAULT || currentRuleVersion == null)) {
256278
currentRuleVersion = wafDiagnostics.rulesetVersion;
257279
statsReporter.setRulesVersion(currentRuleVersion);
258280
if (modulesToUpdateVersionIn != null) {
@@ -282,13 +304,7 @@ private void subscribeAsmFeatures() {
282304
Product.ASM_FEATURES,
283305
AppSecFeaturesDeserializer.INSTANCE,
284306
(configKey, newConfig, hinter) -> {
285-
if (!hasUserWafConfig && !defaultConfigActivated) {
286-
// features activated in runtime
287-
init();
288-
}
289-
if (!initialized) {
290-
throw new IllegalStateException();
291-
}
307+
maybeApplyDefaultConfig();
292308
if (newConfig == null) {
293309
mergedAsmFeatures.removeConfig(configKey);
294310
} else {
@@ -305,10 +321,7 @@ private void subscribeAsmFeatures() {
305321

306322
private void distributeSubConfigurations(
307323
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-
}
324+
maybeApplyDefaultConfig();
312325
for (Map.Entry<String, SubconfigListener> entry : subconfigListeners.entrySet()) {
313326
SubconfigListener listener = entry.getValue();
314327
try {
@@ -320,43 +333,56 @@ private void distributeSubConfigurations(
320333
}
321334
}
322335

336+
private void maybeApplyDefaultConfig() {
337+
if (!usedDDWafConfigKeys.isEmpty()) {
338+
return;
339+
}
340+
// any error here means that we are not able to fetch the default/user config so we print a
341+
// message and stop receiving RC messages as the installation is broken
342+
try {
343+
init();
344+
} catch (Exception e) {
345+
log.error("Error applying default AppSec configuration; AppSec won´t work properly", e);
346+
close();
347+
}
348+
}
349+
323350
@Override
324351
public void init() {
325352
Map<String, Object> wafConfig;
326-
hasUserWafConfig = false;
327353
try {
328354
wafConfig = loadUserWafConfig(tracerConfig);
355+
wafConfigOrigin = LOCAL_STABLE_CONFIG;
329356
} catch (Exception e) {
330357
log.error("Error loading user-provided config", e);
331358
throw new AbortStartupException("Error loading user-provided config", e);
332359
}
333360
if (wafConfig == null) {
334361
try {
335362
wafConfig = loadDefaultWafConfig();
336-
defaultConfigActivated = true;
363+
wafConfigOrigin = DEFAULT;
337364
} catch (IOException e) {
338365
log.error("Error loading default config", e);
339366
throw new AbortStartupException("Error loading default config", e);
340367
}
341-
} else {
342-
hasUserWafConfig = true;
343368
}
344-
this.mergedAsmFeatures = new MergedAsmFeatures();
345-
this.initialized = true;
369+
mergedAsmFeatures.clear();
370+
usedDDWafConfigKeys.clear();
346371

347372
if (wafConfig.isEmpty()) {
348373
throw new IllegalStateException("Expected default waf config to be available");
349374
}
350375
try {
351376
handleWafUpdateResultReport(DEFAULT_WAF_CONFIG_RULE, wafConfig);
377+
usedDDWafConfigKeys.add(DEFAULT_WAF_CONFIG_RULE);
352378
} catch (AppSecModule.AppSecModuleActivationException e) {
353379
throw new RuntimeException(e);
354380
}
355381
}
356382

357383
public void maybeSubscribeConfigPolling() {
358384
if (this.configurationPoller != null) {
359-
if (hasUserWafConfig
385+
if (wafConfigOrigin == LOCAL_STABLE_CONFIG
360386
&& tracerConfig.getAppSecActivation() == ProductActivation.FULLY_ENABLED) {
361387
log.info(
362388
"AppSec will not use remote config because "
@@ -437,6 +463,15 @@ private static Map<String, Object> loadDefaultWafConfig() throws IOException {
437463
}
438464
}
439465

466+
private static boolean hasUserWafConfig(Config tracerConfig) {
467+
String filename = tracerConfig.getAppSecRulesFile();
468+
if (filename == null) {
469+
return false;
470+
}
471+
File file = Paths.get(filename).toFile();
472+
return file.exists() && file.isFile() && file.canRead();
473+
}
474+
440475
private static Map<String, Object> loadUserWafConfig(Config tracerConfig) throws IOException {
441476
log.debug("Loading user waf config");
442477
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)