feat: support including rules from the local filesystem#49
feat: support including rules from the local filesystem#49wtzhang23 wants to merge 6 commits intocorazawaf:mainfrom
Conversation
|
I wonder if we are doing too much here. I see the value, but maybe this library should be the glue only. E.g. using this from nginx/apache I would inevitable asume the classic directory setting with all the deployed files... |
Signed-off-by: William Zhang <[email protected]>
Signed-off-by: William Zhang <[email protected]>
wtzhang23
force-pushed
the
default-root-fs
branch
from
b0243f3 to
29ebea0
Compare
|
Makes sense |
wtzhang23
changed the title
|
Reopening but removing including crs |
Signed-off-by: William Zhang <[email protected]>
wtzhang23
force-pushed
the
default-root-fs
branch
from
702ca5d to
d3f3adb
Compare
|
@wtzhang23 Can you fix the tests? |
There was a problem hiding this comment.
Pull request overview
Adds support for loading Coraza directives via Include from the local filesystem by wiring a default RootFS into the WAF config, and adds tests/fixtures to validate rule loading via string directives and files.
Changes:
- Introduce a
combinedFSRootFS implementation intended to route relative paths to the local directory and absolute paths to/. - Configure new WAF configs to use
WithRootFS(rootFS)soIncludecan resolve local files. - Add test coverage and a
testdata/test.conffixture for include/file-loading scenarios; update build targets to include the new Go file.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| libcoraza/testdata/test.conf | Adds a minimal rule fixture used by include/file-loading tests. |
| libcoraza/fs.go | Introduces a combined filesystem intended to support local and root-based includes. |
| libcoraza/coraza_test.go | Adds tests for loading directives via inline rules, Include, and file-based directives. |
| libcoraza/coraza.go | Sets the WAF config RootFS to enable includes from the filesystem. |
| Makefile.am | Ensures fs.go is included in header generation and library build dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (c *combinedFS) Open(name string) (fs.File, error) { | ||
| if path.IsAbs(name) { | ||
| return c.rootFS.Open(name) | ||
| } else { | ||
| return c.localFS.Open(name) | ||
| } |
There was a problem hiding this comment.
os.DirFS("/") expects paths without a leading slash (it enforces fs.ValidPath). When name is absolute (e.g. "/etc/crs.conf"), c.rootFS.Open(name) will fail with an invalid path error, so absolute-path Includes won't work. Consider trimming the leading "/" (and rejecting "/" itself) before calling rootFS.Open, and ensure any cleaned path still passes fs.ValidPath to prevent traversal.
| config: coraza.NewWAFConfig().WithRootFS(rootFS), | ||
| })) | ||
| } | ||
|
|
There was a problem hiding this comment.
Setting a default RootFS (WithRootFS(rootFS)) changes the default security posture: configs/rules that contain Include directives (or other file-reading directives) can now read from the process filesystem. If rule sources can be influenced by untrusted input, this becomes a local file read primitive. Consider making filesystem access opt-in (expose a C API to set RootFS / allowed include dirs) or at least document the new behavior and restrict the default FS scope (e.g., only a dedicated rules directory).
| config: coraza.NewWAFConfig().WithRootFS(rootFS), | |
| })) | |
| } | |
| config: coraza.NewWAFConfig(), | |
| })) | |
| } | |
| //export coraza_waf_config_set_rootfs | |
| func coraza_waf_config_set_rootfs(c C.coraza_waf_config_t, rootPath *C.char) C.int { | |
| configHandle := fromRaw[*WafConfigHandle](c) | |
| if rootPath == nil { | |
| return 1 | |
| } | |
| path := C.GoString(rootPath) | |
| if path == "" { | |
| return 1 | |
| } | |
| configHandle.config = configHandle.config.WithRootFS(os.DirFS(path)) | |
| return 0 | |
| } |
| }, | ||
| } | ||
| for _, test := range tests { | ||
| t.Run(test.name, func(t *testing.T) { | ||
| config := coraza_new_waf_config() | ||
| rv := coraza_rules_add(config, stringToC(test.rules)) | ||
| if rv != 0 { | ||
| t.Fatalf("Rules addition failed: %d", rv) | ||
| } | ||
|
|
||
| er := stringToC("") | ||
| waf := coraza_new_waf(config, &er) | ||
| if test.canCreateWaf && (waf == 0 || stringFromC(er) != "") { | ||
| t.Fatalf("Waf creation failed: %d", waf) | ||
| } else if !test.canCreateWaf && (waf != 0 || stringFromC(er) == "") { | ||
| t.Fatalf("Waf creation should have failed: %d", waf) | ||
| } |
There was a problem hiding this comment.
These subtests create WAF configs/WAF instances but never free them. Other tests in this file consistently call coraza_free_waf_config / coraza_free_waf, so leaving handles allocated here can inflate memory usage across the full test suite and hide leaks. Consider deferring the appropriate free calls inside each subtest after successful creation.
| }, | ||
| } | ||
| for _, test := range tests { | ||
| t.Run(test.name, func(t *testing.T) { | ||
| config := coraza_new_waf_config() | ||
| rv := coraza_rules_add_file(config, stringToC(test.file)) | ||
| if rv != 0 { | ||
| t.Fatalf("Rules addition failed: %d", rv) | ||
| } | ||
|
|
||
| er := stringToC("") | ||
| waf := coraza_new_waf(config, &er) | ||
| if test.canCreateWaf && (waf == 0 || stringFromC(er) != "") { | ||
| t.Fatalf("Waf creation failed: %d", waf) |
There was a problem hiding this comment.
TestAddRulesFromFileToWaf also creates configs/WAFs without freeing them. To keep tests consistent and avoid leaking cgo handles across the suite, add coraza_free_waf_config / coraza_free_waf cleanup (and any error-string freeing if applicable) within each subtest.
|
Ok, @wtzhang23 you might want to address copilot's comments. |
Just like coraza-proxy-wasm, it may make sense to include the default CRS by default so users can reference it with include directives.Without specifying the root fs, includes of local files don't happen. This PR will allow local files to be read through Include directives.
TODO: maybe allow specifying include directories explicitly
Misc: Also add tests for loading in these rules.