From b506c612a45dfc135bb321c3680b5a0316cb8533 Mon Sep 17 00:00:00 2001 From: chaitanyabodlapati <95430668+chaitanyabodlapati@users.noreply.github.com> Date: Sat, 28 Mar 2026 05:45:51 -0500 Subject: [PATCH] feat(script): Make Script helper work in strict FIPS mode by avoiding client-side SHA-1 (#3700) * script: avoid client-side SHA1 in FIPS mode * modifying the script.go to add new constructor to explicitly control SHA-1 Behaviour(strict FIPS Builds - FIPS140) * fixing the lint errors * Add unit and integration tests for NewScriptServerSHA * fixing the mod format issue * Update go.mod Co-authored-by: Nedyalko Dyakov <1547186+ndyakov@users.noreply.github.com> * Update go.sum Co-authored-by: Nedyalko Dyakov <1547186+ndyakov@users.noreply.github.com> * Update go.sum Co-authored-by: Nedyalko Dyakov <1547186+ndyakov@users.noreply.github.com> * Update script_server_sha_integration_test.go Co-authored-by: Nedyalko Dyakov <1547186+ndyakov@users.noreply.github.com> * Update go.mod Moved the dependency under require block --------- Co-authored-by: Nedyalko Dyakov <1547186+ndyakov@users.noreply.github.com> --- go.mod | 3 +- script.go | 138 +++++++++++++++++- script_server_sha_integration_test.go | 74 ++++++++++ script_server_sha_unit_test.go | 193 ++++++++++++++++++++++++++ 4 files changed, 400 insertions(+), 8 deletions(-) create mode 100644 script_server_sha_integration_test.go create mode 100644 script_server_sha_unit_test.go diff --git a/go.mod b/go.mod index b66c68e4..1240463c 100644 --- a/go.mod +++ b/go.mod @@ -8,9 +8,10 @@ require ( github.com/cespare/xxhash/v2 v2.3.0 github.com/dgryski/go-rendezvous v0.0.0-20200823014737-9f7001d12a5f github.com/zeebo/xxh3 v1.1.0 + go.uber.org/atomic v1.11.0 ) -require go.uber.org/atomic v1.11.0 + require ( github.com/klauspost/cpuid/v2 v2.2.10 // indirect diff --git a/script.go b/script.go index 48e63237..92d508f9 100644 --- a/script.go +++ b/script.go @@ -6,6 +6,7 @@ import ( "encoding/hex" "errors" "io" + "sync" ) type Scripter interface { @@ -24,28 +25,69 @@ var ( ) type Script struct { - src, hash string + src string + mu sync.RWMutex + hash string + serverSHA bool // if true: do not compute SHA-1 in Go; load digest from Redis (SCRIPT LOAD) } func NewScript(src string) *Script { h := sha1.New() _, _ = io.WriteString(h, src) + return &Script{ - src: src, - hash: hex.EncodeToString(h.Sum(nil)), + src: src, + hash: hex.EncodeToString(h.Sum(nil)), + serverSHA: false, + } +} + +// NewScriptServerSHA creates a Script that avoids computing SHA-1 in Go. +// The digest is obtained from Redis via SCRIPT LOAD (server-side hashing), +// then EVALSHA/EVALSHA_RO is used. +func NewScriptServerSHA(src string) *Script { + return &Script{ + src: src, + serverSHA: true, } } func (s *Script) Hash() string { + s.mu.RLock() + defer s.mu.RUnlock() return s.hash } func (s *Script) Load(ctx context.Context, c Scripter) *StringCmd { - return c.ScriptLoad(ctx, s.src) + cmd := c.ScriptLoad(ctx, s.src) + if err := cmd.Err(); err == nil { + s.mu.Lock() + s.hash = cmd.Val() + s.mu.Unlock() + } + return cmd } func (s *Script) Exists(ctx context.Context, c Scripter) *BoolSliceCmd { - return c.ScriptExists(ctx, s.hash) + s.mu.RLock() + hash := s.hash + serverSHA := s.serverSHA + s.mu.RUnlock() + if hash == "" && serverSHA { + // For server-side scripts, obtain digest from Redis first. + // If hash is empty, it means SCRIPT LOAD was not called yet, so we check existence of empty hash which will return false. + // This avoids unnecessary SCRIPT LOAD just to check existence. + if err := s.ensureHash(ctx, c); err != nil { + return c.ScriptExists(ctx, "") + } + s.mu.RLock() + hash = s.hash + s.mu.RUnlock() + } + if hash == "" { + return c.ScriptExists(ctx, "") + } + return c.ScriptExists(ctx, hash) } func (s *Script) Eval(ctx context.Context, c Scripter, keys []string, args ...interface{}) *Cmd { @@ -56,12 +98,94 @@ func (s *Script) EvalRO(ctx context.Context, c Scripter, keys []string, args ... return c.EvalRO(ctx, s.src, keys, args...) } +// ensureHash ensures that s.hash is populated by using SCRIPT LOAD. +// It never calls SHA-1 in Go; Redis computes and returns the digest. +func (s *Script) ensureHash(ctx context.Context, c Scripter) error { + // Fast path: read lock, return if hash is already set. + s.mu.RLock() + if s.hash != "" { + s.mu.RUnlock() + return nil + } + s.mu.RUnlock() + + // Slow path: acquire write lock and load. + s.mu.Lock() + if s.hash != "" { + s.mu.Unlock() + return nil + } + cmd := c.ScriptLoad(ctx, s.src) + if err := cmd.Err(); err != nil { + s.mu.Unlock() + return err + } + s.hash = cmd.Val() + s.mu.Unlock() + return nil +} + func (s *Script) EvalSha(ctx context.Context, c Scripter, keys []string, args ...interface{}) *Cmd { - return c.EvalSha(ctx, s.hash, keys, args...) + // Default behavior: use client-side SHA-1 computed in NewScript. + if !s.serverSHA { + s.mu.RLock() + hash := s.hash + s.mu.RUnlock() + return c.EvalSha(ctx, hash, keys, args...) + } + + // Server-side SHA via SCRIPT LOAD + EVALSHA. + if err := s.ensureHash(ctx, c); err != nil { + return s.Eval(ctx, c, keys, args...) + } + + s.mu.RLock() + hash := s.hash + s.mu.RUnlock() + + r := c.EvalSha(ctx, hash, keys, args...) + if HasErrorPrefix(r.Err(), "NOSCRIPT") { + // Script cache was flushed; reload and retry once. + if err := s.ensureHash(ctx, c); err != nil { + return s.Eval(ctx, c, keys, args...) + } + s.mu.RLock() + hash = s.hash + s.mu.RUnlock() + return c.EvalSha(ctx, hash, keys, args...) + } + + return r } func (s *Script) EvalShaRO(ctx context.Context, c Scripter, keys []string, args ...interface{}) *Cmd { - return c.EvalShaRO(ctx, s.hash, keys, args...) + if !s.serverSHA { + s.mu.RLock() + hash := s.hash + s.mu.RUnlock() + return c.EvalShaRO(ctx, hash, keys, args...) + } + + if err := s.ensureHash(ctx, c); err != nil { + return s.EvalRO(ctx, c, keys, args...) + } + + s.mu.RLock() + hash := s.hash + s.mu.RUnlock() + + r := c.EvalShaRO(ctx, hash, keys, args...) + if HasErrorPrefix(r.Err(), "NOSCRIPT") { + if err := s.ensureHash(ctx, c); err != nil { + return s.EvalRO(ctx, c, keys, args...) + } + s.mu.RLock() + hash = s.hash + s.mu.RUnlock() + return c.EvalShaRO(ctx, hash, keys, args...) + } + + return r } // Run optimistically uses EVALSHA to run the script. If script does not exist diff --git a/script_server_sha_integration_test.go b/script_server_sha_integration_test.go new file mode 100644 index 00000000..71b20460 --- /dev/null +++ b/script_server_sha_integration_test.go @@ -0,0 +1,74 @@ +package redis_test + +import ( + . "github.com/bsm/ginkgo/v2" + . "github.com/bsm/gomega" + "github.com/redis/go-redis/v9" +) + +var _ = Describe("NewScriptServerSHA", func() { + var client *redis.Client + + BeforeEach(func() { + client = redis.NewClient(redisOptions()) + Expect(client.FlushDB(ctx).Err()).NotTo(HaveOccurred()) + }) + + AfterEach(func() { + Expect(client.Close()).NotTo(HaveOccurred()) + }) + + It("should reload script after SCRIPT FLUSH", func() { + s := redis.NewScriptServerSHA("return 1") + + // First run should succeed (loads script via SCRIPT LOAD) + err := s.Run(ctx, client, nil).Err() + Expect(err).NotTo(HaveOccurred()) + + // Flush all scripts from Redis + err = client.Do(ctx, "SCRIPT", "FLUSH").Err() + Expect(err).NotTo(HaveOccurred()) + + // Second run should still succeed (reloads script after NOSCRIPT) + err = s.Run(ctx, client, nil).Err() + Expect(err).NotTo(HaveOccurred()) + }) + + It("should run script with RunRO", func() { + s := redis.NewScriptServerSHA("return 1") + + err := s.RunRO(ctx, client, nil).Err() + Expect(err).NotTo(HaveOccurred()) + }) + + It("should run script with EvalSha", func() { + s := redis.NewScriptServerSHA("return KEYS[1]") + + result, err := s.EvalSha(ctx, client, []string{"mykey"}).Result() + Expect(err).NotTo(HaveOccurred()) + Expect(result).To(Equal("mykey")) + }) + + It("should run script with EvalShaRO", func() { + s := redis.NewScriptServerSHA("return ARGV[1]") + + result, err := s.EvalShaRO(ctx, client, nil, "myarg").Result() + Expect(err).NotTo(HaveOccurred()) + Expect(result).To(Equal("myarg")) + }) + + It("should cache hash after first load", func() { + s := redis.NewScriptServerSHA("return 42") + + // Hash should be empty before first run + Expect(s.Hash()).To(Equal("")) + + // Run the script + result, err := s.Run(ctx, client, nil).Result() + Expect(err).NotTo(HaveOccurred()) + Expect(result).To(Equal(int64(42))) + + // Hash should be populated after run + Expect(s.Hash()).NotTo(Equal("")) + }) +}) diff --git a/script_server_sha_unit_test.go b/script_server_sha_unit_test.go new file mode 100644 index 00000000..2da0a961 --- /dev/null +++ b/script_server_sha_unit_test.go @@ -0,0 +1,193 @@ +package redis + +import ( + "context" + "strings" + "sync" + "testing" + + "github.com/redis/go-redis/v9/internal/proto" +) + +// fakeScripter simulates enough of Scripter for unit testing Script behavior. +// It records which redis commands were invoked and can simulate NOSCRIPT. +type fakeScripter struct { + mu sync.Mutex + + // call counters + scriptLoadCalls int + evalCalls int + evalShaCalls int + evalROCalls int + evalShaROCalls int + + // behavior controls + hashToReturn string + + // If set, the first EvalSha/EvalShaRO returns a NOSCRIPT error. + failFirstEvalShaWithNoScr bool + failFirstEvalShaROWithNoScr bool +} + +func (f *fakeScripter) ScriptLoad(ctx context.Context, script string) *StringCmd { + f.mu.Lock() + f.scriptLoadCalls++ + f.mu.Unlock() + + cmd := NewStringCmd(ctx) + cmd.SetVal(f.hashToReturn) + return cmd +} + +func (f *fakeScripter) ScriptExists(ctx context.Context, hashes ...string) *BoolSliceCmd { + cmd := NewBoolSliceCmd(ctx) + vals := make([]bool, len(hashes)) + for i := range vals { + vals[i] = true + } + cmd.SetVal(vals) + return cmd +} + +func (f *fakeScripter) Eval(ctx context.Context, script string, keys []string, args ...interface{}) *Cmd { + f.mu.Lock() + f.evalCalls++ + f.mu.Unlock() + + cmd := NewCmd(ctx) + cmd.SetErr(nil) + return cmd +} + +func (f *fakeScripter) EvalSha(ctx context.Context, sha1 string, keys []string, args ...interface{}) *Cmd { + f.mu.Lock() + f.evalShaCalls++ + callNum := f.evalShaCalls + fail := f.failFirstEvalShaWithNoScr && callNum == 1 + f.mu.Unlock() + + cmd := NewCmd(ctx) + if fail { + // Use RedisError so go-redis NOSCRIPT detection triggers. + cmd.SetErr(proto.RedisError("NOSCRIPT No matching script. Please use EVAL.")) + return cmd + } + cmd.SetErr(nil) + return cmd +} + +func (f *fakeScripter) EvalRO(ctx context.Context, script string, keys []string, args ...interface{}) *Cmd { + f.mu.Lock() + f.evalROCalls++ + f.mu.Unlock() + + cmd := NewCmd(ctx) + cmd.SetErr(nil) + return cmd +} + +func (f *fakeScripter) EvalShaRO(ctx context.Context, sha1 string, keys []string, args ...interface{}) *Cmd { + f.mu.Lock() + f.evalShaROCalls++ + callNum := f.evalShaROCalls + fail := f.failFirstEvalShaROWithNoScr && callNum == 1 + f.mu.Unlock() + + cmd := NewCmd(ctx) + if fail { + cmd.SetErr(proto.RedisError("NOSCRIPT No matching script. Please use EVAL.")) + return cmd + } + cmd.SetErr(nil) + return cmd +} + +func TestNewScriptServerSHA_Run_UsesScriptLoadAndEvalSha_NoEval(t *testing.T) { + ctx := context.Background() + c := &fakeScripter{hashToReturn: strings.Repeat("a", 40)} + + s := NewScriptServerSHA("return 1") + if err := s.Run(ctx, c, []string{"k"}).Err(); err != nil { + t.Fatalf("Run() err: %v", err) + } + + if c.scriptLoadCalls != 1 { + t.Fatalf("expected ScriptLoad 1 call, got %d", c.scriptLoadCalls) + } + if c.evalShaCalls != 1 { + t.Fatalf("expected EvalSha 1 call, got %d", c.evalShaCalls) + } + if c.evalCalls != 0 { + t.Fatalf("expected Eval 0 calls, got %d", c.evalCalls) + } +} + +func TestNewScriptServerSHA_Run_RetriesOnNoScript_NoEvalFallback(t *testing.T) { + ctx := context.Background() + c := &fakeScripter{ + hashToReturn: strings.Repeat("b", 40), + failFirstEvalShaWithNoScr: true, + } + + s := NewScriptServerSHA("return 1") + if err := s.Run(ctx, c, []string{"k"}).Err(); err != nil { + t.Fatalf("Run() err: %v", err) + } + + // Expected: + // - ScriptLoad once (hash cached in Script) + // - EvalSha twice (first NOSCRIPT, second retry) + if c.scriptLoadCalls != 1 { + t.Fatalf("expected ScriptLoad 1 call (hash cached), got %d", c.scriptLoadCalls) + } + if c.evalShaCalls != 2 { + t.Fatalf("expected EvalSha 2 calls (retry), got %d", c.evalShaCalls) + } + // Critical: serverSHA path should NOT fallback to Eval + if c.evalCalls != 0 { + t.Fatalf("expected Eval 0 calls, got %d", c.evalCalls) + } +} + +func TestNewScriptServerSHA_RunRO_UsesScriptLoadAndEvalShaRO_NoEvalRO(t *testing.T) { + ctx := context.Background() + c := &fakeScripter{hashToReturn: strings.Repeat("c", 40)} + + s := NewScriptServerSHA("return 1") + if err := s.RunRO(ctx, c, []string{"k"}).Err(); err != nil { + t.Fatalf("RunRO() err: %v", err) + } + + if c.scriptLoadCalls != 1 { + t.Fatalf("expected ScriptLoad 1 call, got %d", c.scriptLoadCalls) + } + if c.evalShaROCalls != 1 { + t.Fatalf("expected EvalShaRO 1 call, got %d", c.evalShaROCalls) + } + if c.evalROCalls != 0 { + t.Fatalf("expected EvalRO 0 calls, got %d", c.evalROCalls) + } +} + +func TestNewScriptServerSHA_RunRO_RetriesOnNoScript_NoEvalROFallback(t *testing.T) { + ctx := context.Background() + c := &fakeScripter{ + hashToReturn: strings.Repeat("d", 40), + failFirstEvalShaROWithNoScr: true, + } + + s := NewScriptServerSHA("return 1") + if err := s.RunRO(ctx, c, []string{"k"}).Err(); err != nil { + t.Fatalf("RunRO() err: %v", err) + } + + if c.scriptLoadCalls != 1 { + t.Fatalf("expected ScriptLoad 1 call (hash cached), got %d", c.scriptLoadCalls) + } + if c.evalShaROCalls != 2 { + t.Fatalf("expected EvalShaRO 2 calls (retry), got %d", c.evalShaROCalls) + } + if c.evalROCalls != 0 { + t.Fatalf("expected EvalRO 0 calls, got %d", c.evalROCalls) + } +}