From e15d8dc7ecdafb6e54be04aea4fba5815d394ad4 Mon Sep 17 00:00:00 2001 From: Matt Silverlock Date: Tue, 10 Feb 2026 10:55:23 -0500 Subject: [PATCH] fix Calibrate edge cases, improve test coverage Address findings from code review: - validate params before generating salt in GenerateFromPassword - guard division-by-zero in Calibrate when durPerP truncates to 0 - prevent Calibrate from decrementing P to 0 when constraints are unsatisfiable (return P=1 instead of erroring) - reject negative memMiBytes in Calibrate - document that Calibrate always sets R=8 Test improvements: - fix TestGenerateFromPassword: assert invalid params produce errors (9 of 13 test cases were previously dead code) - add TestHashFormat: validate N$R$P$hexsalt$hexdk structure - add TestKnownHash: RFC 7914 Section 12 vector 3 as regression pin - add TestCheck: direct Params.Check() test with boundary values - expand TestCost to cover non-default params - add nil/empty input coverage to TestCompareHashAndPassword - widen TestCalibrate timing tolerance to reduce CI flakiness Also bump go.mod to go 1.21 (matches CI matrix) and gitignore coverage.out. --- .gitignore | 1 + coverage.out | 73 ------------------------- go.mod | 2 +- scrypt.go | 22 ++++++-- scrypt_test.go | 144 +++++++++++++++++++++++++++++++++++++++++++------ 5 files changed, 147 insertions(+), 95 deletions(-) delete mode 100644 coverage.out diff --git a/.gitignore b/.gitignore index 843bb79..474603f 100644 --- a/.gitignore +++ b/.gitignore @@ -22,5 +22,6 @@ _testmain.go *.exe *.test *.prof +coverage.out *.DS_Store diff --git a/coverage.out b/coverage.out deleted file mode 100644 index c6c67be..0000000 --- a/coverage.out +++ /dev/null @@ -1,73 +0,0 @@ -mode: atomic -github.com/elithrar/simple-scrypt/scrypt.go:61.49,65.16 3 38 -github.com/elithrar/simple-scrypt/scrypt.go:65.16,67.3 1 0 -github.com/elithrar/simple-scrypt/scrypt.go:69.2,69.15 1 38 -github.com/elithrar/simple-scrypt/scrypt.go:77.75,79.16 2 23 -github.com/elithrar/simple-scrypt/scrypt.go:79.16,81.3 1 0 -github.com/elithrar/simple-scrypt/scrypt.go:83.2,83.39 1 23 -github.com/elithrar/simple-scrypt/scrypt.go:83.39,85.3 1 9 -github.com/elithrar/simple-scrypt/scrypt.go:88.2,89.16 2 14 -github.com/elithrar/simple-scrypt/scrypt.go:89.16,91.3 1 0 -github.com/elithrar/simple-scrypt/scrypt.go:95.2,95.91 1 14 -github.com/elithrar/simple-scrypt/scrypt.go:102.65,105.16 2 3 -github.com/elithrar/simple-scrypt/scrypt.go:105.16,107.3 1 1 -github.com/elithrar/simple-scrypt/scrypt.go:110.2,111.16 2 2 -github.com/elithrar/simple-scrypt/scrypt.go:111.16,113.3 1 0 -github.com/elithrar/simple-scrypt/scrypt.go:116.2,116.48 1 2 -github.com/elithrar/simple-scrypt/scrypt.go:116.48,118.3 1 1 -github.com/elithrar/simple-scrypt/scrypt.go:120.2,120.37 1 1 -github.com/elithrar/simple-scrypt/scrypt.go:125.32,127.44 1 35 -github.com/elithrar/simple-scrypt/scrypt.go:127.44,129.3 1 5 -github.com/elithrar/simple-scrypt/scrypt.go:132.2,132.29 1 30 -github.com/elithrar/simple-scrypt/scrypt.go:132.29,134.3 1 1 -github.com/elithrar/simple-scrypt/scrypt.go:137.2,137.29 1 29 -github.com/elithrar/simple-scrypt/scrypt.go:137.29,139.3 1 1 -github.com/elithrar/simple-scrypt/scrypt.go:143.2,143.106 1 28 -github.com/elithrar/simple-scrypt/scrypt.go:143.106,145.3 1 1 -github.com/elithrar/simple-scrypt/scrypt.go:148.2,148.50 1 27 -github.com/elithrar/simple-scrypt/scrypt.go:148.50,150.3 1 1 -github.com/elithrar/simple-scrypt/scrypt.go:153.2,153.44 1 26 -github.com/elithrar/simple-scrypt/scrypt.go:153.44,155.3 1 1 -github.com/elithrar/simple-scrypt/scrypt.go:157.2,157.12 1 25 -github.com/elithrar/simple-scrypt/scrypt.go:163.62,167.20 2 11 -github.com/elithrar/simple-scrypt/scrypt.go:167.20,169.3 1 2 -github.com/elithrar/simple-scrypt/scrypt.go:171.2,175.16 4 9 -github.com/elithrar/simple-scrypt/scrypt.go:175.16,177.3 1 1 -github.com/elithrar/simple-scrypt/scrypt.go:179.2,180.16 2 8 -github.com/elithrar/simple-scrypt/scrypt.go:180.16,182.3 1 1 -github.com/elithrar/simple-scrypt/scrypt.go:184.2,185.16 2 7 -github.com/elithrar/simple-scrypt/scrypt.go:185.16,187.3 1 1 -github.com/elithrar/simple-scrypt/scrypt.go:189.2,190.16 2 6 -github.com/elithrar/simple-scrypt/scrypt.go:190.16,192.3 1 1 -github.com/elithrar/simple-scrypt/scrypt.go:193.2,196.16 3 5 -github.com/elithrar/simple-scrypt/scrypt.go:196.16,198.3 1 1 -github.com/elithrar/simple-scrypt/scrypt.go:199.2,201.39 2 4 -github.com/elithrar/simple-scrypt/scrypt.go:201.39,203.3 1 1 -github.com/elithrar/simple-scrypt/scrypt.go:205.2,205.30 1 3 -github.com/elithrar/simple-scrypt/scrypt.go:211.40,215.2 2 8 -github.com/elithrar/simple-scrypt/scrypt.go:225.86,227.72 2 8 -github.com/elithrar/simple-scrypt/scrypt.go:227.72,229.3 1 8 -github.com/elithrar/simple-scrypt/scrypt.go:229.8,229.41 1 0 -github.com/elithrar/simple-scrypt/scrypt.go:229.41,231.3 1 0 -github.com/elithrar/simple-scrypt/scrypt.go:232.2,232.18 1 8 -github.com/elithrar/simple-scrypt/scrypt.go:232.18,234.3 1 0 -github.com/elithrar/simple-scrypt/scrypt.go:235.2,235.21 1 8 -github.com/elithrar/simple-scrypt/scrypt.go:235.21,237.3 1 0 -github.com/elithrar/simple-scrypt/scrypt.go:238.2,239.16 2 8 -github.com/elithrar/simple-scrypt/scrypt.go:239.16,241.3 1 0 -github.com/elithrar/simple-scrypt/scrypt.go:242.2,263.50 6 8 -github.com/elithrar/simple-scrypt/scrypt.go:263.50,265.3 1 122 -github.com/elithrar/simple-scrypt/scrypt.go:266.2,270.78 3 8 -github.com/elithrar/simple-scrypt/scrypt.go:270.78,272.3 1 0 -github.com/elithrar/simple-scrypt/scrypt.go:273.2,276.20 2 8 -github.com/elithrar/simple-scrypt/scrypt.go:276.20,280.79 3 10 -github.com/elithrar/simple-scrypt/scrypt.go:280.79,282.4 1 0 -github.com/elithrar/simple-scrypt/scrypt.go:283.3,283.26 1 10 -github.com/elithrar/simple-scrypt/scrypt.go:288.2,288.20 1 8 -github.com/elithrar/simple-scrypt/scrypt.go:288.20,292.23 2 23 -github.com/elithrar/simple-scrypt/scrypt.go:292.23,295.4 1 12 -github.com/elithrar/simple-scrypt/scrypt.go:295.9,297.4 1 11 -github.com/elithrar/simple-scrypt/scrypt.go:299.3,300.79 2 23 -github.com/elithrar/simple-scrypt/scrypt.go:300.79,302.4 1 0 -github.com/elithrar/simple-scrypt/scrypt.go:303.3,303.26 1 23 -github.com/elithrar/simple-scrypt/scrypt.go:306.2,308.21 2 8 diff --git a/go.mod b/go.mod index 523e847..5db6156 100644 --- a/go.mod +++ b/go.mod @@ -1,5 +1,5 @@ module github.com/elithrar/simple-scrypt -go 1.20 +go 1.21 require golang.org/x/crypto v0.31.0 diff --git a/scrypt.go b/scrypt.go index 170f2b9..ee483f6 100644 --- a/scrypt.go +++ b/scrypt.go @@ -75,12 +75,12 @@ func GenerateRandomBytes(n int) ([]byte, error) { // If the parameters provided are less than the minimum acceptable values, // an error will be returned. func GenerateFromPassword(password []byte, params Params) ([]byte, error) { - salt, err := GenerateRandomBytes(params.SaltLen) - if err != nil { + if err := params.Check(); err != nil { return nil, err } - if err := params.Check(); err != nil { + salt, err := GenerateRandomBytes(params.SaltLen) + if err != nil { return nil, err } @@ -218,6 +218,9 @@ func Cost(hash []byte) (Params, error) { // The returned params will not use more memory than the given (MiB); // will not take more time than the given timeout, but more than timeout/2. // +// R is always set to 8 regardless of the input params, per +// https://blog.filippo.io/the-scrypt-parameters/. +// // The default timeout (when the timeout arg is zero) is 200ms. // The default memMiBytes (when memMiBytes is zero) is 16MiB. // The default parameters (when params == Params{}) is DefaultParams. @@ -234,6 +237,9 @@ func Calibrate(timeout time.Duration, memMiBytes int, params Params) (Params, er if memMiBytes == 0 { memMiBytes = 16 } + if memMiBytes < 0 { + return p, ErrInvalidParams + } salt, err := GenerateRandomBytes(p.SaltLen) if err != nil { return p, err @@ -286,7 +292,11 @@ func Calibrate(timeout time.Duration, memMiBytes int, params Params) (Params, er // the further away we are from timeout the bigger the steps should be for dur < timeout { // the theoretical optimal p; can not be used because of inaccurate measuring - optimalP := int(int64(timeout) / (int64(dur) / int64(p.P))) + durPerP := int64(dur) / int64(p.P) + if durPerP == 0 { + durPerP = 1 + } + optimalP := int(int64(timeout) / durPerP) if optimalP > p.P+1 { // use average between optimal p and current p @@ -302,7 +312,9 @@ func Calibrate(timeout time.Duration, memMiBytes int, params Params) (Params, er dur = time.Since(start) } // lower by one to get shorter duration than timeout - p.P-- + if p.P > 1 { + p.P-- + } return p, p.Check() } diff --git a/scrypt_test.go b/scrypt_test.go index 4eeea13..f6cf507 100644 --- a/scrypt_test.go +++ b/scrypt_test.go @@ -1,8 +1,11 @@ package scrypt import ( + "encoding/hex" "fmt" "reflect" + "strconv" + "strings" "testing" "time" ) @@ -57,12 +60,56 @@ func TestGenerateRandomBytes(t *testing.T) { func TestGenerateFromPassword(t *testing.T) { for _, v := range testParams { _, err := GenerateFromPassword([]byte(password), v.params) - if err != nil && v.pass == true { - t.Fatalf("no error was returned when expected for params: %+v", v.params) + if v.pass && err != nil { + t.Fatalf("unexpected error for valid params %+v: %v", v.params, err) + } + if !v.pass && err == nil { + t.Fatalf("expected error for invalid params %+v, got nil", v.params) } } } +func TestHashFormat(t *testing.T) { + hash, err := GenerateFromPassword([]byte(password), DefaultParams) + if err != nil { + t.Fatal(err) + } + + parts := strings.Split(string(hash), "$") + if len(parts) != 5 { + t.Fatalf("expected 5 dollar-separated fields, got %d: %s", len(parts), hash) + } + + n, err := strconv.Atoi(parts[0]) + if err != nil || n != DefaultParams.N { + t.Errorf("N field: got %q, want %d", parts[0], DefaultParams.N) + } + r, err := strconv.Atoi(parts[1]) + if err != nil || r != DefaultParams.R { + t.Errorf("R field: got %q, want %d", parts[1], DefaultParams.R) + } + p, err := strconv.Atoi(parts[2]) + if err != nil || p != DefaultParams.P { + t.Errorf("P field: got %q, want %d", parts[2], DefaultParams.P) + } + + salt, err := hex.DecodeString(parts[3]) + if err != nil { + t.Fatalf("salt is not valid hex: %v", err) + } + if len(salt) != DefaultParams.SaltLen { + t.Errorf("salt length: got %d, want %d", len(salt), DefaultParams.SaltLen) + } + + dk, err := hex.DecodeString(parts[4]) + if err != nil { + t.Fatalf("dk is not valid hex: %v", err) + } + if len(dk) != DefaultParams.DKLen { + t.Errorf("dk length: got %d, want %d", len(dk), DefaultParams.DKLen) + } +} + func TestCompareHashAndPassword(t *testing.T) { hash, err := GenerateFromPassword([]byte(password), DefaultParams) if err != nil { @@ -82,21 +129,36 @@ func TestCompareHashAndPassword(t *testing.T) { t.Fatalf("did not identify an invalid hash") } + if err := CompareHashAndPassword(nil, []byte(password)); err == nil { + t.Fatal("expected error for nil hash") + } + if err := CompareHashAndPassword([]byte{}, []byte(password)); err == nil { + t.Fatal("expected error for empty hash") + } + if err := CompareHashAndPassword(hash, nil); err == nil { + t.Fatal("expected error for nil password, got match") + } } func TestCost(t *testing.T) { - hash, err := GenerateFromPassword([]byte(password), DefaultParams) - if err != nil { - t.Fatal(err) - } + for _, want := range []Params{ + DefaultParams, + {65536, 8, 1, 16, 64}, + {4096, 8, 1, 32, 32}, + } { + hash, err := GenerateFromPassword([]byte(password), want) + if err != nil { + t.Fatalf("GenerateFromPassword(%+v): %v", want, err) + } - params, err := Cost(hash) - if err != nil { - t.Fatal(err) - } + got, err := Cost(hash) + if err != nil { + t.Fatalf("Cost(%+v): %v", want, err) + } - if !reflect.DeepEqual(params, DefaultParams) { - t.Fatal("cost mismatch: parameters used did not match those retrieved") + if !reflect.DeepEqual(got, want) { + t.Fatalf("cost mismatch: got %+v, want %+v", got, want) + } } } @@ -109,6 +171,56 @@ func TestDecodeHash(t *testing.T) { } } +// TestKnownHash verifies CompareHashAndPassword against a hash constructed from +// the RFC 7914 Section 12 test vector (P="pleaseletmein", S="SodiumChloride", +// N=16384, r=8, p=1, dkLen=64). This catches regressions in the underlying +// scrypt implementation or changes to the hash encoding format. +func TestKnownHash(t *testing.T) { + // salt = hex("SodiumChloride"), dk = RFC 7914 expected output + known := []byte("16384$8$1$536f6469756d43686c6f72696465$" + + "7023bdcb3afd7348461c06cd81fd38eb" + + "fda8fbba904f8e3ea9b543f6545da1f2" + + "d5432955613f0fcf62d49705242a9af9" + + "e61e85dc0d651e40dfcf017b45575887") + if err := CompareHashAndPassword(known, []byte("pleaseletmein")); err != nil { + t.Fatalf("RFC 7914 known-good hash failed verification: %v", err) + } + + if err := CompareHashAndPassword(known, []byte("wrong-password")); err == nil { + t.Fatal("known hash matched incorrect password") + } +} + +func TestCheck(t *testing.T) { + tests := []struct { + pass bool + params Params + desc string + }{ + {true, Params{2, 1, 1, 8, 16}, "minimum valid params"}, + {true, Params{16384, 8, 1, 16, 32}, "default params"}, + {false, Params{3, 8, 1, 16, 32}, "N not power of 2"}, + {false, Params{0, 8, 1, 16, 32}, "N is zero"}, + {false, Params{1, 8, 1, 16, 32}, "N is 1"}, + {false, Params{-1, 8, 1, 16, 32}, "N is negative"}, + {false, Params{16384, 0, 1, 16, 32}, "R is zero"}, + {false, Params{16384, 8, 0, 16, 32}, "P is zero"}, + {false, Params{16384, 8, 1, 7, 32}, "SaltLen below minimum"}, + {true, Params{16384, 8, 1, 8, 32}, "SaltLen at minimum"}, + {false, Params{16384, 8, 1, 16, 15}, "DKLen below minimum"}, + {true, Params{16384, 8, 1, 16, 16}, "DKLen at minimum"}, + } + for _, tc := range tests { + err := tc.params.Check() + if tc.pass && err != nil { + t.Errorf("%s: unexpected error: %v", tc.desc, err) + } + if !tc.pass && err == nil { + t.Errorf("%s: expected error, got nil", tc.desc) + } + } +} + func TestCalibrate(t *testing.T) { timeout := 500 * time.Millisecond for testNum, tc := range []struct { @@ -141,10 +253,10 @@ func TestCalibrate(t *testing.T) { if err != nil { t.Fatalf("%d. GenerateFromPassword with %#v: %v", testNum, p, err) } - if dur < timeout/2 { - t.Errorf("%d. GenerateFromPassword was too fast (expected between %s and %s, got %s) with %#v.", testNum, timeout/2, timeout+timeout/2, dur, p) - } else if timeout+timeout/2 < dur { - t.Errorf("%d. GenerateFromPassword took too long (expected between %s and %s, got %s) with %#v.", testNum, timeout/2, timeout+timeout/2, dur, p) + if dur < timeout/4 { + t.Errorf("%d. GenerateFromPassword was too fast (expected at least %s, got %s) with %#v.", testNum, timeout/4, dur, p) + } else if 3*timeout < dur { + t.Errorf("%d. GenerateFromPassword took too long (expected at most %s, got %s) with %#v.", testNum, 3*timeout, dur, p) } } }