FIPS-compliant SHA-1 loading option for Lua scripts without performance penalty #914
+491
−35
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces a new
WithLoadSha1()option forNewLuaScriptandNewLuaScriptReadOnlyconstructors,enabling SHA-1 hash loading from Redis instead of client-side calculation. This provides FIPS compliance
while maintaining EVALSHA performance benefits.
Problem:
fips140=only)NewLuaScriptNoSha()always uses EVAL, sending full script every time. It comes with performance cost: slower than EVALSHA - ~36% slower in benchmarks here, +47.84% reported in Support Lua scripts without SHA-1 usage for FIPS compliancy #836.Proposed solution:
Allow SHA-1 to be obtained from Redis via
SCRIPT LOADresponse which returns calculated hash. This maintains EVALSHA performance without client-side usage of sha1 package. Delegates compliance problems to Redis server only (most likely approved already).Benchmark run over local Redis 7.4.0 which show that
WithLoadSha1()performs on par with defaultNewLuaScript()whileNewLuaScriptNoSha()is significantly slower:Implementation notes
I initially added LuaOption to NewLuaScriptNoSha, but after thinking more about it, it made more sense to add to the
main NewLuaScript constructor instead. NoSha constructors are left special for backwards compatibility, and they anyway
solve a different use case (always EVAL, avoid collisions concern, so may still make sense for some users I believe).
Adding LuaOption to NewLuaScriptNoSha is also looks a bit ugly because it's not a main use case for Lua scripts.
ExecMulti
TBH I do not quite understand why ExecMulti loads scripts every time to all nodes, most like there is a valid reason - very curious to understand. So changed it a bit blindly - but seems to be working.