Skip to content

Commit 0684458

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

File tree

3 files changed

+98
-31
lines changed

3 files changed

+98
-31
lines changed

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

Lines changed: 30 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -75,8 +75,8 @@ public class AppSecConfigServiceImpl implements AppSecConfigService {
7575
private final ConfigurationPoller configurationPoller;
7676
private WafBuilder wafBuilder;
7777

78-
private MergedAsmFeatures mergedAsmFeatures;
79-
private volatile boolean initialized;
78+
private final MergedAsmFeatures mergedAsmFeatures = new MergedAsmFeatures();
79+
private volatile State state = State.UNINITIALIZED;
8080

8181
private final ConcurrentHashMap<String, SubconfigListener> subconfigListeners =
8282
new ConcurrentHashMap<>();
@@ -95,8 +95,6 @@ public class AppSecConfigServiceImpl implements AppSecConfigService {
9595
.build()
9696
.adapter(Types.newParameterizedType(Map.class, String.class, Object.class));
9797

98-
private boolean hasUserWafConfig;
99-
private boolean defaultConfigActivated;
10098
private final Set<String> usedDDWafConfigKeys = new HashSet<>();
10199
private final String DEFAULT_WAF_CONFIG_RULE = "DEFAULT_WAF_CONFIG";
102100
private String currentRuleVersion;
@@ -119,7 +117,7 @@ private void subscribeConfigurationPoller() {
119117
// see also close() method
120118
subscribeAsmFeatures();
121119

122-
if (!hasUserWafConfig) {
120+
if (state != State.USER_CONFIG) {
123121
subscribeRulesAndData();
124122
} else {
125123
log.debug("Will not subscribe to ASM, ASM_DD and ASM_DATA (AppSec custom rules in use)");
@@ -173,10 +171,7 @@ private class AppSecConfigChangesListener implements ProductListener {
173171
@Override
174172
public void accept(ConfigKey configKey, byte[] content, PollingRateHinter pollingRateHinter)
175173
throws IOException {
176-
if (!initialized) {
177-
throw new IllegalStateException();
178-
}
179-
174+
maybeInit();
180175
if (content == null) {
181176
try {
182177
wafBuilder.removeConfig(configKey.toString());
@@ -210,15 +205,15 @@ private class AppSecConfigChangesDDListener extends AppSecConfigChangesListener
210205
@Override
211206
public void accept(ConfigKey configKey, byte[] content, PollingRateHinter pollingRateHinter)
212207
throws IOException {
213-
if (defaultConfigActivated) { // if we get any config, remove the default one
208+
if (state == State.DEFAULT_CONFIG) { // if we get any config, remove the default one
214209
log.debug("Removing default config");
215210
try {
216211
wafBuilder.removeConfig(DEFAULT_WAF_CONFIG_RULE);
217212
} catch (UnclassifiedWafException e) {
218213
throw new RuntimeException(e);
219214
}
220-
defaultConfigActivated = false;
221215
}
216+
state = State.REMOTE_CONFIG;
222217
super.accept(configKey, content, pollingRateHinter);
223218
usedDDWafConfigKeys.add(configKey.toString());
224219
}
@@ -228,6 +223,9 @@ public void remove(ConfigKey configKey, PollingRateHinter pollingRateHinter)
228223
throws IOException {
229224
super.remove(configKey, pollingRateHinter);
230225
usedDDWafConfigKeys.remove(configKey.toString());
226+
if (usedDDWafConfigKeys.isEmpty()) {
227+
init(); // initialize again the default config
228+
}
231229
}
232230
}
233231

@@ -252,7 +250,7 @@ private void handleWafUpdateResultReport(String configKey, Map<String, Object> r
252250
if (wafDiagnostics.rulesetVersion != null
253251
&& !wafDiagnostics.rulesetVersion.isEmpty()
254252
&& !wafDiagnostics.rules.getLoaded().isEmpty()
255-
&& (!defaultConfigActivated || currentRuleVersion == null)) {
253+
&& (state != State.DEFAULT_CONFIG || currentRuleVersion == null)) {
256254
currentRuleVersion = wafDiagnostics.rulesetVersion;
257255
statsReporter.setRulesVersion(currentRuleVersion);
258256
if (modulesToUpdateVersionIn != null) {
@@ -282,13 +280,7 @@ private void subscribeAsmFeatures() {
282280
Product.ASM_FEATURES,
283281
AppSecFeaturesDeserializer.INSTANCE,
284282
(configKey, newConfig, hinter) -> {
285-
if (!hasUserWafConfig && !defaultConfigActivated) {
286-
// features activated in runtime
287-
init();
288-
}
289-
if (!initialized) {
290-
throw new IllegalStateException();
291-
}
283+
maybeInit();
292284
if (newConfig == null) {
293285
mergedAsmFeatures.removeConfig(configKey);
294286
} else {
@@ -305,10 +297,6 @@ private void subscribeAsmFeatures() {
305297

306298
private void distributeSubConfigurations(
307299
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-
}
312300
for (Map.Entry<String, SubconfigListener> entry : subconfigListeners.entrySet()) {
313301
SubconfigListener listener = entry.getValue();
314302
try {
@@ -320,29 +308,33 @@ private void distributeSubConfigurations(
320308
}
321309
}
322310

311+
private void maybeInit() {
312+
if (state == State.UNINITIALIZED) {
313+
init();
314+
}
315+
}
316+
323317
@Override
324318
public void init() {
325319
Map<String, Object> wafConfig;
326-
hasUserWafConfig = false;
327320
try {
328321
wafConfig = loadUserWafConfig(tracerConfig);
322+
state = State.USER_CONFIG;
329323
} catch (Exception e) {
330324
log.error("Error loading user-provided config", e);
331325
throw new AbortStartupException("Error loading user-provided config", e);
332326
}
333327
if (wafConfig == null) {
334328
try {
335329
wafConfig = loadDefaultWafConfig();
336-
defaultConfigActivated = true;
330+
state = State.DEFAULT_CONFIG;
337331
} catch (IOException e) {
338332
log.error("Error loading default config", e);
339333
throw new AbortStartupException("Error loading default config", e);
340334
}
341-
} else {
342-
hasUserWafConfig = true;
343335
}
344-
this.mergedAsmFeatures = new MergedAsmFeatures();
345-
this.initialized = true;
336+
mergedAsmFeatures.clear();
337+
usedDDWafConfigKeys.clear();
346338

347339
if (wafConfig.isEmpty()) {
348340
throw new IllegalStateException("Expected default waf config to be available");
@@ -356,7 +348,7 @@ public void init() {
356348

357349
public void maybeSubscribeConfigPolling() {
358350
if (this.configurationPoller != null) {
359-
if (hasUserWafConfig
351+
if (state == State.USER_CONFIG
360352
&& tracerConfig.getAppSecActivation() == ProductActivation.FULLY_ENABLED) {
361353
log.info(
362354
"AppSec will not use remote config because "
@@ -467,6 +459,7 @@ private static int countRules(Map<String, Object> config) {
467459

468460
@Override
469461
public void close() {
462+
state = State.UNINITIALIZED;
470463
if (this.configurationPoller == null) {
471464
return;
472465
}
@@ -558,4 +551,11 @@ private static WafConfig createWafConfig(Config config) {
558551
}
559552
return wafConfig;
560553
}
554+
555+
private enum State {
556+
UNINITIALIZED,
557+
DEFAULT_CONFIG,
558+
USER_CONFIG,
559+
REMOTE_CONFIG
560+
}
561561
}

‎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: 63 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import java.nio.file.Files
2020
import java.nio.file.Path
2121
import java.nio.file.Paths
2222

23+
2324
import static datadog.remoteconfig.Capabilities.CAPABILITY_ASM_ACTIVATION
2425
import static datadog.remoteconfig.Capabilities.CAPABILITY_ASM_AUTO_USER_INSTRUM_MODE
2526
import static datadog.remoteconfig.Capabilities.CAPABILITY_ASM_CUSTOM_BLOCKING_RESPONSE
@@ -130,7 +131,6 @@ class AppSecConfigServiceImplSpecification extends DDSpecification {
130131
appSecConfigService.maybeSubscribeConfigPolling()
131132

132133
then:
133-
2 * config.getAppSecActivation() >> ProductActivation.ENABLED_INACTIVE
134134
1 * poller.addListener(Product.ASM_FEATURES, _, _)
135135
1 * poller.addConfigurationEndListener(_)
136136
0 * poller.addListener(*_)
@@ -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: 'start with the system not yet initialized'
791+
service.maybeSubscribeConfigPolling()
792+
793+
then: 'state is UNINITIALIZED'
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.state == AppSecConfigServiceImpl.State.UNINITIALIZED
802+
803+
when: 'ASM is enabled via RC'
804+
listeners.savedFeaturesListener.accept('asm_features conf',
805+
listeners.savedFeaturesDeserializer.deserialize('{"asm":{"enabled": true}}'.bytes),
806+
NOOP)
807+
808+
then: 'state is DEFAULT_CONFIG'
809+
service.state == AppSecConfigServiceImpl.State.DEFAULT_CONFIG
810+
811+
when: 'Received a new config via RC'
812+
listeners.savedWafDataChangesListener.accept(key, '''{"rules_override": [{"rules_target": [{"rule_id": "foo"}], "enabled": false}]}'''.getBytes(), NOOP)
813+
814+
then: 'state is REMOTE_CONFIG'
815+
service.state == AppSecConfigServiceImpl.State.REMOTE_CONFIG
816+
817+
when: 'Remove all configs via RC'
818+
listeners.savedWafDataChangesListener.remove(key, NOOP)
819+
820+
then: 'state is back to DEFAULT_CONFIG'
821+
service.state == AppSecConfigServiceImpl.State.DEFAULT_CONFIG
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)