fix(transformations): compute empty md5 and sha1 digests lazily

[jspdown]

The md5 and sha1 transformations compute the digest of an empty input in an init function. As a result, a binary built with GODEBUG=fips140=only panics at startup as soon as the package is imported, because MD5 and SHA-1 are not allowed in that mode, even when the transformation is never used. This change computes the empty digest lazily with a sync.Once instead of in init, so importing the package no longer panics when these transformations are not exercised. Normal usage is unchanged.

Summary by CodeRabbit

• Refactor
  • Optimized hash computation initialization for improved application startup performance. Hash values are now computed on-demand rather than during initialization, reducing startup overhead.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f82b7f9e-d2ae-4992-99e6-4bfcf4c1e181

📥 Commits

Reviewing files that changed from the base of the PR and between 0ab7cb5 and fa1b616.

📒 Files selected for processing (2)

• internal/transformations/md5.go
• internal/transformations/sha1.go

---

📝 Walkthrough

Walkthrough

Two transformation files (md5.go, sha1.go) replace eager package-level init() functions that precomputed empty-string digests at startup with sync.Once-guarded lazy initialization. The digests are now computed and cached on the first call to the respective transformation function when given empty input.

Changes

Lazy sync.Once digest initialization

Layer / File(s)	Summary
Replace eager init() with sync.Once lazy computation internal/transformations/md5.go, internal/transformations/sha1.go	Adds sync import, package-level emptyMD5/emptyMD5Once and emptySHA1/emptySHA1Once variables, and updates the empty-input branch in each transformer to compute and cache the digest via Once.Do. Removes the former init() functions from both files.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and accurately describes the main change: converting eager computation of MD5 and SHA1 digests in init() functions to lazy computation using sync.Once.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

❌ Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.57%. Comparing base (0ab7cb5) to head (fa1b616).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/transformations/md5.go	0.00%	4 Missing ⚠️
internal/transformations/sha1.go	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1629      +/-   ##
==========================================
- Coverage   87.66%   87.57%   -0.09%     
==========================================
  Files         178      178              
  Lines        9134     9136       +2     
==========================================
- Hits         8007     8001       -6     
- Misses        858      866       +8     
  Partials      269      269              

Flag	Coverage Δ
coraza.no_memoize	87.66% <0.00%> (-0.09%)	⬇️
coraza.rule.case_sensitive_args_keys	87.54% <0.00%> (-0.09%)	⬇️
coraza.rule.mandatory_rule_id_check	87.56% <0.00%> (-0.09%)	⬇️
coraza.rule.multiphase_evaluation	87.33% <0.00%> (-0.09%)	⬇️
coraza.rule.no_regex_multiline	87.53% <0.00%> (-0.09%)	⬇️
coraza.rule.rx_prefilter	87.57% <0.00%> (-0.09%)	⬇️
default	87.57% <0.00%> (-0.09%)	⬇️
examples+	16.21% <0.00%> (-0.08%)	⬇️
examples+coraza.no_memoize	85.53% <0.00%> (-0.09%)	⬇️
examples+coraza.rule.case_sensitive_args_keys	85.50% <0.00%> (-0.09%)	⬇️
examples+coraza.rule.mandatory_rule_id_check	85.61% <0.00%> (-0.09%)	⬇️
examples+coraza.rule.multiphase_evaluation	87.33% <0.00%> (-0.09%)	⬇️
examples+coraza.rule.no_regex_multiline	85.44% <0.00%> (-0.09%)	⬇️
examples+coraza.rule.rx_prefilter	85.79% <0.00%> (-0.09%)	⬇️
examples+no_fs_access	84.85% <0.00%> (-0.09%)	⬇️
ftw	87.57% <0.00%> (-0.09%)	⬇️
no_fs_access	86.91% <0.00%> (-0.09%)	⬇️
tinygo	87.57% <0.00%> (-0.09%)	⬇️

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

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[M4tteoP]

Thanks for raising this. Overall looks good to me, but I get that this fixes it just at startup time. At runtime, if md5 or sha1 are used, it would still lead to a panic. Should we explore some other options, returning a more graceful error if that happens?

[jcchavezs]

@M4tteoP I think that is a correct assumption, I think a. the best is to hardcode the value and b. we should support GODEBUG=fips140=only in our test suite.

[jspdown]

Thanks for the feedback.

This PR could indeed go further and allow using Coraza with the GODEBUG=fips140=on.
Usually people won't use GODEBUG=fips140=only outside of tests. Quoting https://go.dev/doc/security/fips140:

It is not intended to be used in production, it is not required by the Security Policy, it introduces crashes and potentially unhandled errors by design, and it may have false positives or false negatives.

The GODEBUG=fips140=on on the other side will use the FIPS 140-3 validated Go Cryptographic Module but won't panic if a non-compliant function is used.

As you mentioned, a solution could be to simply return an error if fips.Enabled is true when the md5 or sha1 transformation are invoked.
But that won't be enough. There an other use of md5 in the code that will prevent compliant use: The validateSchema uses MD5 for cache key. This one can easily be fixed by replacing MD5 by FNV. A cache key is not a security function, so it's fine to use a non-cryptographic hash function for that purpose and it doesn't violate compliance. I will let me confirm that this is something we want to do.

To verify that we satisfy FIPS 140-3 constrains we could add a new test target:

GODEBUG=fips140=only go test ./...
GODEBUG=fips140=only go test ./testing/coreruleset  

This will report any violation with a panic or a failing test.