GitHub build & quality assurance workflow updates

[eduar-hte]

This PR introduces the following changes to the GitHub build & QA workflows:

• Fixed 32-bit Linux build
  • ssdeep & GeoIP are not available as 32-bit packages in the Ubuntu 22.04, so they're not installed. This also means that the build configurations without them are redundant.
• Fixed clang builds on Linux (both on 64-bit & 32-bit)
• Installed YAJL package on macOS builds as it's a required dependency.
  • Additionally, this fixed the execution of tests on those builds (previously, execution would show that 0 tests were run as YAJL is necessary to parse test files).
• Include build configurations 'without libxml' to Linux/macOS/Windows builds
  • This required annotating regression testcases that depend on libxml support.
  • Updating the regression test executable to detect whether it was built with/without libxml support in order to run/skip those testcases.
• Include build configuration 'with lmdb' to Linux/macOS builds.
  • There is currently a 'without lmdb' configuration in both platforms, but LMDB is disabled by default and only included in the build if explicitly turn on.
• Removed 'without YAJL' builds from Linux/macOS build configurations as it's a required 3rd party dependency.
• Added without ssdeep build to Linux build configurations.
• Moved cppcheck (check-static) out of every Linux build configuration and run this checks only once to improve workflow times.
• Updated actions/checkout version on Linux/macOS build to remove deprecation warnings on workflow execution.
• Simplified and standardize job names.

[airween]

Hi @eduar-hte,

thanks again this awesome PR!

This PR introduces the following changes to the GitHub build & QA workflows:

* Fixed 32-bit Linux build
  * ssdeep & GeoIP are not available as 32-bit packages in the Ubuntu 22.04, so they're not installed. This also means that the build configurations without them are redundant.

These are great, these were on my list as I realized that the 32 bit build is the same as the 64 bit. Thanks.

* Fixed clang builds on Linux (both on 64-bit & 32-bit)
* Installed YAJL package on macOS builds as it's a required dependency.
  * Additionally, this fixed the execution of tests on those builds (previously, execution would show that 0 tests were run as YAJL is necessary to parse test files).

oh, thanks!

* Include build configurations 'without libxml' to Linux/macOS/Windows builds
  * This required annotating regression testcases that depend on libxml support.
  * Updating the regression test executable to detect whether it was built with/without libxml support in order to run/skip those testcases.

Hmmm... I'm a bit confused here. Why did you add this? As we discussed (or I suggested) here XML and JSON parsers should be mandatory too. So I think we don't need --without-xml option - what do you think?

* Removed 'without YAJL' builds from Linux/macOS build configurations as it's a required 3rd party dependency.

Yes, this is a good step.

* Added without ssdeep build to Linux build configurations.

Thanks.

* Moved cppcheck (check-static) out of every Linux build configuration and run this checks only once to improve workflow times.

Ah, this is very good, thanks.

* Updated actions/checkout version on Linux/macOS build to remove deprecation warnings on workflow execution.
* Simplified and standardize job names.

Thanks for all.

Here you changed the enabled value from 1 to 0 - what is the reason that you disabled these tests?

Thanks again.

[airween]

There is a new SonarCloud issue, but I think actually we can mark it as FP, so you don't need to care of that.

[eduar-hte]

Here you changed the enabled value from 1 to 0 - what is the reason that you disabled these tests?

Oh, that is a mistake. Those are the tests that are disabled on Windows (because of the incorrect dependency on a specific order from iterating on a std::unordered_map container), which I should not have included in that commit.

I've just reverted those changes. Nice catch!

• Include build configurations 'without libxml' to Linux/macOS/Windows builds
  • This required annotating regression testcases that depend on libxml support.
  • Updating the regression test executable to detect whether it was built with/without libxml support in order to run/skip those testcases.

Hmmm... I'm a bit confused here. Why did you add this? As we discussed (or I suggested) here XML and JSON parsers should be mandatory too. So I think we don't need --without-xml option - what do you think?

Yes, you mentioned in passing that it's an important component of the library. At the same time, in the limited scope of this PR (just to update the GH workflows) I thought that given that it's not a mandatory dependency and the codebase currently has support to disable use of that library (through the compiler definition WITH_LIBXML2), the QA runs should try and cover that configuration.

Of course, a separate PR could be done to remove those compiler guards and this new build configuration, and I'd be ok with it (it didn't take a lot of effort to add support for regression for that resource and annotate the testcases).

[eduar-hte]

BTW, because SonarCloud configuration is not done through a GitHub workflow I couldn't see if it was possible to configure it to analyze the code as standard C++ 17.

It currently reports issues and suggests using newer features from C++20 (such as std::format or ranges, see here and here) which add a bit of noise.

[eduar-hte]

Yes, you mentioned in passing that it's an important component of the library. At the same time, in the limited scope of this PR (just to update the GH workflows) I thought that given that it's not a mandatory dependency and the codebase currently has support to disable use of that library (through the compiler definition WITH_LIBXML2), the QA runs should try and cover that configuration.

With regards to dependencies, I think the section in README.md could be updated to be more accurate. The line that mentions YAJL, libpcre & libXML2 starts by stating that these are mandatory dependencies but clarifies that two out of the three are not yet so. I think either pcre or pcre2 are mandatory to build the library, while as we discussed libXML2 seems optional (at least for the time). I think that sentence would be more accurate and read better like this:

• Other dependencies include YAJL, as ModSecurity uses JSON for producing logs and its testing framework, libpcre for processing regular expressions in SecRules, and libXML2 (not yet mandatory) which is used for parsing XML requests.

One other minor issue is that the last sentence implies that libinjection is not mandatory, and that if the library is missing you just won't get the associated operator, but something I found out a few times when I forgot to init & update submodules on my Unix builds is that configure won't complete without it, which actually makes it mandatory. :-)

checking for pkg-config... /usr/bin/pkg-config
checking pkg-config is at least version 0.9.0... yes
configure: error: 

  libInjection was not found within ModSecurity source directory.

  libInjection code is available as part of ModSecurity source code in a format
  of a git-submodule. git-submodule allow us to specify the correct version of
  libInjection and still uses the libInjection repository to download it.

  You can download libInjection using git:

     $ git submodule init
     $ git submodule update

(edit) I ended up creating another quick PR (#3145) to move this discussion there and update README, but I think it'd be better if I create a PR to actually update configure and the codebase to enforce the mandatory dependencies (such as YAJL, libXML2 and probably PCRE2 -as the original pcre has been end of life since 2021-).

[eduar-hte]

I updated the Unix build configurations with the following change:

• Include build configuration 'with lmdb' to Linux/macOS builds.
  • There is currently a 'without lmdb' configuration in both platforms, but LMDB is disabled by default and only included in the build if explicitly turn on.

If you look at the configure step of builds (such as this), you'll see in all the configurations (not just for the one in the strategy created to build 'without lmdb'):

   + YAJL                                          ....found v2.1.0
      -lyajl, -DWITH_YAJL -I/usr/include/yajl
   + LMDB                                          ....disabled
   + LibXML2                                       ....found v2.9.13
      -lxml2, -I/usr/include/libxml2 -DWITH_LIBXML2

[airween]

Oh, that is a mistake. Those are the tests that are disabled on Windows (because of the incorrect dependency on a specific order from iterating on a std::unordered_map container), which I should not have included in that commit.

I've just reverted those changes. Nice catch!

no worries, thanks!

Yes, you mentioned in passing that it's an important component of the library. At the same time, in the limited scope of this PR (just to update the GH workflows) I thought that given that it's not a mandatory dependency and the codebase currently has support to disable use of that library (through the compiler definition WITH_LIBXML2), the QA runs should try and cover that configuration.

Of course, a separate PR could be done to remove those compiler guards and this new build configuration, and I'd be ok with it (it didn't take a lot of effort to add support for regression for that resource and annotate the testcases).

Okay, we can cover all possible build options, therefore if we enable the flag --without-xml then we also need to check the codebase with --without-yajl - what do you think?

[airween]

BTW, because SonarCloud configuration is not done through a GitHub workflow I couldn't see if it was possible to configure it to analyze the code as standard C++ 17.

I try to take a look how is it possible.

It currently reports issues and suggests using newer features from C++20 (such as std::format or ranges, see here and here) which add a bit of noise.

Just FYI: there is an intention to bump the C++ version to 20 - see this comment.

[eduar-hte]

Of course, a separate PR could be done to remove those compiler guards and this new build configuration, and I'd be ok with it (it didn't take a lot of effort to add support for regression for that resource and annotate the testcases).

Okay, we can cover all possible build options, therefore if we enable the flag --without-xml then we also need to check the codebase with --without-yajl - what do you think?

I thought about that but didn't do it because I wanted to follow what the project documentation currently states.

Moreover, the last couple of days I worked on what I suggested in the previous comment, about actually enforcing the mandatory dependencies (yajl, pcre & libxml2) to simplify the build (reduces the number of combinations to test across OS & architectures) and codebase (reduces unnecessary #ifdef blocks).

I'd like to like to follow up this PR with those changes, which I didn't want to include in this one in order to keep this one simple (and aligned with the current documentation), and then analyze and discuss those changes separately. What do you think?

[airween]

With regards to dependencies, I think the section in README.md could be updated to be more accurate.

Agree. Thanks for spotting that.

The line that mentions YAJL, libpcre & libXML2 starts by stating that these are mandatory dependencies but clarifies that two out of the three are not yet so. I think either pcre or pcre2 are mandatory to build the library, while as we discussed libXML2 seems optional (at least for the time). I think that sentence would be more accurate and read better like this:

* Other dependencies include YAJL, as ModSecurity uses JSON for producing logs and its testing framework, libpcre  for processing regular expressions in SecRules, and libXML2 (not yet mandatory) which is used for parsing XML requests.

or we should make them really mandatory.

One other minor issue is that the last sentence implies that libinjection is not mandatory, and that if the library is missing you just won't get the associated operator, but something I found out a few times when I forgot to init & update submodules on my Unix builds is that configure won't complete without it, which actually makes it mandatory. :-)

checking for pkg-config... /usr/bin/pkg-config
checking pkg-config is at least version 0.9.0... yes
configure: error: 

  libInjection was not found within ModSecurity source directory.

  libInjection code is available as part of ModSecurity source code in a format
  of a git-submodule. git-submodule allow us to specify the correct version of
  libInjection and still uses the libInjection repository to download it.

  You can download libInjection using git:

     $ git submodule init
     $ git submodule update

Yeah, unfortunately libinjection is an orphaned project (I mean there are not so much updates and maintaining), so operating systems does not like these libraries. So I think actually the best way is to use that as submodule. But yes, I know it s*cks. :)

(edit) I ended up creating another quick PR (#3145) to move this discussion there and update README, but I think it'd be better if I create a PR to actually update configure and the codebase to enforce the mandatory dependencies (such as YAJL, libXML2 and probably PCRE2 -as the original pcre has been end of life since 2021-).

Right, thank you.

[airween]

I updated the Unix build configurations with the following change:

* Include build configuration 'with lmdb' to Linux/macOS builds.

  * There is currently a 'without lmdb' configuration in both platforms, but LMDB is disabled by default and only included in the build if explicitly turn on.

If you look at the configure step of builds (such as this), you'll see in all the configurations (not just for the one in the strategy created to build 'without lmdb'):

   + YAJL                                          ....found v2.1.0
      -lyajl, -DWITH_YAJL -I/usr/include/yajl
   + LMDB                                          ....disabled
   + LibXML2                                       ....found v2.9.13
      -lxml2, -I/usr/include/libxml2 -DWITH_LIBXML2

Nice catch! Seems like this has changed meanwhile, because LMDB was included if the configure script detected it. But yes, this modification is necessary, thanks.

[eduar-hte]

The line that mentions YAJL, libpcre & libXML2 starts by stating that these are mandatory dependencies but clarifies that two out of the three are not yet so. I think either pcre or pcre2 are mandatory to build the library, while as we discussed libXML2 seems optional (at least for the time). I think that sentence would be more accurate and read better like this:

or we should make them really mandatory.

I agree, that's why I started working on that separate PR I just mentioned to do that (and then update README.md 🙂).

Yeah, unfortunately libinjection is an orphaned project (I mean there are not so much updates and maintaining), so operating systems does not like these libraries. So I think actually the best way is to use that as submodule. But yes, I know it s*cks. :)

Curiously, while working on enforcing the mandatory dependencies I realized that configure actually states that both submodules are mandatory dependencies!

ModSecurity -  for Linux

 Mandatory dependencies
   + libInjection                                  ....
   + SecLang tests                                 ....a3d4405

 Optional dependencies
   + GeoIP/MaxMind                                 ....found 
      * (MaxMind) v1.5.2

[airween]

Okay, we can cover all possible build options, therefore if we enable the flag --without-xml then we also need to check the codebase with --without-yajl - what do you think?

I thought about that but didn't do it because I wanted to follow what the project documentation currently states.

oh, that makes sense - thanks for clarify it.

Moreover, the last couple of days I worked on what I suggested in the previous comment, about actually enforcing the mandatory dependencies (yajl, pcre & libxml2) to simplify the build (reduces the number of combinations to test across OS & architectures) and codebase (reduces unnecessary #ifdef blocks).

I'd like to like to follow up this PR with those changes, which I didn't want to include in this one in order to keep this one simple (and aligned with the current documentation), and then analyze and discuss those changes separately. What do you think?

Agree - I'll take a look again this PR and probably will merge it soon. Then you can send the next one.

Thank you for your patience and your work.

[airween]

or we should make them really mandatory.

I agree, that's why I started working on that separate PR I just mentioned to do that (and then update README.md 🙂).

Right,

Curiously, while working on enforcing the mandatory dependencies I realized that configure actually states that both submodules are mandatory dependencies!

ModSecurity -  for Linux

 Mandatory dependencies
   + libInjection                                  ....
   + SecLang tests                                 ....a3d4405

 Optional dependencies
   + GeoIP/MaxMind                                 ....found 
      * (MaxMind) v1.5.2

Hmm... I don't see the reason why "SecLang tests" is mandatory - probably we should figure out this. Libinjection is... well, I think it's like pcre(2), so yes, I can agree that.

[sonarcloud[bot]]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

[airween]

Thank you again this great PR, merging it.

[eduar-hte]

You're welcome. I'll follow up later about adjusting the mandatory dependencies after rebasing.