WAF Builder: independent configuration manager to generate WAF instances #363

Anilm3 · 2025-01-07T17:13:35Z

This PR introduces a large change which is hereby referred to as the WAF builder. This new builder allows for the generation of WAF instances through independent configurations, while keeping track of all independent configurations so that they can be updated and / or removed in order to update an existing instance.

Configurations are uniquely identified by their path and can provide any and all of the supported primitives, which will be subsequently consolidated in order to generate a consistent instance. Generally, the WAF builder holds both the configurations (and specifications), as well as common evaluation primitives, for example, generated rules are kept and reused on each new instance for as long as they don't need to be updated. Similarly, expressions within filters, rules and processors are also preserved across updates, when relevant.

The change has also involved a large refactoring of the ruleset parser into multiple independent parsers all feeding into a single configuration, while keeping track of their individual changes. This refactor has resulted in configuration parsers and a separate set of "builders" which can be used to generate instances of each evaluation primitive based on all of the required elements that feed into their creation.

An example of using the new builder interface can be the following:

ddwaf_builder builder = ddwaf_builder_init(nullptr);

ddwaf_object rules = read_file("custom_rules.yaml");
ddwaf_builder_add_or_update_config(builder, "rules", sizeof("rules") - 1, &rules, nullptr);
ddwaf_object_free(&rules);

ddwaf_object custom_rules = read_file("custom_rules.yaml");
ddwaf_builder_add_or_update_config(builder, "custom_rules", sizeof("custom_rules") - 1, &custom_rules, nullptr);
ddwaf_object_free(&custom_rules);

ddwaf_handle handle = ddwaf_builder_build_instance(builder);

// Use the handle...

ddwaf_destroy(&handle); // the handle can be destroyed at any point

ddwaf_object custom_rules2 = read_file("custom_rules_2.yaml");
ddwaf_builder_add_or_update_config(builder, "custom_rules", sizeof("custom_rules") - 1, &custom_rules2, nullptr);
ddwaf_object_free(&custom_rules2);

handle = ddwaf_builder_build_instance(builder);

// Use the handle...

ddwaf_destroy(handle);

// Remove the custom rules
ddwaf_builder_remove_config(builder, "custom_rules", sizeof("custom_rules") - 1);
handle = ddwaf_builder_build_instance(builder);

// Use the handle...

ddwaf_destroy(handle);

ddwaf_builder_destroy(builder);

Pending tasks (to be addressed in new PRs)

Required:

More test variants for configuration updates
Diagnostics severity level (error, warning)
Smoketest update to include builder symbols

Optional:

WAF builder "fuzzer" test
Rename ddwaf_config?

Related Jira: APPSEC-55064

… spec

codecov-commenter · 2025-01-07T17:36:32Z

Codecov Report

Attention: Patch coverage is 87.05752% with 117 lines in your changes missing coverage. Please review.

Project coverage is 85.32%. Comparing base (73ad8e7) to head (b3101b5).

Files with missing lines	Patch %	Lines
src/interface.cpp	41.81%	23 Missing and 9 partials ⚠️
src/configuration/configuration_manager.cpp	83.43%	2 Missing and 26 partials ⚠️
src/configuration/legacy_rule_parser.cpp	71.73%	10 Missing and 3 partials ⚠️
src/transformer/common/utf8.cpp	73.68%	2 Missing and 3 partials ⚠️
src/builder/ruleset_builder.cpp	95.91%	1 Missing and 3 partials ⚠️
src/configuration/common/common.hpp	69.23%	2 Missing and 2 partials ⚠️
src/builder/action_mapper_builder.cpp	84.21%	1 Missing and 2 partials ⚠️
src/configuration/actions_parser.cpp	87.50%	0 Missing and 3 partials ⚠️
...c/configuration/common/configuration_collector.hpp	96.00%	1 Missing and 2 partials ⚠️
src/configuration/rule_override_parser.cpp	76.92%	2 Missing and 1 partial ⚠️
... and 11 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #363      +/-   ##
==========================================
+ Coverage   85.13%   85.32%   +0.18%     
==========================================
  Files         157      163       +6     
  Lines        7993     8169     +176     
  Branches     3551     3600      +49     
==========================================
+ Hits         6805     6970     +165     
+ Misses        459      439      -20     
- Partials      729      760      +31

Flag	Coverage Δ
waf_test	`85.32% <87.05%> (+0.18%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pr-commenter · 2025-01-07T18:06:03Z

Benchmarks

Benchmark execution time: 2025-01-30 13:30:14

Comparing candidate commit fd841fc in PR branch anilm3/waf_builder with baseline commit 73ad8e7 in branch master.

Found 1 performance improvements and 0 performance regressions! Performance is the same for 0 metrics, 0 unstable metrics.

scenario:global-benchmark.random

🟩 execution_time [-17.852ms; -17.794ms] or [-5.975%; -5.955%]

src/builder/action_mapper_builder.hpp

src/builder/action_mapper_builder.cpp

src/builder/rule_builder.hpp

cataphract · 2025-01-29T18:47:24Z

src/builder/rule_builder.hpp

+            }
+        }
+
+        ancillary_tags_.merge(spec_.tags);


do you really want to modify spec_.tags here?

As mentioned below, the rule builders are single use, I added a comment to make sure it's clear.

cataphract · 2025-01-29T18:52:47Z

src/builder/rule_builder.hpp

+
+        return std::make_shared<core_rule>(std::move(id_), std::move(spec_.name),
+            std::move(ancillary_tags_), std::move(spec_.expr), std::move(spec_.actions),
+            spec_.enabled, spec_.source, verdict);


your builders get to a bad state after calls to build(). Perhaps this is something you want to tackle in the future to prevent misusage

In reference to this comment and the one about tags.

This particular builder is currently single-use so after build it's expected that it won't be useful (e.g. same as after move). I guess I could reset the state and throw if a second build is attempted, but it seems unnecessary.

I've added a comment to make it clear that after build it should be considered invalid.

I did consider caching the rule builders and reusing them later but I'll leave that for a future improvement.

src/builder/ruleset_builder.cpp

cataphract · 2025-01-29T19:17:26Z

src/builder/ruleset_builder.cpp

+    }
+
+    auto rs = std::make_shared<ruleset>();
+    rs->insert_rules(final_base_rules_.items(), final_user_rules_.items());


why are rules individually managed through shared_ptr instead of final_base_rules_ being themselves behind a shared pointer (like the actions). Because AFAICT, whenever we have a base_rule_update, we rebuild the rules from scratch:

if ((current_changes & base_rule_update) != change_set::none) { final_base_rules_.clear(); // ... for (const auto &builder : rule_builders) { if (builder->is_enabled()) { final_base_rules_.emplace(builder->build(*actions_)); } }

Same applies to the user rules.

As mentioned earlier, I would feel much safer if these rules had their immutability enforced by the compiler, to prevent races.

I'll address this in the next PR.

cataphract · 2025-01-29T19:40:12Z

src/builder/ruleset_builder.cpp

+    rs->insert_filters(rule_filters_);
+    rs->insert_filters(input_filters_);
+    rs->insert_preprocessors(preprocessors_);
+    rs->insert_postprocessors(postprocessors_);
+    rs->rule_matchers = rule_matchers_;
+    rs->exclusion_matchers = exclusion_matchers_;


same comment as with the rules. These are always build all together, so they could all be behind one single (for instance) shared_ptr<const map<string, unique_ptr<matcher::base>>>.

It seems the only case where this doesn't apply is the scanners, where a copy of the vector is needed because global_config.scanners can be modified at any point and actions, which already do it that way

Same as above, I'll address this in the next PR.

…builder

Anilm3 · 2025-01-30T14:03:28Z

The remaining comments will be addressed in the next PR as this one is just too large at this stage.

Anilm3 added 23 commits December 6, 2024 22:29

Configuration parser / manager

2cb9137

Restructure the code, remove intermediate container types, split data…

5367308

… spec

Format

3ea643b

Lint

bcc7b94

Minor change

347ae12

Merge branch 'master' into anilm3/waf_builder

57ed5f7

Split actions

7adcde8

Connect new configuration manager and relevant builders

754008c

Delete parser stuff

2dcb3f8

Changes... wip

b1d8dcf

Fix bugs and update tests

a5b8535

Changes to the interface, tests and other fixes

b5976ff

Update more tests and add more fixes

6eab052

Fix more tests

2729947

Fix more tests

f948219

Add support for legacy ruleset schema

44ecc2e

Add parser tests

9cedae0

Add more parser tests

2e2a891

More tests

5044c50

Fix typos

ddc8f91

Remaining parser tests

f473ed1

Fixes and missing changes

a1839b7

Fixes and missing changes

9308259

More fixes

185e42f

Anilm3 added 4 commits January 7, 2025 20:04

Split builder and fix lint

014f86d

Rename manager to avoid static linking problems

0b335a3

Install mono on package-nuget job

dc793bf

Add test

e70b634

Anilm3 added 7 commits January 14, 2025 10:19

Fix?

f95af01

Small changes and tests

759ed60

Expand tests

931c343

Cleanup of TODO's

c0da2d0

Small improvements

7e5a5fa

Add diagnostic tests

d72461c

Merge branch 'master' into anilm3/waf_builder

5623d60

Anilm3 changed the title ~~[WIP] WAF Builder~~ WAF Builder: independent configuration manager to generate WAF instances Jan 22, 2025

Anilm3 marked this pull request as ready for review January 22, 2025 18:27

Anilm3 requested a review from a team as a code owner January 22, 2025 18:27

Cleanup

0f0df28

Anilm3 self-assigned this Jan 22, 2025

Anilm3 added 2 commits January 23, 2025 09:33

Merge branch 'master' into anilm3/waf_builder

e2b36fe

Amend format change

d996b9b

estringana approved these changes Jan 27, 2025

View reviewed changes

Anilm3 added 3 commits January 27, 2025 11:10

Minor changes

c83a87f

Add log messages on action downgrade

d0d798c

Minor improvement

9584de6

Anilm3 requested a review from cataphract January 28, 2025 08:51

cataphract reviewed Jan 29, 2025

View reviewed changes

Anilm3 added 6 commits January 30, 2025 10:36

Make mapper const and some minor fixes

615f1da

Tidy-19 fix

39bb4d3

Second round of tidy fix

b378f86

Third round...

ee2a94c

Use clang-format-17 instead of 19, add clarification comment on rule_…

fd841fc

…builder

Tidy 17 fixes....

b3101b5

Anilm3 merged commit 1de83d9 into master Jan 30, 2025
50 of 51 checks passed

Anilm3 deleted the anilm3/waf_builder branch January 30, 2025 14:03

Anilm3 mentioned this pull request Feb 5, 2025

Wrap containers in the ruleset within shared pointers to reduce copies #366

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WAF Builder: independent configuration manager to generate WAF instances #363

WAF Builder: independent configuration manager to generate WAF instances #363

Anilm3 commented Jan 7, 2025 •

edited

Loading

codecov-commenter commented Jan 7, 2025 •

edited

Loading

pr-commenter bot commented Jan 7, 2025 •

edited

Loading

cataphract Jan 29, 2025

Anilm3 Jan 30, 2025

cataphract Jan 29, 2025

Anilm3 Jan 30, 2025

Anilm3 Jan 30, 2025

cataphract Jan 29, 2025

Anilm3 Jan 30, 2025

cataphract Jan 29, 2025

Anilm3 Jan 30, 2025

Anilm3 commented Jan 30, 2025

WAF Builder: independent configuration manager to generate WAF instances #363

WAF Builder: independent configuration manager to generate WAF instances #363

Conversation

Anilm3 commented Jan 7, 2025 • edited Loading

codecov-commenter commented Jan 7, 2025 • edited Loading

Codecov Report

pr-commenter bot commented Jan 7, 2025 • edited Loading

Benchmarks

scenario:global-benchmark.random

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anilm3 commented Jan 30, 2025

Anilm3 commented Jan 7, 2025 •

edited

Loading

codecov-commenter commented Jan 7, 2025 •

edited

Loading

pr-commenter bot commented Jan 7, 2025 •

edited

Loading