search

owasp-modsecurity / ModSecurity

ModSecurity is an open source, cross platform web application firewall (WAF) engine for Apache, IIS and Nginx. It has a robust event-based programming language which provides protection from a range of attacks against web applications and allows for HTTP traffic monitoring, logging and real-time analysis.
https://www.modsecurity.org
Apache License 2.0
7.67k stars 1.54k forks source link

Add support to build libModSecurity v3 on Windows #3132

Closed eduar-hte closed 3 days ago

eduar-hte commented 3 weeks ago

This PR includes changes to support building libModSecurity v3 on Windows.

Work was initially done on a snapshot of v3.0.12 to work on the latest stable version of the library, and then changes were applied to v3/master up to the latest commit at the time the PR was created (commit 07e5a70).

The library builds with no warnings and was tested as a 64-bit DLL on Windows, running all unit & regression tests to validate the build. Additionally, the updated code was built on Linux, and unit & regression tests were executed on that version too. See below for more information on the results.

Building the library on Windows is done using MSVC C++ compiler, CMake and Conan package manager, so a new CMake build configuration was introduced for this platform. The following libModSecurity optional dependencies were integrated through available Conan package managers:

These optional features can be enabled/disabled through flag variables in the CMake configuration file. The optional third-party dependencies GeoIP and ssdeep are not included in the port due to lack of Conan packages.

Additionally, a Windows Docker container configuration is included to simplify prerequisites setup and build of the library and associated binaries.

More information on building the library on Windows is available in build/win32/README.md.

Miscellaneous

Summary of changes

The following is a summary of the changes (more detail is available in each commit):

NOTE: The list does not include changes to files to update included header files to compile with the MSVC++ compiler on Windows and other minor changes.

Unit & regression test results

All changes to the latest version of the library on v3/master were built on Ubuntu 18.04 LTS and Ubuntu 22.04 LTS following the build instructions in compilation recipes, with equal results to the unmodified version of libModSecurity.

A 64-bit Windows build of the library obtains the same unit tests results as the Linux build. Regression test execution is also succesful, but 21 of the tests fail due to three different reasons:

13 tests are skipped due to the library not including support for GeoIP and ssdeep at this time (variable-GEO.json depends on GeoIP, and operator-fuzzyhash.json depends on ssdeep).

The following is a list of the failed tests and analysis:

airween commented 3 weeks ago

Hi @eduar-hte,

first of all, let me congratulate you on this pull request: this is an awesome work.

Special thanks for detailed description you attached for the patch.

Although I'm sure the library has built in your environments (both on Linux and Windows), but as you can see there are some issues in our pipeline. Before we merge that, it's unavoidable to fix them.

The most critical issue is the make check-static result here. The fix is trivial, I think that would be enough to modify the test/cppcheck_suppressions.txt file, and change line number, because that line moved up by removing of these two lines.

Also you can take a look the SonarCloud issues, but I'm not sure those are really critical.

After merging this PR with the main codebase, we need to decide how to proceed. It seems like Nginx supported on Windows, but I don't know how do the modules operate on it. If the Nginx module architecture is also works on Windows, then we should make the ModSecurity-nginx connector.

What do you think?

We also welcome you on our Slack server in the channel #project-modsecurity, if you are able to join.

eduar-hte commented 3 weeks ago

The most critical issue is the make check-static result here. The fix is trivial, I think that would be enough to modify the test/cppcheck_suppressions.txt file, and change line number, because that line moved up by removing of these two lines.

I adjusted this in commit d19047a but I reviewed the rest of the files I updated in the PR to adjust other lines as well, see commit 3f9b38c. I think these builds need to be launched again to see if the latest update solves all the issues.

Also you can take a look the SonarCloud issues, but I'm not sure those are really critical.

  • here is a Docker privileging problem; I'm not a Docker user, especially not on Windows, so please help me to decide is this a critical problem or not

I don't think this needs to change, the recommendation is to run the container with low privileges (for example, if you're using the container to run a server). This container, however, is used just to build the library when the container image is built, so the container is not expected to run afterwards. Additionally, admin privileges are required during most of the container image generation, in order to install the prerequisites (MSVC C++ compiler, CMake, Conan package manager, GIT).

  • the others are so called "reliability" issues, perhaps there are few (or all) false positives, but we also need to investigate them.

I've updated some of these today in commits 14ed9d9, 96ff573, 9cd9024 & 459c0c0. These are my notes after reviewing the rest of the issues:

After merging this PR with the main codebase, we need to decide how to proceed. It seems like Nginx supported on Windows, but I don't know how do the modules operate on it. If the Nginx module architecture is also works on Windows, then we should make the ModSecurity-nginx connector.

We're integrating libModSecurity on a custom web server on Windows. I've only looked at the Apache & Nginx ModSecurity connectors as a reference on how to use the library. At some point I saw that Nginx should compile on Windows as well, but I didn't try it.

airween commented 3 weeks ago

The most critical issue is the make check-static result here.

I adjusted this in commit d19047a but I reviewed the rest of the files I updated in the PR to adjust other lines as well, see commit 3f9b38c. I think these builds need to be launched again to see if the latest update solves all the issues.

Thanks!

Because this is your first issue, the CI workflow does not start automatically when you update the PR, so you weren't able to see that there is another check-static issue:

2024-04-27T07:09:25.0572337Z Checking src/modsecurity.cc ...
2024-04-27T07:09:25.0573014Z Defines:MS_CPPCHECK_DISABLED_FOR_PARSER=1
2024-04-27T07:09:25.0573902Z Undefines: MBEDTLS_MD5_ALT; MBEDTLS_SHA1_ALT; YYSTYPE; YY_USER_INIT
2024-04-27T07:09:25.0575313Z Includes: -Iheaders/ -I./ -Iothers/ -Isrc/ -Iothers/mbedtls/ -Isrc/parser/
2024-04-27T07:09:25.0576153Z Platform:Native
2024-04-27T07:09:25.0922287Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1...
2024-04-27T07:09:26.5722454Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;AIX...
2024-04-27T07:09:28.0162535Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;DRAGONFLY...
2024-04-27T07:09:29.4353636Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;FREEBSD...
2024-04-27T07:09:30.8117204Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;HPUX...
2024-04-27T07:09:32.2319299Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;LINUX...
2024-04-27T07:09:33.6444501Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;MACOSX...
2024-04-27T07:09:35.0453940Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;MSC_WITH_CURL...
2024-04-27T07:09:36.4378081Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;NETBSD...
2024-04-27T07:09:37.8554729Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;NO_LOGS...
2024-04-27T07:09:39.3005270Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;OPENBSD...
2024-04-27T07:09:40.7184693Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;SOLARIS...
2024-04-27T07:09:42.1278565Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;WIN32...
2024-04-27T07:09:43.5504218Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;WITH_GEOIP...
2024-04-27T07:09:44.9903719Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;WITH_LIBXML2...
2024-04-27T07:09:47.2117050Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;WITH_MAXMIND...
2024-04-27T07:09:48.6488191Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;WITH_PCRE2...
2024-04-27T07:09:50.1179638Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;WITH_YAJL...
2024-04-27T07:09:51.5599001Z warning: src/modsecurity.cc,265,performance,redundantCopyLocalConst,The const variable 'startingAt' is assigned a copy of the data. You can avoid the unnecessary data copying by converting 'startingAt' to const reference.
2024-04-27T07:09:51.5602109Z warning: src/modsecurity.cc,267,performance,redundantCopyLocalConst,The const variable 'size' is assigned a copy of the data. You can avoid the unnecessary data copying by converting 'size' to const reference.
2024-04-27T07:09:51.5604892Z warning: src/modsecurity.cc,349,performance,redundantCopyLocalConst,The const variable 'startingAt' is assigned a copy of the data. You can avoid the unnecessary data copying by converting 'startingAt' to const reference.
2024-04-27T07:09:51.5607899Z warning: src/modsecurity.cc,351,performance,redundantCopyLocalConst,The const variable 'size' is assigned a copy of the data. You can avoid the unnecessary data copying by converting 'size' to const reference.
2024-04-27T07:09:51.5798124Z 118/218 files checked 45% done

Seems like these came with the new modifications.

  • here is a Docker privileging problem; I'm not a Docker user, especially not on Windows, so please help me to decide is this a critical problem or not

I don't think this needs to change, the recommendation is to run the container with low privileges (for example, if you're using the container to run a server). This container, however, is used just to build the library when the container image is built, so the container is not expected to run afterwards. Additionally, admin privileges are required during most of the container image generation, in order to install the prerequisites (MSVC C++ compiler, CMake, Conan package manager, GIT).

Okay, thanks for clarification. We can mark that issue as false positive with this explanation.

  • the others are so called "reliability" issues, perhaps there are few (or all) false positives, but we also need to investigate them.

I've updated some of these today in commits 14ed9d9, 96ff573, 9cd9024 & 459c0c0. These are my notes after reviewing the rest of the issues:

* `src/utils/msc_tree.cc`: There are several reported issues in this file (about potential memory leak, null pointer dereference, removing unused variables, adding const to some function parameters, etc.) I only updated this file in commit [2207a92](https://github.com/owasp-modsecurity/ModSecurity/commit/2207a92da7605ac04347bb20e97c45c8441ce71a) to adjust the included header files to compile with MSVC, so I don't think these issues apply to this PR.

thanks - not just for this but the others too.

Unfortunately - as you wrote too - not all commits apply to this PR. I mean some modifications makes library to build on Windows, but some other are code cleaning - which is really appreciated and very useful.

* `src/utils/string.cc`: I just removed unused header files. I'd rather not introduce changes unrelated to this PR.

Same as above: if I'm not wrong, this is a code cleaning.

I think it's important to split up the two things: feature request and clean up. Both are really important and very welcome, but when someone will inspect the code later it's hard to decide why the author sent his feature request PR with unnecessary modifications.

Because you also mentioned this above, I really hope you understand this.

After merging this PR with the main codebase, we need to decide how to proceed. It seems like Nginx supported on Windows, but I don't know how do the modules operate on it. If the Nginx module architecture is also works on Windows, then we should make the ModSecurity-nginx connector.

We're integrating libModSecurity on a custom web server on Windows. I've only looked at the Apache & Nginx ModSecurity connectors as a reference on how to use the library. At some point I saw that Nginx should compile on Windows as well, but I didn't try it.

Thank you, it seems very interesting (the custom web server).

Thanks for all.

eduar-hte commented 3 weeks ago

Because this is your first issue, the CI workflow does not start automatically when you update the PR, so you weren't able to see that there is another check-static issue:

2024-04-27T07:09:50.1179638Z Checking src/modsecurity.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;WITH_YAJL...
2024-04-27T07:09:51.5599001Z warning: src/modsecurity.cc,265,performance,redundantCopyLocalConst,The const variable 'startingAt' is assigned a copy of the data. You can avoid the unnecessary data copying by converting 'startingAt' to const reference.
2024-04-27T07:09:51.5602109Z warning: src/modsecurity.cc,267,performance,redundantCopyLocalConst,The const variable 'size' is assigned a copy of the data. You can avoid the unnecessary data copying by converting 'size' to const reference.
2024-04-27T07:09:51.5604892Z warning: src/modsecurity.cc,349,performance,redundantCopyLocalConst,The const variable 'startingAt' is assigned a copy of the data. You can avoid the unnecessary data copying by converting 'startingAt' to const reference.
2024-04-27T07:09:51.5607899Z warning: src/modsecurity.cc,351,performance,redundantCopyLocalConst,The const variable 'size' is assigned a copy of the data. You can avoid the unnecessary data copying by converting 'size' to const reference.
2024-04-27T07:09:51.5798124Z 118/218 files checked 45% done

Seems like these came with the new modifications.

I think these need a suppression too, because the copy is needed to avoid the use after free that I fixed in commit 7cb67b0 (and which seems to have been introduced in ad28de4). The problem is that the current code is creating a reference to the last element in the list (lines 265 & 267) but then the last element is immediately removed (lines 266 & 268), which means that you have a dangling reference. Then the references are accessed in lines 273-274 & 278-279 which is incorrect. In this case, if you heed the warning, you're actually introducing a bug! :-)

(Lines 349 & 351 didn't have the problem but I marked the variables as const because they were not modified after assignment, and for consistency with the code in lines 265 & 267)

Can I add these suppressions in the code itself? If that works with your setup, I think it'd improve maintainability of the project, as you wouldn't need to update test/cppcheck_suppressions.txt when line numbers change because of unrelated updates.

eduar-hte commented 3 weeks ago

Can I add these suppressions in the code itself? If that works with your setup, I think it'd improve maintainability of the project, as you wouldn't need to update test/cppcheck_suppressions.txt when line numbers change because of unrelated updates.

I tried this in commit cc24a4f. Let's see if it works and what you think.

PS: I checked if by adding these lines I needed to update the line numbers of other suppressions on the same file and I think the ones about line 206 may not be necessary (I'll revert commit 30e1cd8 if I'm mistaken). Additionally, I don't think an r-value reference is necessary in the previous line, as the compiler will perform a return value optimization (RVO) to avoid a copy.

eduar-hte commented 2 weeks ago

I tried this in commit cc24a4f. Let's see if it works and what you think.

I enabled actions in my fork so I could try this. I found out that the current configuration used to run cppcheck needs the --inline-suppr argument for the inline suppressions to work. I've added that configuration in commit e9540ea. Once again, I hope you're ok with this.

PS: I checked if by adding these lines I needed to update the line numbers of other suppressions on the same file and I think the ones about line 206 may not be necessary (I'll revert commit 30e1cd8 if I'm mistaken). Additionally, I don't think an r-value reference is necessary in the previous line, as the compiler will perform a return value optimization (RVO) to avoid a copy.

No warnings/error were reported on line 206 either.

eduar-hte commented 2 weeks ago

I also tried out a workflow to build the Windows version of the library adding the following configuration in a branch in my fork:

  build-windows:
    runs-on: ${{ matrix.os }}
    strategy:
      matrix:
        os: [windows-2022]
        platform: [x86, x86_64]
    steps:
      - uses: actions/checkout@v4
        with:
          submodules: true
      - name: Install Conan
        run: |
          pip3 install conan --upgrade
          conan profile detect
      - uses: ammaraskar/msvc-problem-matcher@master
      - name: vcbuild ${{ matrix.platform }}
        run: vcbuild.bat Release ${{ matrix.platform }}
        shell: cmd

It's simpler than the Dockerfile I added as a reference!

The 64-bit build takes less than 5 minutes, which is similar to the build time of the Linux/macOS versions. However, the 32-bit version took around 33 minutes to build. This is because the Conan dependencies need to be built in this configuration (while the 64-bit build can use the compiled binaries available in the package).

eduar-hte commented 2 weeks ago

I enabled actions in my fork so I could try this. I found out that the current configuration used to run cppcheck needs the --inline-suppr argument for the inline suppressions to work. I've added that configuration in commit e9540ea. Once again, I hope you're ok with this.

I ended up looking at inlining cppcheck suppressions a bit more and created a new PR (#3134) to remove those that reference line numbers so it hopefully streamlines the process of submitting changes (by avoiding the build errors and the need for additional commits to update test/cppcheck_suppressions.txt).

I've seen this recently also in PR #3104, where guidance is being introduced to check if this file needs updating after making changes to the library (see commit 2c8684e).

eduar-hte commented 2 weeks ago

After merging this PR with the main codebase, we need to decide how to proceed. It seems like Nginx supported on Windows, but I don't know how do the modules operate on it. If the Nginx module architecture is also works on Windows, then we should make the ModSecurity-nginx connector.

Though this was unrelated to the work we're doing with libModSecurity v3, I ended up taking a stab at building the ModSecurity-nginx module and have just created ModSecurity-nginx PR #321 for that! :-)

airween commented 2 weeks ago

Okay, so now please pick up the changes from current v3/master, and let's see what happens.

airween commented 1 week ago

I started to review this PR, there are one relevant request change. I try to continue it in next days, I would like to review shared_files.cc and system.cc modifications. The other looks good to me.

eduar-hte commented 1 week ago

I started to review this PR, there are one relevant request change. I try to continue it in next days, I would like to review shared_files.cc and system.cc modifications. The other looks good to me.

system.c changes are simple, in WIN32 blocks and reference third-party implementations of glob and clock_gettime (which I initially didn't implement, as the macOS version doesn't currently implement this, but found out the public domain implementation from mingw-w64's winpthreads and decided to include it).

shared_files.* is more involved. I mean, the implementation in the PR is actually pretty simple and straightforward, but you need to go through all the history referenced in the commit message to analyze how the removed code was both dead-code and also had several minor issues introduced over time. I hope I was able to explain that well.

airween commented 1 week ago

system.c changes are simple, in WIN32 blocks and reference third-party implementations of glob and clock_gettime (which I initially didn't implement, as the macOS version doesn't currently implement this, but found out the public domain implementation from mingw-w64's winpthreads and decided to include it).

yepp, thanks, everything looks good.

shared_files.* is more involved. I mean, the implementation in the PR is actually pretty simple and straightforward, but you need to go through all the history referenced in the commit message to analyze how the removed code was both dead-code and also had several minor issues introduced over time. I hope I was able to explain that well.

yes, that's very impressive fix - thanks.

Let's finish the modsecurity.cc affected conversation above, and I'll merge this.

eduar-hte commented 6 days ago

I added a new commit to set up a test suite using CTest for the Windows build.

This only required a minor update to unit.cc & regression.cc to have them set their exit code to the number of tests that failed, so that CTest can use this to determine whether the test failed or not. This doesn't affect the Unix execution of the tests because those determine whether a test failed by analyzing their console output (processed through the shell scripts custom-test-driver & test-suite.sh used to run them).

There's a new /test/test-suite.in with the list of tests currently in Makefile.am because I wanted to share the definition of the tests between the Unix and Windows builds. I don't know much about autoconf/automake so I'm just doing a simple include of the file from Makefile.am. That means that each line of the file has a TESTS+= prefix (necessary for Makefile.am), and not just the test filename, and I have to ignore that prefix when processing the file on the Windows build to setup the CTest test suite. I couldn't figure out how to generate the TESTS variable from a file with just a list of filenames with autoconf/automake.

I've updated the GitHub workflow that builds libModSecurity on Windows to also run the same tests that make check do on the Unix builds. There are a couple of steps that handle the issues I found when running the current tests on Windows:

build-windows:
    runs-on: ${{ matrix.os }}
    strategy:
      matrix:
        os: [windows-2022]
        platform: [x86_64]
        configuration: [Release]
    steps:
      - uses: actions/checkout@v4
        with:
          submodules: true
      - name: Install Conan
        run: |
          pip3 install conan --upgrade
          conan profile detect
      - uses: ammaraskar/msvc-problem-matcher@master
      - name: Build ${{ matrix.configuration }} ${{ matrix.platform }}
        shell: cmd
        run: vcbuild.bat ${{ matrix.configuration }} ${{ matrix.platform }}
      - name: Set up test environment
        working-directory: build\win32\build\${{ matrix.configuration }}
        env:
          BASE_DIR: ..\..\..\..
        shell: cmd
        run: |
          copy unit_tests.exe %BASE_DIR%\test
          copy regression_tests.exe %BASE_DIR%\test
          copy libModSecurity.dll %BASE_DIR%\test
          copy %BASE_DIR%\unicode.mapping %BASE_DIR%\test
          md \tmp
          md \bin
          copy "C:\Program Files\Git\usr\bin\echo.exe" \bin
          copy "C:\Program Files\Git\usr\bin\echo.exe" \bin\echo
      - name: Disable tests that don't work on Windows
        working-directory: test\test-cases\regression
        shell: cmd
        run: |
          jq "map(if .title == \"Test match variable (1/n)\" then .enabled = 0 else . end)" issue-2423-msg-in-chain.json > tmp.json && move /Y tmp.json issue-2423-msg-in-chain.json
          jq "map(if .title == \"Test match variable (2/n)\" then .enabled = 0 else . end)" issue-2423-msg-in-chain.json > tmp.json && move /Y tmp.json issue-2423-msg-in-chain.json
          jq "map(if .title == \"Test match variable (3/n)\" then .enabled = 0 else . end)" issue-2423-msg-in-chain.json > tmp.json && move /Y tmp.json issue-2423-msg-in-chain.json
          jq "map(if .title == \"Variable offset - FILES_NAMES\" then .enabled = 0 else . end)" offset-variable.json > tmp.json && move /Y tmp.json offset-variable.json
      - name: Run tests
        working-directory: build\win32\build
        run: |
          ctest -C ${{ matrix.configuration }} --output-on-failure
airween commented 5 days ago

I added a new commit to set up a test suite using CTest for the Windows build.

This only required a minor update to unit.cc & regression.cc to have them set their exit code to the number of tests that failed, so that CTest can use this to determine whether the test failed or not. This doesn't affect the Unix execution of the tests because those determine whether a test failed by analyzing their console output (processed through the shell scripts custom-test-driver & test-suite.sh used to run them).

There's a new /test/test-suite.in with the list of tests currently in Makefile.am because I wanted to share the definition of the tests between the Unix and Windows builds. I don't know much about autoconf/automake so I'm just doing a simple include of the file from Makefile.am. That means that each line of the file has a TESTS+= prefix (necessary for Makefile.am), and not just the test filename, and I have to ignore that prefix when processing the file on the Windows build to setup the CTest test suite. I couldn't figure out how to generate the TESTS variable from a file with just a list of filenames with autoconf/automake.

Thanks, let me check this in local - I'll try that soon.

I've updated the GitHub workflow that builds libModSecurity on Windows to also run the same tests that make check do on the Unix builds. There are a couple of steps that handle the issues I found when running the current tests on Windows:

Sorry, but I don't see any modification if .github directory at all - see the Files changed tab. Beside that I think if a new architecture appears in workflow, then we must see that in between tests results. But I also don't see even between your tests.

* One step creates a `tmp` directory at the root of the drive where the build and tests are done, which makes those tests that configure `SecAuditLog` work.

Right,

* This step also creates a `bin` directory at the root of the drive too, and then copies `echo.exe` from the `usr/bin` tools included with Git in the `windows-2022` runner image to that directory, which allows tests that run `/bin/echo` to work.

right,

  * `echo.exe` is also copied as `\bin\echo` for the test that checks if the file `/bin/echo` exist.

okay,

* The other step (`Disable tests that don't work on Windows`) sets the `enabled` property to zero in the JSON tests that fail on Windows because the tests relies (incorrectly) on the order on which a `std::unordered_map` is iterated.

right.

  * I had to update the `offset-variable.json` to remove a couple of comments in the JSON file, as this is not JSON compliant, and breaks the processing of the file to turn the test off.

This is okay.

Thanks - it would be very to see the Windows test in the Github workflow now.

eduar-hte commented 5 days ago

Thanks - it would be very to see the Windows test in the Github workflow now.

oh, I've never submitted committed the changes to the workflow in my PR because I thought it'd be restricted. I'll try that now and let you know.

airween commented 5 days ago

Thanks - it would be very to see the Windows test in the Github workflow now.

oh, I've never submitted committed the changes to the workflow in my PR because I thought it'd be restricted. I'll try that now and let you know.

No, there is no any restriction.

If you send the commit, GH will start the workflow at your branch separately. You can wait for the finish, if you want to check (but the workflow will start on main repository immediately, because the PR is active).

This is the goal (and a big advantage :smiley:).

sonarcloud[bot] commented 4 days ago

Quality Gate Failed Quality Gate failed

Failed conditions
E Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

eduar-hte commented 4 days ago

If you send the commit, GH will start the workflow at your branch separately. You can wait for the finish, if you want to check (but the workflow will start on main repository immediately, because the PR is active).

I added the Windows build yesterday.

I've made an update to also support building libModSecurity on Windows without some of the third party dependencies (LMDB, LUA, LibXML2, MaxMind & cURL), and have now configured builds without each of them.

NOTE: There is not a build in the GH workflow without LibXML2 (though the Windows build configuration now supports it) because the regression tests assume that XML support is always on. I guess that's the reason why this is not turned off either for Linux/macOS builds.

airween commented 3 days ago

I added the Windows build yesterday.

yes, I saw it - it ran successfully.

I've made an update to also support building libModSecurity on Windows without some of the third party dependencies (LMDB, LUA, LibXML2, MaxMind & cURL), and have now configured builds without each of them.

really nice, thank you.

NOTE: There is not a build in the GH workflow without LibXML2 (though the Windows build configuration now supports it) because the regression tests assume that XML support is always on. I guess that's the reason why this is not turned off either for Linux/macOS builds.

Yes, thank you for spotting that. That makes sense (because XML parser is a really important component of the library), but I don't understand why is there a test without YAJL. Never mind.

Thank you for this huge work, now everything is fine for me.

Please let me know if you finished the work or you still want to add something - I'm ready to merge this PR.

eduar-hte commented 3 days ago

Thank you for this huge work, now everything is fine for me.

Please let me know if you finished the work or you still want to add something - I'm ready to merge this PR.

That's great to hear. I'm not working on any further changes on the Windows port now, so I think you should just go ahead and integrate it (and then I can update the ModSecurity-nginx PR to sync with this -it currently references my repository in order to build libModSecurity on Windows).

airween commented 3 days ago

Merged, many thanks for this awesome work!

eduar-hte commented 3 days ago

NOTE: There is not a build in the GH workflow without LibXML2 (though the Windows build configuration now supports it) because the regression tests assume that XML support is always on. I guess that's the reason why this is not turned off either for Linux/macOS builds.

I'll create a quick PR to annotate the regression tests that depend on libxml and have them skipped if the build doesn't include that feature.

Then I'll add the Windows build configuration to build without libxml.

Yes, thank you for spotting that. That makes sense (because XML parser is a really important component of the library), but I don't understand why is there a test without YAJL. Never mind.

I'll try and take care of that too.