Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset.

In case there are security findings, they will be communicated to you as a comment inside the PR.

Hope you’ll enjoy using Jit.

Questions? Comments? Want to learn more? Get in touch with us.

@chaitanyabodlapati thank you for opening this pr. I am no a fan of using init here. Would prefer the application to control this with an option. What do you think?

Thanks for the feedback — that makes sense.
We used init() because in our packaging we ship client as a prebuilt binary, so customers can't realistically change application code just to enable this. We need something that can be turned on through deployment configuration.
Would you be okay if we remove the init() logic, and instead support a simple environment variable switch (and/or an explicit option) so this can be enabled without code changes?
Default behavior would stay the same unless the env var/option is set.

@chaitanyabodlapati If you are shipping the client as a prebuilt binary I would suggest to do a wrapper or a patch on your end. Such environment variables make sense in tests and applications, rarely in libraries and only for such that are specifically made to run in a given setup. Let's discuss another way to facilitate this. Explicit option would be a breaking change. Let me see if I understand your usecase correctly:

• your users do not have access to the initialisation of the client (e.g. Options)
• do they have access to the methods called? If so, can you have your own (or maybe it can be provided in the library) "constructor" for Script that uses server side SHA?

Thanks, that’s a fair point — let me give a bit more context on our use case.

We ship Traefik (which uses go-redis v9) as part of a bigger product as a single prebuilt binary. Our customers do not write Go code or construct redis.Client / redis.Script themselves — that all happens inside Traefik and our packaging. What they can change is only the runtime environment (for example, whether they run with strict FIPS settings).

In strict FIPS environments we run with GODEBUG=fips140=only. In that mode, calling crypto/sha1 from user code is not allowed. Since NewScript always uses crypto/sha1 today, creating a script will fail in FIPS mode, and Traefik’s Redis rate limiting stops working. We can’t ask customers to patch Traefik or go-redis, and we also don’t want to ship separate FIPS and non-FIPS binaries — it’s one build, and the environment decides.

What we would like is a way to use server-side SHA for scripts when we are in FIPS mode, without changing the behavior for everyone else:

Non-FIPS: keep current behavior (NewScript does SHA-1 in Go and calls EVALSHA, as today).
FIPS: avoid SHA-1 in Go, and instead use SCRIPT LOAD to get the hash from Redis and then call EVALSHA with that hash (with a NOSCRIPT reload).

We understand your concerns about init() and env vars inside the library, and we agree that changing NewScript’s signature would be a breaking change. So instead of the env-toggle version, we’d be happy with a small API addition, for example:

keep NewScript(src string) exactly as it is today, and
add a new constructor like NewScriptFromServer(src string) that implements the “server-side SHA” behavior.

On our side, we can then do the runtime decision in Traefik:

non-FIPS: call NewScript(...)
FIPS: call NewScriptFromServer(...)

That way the library stays clean (no env vars, no init() logic), existing users see no change.

Would that approach work for you? If yes, I can update the PR to drop the env-based toggle and instead implement the separate “server-SHA” constructor.

Funny enough, we at @CampusTech are suddenly in need of a FIPS-140 compliant Redis library too (and must run GODEBUG=fips140=only as well), so will be keeping an eye on this PR and happy to jump in to help out if needed @chaitanyabodlapati. I'm shocked by your impeccable timing! Thanks!

@chaitanyabodlapati this additional method that will directly use Server for hashing is the correct approach for a library in my mind. Feel free to implement it. This will provide the tools to the application to decide how to create the Script. Thank you!

Thanks for the feedback .
I updated this PR per your suggestion:
• Removed the init()-based switching logic.
• Kept NewScript(src) unchanged (still computes SHA-1 client-side and uses EVALSHA as today).
• Added an opt-in constructor NewScriptServerSHA(src) which avoids client-side SHA-1 by:
- using SCRIPT LOAD to obtain the digest from Redis (server-side hashing),
- then executing via EVALSHA,
-and reloading/retrying once on NOSCRIPT.
This keeps the default behavior the same for everyone, while allowing applications running in strict FIPS mode to explicitly choose the server-side digest path.

@chaitanyabodlapati thanks for pushing the fix 🙌

i’m seeing the same issue on my side and noticed a few subtle concerns in the PR:

1. script load is only needed once for the lifetime of the script to compute the hash and store it for later use. it won’t negatively impact non-FIPS mode. in fact, it could be the main way we load scripts, so no need to branch into a server-side implementation.
2. if script load errors, we shouldn’t swallow it and then run eval under the hood. that’s risky, can hide subtle code issues, and may force the unoptimized path every time.
3. in cluster mode, script load runs on a random node, so it can’t be used to fix NOSCRIPT. instead, just call eval so it gets cached on the correct shard node based on the key, and subsequent calls will have it.

TLDR: script load only needs to run once and we should store the hash (sha1) to avoid computing it client-side.

@panga 1) and 2) sound like application logic, not client logic. In my opinion, since this is achievable with what the client provides it can be implemented in the application. 3) is something that can be addressed by getting a specific node client with MasterForKey as well.

My point on 3) is that EVAL encapsulates the full workflow: 1. locate the node for the key, 2. load the script (and cache), and 3. execute it.

This avoids introducing custom coordination logic and eliminates an additional Redis round-trip.

left the required changes as suggestions

i was planning to address this, since my review was what delayed the merge of this PR, but the maintainer_can_modify is false, so @chaitanyabodlapati, feel free to address this and ping me when it is done.

@ndyakov This PR is coming from a fork, so I don't have the ability to enable "maintainer can modify" on my side.
I've pushed the requested changes. When you get a chance, could you please take another look and let me know if there's anything else you'd like me to update?

@ndyakov I noticed my comment about cluster mode doesn’t seem to have been addressed in the merged code. Is this something you plan to look at before releasing?

@panga feel free to submit an issue. It is a known problem for the team but with lower priority compared to other work we are doing at this time. If someone from the community would like to take a shot at it that would be great.