From d1f1c7a9cd244d8885908cc63cff9cb0ce07af5a Mon Sep 17 00:00:00 2001 From: johnabass Date: Fri, 13 Sep 2024 12:35:57 -0700 Subject: [PATCH 1/8] simplified the Hasher and Comparer interfaces; avoid stuttering --- basculehash/bcrypt.go | 15 ++++---- basculehash/bcrypt_test.go | 61 +++++++++++--------------------- basculehash/comparer.go | 19 ---------- basculehash/digest.go | 42 ++++++++++++++++++++++ basculehash/hasher.go | 64 +++++++++++++++++++++++++++++----- basculehash/principals.go | 44 +++++++++++++++++++++++ basculehash/principals_test.go | 17 +++++++++ basculehash/store.go | 54 ++++++++++++++++++++++++++++ basculehash/testSuite_test.go | 28 +++++++++++++++ basculehash/validator.go | 55 +++++++++++++++++++++++++++++ 10 files changed, 321 insertions(+), 78 deletions(-) delete mode 100644 basculehash/comparer.go create mode 100644 basculehash/digest.go create mode 100644 basculehash/principals.go create mode 100644 basculehash/principals_test.go create mode 100644 basculehash/store.go create mode 100644 basculehash/testSuite_test.go create mode 100644 basculehash/validator.go diff --git a/basculehash/bcrypt.go b/basculehash/bcrypt.go index 1e9b790..c0ea9c3 100644 --- a/basculehash/bcrypt.go +++ b/basculehash/bcrypt.go @@ -4,8 +4,6 @@ package basculehash import ( - "io" - "golang.org/x/crypto/bcrypt" ) @@ -20,18 +18,17 @@ type Bcrypt struct { Cost int } +var _ Hasher = Bcrypt{} +var _ Comparer = Bcrypt{} + // Hash executes the bcrypt algorithm and write the output to dst. -func (b Bcrypt) Hash(dst io.Writer, plaintext []byte) (n int, err error) { +func (b Bcrypt) Hash(plaintext []byte) (Digest, error) { hashed, err := bcrypt.GenerateFromPassword(plaintext, b.Cost) - if err == nil { - n, err = dst.Write(hashed) - } - - return + return Digest(hashed), err } // Matches attempts to match a plaintext against its bcrypt hashed value. -func (b Bcrypt) Matches(plaintext, hash []byte) (ok bool, err error) { +func (b Bcrypt) Matches(plaintext []byte, hash Digest) (ok bool, err error) { err = bcrypt.CompareHashAndPassword(hash, plaintext) ok = (err == nil) return diff --git a/basculehash/bcrypt_test.go b/basculehash/bcrypt_test.go index 3ff1939..ddfc992 100644 --- a/basculehash/bcrypt_test.go +++ b/basculehash/bcrypt_test.go @@ -4,68 +4,47 @@ package basculehash import ( - "bytes" "fmt" - "strings" "testing" "github.com/stretchr/testify/suite" "golang.org/x/crypto/bcrypt" ) -const bcryptPlaintext string = "bcrypt plaintext" - type BcryptTestSuite struct { - suite.Suite + TestSuite + + plaintext []byte } -// goodHash returns a hash that is expected to be successful. -// The plaintext() is hashed with the given cost. -func (suite *BcryptTestSuite) goodHash(cost int) []byte { - var ( - b bytes.Buffer - hasher = Bcrypt{Cost: cost} - _, err = hasher.Hash(&b, []byte(bcryptPlaintext)) - ) +func (suite *BcryptTestSuite) SetupSubTest() { + suite.SetupTest() +} - suite.Require().NoError(err) - return b.Bytes() +func (suite *BcryptTestSuite) SetupTest() { + suite.plaintext = []byte("bcrypt plaintext") } func (suite *BcryptTestSuite) TestHash() { suite.Run("DefaultCost", func() { - var ( - o strings.Builder - hasher = Bcrypt{} - - n, err = hasher.Hash(&o, []byte(bcryptPlaintext)) + suite.goodHash( + Bcrypt{}, + suite.plaintext, ) - - suite.NoError(err) - suite.Equal(o.Len(), n) }) suite.Run("CustomCost", func() { - var ( - o strings.Builder - hasher = Bcrypt{Cost: 12} - - n, err = hasher.Hash(&o, []byte(bcryptPlaintext)) + suite.goodHash( + Bcrypt{Cost: 12}, + suite.plaintext, ) - - suite.NoError(err) - suite.Equal(o.Len(), n) }) suite.Run("CostTooHigh", func() { - var ( - o strings.Builder - hasher = Bcrypt{Cost: bcrypt.MaxCost + 100} - - _, err = hasher.Hash(&o, []byte(bcryptPlaintext)) + suite.badHash( + Bcrypt{Cost: bcrypt.MaxCost + 100}, + suite.plaintext, ) - - suite.Error(err) }) } @@ -74,9 +53,9 @@ func (suite *BcryptTestSuite) TestMatches() { for _, cost := range []int{0 /* default */, 4, 8} { suite.Run(fmt.Sprintf("cost=%d", cost), func() { var ( - hashed = suite.goodHash(cost) hasher = Bcrypt{Cost: cost} - ok, err = hasher.Matches([]byte(bcryptPlaintext), hashed) + hashed = suite.goodHash(hasher, suite.plaintext) + ok, err = hasher.Matches(suite.plaintext, hashed) ) suite.True(ok) @@ -89,8 +68,8 @@ func (suite *BcryptTestSuite) TestMatches() { for _, cost := range []int{0 /* default */, 4, 8} { suite.Run(fmt.Sprintf("cost=%d", cost), func() { var ( - hashed = suite.goodHash(cost) hasher = Bcrypt{Cost: cost} + hashed = suite.goodHash(hasher, suite.plaintext) ok, err = hasher.Matches([]byte("a different plaintext"), hashed) ) diff --git a/basculehash/comparer.go b/basculehash/comparer.go deleted file mode 100644 index 7e27e55..0000000 --- a/basculehash/comparer.go +++ /dev/null @@ -1,19 +0,0 @@ -// SPDX-FileCopyrightText: 2024 Comcast Cable Communications Management, LLC -// SPDX-License-Identifier: Apache-2.0 - -package basculehash - -// Comparer is a strategy for comparing plaintext values with a -// hash value from a Hasher. -type Comparer interface { - // Matches tests if the given plaintext matches the given hash. - // For example, this method can test if a password matches the - // one-way hashed password from a config file or database. - // - // If this method returns true, the error will always be nil. - // If this method returns false, the error may be non-nil to - // indicate that the match failed due to a problem, such as - // the hash not being parseable. Client code that is just - // interested in a yes/no answer can disregard the error return. - Matches(plaintext, hash []byte) (bool, error) -} diff --git a/basculehash/digest.go b/basculehash/digest.go new file mode 100644 index 0000000..34160c8 --- /dev/null +++ b/basculehash/digest.go @@ -0,0 +1,42 @@ +// SPDX-FileCopyrightText: 2024 Comcast Cable Communications Management, LLC +// SPDX-License-Identifier: Apache-2.0 + +package basculehash + +import "io" + +// Digest is the result of applying a Hasher to plaintext. +// A digest must be valid UTF-8, preferably using the format +// described by https://github.com/P-H-C/phc-string-format/blob/master/phc-sf-spec.md. +type Digest []byte + +// Copy returns a distinct copy of this digest. +func (d Digest) Copy() Digest { + clone := make(Digest, len(d)) + copy(clone, d) + return clone +} + +// String returns this Digest as is, but cast as a string. +func (d Digest) String() string { + return string(d) +} + +// MarshalText simply returns this Digest as a byte slice. This method ensures +// that the digest is written as is instead of encoded as base64 or some other +// encoding. +func (d Digest) MarshalText() ([]byte, error) { + return []byte(d), nil +} + +// UnmarshalText uses the given text as is. +func (d *Digest) UnmarshalText(text []byte) error { + *d = text + return nil +} + +// WriteTo writes this digest to the given writer. +func (d Digest) WriteTo(dst io.Writer) (int64, error) { + c, err := dst.Write(d) + return int64(c), err +} diff --git a/basculehash/hasher.go b/basculehash/hasher.go index baeebe7..e1efeea 100644 --- a/basculehash/hasher.go +++ b/basculehash/hasher.go @@ -3,16 +3,62 @@ package basculehash -import ( - "io" -) - // Hasher is a strategy for one-way hashing. +// +// Comparer is the interface for comparing hash digests with plaintext. +// A given Comparer will correspond to the format written by a Hasher. type Hasher interface { - // Hash writes the hash of a plaintext to a writer. The number of - // bytes written along with any error is returned. + // Hash returns a digest of the given plaintext. The returned Digest + // must be recognizable to a Comparer in order to be validated. + // + // If this method returns a nil error, it MUST return a valid Digest. + // If this method returns an error, the Digest is not guaranteed to have + // any particular value and should be discarded. // - // The format of the written hash must be ASCII. The recommended - // format is the modular crypt format, which bcrypt uses. - Hash(dst io.Writer, plaintext []byte) (int, error) + // The format of the digest must be ASCII. The recommended format is + // the PHC format documented at: + // + // https://github.com/P-H-C/phc-string-format/blob/master/phc-sf-spec.md + Hash(plaintext []byte) (Digest, error) +} + +// Comparer is a strategy for comparing plaintext values with a +// hash digest from a Hasher. +type Comparer interface { + // Matches tests if the given plaintext matches the given hash. + // For example, this method can test if a password matches the + // one-way hashed password from a config file or database. + // + // If this method returns true, the error will always be nil. + // If this method returns false, the error may be non-nil to + // indicate that the match failed due to a problem, such as + // the hash not being parseable. Client code that is just + // interested in a yes/no answer can disregard the error return. + Matches(plaintext []byte, d Digest) (bool, error) +} + +// HasherComparer provides both hashing and corresponding comparison. +// This is the typical interface that a hashing algorithm will implement. +type HasherComparer interface { + Hasher + Comparer +} + +var defaultHash HasherComparer = Bcrypt{} + +// Default returns the default algorithm to use for comparing +// hashed passwords. +func Default() HasherComparer { + return defaultHash +} + +// Matches tests if the given Comparer strategy indicates a match +// between the plaintext and the digest. If cmp is nil, the +// DefaultComparer() is used. +func Matches(cmp Comparer, plaintext []byte, d Digest) (bool, error) { + if cmp == nil { + cmp = Default() + } + + return cmp.Matches(plaintext, d) } diff --git a/basculehash/principals.go b/basculehash/principals.go new file mode 100644 index 0000000..026e7a6 --- /dev/null +++ b/basculehash/principals.go @@ -0,0 +1,44 @@ +// SPDX-FileCopyrightText: 2024 Comcast Cable Communications Management, LLC +// SPDX-License-Identifier: Apache-2.0 + +package basculehash + +// Principals is a mapping between user names and associated +// hashed password digest. This zero value of this type is +// ready to use. +// +// This type is appropriate as a validator if the set of principals +// is fixed and will not change. If the set of credentials needs to +// be mutable, use a Store instead. +type Principals map[string]Digest + +// Get returns the Digest associated with the principal. This method +// returns false if the principal did not exist. +func (p Principals) Get(principal string) (d Digest, exists bool) { + d, exists = p[principal] + return +} + +// Set adds or replaces the given principal and its associated digest. +// If a caller intends to retain the Digest, a copy should be made +// before calling this method. +func (p *Principals) Set(principal string, d Digest) { + if *p == nil { + *p = make(Principals) + } + + (*p)[principal] = d +} + +// Matches tests if a given principal's password matches the associated +// digest. If no such principal exists, this method returns false with a nil error. +// +// If cmp is nil, DefaultComparer is used. +func (p Principals) Matches(cmp Comparer, principal string, plaintext []byte) (match bool, err error) { + d, exists := p[principal] + if exists { + match, err = Matches(cmp, []byte(principal), d) + } + + return +} diff --git a/basculehash/principals_test.go b/basculehash/principals_test.go new file mode 100644 index 0000000..3c2c6ea --- /dev/null +++ b/basculehash/principals_test.go @@ -0,0 +1,17 @@ +package basculehash + +import ( + "testing" + + "github.com/stretchr/testify/suite" +) + +type PrincipalsTestSuite struct { + TestSuite + + hasher Hasher +} + +func TestPrincipals(t *testing.T) { + suite.Run(t, new(PrincipalsTestSuite)) +} diff --git a/basculehash/store.go b/basculehash/store.go new file mode 100644 index 0000000..3c3084d --- /dev/null +++ b/basculehash/store.go @@ -0,0 +1,54 @@ +// SPDX-FileCopyrightText: 2024 Comcast Cable Communications Management, LLC +// SPDX-License-Identifier: Apache-2.0 + +package basculehash + +import ( + "sync" +) + +// Store is an in-memory, threadsafe store of principals together with hashed +// password digests. A Store instance is safe for concurrent +// reads and writes. Instances of this type must not be copied after +// creation. +// +// The zero value of this type is valid and ready to use. +type Store struct { + lock sync.RWMutex + principals Principals +} + +// set updates the principals data under the lock. +// This internal method does not copy the digest. +func (s *Store) set(principal string, d Digest) { + s.lock.Lock() + s.principals.Set(principal, d) + s.lock.Unlock() +} + +// Set adds or updates a principal's password. +// +// This method does not retain d. A distinct copy of the digest +// is made and used internally. +func (s *Store) Set(principal string, d Digest) { + s.set( + principal, + d.Copy(), + ) +} + +// Matches tests if the given principal's hashed password matches the +// plaintext password. This method returns true if both (1) the principal +// exists, and (2) the password matches. If either condition is false, +// this method returns false. +func (s *Store) Matches(cmp Comparer, principal string, plaintext []byte) (bool, error) { + s.lock.RLock() + digest, exists := s.principals.Get(principal) + s.lock.RUnlock() + + if exists { + return Matches(cmp, plaintext, digest) + } + + return false, nil +} diff --git a/basculehash/testSuite_test.go b/basculehash/testSuite_test.go new file mode 100644 index 0000000..a50c576 --- /dev/null +++ b/basculehash/testSuite_test.go @@ -0,0 +1,28 @@ +// SPDX-FileCopyrightText: 2024 Comcast Cable Communications Management, LLC +// SPDX-License-Identifier: Apache-2.0 + +package basculehash + +import "github.com/stretchr/testify/suite" + +// TestSuite has common infrastructure for hashing test suites. +type TestSuite struct { + suite.Suite +} + +// goodHash asserts that a hasher did create a digest successfully, +// and returns that Digest. +func (suite *TestSuite) goodHash(h Hasher, plaintext []byte) Digest { + d, err := h.Hash(plaintext) + suite.Require().NoError(err) + suite.Require().NotEmpty(d) + return d +} + +// badHash asserts that the hash fails. The digest and error are returned +// for any future asserts. +func (suite *TestSuite) badHash(h Hasher, plaintext []byte) (Digest, error) { + d, err := h.Hash(plaintext) + suite.Require().Error(err) + return d, err // hashers are not required to return empty digests on error +} diff --git a/basculehash/validator.go b/basculehash/validator.go new file mode 100644 index 0000000..00fbdee --- /dev/null +++ b/basculehash/validator.go @@ -0,0 +1,55 @@ +// SPDX-FileCopyrightText: 2024 Comcast Cable Communications Management, LLC +// SPDX-License-Identifier: Apache-2.0 + +package basculehash + +import ( + "context" + + "github.com/xmidt-org/bascule" +) + +// Matcher is the common interface between a Principals and a Store. +type Matcher interface { + // Matches checks the associated digest with the given plaintext. + Matches(Comparer, string, []byte) (bool, error) +} + +type matcherValidator[S any] struct { + cmp Comparer + m Matcher +} + +func (mv *matcherValidator[S]) Validate(ctx context.Context, _ S, t bascule.Token) (next bascule.Token, err error) { + next = t + password, ok := bascule.GetPassword(t) + if !ok { + return + } + + matched, matchErr := mv.m.Matches(mv.cmp, t.Principal(), []byte(password)) + switch { + case !matched: + err = bascule.ErrBadCredentials + + case matchErr != nil: + err = bascule.ErrBadCredentials + } + + return +} + +// NewValidator returns a bascule.Validator that always uses the same hash +// Comparer. The source S is unused, but conforms to the Validator interface. +func NewValidator[S any](cmp Comparer, m Matcher) bascule.Validator[S] { + v := &matcherValidator[S]{ + cmp: cmp, + m: m, + } + + if v.cmp == nil { + v.cmp = Default() + } + + return v +} From 778a0374ab4c36e360538a091e5aff10d07ccf83 Mon Sep 17 00:00:00 2001 From: johnabass Date: Fri, 13 Sep 2024 13:05:56 -0700 Subject: [PATCH 2/8] chore: unit tests for digest and hasher --- basculehash/bcrypt_test.go | 10 ------ basculehash/digest_test.go | 61 ++++++++++++++++++++++++++++++++++ basculehash/hasher_test.go | 43 ++++++++++++++++++++++++ basculehash/principals_test.go | 3 ++ basculehash/testSuite_test.go | 14 +++++++- 5 files changed, 120 insertions(+), 11 deletions(-) create mode 100644 basculehash/digest_test.go create mode 100644 basculehash/hasher_test.go diff --git a/basculehash/bcrypt_test.go b/basculehash/bcrypt_test.go index ddfc992..8a4bc4f 100644 --- a/basculehash/bcrypt_test.go +++ b/basculehash/bcrypt_test.go @@ -13,16 +13,6 @@ import ( type BcryptTestSuite struct { TestSuite - - plaintext []byte -} - -func (suite *BcryptTestSuite) SetupSubTest() { - suite.SetupTest() -} - -func (suite *BcryptTestSuite) SetupTest() { - suite.plaintext = []byte("bcrypt plaintext") } func (suite *BcryptTestSuite) TestHash() { diff --git a/basculehash/digest_test.go b/basculehash/digest_test.go new file mode 100644 index 0000000..56ecaa3 --- /dev/null +++ b/basculehash/digest_test.go @@ -0,0 +1,61 @@ +// SPDX-FileCopyrightText: 2024 Comcast Cable Communications Management, LLC +// SPDX-License-Identifier: Apache-2.0 + +package basculehash + +import ( + "bytes" + "testing" + + "github.com/stretchr/testify/suite" +) + +type DigestTestSuite struct { + TestSuite + + digest Digest +} + +func (suite *DigestTestSuite) SetupTest() { + suite.TestSuite.SetupTest() + suite.digest = suite.goodHash(Default(), suite.plaintext) +} + +func (suite *DigestTestSuite) TestCopy() { + clone := suite.digest.Copy() + suite.Equal(suite.digest, clone) + suite.NotSame(suite.digest, clone) +} + +func (suite *DigestTestSuite) TestString() { + suite.Equal( + suite.digest, + Digest(suite.digest.String()), + ) +} + +func (suite *DigestTestSuite) TestMarshalText() { + text, err := suite.digest.MarshalText() + suite.Require().NoError(err) + + var clone Digest + err = clone.UnmarshalText(text) + suite.Require().NoError(err) + suite.Equal(suite.digest, clone) +} + +func (suite *DigestTestSuite) TestWriteTo() { + var o bytes.Buffer + n, err := suite.digest.WriteTo(&o) + suite.Equal(int64(len(suite.digest)), n) + suite.Require().NoError(err) + + suite.Equal( + suite.digest, + Digest(o.Bytes()), + ) +} + +func TestDigest(t *testing.T) { + suite.Run(t, new(DigestTestSuite)) +} diff --git a/basculehash/hasher_test.go b/basculehash/hasher_test.go new file mode 100644 index 0000000..60c5acb --- /dev/null +++ b/basculehash/hasher_test.go @@ -0,0 +1,43 @@ +// SPDX-FileCopyrightText: 2024 Comcast Cable Communications Management, LLC +// SPDX-License-Identifier: Apache-2.0 + +package basculehash + +import ( + "testing" + + "github.com/stretchr/testify/suite" +) + +type HasherTestSuite struct { + TestSuite +} + +func (suite *HasherTestSuite) testMatches(cmp Comparer, d Digest) { + suite.Run("Success", func() { + matched, err := Matches(cmp, suite.plaintext, d) + suite.True(matched) + suite.NoError(err) + }) + + suite.Run("Fail", func() { + matched, err := Matches(cmp, suite.plaintext, Digest("this will never match")) + suite.False(matched) + suite.Error(err) + }) +} + +func (suite *HasherTestSuite) TestMatches() { + suite.Run("Default", func() { + suite.testMatches(nil, suite.goodHash(Default(), suite.plaintext)) + }) + + suite.Run("Custom", func() { + custom := Bcrypt{Cost: 9} + suite.testMatches(custom, suite.goodHash(custom, suite.plaintext)) + }) +} + +func TestHasher(t *testing.T) { + suite.Run(t, new(HasherTestSuite)) +} diff --git a/basculehash/principals_test.go b/basculehash/principals_test.go index 3c2c6ea..f450da0 100644 --- a/basculehash/principals_test.go +++ b/basculehash/principals_test.go @@ -1,3 +1,6 @@ +// SPDX-FileCopyrightText: 2024 Comcast Cable Communications Management, LLC +// SPDX-License-Identifier: Apache-2.0 + package basculehash import ( diff --git a/basculehash/testSuite_test.go b/basculehash/testSuite_test.go index a50c576..558c225 100644 --- a/basculehash/testSuite_test.go +++ b/basculehash/testSuite_test.go @@ -3,11 +3,23 @@ package basculehash -import "github.com/stretchr/testify/suite" +import ( + "github.com/stretchr/testify/suite" +) // TestSuite has common infrastructure for hashing test suites. type TestSuite struct { suite.Suite + + plaintext []byte +} + +func (suite *TestSuite) SetupSubTest() { + suite.SetupTest() +} + +func (suite *TestSuite) SetupTest() { + suite.plaintext = []byte("here is some plaintext") } // goodHash asserts that a hasher did create a digest successfully, From 3d13af2d28c58e5c40c7e773a899ff0570885dda Mon Sep 17 00:00:00 2001 From: johnabass Date: Fri, 13 Sep 2024 13:47:12 -0700 Subject: [PATCH 3/8] return bascule.ErrBadCredentials when a principal is not found; unit tests --- basculehash/bcrypt_test.go | 23 ++++---- basculehash/digest_test.go | 4 +- basculehash/hasher_test.go | 8 ++- basculehash/principals.go | 23 +++++++- basculehash/principals_test.go | 102 ++++++++++++++++++++++++++++++++- basculehash/store.go | 16 +++++- basculehash/testSuite_test.go | 18 ++++-- 7 files changed, 171 insertions(+), 23 deletions(-) diff --git a/basculehash/bcrypt_test.go b/basculehash/bcrypt_test.go index 8a4bc4f..6151a00 100644 --- a/basculehash/bcrypt_test.go +++ b/basculehash/bcrypt_test.go @@ -18,22 +18,19 @@ type BcryptTestSuite struct { func (suite *BcryptTestSuite) TestHash() { suite.Run("DefaultCost", func() { suite.goodHash( - Bcrypt{}, - suite.plaintext, + Bcrypt{}.Hash(suite.plaintext), ) }) suite.Run("CustomCost", func() { suite.goodHash( - Bcrypt{Cost: 12}, - suite.plaintext, + Bcrypt{Cost: 12}.Hash(suite.plaintext), ) }) suite.Run("CostTooHigh", func() { suite.badHash( - Bcrypt{Cost: bcrypt.MaxCost + 100}, - suite.plaintext, + Bcrypt{Cost: bcrypt.MaxCost + 100}.Hash(suite.plaintext), ) }) } @@ -43,8 +40,11 @@ func (suite *BcryptTestSuite) TestMatches() { for _, cost := range []int{0 /* default */, 4, 8} { suite.Run(fmt.Sprintf("cost=%d", cost), func() { var ( - hasher = Bcrypt{Cost: cost} - hashed = suite.goodHash(hasher, suite.plaintext) + hasher = Bcrypt{Cost: cost} + hashed = suite.goodHash( + hasher.Hash(suite.plaintext), + ) + ok, err = hasher.Matches(suite.plaintext, hashed) ) @@ -58,8 +58,11 @@ func (suite *BcryptTestSuite) TestMatches() { for _, cost := range []int{0 /* default */, 4, 8} { suite.Run(fmt.Sprintf("cost=%d", cost), func() { var ( - hasher = Bcrypt{Cost: cost} - hashed = suite.goodHash(hasher, suite.plaintext) + hasher = Bcrypt{Cost: cost} + hashed = suite.goodHash( + hasher.Hash(suite.plaintext), + ) + ok, err = hasher.Matches([]byte("a different plaintext"), hashed) ) diff --git a/basculehash/digest_test.go b/basculehash/digest_test.go index 56ecaa3..cac42ad 100644 --- a/basculehash/digest_test.go +++ b/basculehash/digest_test.go @@ -18,7 +18,9 @@ type DigestTestSuite struct { func (suite *DigestTestSuite) SetupTest() { suite.TestSuite.SetupTest() - suite.digest = suite.goodHash(Default(), suite.plaintext) + suite.digest = suite.goodHash( + Default().Hash(suite.plaintext), + ) } func (suite *DigestTestSuite) TestCopy() { diff --git a/basculehash/hasher_test.go b/basculehash/hasher_test.go index 60c5acb..563d0b5 100644 --- a/basculehash/hasher_test.go +++ b/basculehash/hasher_test.go @@ -29,12 +29,16 @@ func (suite *HasherTestSuite) testMatches(cmp Comparer, d Digest) { func (suite *HasherTestSuite) TestMatches() { suite.Run("Default", func() { - suite.testMatches(nil, suite.goodHash(Default(), suite.plaintext)) + suite.testMatches(nil, suite.goodHash( + Default().Hash(suite.plaintext)), + ) }) suite.Run("Custom", func() { custom := Bcrypt{Cost: 9} - suite.testMatches(custom, suite.goodHash(custom, suite.plaintext)) + suite.testMatches(custom, + suite.goodHash(custom.Hash(suite.plaintext)), + ) }) } diff --git a/basculehash/principals.go b/basculehash/principals.go index 026e7a6..12b0d22 100644 --- a/basculehash/principals.go +++ b/basculehash/principals.go @@ -3,6 +3,8 @@ package basculehash +import "github.com/xmidt-org/bascule" + // Principals is a mapping between user names and associated // hashed password digest. This zero value of this type is // ready to use. @@ -12,6 +14,11 @@ package basculehash // be mutable, use a Store instead. type Principals map[string]Digest +// Len returns the number of principals in this set. +func (p Principals) Len() int { + return len(p) +} + // Get returns the Digest associated with the principal. This method // returns false if the principal did not exist. func (p Principals) Get(principal string) (d Digest, exists bool) { @@ -30,14 +37,26 @@ func (p *Principals) Set(principal string, d Digest) { (*p)[principal] = d } +// Delete removes the given principal from this set, returning any existing +// Digest and an indicator of whether it existed. +func (p *Principals) Delete(principal string) (d Digest, exists bool) { + if d, exists = (*p)[principal]; exists { + delete(*p, principal) + } + + return +} + // Matches tests if a given principal's password matches the associated -// digest. If no such principal exists, this method returns false with a nil error. +// digest. If no such principal exists, this method returns bascule.ErrBadCredentials. // // If cmp is nil, DefaultComparer is used. func (p Principals) Matches(cmp Comparer, principal string, plaintext []byte) (match bool, err error) { d, exists := p[principal] if exists { - match, err = Matches(cmp, []byte(principal), d) + match, err = Matches(cmp, plaintext, d) + } else { + err = bascule.ErrBadCredentials } return diff --git a/basculehash/principals_test.go b/basculehash/principals_test.go index f450da0..c623a13 100644 --- a/basculehash/principals_test.go +++ b/basculehash/principals_test.go @@ -11,8 +11,108 @@ import ( type PrincipalsTestSuite struct { TestSuite +} + +// exists asserts that a given principal exists, and returns the Digest. +func (suite *PrincipalsTestSuite) exists(p Principals, principal string) Digest { + d, ok := p.Get(principal) + suite.Require().True(ok) + return d +} + +// notExists asserts that the given principal did not exist. +func (suite *PrincipalsTestSuite) notExists(p Principals, principal string) { + d, ok := p.Get(principal) + suite.Require().False(ok) + suite.Require().Empty(d) +} + +// deleteExists deletes the given principal and asserts that the principal +// did exist. +func (suite *PrincipalsTestSuite) deleteExists(p Principals, principal string) Digest { + original := p.Len() + d, ok := p.Delete(principal) + suite.Require().True(ok) + suite.Equal(original, p.Len()+1) + + return d +} + +// deleteNotExists deletes the given principal, asserting that the deletion did +// not modify the Principals because the principal did not exist. +func (suite *PrincipalsTestSuite) deleteNotExists(p Principals, principal string) { + original := p.Len() + d, ok := p.Delete(principal) + suite.Require().False(ok) + suite.Empty(d) + suite.Equal(original, p.Len()) +} + +func (suite *PrincipalsTestSuite) TestGetSetDelete() { + suite.T().Log("empty Principals") + var p Principals + suite.Zero(p.Len()) + suite.notExists(p, "joe") + suite.deleteNotExists(p, "joe") + + suite.T().Log("add a principal") + joeDigest := suite.goodHash(Default().Hash(suite.plaintext)) + p.Set("joe", joeDigest) + suite.Equal(1, p.Len()) + suite.Equal(joeDigest, suite.exists(p, "joe")) + + suite.T().Log("add another principal") + fredDigest := suite.goodHash(Default().Hash(suite.plaintext)) + p.Set("fred", fredDigest) + suite.Equal(2, p.Len()) + suite.Equal(joeDigest, suite.exists(p, "joe")) + suite.Equal(fredDigest, suite.exists(p, "fred")) + + suite.T().Log("replace a principal") + newJoeDigest := suite.goodHash(Default().Hash(suite.plaintext)) + suite.Require().NotEqual(newJoeDigest, joeDigest) // hashes should always generate salt to make them distinct + p.Set("joe", newJoeDigest) + suite.Equal(2, p.Len()) + suite.Equal(newJoeDigest, suite.exists(p, "joe")) + suite.Equal(fredDigest, suite.exists(p, "fred")) + + suite.T().Log("delete a principal") + suite.Equal(fredDigest, suite.deleteExists(p, "fred")) + suite.Equal(1, p.Len()) + suite.Equal(newJoeDigest, suite.exists(p, "joe")) +} + +func (suite *PrincipalsTestSuite) testMatches(cmp Comparer, d Digest) { + p := Principals{ + "joe": d, + } + + suite.match( + p.Matches(cmp, "joe", suite.plaintext), + ) + + suite.noMatch( + p.Matches(cmp, "joe", []byte("this will never match")), + ) + + suite.noMatch( + p.Matches(cmp, "doesnotexist", suite.plaintext), + ) +} + +func (suite *PrincipalsTestSuite) TestMatches() { + suite.Run("Default", func() { + suite.testMatches(nil, + suite.goodHash(Default().Hash(suite.plaintext)), + ) + }) - hasher Hasher + suite.Run("Custom", func() { + custom := Bcrypt{Cost: 8} + suite.testMatches(custom, + suite.goodHash(custom.Hash(suite.plaintext)), + ) + }) } func TestPrincipals(t *testing.T) { diff --git a/basculehash/store.go b/basculehash/store.go index 3c3084d..4ecdfad 100644 --- a/basculehash/store.go +++ b/basculehash/store.go @@ -5,6 +5,8 @@ package basculehash import ( "sync" + + "github.com/xmidt-org/bascule" ) // Store is an in-memory, threadsafe store of principals together with hashed @@ -26,6 +28,14 @@ func (s *Store) set(principal string, d Digest) { s.lock.Unlock() } +// Len returns the count of principals currently in this Store. +func (s *Store) Len() (n int) { + s.lock.RLock() + n = len(s.principals) + s.lock.RUnlock() + return +} + // Set adds or updates a principal's password. // // This method does not retain d. A distinct copy of the digest @@ -39,8 +49,8 @@ func (s *Store) Set(principal string, d Digest) { // Matches tests if the given principal's hashed password matches the // plaintext password. This method returns true if both (1) the principal -// exists, and (2) the password matches. If either condition is false, -// this method returns false. +// exists, and (2) the password matches. If the principal does not exist, +// this method returns bascule.ErrBadCredentials. func (s *Store) Matches(cmp Comparer, principal string, plaintext []byte) (bool, error) { s.lock.RLock() digest, exists := s.principals.Get(principal) @@ -50,5 +60,5 @@ func (s *Store) Matches(cmp Comparer, principal string, plaintext []byte) (bool, return Matches(cmp, plaintext, digest) } - return false, nil + return false, bascule.ErrBadCredentials } diff --git a/basculehash/testSuite_test.go b/basculehash/testSuite_test.go index 558c225..31bb93b 100644 --- a/basculehash/testSuite_test.go +++ b/basculehash/testSuite_test.go @@ -24,8 +24,7 @@ func (suite *TestSuite) SetupTest() { // goodHash asserts that a hasher did create a digest successfully, // and returns that Digest. -func (suite *TestSuite) goodHash(h Hasher, plaintext []byte) Digest { - d, err := h.Hash(plaintext) +func (suite *TestSuite) goodHash(d Digest, err error) Digest { suite.Require().NoError(err) suite.Require().NotEmpty(d) return d @@ -33,8 +32,19 @@ func (suite *TestSuite) goodHash(h Hasher, plaintext []byte) Digest { // badHash asserts that the hash fails. The digest and error are returned // for any future asserts. -func (suite *TestSuite) badHash(h Hasher, plaintext []byte) (Digest, error) { - d, err := h.Hash(plaintext) +func (suite *TestSuite) badHash(d Digest, err error) (Digest, error) { suite.Require().Error(err) return d, err // hashers are not required to return empty digests on error } + +// match asserts that the result from a match operation is successful +func (suite *TestSuite) match(matched bool, err error) { + suite.Require().True(matched) + suite.Require().NoError(err) +} + +// noMatch asserts that the result from a match operation failed. +func (suite *TestSuite) noMatch(matched bool, err error) { + suite.Require().False(matched) + suite.Require().Error(err) +} From c365546822470f469995456b97b13c25bf572a94 Mon Sep 17 00:00:00 2001 From: johnabass Date: Fri, 13 Sep 2024 13:48:45 -0700 Subject: [PATCH 4/8] pruned Store.Len, as it's inherently racey --- basculehash/store.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/basculehash/store.go b/basculehash/store.go index 4ecdfad..d63ab78 100644 --- a/basculehash/store.go +++ b/basculehash/store.go @@ -28,14 +28,6 @@ func (s *Store) set(principal string, d Digest) { s.lock.Unlock() } -// Len returns the count of principals currently in this Store. -func (s *Store) Len() (n int) { - s.lock.RLock() - n = len(s.principals) - s.lock.RUnlock() - return -} - // Set adds or updates a principal's password. // // This method does not retain d. A distinct copy of the digest From 661f2e18e6e1afba618bb5c7e82242a8f6fbc517 Mon Sep 17 00:00:00 2001 From: johnabass Date: Thu, 19 Sep 2024 11:21:59 -0700 Subject: [PATCH 5/8] simplified matcher interface; fleshed out unit tests --- basculehash/bcrypt.go | 6 +- basculehash/bcrypt_test.go | 14 +-- basculehash/hasher.go | 48 ++++++-- basculehash/hasher_test.go | 202 ++++++++++++++++++++++++++++++++- basculehash/principals.go | 49 ++++---- basculehash/principals_test.go | 119 ++++--------------- basculehash/store.go | 115 +++++++++++++++---- basculehash/store_test.go | 51 +++++++++ basculehash/validator.go | 16 +-- 9 files changed, 441 insertions(+), 179 deletions(-) create mode 100644 basculehash/store_test.go diff --git a/basculehash/bcrypt.go b/basculehash/bcrypt.go index c0ea9c3..0e247ec 100644 --- a/basculehash/bcrypt.go +++ b/basculehash/bcrypt.go @@ -28,8 +28,6 @@ func (b Bcrypt) Hash(plaintext []byte) (Digest, error) { } // Matches attempts to match a plaintext against its bcrypt hashed value. -func (b Bcrypt) Matches(plaintext []byte, hash Digest) (ok bool, err error) { - err = bcrypt.CompareHashAndPassword(hash, plaintext) - ok = (err == nil) - return +func (b Bcrypt) Matches(plaintext []byte, hash Digest) error { + return bcrypt.CompareHashAndPassword(hash, plaintext) } diff --git a/basculehash/bcrypt_test.go b/basculehash/bcrypt_test.go index 6151a00..c8d11f5 100644 --- a/basculehash/bcrypt_test.go +++ b/basculehash/bcrypt_test.go @@ -44,12 +44,11 @@ func (suite *BcryptTestSuite) TestMatches() { hashed = suite.goodHash( hasher.Hash(suite.plaintext), ) - - ok, err = hasher.Matches(suite.plaintext, hashed) ) - suite.True(ok) - suite.NoError(err) + suite.NoError( + hasher.Matches(suite.plaintext, hashed), + ) }) } }) @@ -62,12 +61,11 @@ func (suite *BcryptTestSuite) TestMatches() { hashed = suite.goodHash( hasher.Hash(suite.plaintext), ) - - ok, err = hasher.Matches([]byte("a different plaintext"), hashed) ) - suite.False(ok) - suite.Error(err) + suite.Error( + hasher.Matches([]byte("a different plaintext"), hashed), + ) }) } }) diff --git a/basculehash/hasher.go b/basculehash/hasher.go index e1efeea..a3448e8 100644 --- a/basculehash/hasher.go +++ b/basculehash/hasher.go @@ -28,13 +28,7 @@ type Comparer interface { // Matches tests if the given plaintext matches the given hash. // For example, this method can test if a password matches the // one-way hashed password from a config file or database. - // - // If this method returns true, the error will always be nil. - // If this method returns false, the error may be non-nil to - // indicate that the match failed due to a problem, such as - // the hash not being parseable. Client code that is just - // interested in a yes/no answer can disregard the error return. - Matches(plaintext []byte, d Digest) (bool, error) + Matches(plaintext []byte, d Digest) error } // HasherComparer provides both hashing and corresponding comparison. @@ -54,11 +48,47 @@ func Default() HasherComparer { // Matches tests if the given Comparer strategy indicates a match // between the plaintext and the digest. If cmp is nil, the -// DefaultComparer() is used. -func Matches(cmp Comparer, plaintext []byte, d Digest) (bool, error) { +// Default() is used. +func Matches(cmp Comparer, plaintext []byte, d Digest) error { if cmp == nil { cmp = Default() } return cmp.Matches(plaintext, d) } + +// Matcher is anything that can test if a principal's digest matches a digest. +type Matcher interface { + // Matches checks the associated digest with the given plaintext. + Matches(cmp Comparer, principal string, plaintext []byte) error +} + +// Credentials is a set of principals and associated digests. Each principal may +// have exactly zero (0) or one (1) associated Digest. +// +// All implementations of this interface in this package can be marshaled and +// unmarshaled from JSON. +type Credentials interface { + Matcher + + // Len returns the count of principals in this set. + Len() int + + // Get returns the Digest associated with the given Principal. + // This method returns false if the principal did not exist. + Get(principal string) (d Digest, exists bool) + + // Set associates a principal with a Digest. If the principal already + // exists, its digest is replaced. + // + // This method does not retain d. An copy is made and stored internally. + Set(principal string, d Digest) + + // Delete removes the principal from this set. If the principal did + // not exist, it returns false. + Delete(principal string) (d Digest, existed bool) + + // Update performs a bulk update of these credentials. Any existing + // principals are replaced. + Update(p Principals) +} diff --git a/basculehash/hasher_test.go b/basculehash/hasher_test.go index 563d0b5..e1411f7 100644 --- a/basculehash/hasher_test.go +++ b/basculehash/hasher_test.go @@ -4,9 +4,12 @@ package basculehash import ( + "encoding/json" + "fmt" "testing" "github.com/stretchr/testify/suite" + "golang.org/x/crypto/bcrypt" ) type HasherTestSuite struct { @@ -15,15 +18,15 @@ type HasherTestSuite struct { func (suite *HasherTestSuite) testMatches(cmp Comparer, d Digest) { suite.Run("Success", func() { - matched, err := Matches(cmp, suite.plaintext, d) - suite.True(matched) - suite.NoError(err) + suite.NoError( + Matches(cmp, suite.plaintext, d), + ) }) suite.Run("Fail", func() { - matched, err := Matches(cmp, suite.plaintext, Digest("this will never match")) - suite.False(matched) - suite.Error(err) + suite.Error( + Matches(cmp, suite.plaintext, Digest("this will never match")), + ) }) } @@ -45,3 +48,190 @@ func (suite *HasherTestSuite) TestMatches() { func TestHasher(t *testing.T) { suite.Run(t, new(HasherTestSuite)) } + +// CredentialsTestSuite runs a standard battery of tests against +// a Credentials implementation. +// +// Tests of UnmarshalJSON need to be done in tests of concrete types +// due to the way unmarshalling works in golang. +type CredentialsTestSuite[C Credentials] struct { + TestSuite + + // Implementations should supply SetupTest and SetupSubTest + // methods that populate this member. Don't forget to call + // TestSuite.SetupTest and TestSuite.SetupSubTest! + credentials C + + hasher Hasher + comparer Comparer +} + +// SetupSuite initializes a hasher and comparer to use when verifying +// and creating digests. +func (suite *CredentialsTestSuite[C]) SetupSuite() { + suite.hasher = Bcrypt{Cost: bcrypt.MinCost} + suite.comparer = Bcrypt{Cost: bcrypt.MinCost} +} + +// assertLen asserts that the credentials under test have the given length. +func (suite *CredentialsTestSuite[C]) assertLen(expected int) { + suite.Equal(expected, suite.credentials.Len()) +} + +// exists asserts that a given principal exists with the given Digest. +func (suite *CredentialsTestSuite[C]) exists(principal string, expected Digest) { + d, ok := suite.credentials.Get(principal) + suite.Require().True(ok) + suite.Require().Equal(expected, d) +} + +// notExists asserts that the given principal did not exist. +func (suite *CredentialsTestSuite[C]) notExists(principal string) { + d, ok := suite.credentials.Get(principal) + suite.Require().False(ok) + suite.Require().Empty(d) +} + +// deleteExists deletes the given principal and asserts that the principal +// did exist. +func (suite *CredentialsTestSuite[C]) deleteExists(principal string, expected Digest) { + original := suite.credentials.Len() + d, ok := suite.credentials.Delete(principal) + suite.Require().True(ok) + suite.Equal(original, suite.credentials.Len()+1) + suite.Require().Equal(expected, d) +} + +// deleteNotExists deletes the given principal, asserting that the deletion did +// not modify the Principals because the principal did not exist. +func (suite *CredentialsTestSuite[C]) deleteNotExists(principal string) { + original := suite.credentials.Len() + d, ok := suite.credentials.Delete(principal) + suite.Require().False(ok) + suite.Empty(d) + suite.Equal(original, suite.credentials.Len()) +} + +// defaultHash creates a distinct hash of the suite plaintext for testing. +func (suite *CredentialsTestSuite[C]) defaultHash() Digest { + return suite.goodHash( + suite.hasher.Hash( + suite.plaintext, + ), + ) +} + +// defaultMatch asserts that the given principal matches with the defaults. +func (suite *CredentialsTestSuite[C]) defaultMatch(principal string) { + suite.NoError( + suite.credentials.Matches(suite.comparer, principal, suite.plaintext), + ) +} + +// defaultNoMatch tests if the principal does not match against the defaults. +// The match error is returned for further asserts. +func (suite *CredentialsTestSuite[C]) defaultNoMatch(principal string) error { + err := suite.credentials.Matches(suite.comparer, principal, suite.plaintext) + suite.Require().Error(err) + return err +} + +func (suite *CredentialsTestSuite[C]) TestGetSetDelete() { + suite.T().Log("empty") + suite.assertLen(0) + suite.deleteNotExists("joe") + + suite.T().Log("add") + joeDigest := suite.defaultHash() + suite.credentials.Set("joe", joeDigest) + suite.assertLen(1) + suite.exists("joe", joeDigest) + + suite.T().Log("add another") + fredDigest := suite.defaultHash() + suite.credentials.Set("fred", fredDigest) + suite.assertLen(2) + suite.exists("joe", joeDigest) + suite.exists("fred", fredDigest) + + suite.T().Log("replace") + newJoeDigest := suite.defaultHash() + suite.Require().NotEqual(newJoeDigest, joeDigest) // hashes should always generate salt to make them distinct + suite.credentials.Set("joe", newJoeDigest) + suite.assertLen(2) + suite.exists("joe", newJoeDigest) + suite.exists("fred", fredDigest) + + suite.T().Log("delete a principal") + suite.deleteExists("fred", fredDigest) + suite.assertLen(1) + suite.exists("joe", newJoeDigest) +} + +func (suite *CredentialsTestSuite[C]) TestMatches() { + // initial condition: + suite.defaultNoMatch("joe") + + suite.credentials.Set("joe", suite.defaultHash()) + suite.defaultMatch("joe") + + suite.credentials.Set("fred", suite.defaultHash()) + suite.defaultMatch("joe") + suite.defaultMatch("fred") + + suite.credentials.Set("joe", suite.goodHash(Default().Hash([]byte("a different password")))) + suite.defaultNoMatch("joe") + suite.defaultMatch("fred") +} + +func (suite *CredentialsTestSuite[C]) TestUpdate() { + suite.credentials.Update(nil) + suite.assertLen(0) + + joeDigest := suite.defaultHash() + fredDigest := suite.defaultHash() + suite.credentials.Update(Principals{ + "joe": joeDigest, + "fred": fredDigest, + }) + + suite.assertLen(2) + suite.exists("joe", joeDigest) + suite.exists("fred", fredDigest) + + joeDigest = suite.defaultHash() + moeDigest := suite.defaultHash() + suite.credentials.Update(Principals{ + "joe": joeDigest, + "moe": moeDigest, + }) + + suite.assertLen(3) + suite.exists("joe", joeDigest) + suite.exists("fred", fredDigest) + suite.exists("moe", moeDigest) +} + +// TestMarshalJSON can happen here since we can marshal things abstractly. +func (suite *CredentialsTestSuite[C]) TestMarshalJSON() { + var ( + joeDigest = suite.defaultHash() + fredDigest = suite.defaultHash() + + expectedJSON = fmt.Sprintf( + `{ + "joe": "%s", + "fred": "%s" + }`, + joeDigest, + fredDigest, + ) + ) + + suite.credentials.Set("joe", joeDigest) + suite.credentials.Set("fred", fredDigest) + + data, err := json.Marshal(suite.credentials) + suite.Require().NoError(err) + suite.JSONEq(expectedJSON, string(data)) +} diff --git a/basculehash/principals.go b/basculehash/principals.go index 12b0d22..325e827 100644 --- a/basculehash/principals.go +++ b/basculehash/principals.go @@ -3,17 +3,19 @@ package basculehash -import "github.com/xmidt-org/bascule" +import "fmt" -// Principals is a mapping between user names and associated -// hashed password digest. This zero value of this type is -// ready to use. +// Principals is a Credentials implementation that is a simple map +// of principals to digests. This type is not safe for concurrent +// usage. // -// This type is appropriate as a validator if the set of principals -// is fixed and will not change. If the set of credentials needs to -// be mutable, use a Store instead. +// This type is appropriate if the set of credentials is either immutable +// or protected from concurrent updates by some other means. type Principals map[string]Digest +var _ Matcher = Principals{} +var _ Credentials = Principals{} + // Len returns the number of principals in this set. func (p Principals) Len() int { return len(p) @@ -27,36 +29,37 @@ func (p Principals) Get(principal string) (d Digest, exists bool) { } // Set adds or replaces the given principal and its associated digest. -// If a caller intends to retain the Digest, a copy should be made -// before calling this method. -func (p *Principals) Set(principal string, d Digest) { - if *p == nil { - *p = make(Principals) - } - - (*p)[principal] = d +func (p Principals) Set(principal string, d Digest) { + p[principal] = d.Copy() } // Delete removes the given principal from this set, returning any existing // Digest and an indicator of whether it existed. -func (p *Principals) Delete(principal string) (d Digest, exists bool) { - if d, exists = (*p)[principal]; exists { - delete(*p, principal) +func (p Principals) Delete(principal string) (d Digest, existed bool) { + if d, existed = (p)[principal]; existed { + delete(p, principal) } return } +// Update performs a bulk update of credentials. Each digest is copied +// before storing in this instance. +func (p Principals) Update(more Principals) { + for principal, digest := range more { + p[principal] = digest.Copy() + } +} + // Matches tests if a given principal's password matches the associated // digest. If no such principal exists, this method returns bascule.ErrBadCredentials. // // If cmp is nil, DefaultComparer is used. -func (p Principals) Matches(cmp Comparer, principal string, plaintext []byte) (match bool, err error) { - d, exists := p[principal] - if exists { - match, err = Matches(cmp, plaintext, d) +func (p Principals) Matches(cmp Comparer, principal string, plaintext []byte) (err error) { + if d, exists := p[principal]; exists { + err = Matches(cmp, plaintext, d) } else { - err = bascule.ErrBadCredentials + err = fmt.Errorf("No such principal: %s", principal) } return diff --git a/basculehash/principals_test.go b/basculehash/principals_test.go index c623a13..56a2444 100644 --- a/basculehash/principals_test.go +++ b/basculehash/principals_test.go @@ -4,115 +4,46 @@ package basculehash import ( + "encoding/json" + "fmt" "testing" "github.com/stretchr/testify/suite" ) type PrincipalsTestSuite struct { - TestSuite + CredentialsTestSuite[Principals] } -// exists asserts that a given principal exists, and returns the Digest. -func (suite *PrincipalsTestSuite) exists(p Principals, principal string) Digest { - d, ok := p.Get(principal) - suite.Require().True(ok) - return d +func (suite *PrincipalsTestSuite) SetupSubTest() { + suite.SetupTest() } -// notExists asserts that the given principal did not exist. -func (suite *PrincipalsTestSuite) notExists(p Principals, principal string) { - d, ok := p.Get(principal) - suite.Require().False(ok) - suite.Require().Empty(d) +func (suite *PrincipalsTestSuite) SetupTest() { + suite.CredentialsTestSuite.SetupTest() + suite.credentials = Principals{} } -// deleteExists deletes the given principal and asserts that the principal -// did exist. -func (suite *PrincipalsTestSuite) deleteExists(p Principals, principal string) Digest { - original := p.Len() - d, ok := p.Delete(principal) - suite.Require().True(ok) - suite.Equal(original, p.Len()+1) +func (suite *PrincipalsTestSuite) TestUnmarshalJSON() { + var ( + joeDigest = suite.defaultHash() + fredDigest = suite.defaultHash() - return d -} - -// deleteNotExists deletes the given principal, asserting that the deletion did -// not modify the Principals because the principal did not exist. -func (suite *PrincipalsTestSuite) deleteNotExists(p Principals, principal string) { - original := p.Len() - d, ok := p.Delete(principal) - suite.Require().False(ok) - suite.Empty(d) - suite.Equal(original, p.Len()) -} - -func (suite *PrincipalsTestSuite) TestGetSetDelete() { - suite.T().Log("empty Principals") - var p Principals - suite.Zero(p.Len()) - suite.notExists(p, "joe") - suite.deleteNotExists(p, "joe") - - suite.T().Log("add a principal") - joeDigest := suite.goodHash(Default().Hash(suite.plaintext)) - p.Set("joe", joeDigest) - suite.Equal(1, p.Len()) - suite.Equal(joeDigest, suite.exists(p, "joe")) - - suite.T().Log("add another principal") - fredDigest := suite.goodHash(Default().Hash(suite.plaintext)) - p.Set("fred", fredDigest) - suite.Equal(2, p.Len()) - suite.Equal(joeDigest, suite.exists(p, "joe")) - suite.Equal(fredDigest, suite.exists(p, "fred")) - - suite.T().Log("replace a principal") - newJoeDigest := suite.goodHash(Default().Hash(suite.plaintext)) - suite.Require().NotEqual(newJoeDigest, joeDigest) // hashes should always generate salt to make them distinct - p.Set("joe", newJoeDigest) - suite.Equal(2, p.Len()) - suite.Equal(newJoeDigest, suite.exists(p, "joe")) - suite.Equal(fredDigest, suite.exists(p, "fred")) - - suite.T().Log("delete a principal") - suite.Equal(fredDigest, suite.deleteExists(p, "fred")) - suite.Equal(1, p.Len()) - suite.Equal(newJoeDigest, suite.exists(p, "joe")) -} - -func (suite *PrincipalsTestSuite) testMatches(cmp Comparer, d Digest) { - p := Principals{ - "joe": d, - } - - suite.match( - p.Matches(cmp, "joe", suite.plaintext), - ) - - suite.noMatch( - p.Matches(cmp, "joe", []byte("this will never match")), - ) - - suite.noMatch( - p.Matches(cmp, "doesnotexist", suite.plaintext), - ) -} - -func (suite *PrincipalsTestSuite) TestMatches() { - suite.Run("Default", func() { - suite.testMatches(nil, - suite.goodHash(Default().Hash(suite.plaintext)), + jsonValue = fmt.Sprintf( + `{ + "joe": "%s", + "fred": "%s" + }`, + joeDigest, + fredDigest, ) - }) + ) - suite.Run("Custom", func() { - custom := Bcrypt{Cost: 8} - suite.testMatches(custom, - suite.goodHash(custom.Hash(suite.plaintext)), - ) - }) + err := json.Unmarshal([]byte(jsonValue), &suite.credentials) + suite.Require().NoError(err) + suite.Equal(2, suite.credentials.Len()) + suite.exists("joe", joeDigest) + suite.exists("fred", fredDigest) } func TestPrincipals(t *testing.T) { diff --git a/basculehash/store.go b/basculehash/store.go index d63ab78..3533280 100644 --- a/basculehash/store.go +++ b/basculehash/store.go @@ -4,15 +4,14 @@ package basculehash import ( + "encoding/json" + "fmt" "sync" - - "github.com/xmidt-org/bascule" ) -// Store is an in-memory, threadsafe store of principals together with hashed -// password digests. A Store instance is safe for concurrent -// reads and writes. Instances of this type must not be copied after -// creation. +// Store is an in-memory, threadsafe Credentials implementation. +// A Store instance is safe for concurrent reads and writes. +// Instances of this type must not be copied after creation. // // The zero value of this type is valid and ready to use. type Store struct { @@ -20,37 +19,109 @@ type Store struct { principals Principals } -// set updates the principals data under the lock. -// This internal method does not copy the digest. -func (s *Store) set(principal string, d Digest) { - s.lock.Lock() - s.principals.Set(principal, d) - s.lock.Unlock() +var _ Matcher = (*Store)(nil) +var _ Credentials = (*Store)(nil) + +// Len returns the number of principals contained in this Store. +func (s *Store) Len() (n int) { + s.lock.RLock() + n = s.principals.Len() + s.lock.RUnlock() + return +} + +// Get returns the Digest associated with the principal. This method +// returns false to indicate that the principal did not exist. +func (s *Store) Get(principal string) (d Digest, exists bool) { + s.lock.RLock() + d, exists = s.principals.Get(principal) + s.lock.RUnlock() + return } // Set adds or updates a principal's password. -// -// This method does not retain d. A distinct copy of the digest -// is made and used internally. func (s *Store) Set(principal string, d Digest) { - s.set( - principal, - d.Copy(), - ) + clone := d.Copy() + s.lock.Lock() + + if s.principals == nil { + s.principals = make(Principals) + } + + s.principals.Set(principal, clone) + + s.lock.Unlock() +} + +// Delete removes the principal from this Store. This method returns +// true if the deletion occurred, false if the principal didn't exist. +func (s *Store) Delete(principal string) (d Digest, existed bool) { + s.lock.Lock() + d, existed = s.principals.Delete(principal) + s.lock.Unlock() + return +} + +// Update performs a bulk update to this Store. Copies are made of +// all digests before storing internally. +func (s *Store) Update(more Principals) { + names := make([]string, 0, len(more)) + digests := make([]Digest, 0, len(more)) + for principal, digest := range more { + names = append(names, principal) + digests = append(digests, digest.Copy()) + } + + s.lock.Lock() + + if s.principals == nil { + s.principals = make(Principals) + } + + for i := 0; i < len(names); i++ { + s.principals[names[i]] = digests[i] // a copy was already made + } + + s.lock.Unlock() } // Matches tests if the given principal's hashed password matches the // plaintext password. This method returns true if both (1) the principal // exists, and (2) the password matches. If the principal does not exist, // this method returns bascule.ErrBadCredentials. -func (s *Store) Matches(cmp Comparer, principal string, plaintext []byte) (bool, error) { +func (s *Store) Matches(cmp Comparer, principal string, plaintext []byte) (err error) { s.lock.RLock() digest, exists := s.principals.Get(principal) s.lock.RUnlock() if exists { - return Matches(cmp, plaintext, digest) + err = Matches(cmp, plaintext, digest) + } else { + err = fmt.Errorf("No such principal: %s", principal) + } + + return +} + +// MarshalJSON writes the current state of this Store to JSON. +func (s *Store) MarshalJSON() (data []byte, err error) { + s.lock.RLock() + data, err = json.Marshal(s.principals) + s.lock.RUnlock() + return +} + +// UnmarshalJSON unmarshals data and replaces the current set of principals. +// If unmarshalling returned an error, this Store's state remains unchanged. +func (s *Store) UnmarshalJSON(data []byte) (err error) { + s.lock.Lock() + + var unmarshaled Principals + if err = json.Unmarshal(data, &unmarshaled); err == nil { + s.principals = unmarshaled } - return false, bascule.ErrBadCredentials + s.lock.Unlock() + + return } diff --git a/basculehash/store_test.go b/basculehash/store_test.go new file mode 100644 index 0000000..b7fd9d6 --- /dev/null +++ b/basculehash/store_test.go @@ -0,0 +1,51 @@ +// SPDX-FileCopyrightText: 2024 Comcast Cable Communications Management, LLC +// SPDX-License-Identifier: Apache-2.0 + +package basculehash + +import ( + "encoding/json" + "fmt" + "testing" + + "github.com/stretchr/testify/suite" +) + +type StoreTestSuite struct { + CredentialsTestSuite[*Store] +} + +func (suite *StoreTestSuite) SetupSubTest() { + suite.SetupTest() +} + +func (suite *StoreTestSuite) SetupTest() { + suite.CredentialsTestSuite.SetupTest() + suite.credentials = new(Store) +} + +func (suite *StoreTestSuite) TestUnmarshalJSON() { + var ( + joeDigest = suite.defaultHash() + fredDigest = suite.defaultHash() + + jsonValue = fmt.Sprintf( + `{ + "joe": "%s", + "fred": "%s" + }`, + joeDigest, + fredDigest, + ) + ) + + err := json.Unmarshal([]byte(jsonValue), suite.credentials) + suite.Require().NoError(err) + suite.Equal(2, suite.credentials.Len()) + suite.exists("joe", joeDigest) + suite.exists("fred", fredDigest) +} + +func TestStore(t *testing.T) { + suite.Run(t, new(StoreTestSuite)) +} diff --git a/basculehash/validator.go b/basculehash/validator.go index 00fbdee..b28a937 100644 --- a/basculehash/validator.go +++ b/basculehash/validator.go @@ -5,16 +5,11 @@ package basculehash import ( "context" + "errors" "github.com/xmidt-org/bascule" ) -// Matcher is the common interface between a Principals and a Store. -type Matcher interface { - // Matches checks the associated digest with the given plaintext. - Matches(Comparer, string, []byte) (bool, error) -} - type matcherValidator[S any] struct { cmp Comparer m Matcher @@ -27,13 +22,8 @@ func (mv *matcherValidator[S]) Validate(ctx context.Context, _ S, t bascule.Toke return } - matched, matchErr := mv.m.Matches(mv.cmp, t.Principal(), []byte(password)) - switch { - case !matched: - err = bascule.ErrBadCredentials - - case matchErr != nil: - err = bascule.ErrBadCredentials + if err = mv.m.Matches(mv.cmp, t.Principal(), []byte(password)); err != nil { + err = errors.Join(bascule.ErrBadCredentials, err) } return From 98e72d5acfd063bcf8fdafd211f44de10d67ac8a Mon Sep 17 00:00:00 2001 From: johnabass Date: Thu, 19 Sep 2024 12:36:45 -0700 Subject: [PATCH 6/8] streamlined the Credentials interface; ensure support for remote credentials --- basculehash/credentials.go | 25 ++++ basculehash/credentials_test.go | 110 +++++++++++++++ basculehash/hasher.go | 47 ------- basculehash/hasher_test.go | 237 -------------------------------- basculehash/principals.go | 41 ++---- basculehash/principals_test.go | 24 ---- basculehash/store.go | 57 ++------ basculehash/store_test.go | 24 +++- basculehash/testSuite_test.go | 12 -- basculehash/validator.go | 27 ++-- 10 files changed, 198 insertions(+), 406 deletions(-) create mode 100644 basculehash/credentials.go create mode 100644 basculehash/credentials_test.go delete mode 100644 basculehash/hasher_test.go diff --git a/basculehash/credentials.go b/basculehash/credentials.go new file mode 100644 index 0000000..4a1dc6a --- /dev/null +++ b/basculehash/credentials.go @@ -0,0 +1,25 @@ +// SPDX-FileCopyrightText: 2024 Comcast Cable Communications Management, LLC +// SPDX-License-Identifier: Apache-2.0 + +package basculehash + +import "context" + +// Credentials is a source of principals and their associated digests. A +// credentials instance may be in-memory or a remote system. +type Credentials interface { + // Get returns the Digest associated with the given Principal. + // This method returns false if the principal did not exist. + Get(ctx context.Context, principal string) (d Digest, exists bool) + + // Set associates a principal with a Digest. If the principal already + // exists, its digest is replaced. + Set(ctx context.Context, principal string, d Digest) + + // Delete removes one or more principals from this set. + Delete(ctx context.Context, principals ...string) + + // Update performs a bulk update of these credentials. Any existing + // principals are replaced. + Update(ctx context.Context, p Principals) +} diff --git a/basculehash/credentials_test.go b/basculehash/credentials_test.go new file mode 100644 index 0000000..f8527ad --- /dev/null +++ b/basculehash/credentials_test.go @@ -0,0 +1,110 @@ +// SPDX-FileCopyrightText: 2024 Comcast Cable Communications Management, LLC +// SPDX-License-Identifier: Apache-2.0 + +package basculehash + +import ( + "context" + + "golang.org/x/crypto/bcrypt" +) + +// CredentialsTestSuite runs a standard battery of tests against +// a Credentials implementation. +// +// Tests of UnmarshalJSON need to be done in tests of concrete types +// due to the way unmarshalling works in golang. +type CredentialsTestSuite[C Credentials] struct { + TestSuite + + // Implementations should supply SetupTest and SetupSubTest + // methods that populate this member. Don't forget to call + // TestSuite.SetupTest and TestSuite.SetupSubTest! + credentials C + + testCtx context.Context + hasher Hasher +} + +// SetupSuite initializes a hasher and comparer to use when verifying +// and creating digests. +func (suite *CredentialsTestSuite[C]) SetupSuite() { + suite.testCtx = context.Background() + suite.hasher = Bcrypt{Cost: bcrypt.MinCost} +} + +// exists asserts that a given principal exists with the given Digest. +func (suite *CredentialsTestSuite[C]) exists(principal string, expected Digest) { + d, ok := suite.credentials.Get(suite.testCtx, principal) + suite.Require().True(ok) + suite.Require().Equal(expected, d) +} + +// notExists asserts that the given principal did not exist. +func (suite *CredentialsTestSuite[C]) notExists(principal string) { + d, ok := suite.credentials.Get(suite.testCtx, principal) + suite.Require().False(ok) + suite.Require().Empty(d) +} + +// defaultHash creates a distinct hash of the suite plaintext for testing. +func (suite *CredentialsTestSuite[C]) defaultHash() Digest { + return suite.goodHash( + suite.hasher.Hash( + suite.plaintext, + ), + ) +} + +func (suite *CredentialsTestSuite[C]) TestGetSetDelete() { + suite.T().Log("delete from empty") + suite.credentials.Delete(suite.testCtx, "joe") + + suite.T().Log("add") + joeDigest := suite.defaultHash() + suite.credentials.Set(suite.testCtx, "joe", joeDigest) + suite.exists("joe", joeDigest) + + suite.T().Log("add another") + fredDigest := suite.defaultHash() + suite.credentials.Set(suite.testCtx, "fred", fredDigest) + suite.exists("joe", joeDigest) + suite.exists("fred", fredDigest) + + suite.T().Log("replace") + newJoeDigest := suite.defaultHash() + suite.Require().NotEqual(newJoeDigest, joeDigest) // hashes should always generate salt to make them distinct + suite.credentials.Set(suite.testCtx, "joe", newJoeDigest) + suite.exists("joe", newJoeDigest) + suite.exists("fred", fredDigest) + + suite.T().Log("delete a principal") + suite.credentials.Delete(suite.testCtx, "fred") + suite.notExists("fred") + suite.exists("joe", newJoeDigest) +} + +func (suite *CredentialsTestSuite[C]) TestUpdate() { + suite.credentials.Update(suite.testCtx, nil) + + joeDigest := suite.defaultHash() + fredDigest := suite.defaultHash() + suite.credentials.Update(suite.testCtx, Principals{ + "joe": joeDigest, + "fred": fredDigest, + }) + + suite.exists("joe", joeDigest) + suite.exists("fred", fredDigest) + + joeDigest = suite.defaultHash() + moeDigest := suite.defaultHash() + suite.credentials.Update(suite.testCtx, Principals{ + "joe": joeDigest, + "moe": moeDigest, + }) + + suite.exists("joe", joeDigest) + suite.exists("fred", fredDigest) + suite.exists("moe", moeDigest) +} diff --git a/basculehash/hasher.go b/basculehash/hasher.go index a3448e8..cc1f3e8 100644 --- a/basculehash/hasher.go +++ b/basculehash/hasher.go @@ -45,50 +45,3 @@ var defaultHash HasherComparer = Bcrypt{} func Default() HasherComparer { return defaultHash } - -// Matches tests if the given Comparer strategy indicates a match -// between the plaintext and the digest. If cmp is nil, the -// Default() is used. -func Matches(cmp Comparer, plaintext []byte, d Digest) error { - if cmp == nil { - cmp = Default() - } - - return cmp.Matches(plaintext, d) -} - -// Matcher is anything that can test if a principal's digest matches a digest. -type Matcher interface { - // Matches checks the associated digest with the given plaintext. - Matches(cmp Comparer, principal string, plaintext []byte) error -} - -// Credentials is a set of principals and associated digests. Each principal may -// have exactly zero (0) or one (1) associated Digest. -// -// All implementations of this interface in this package can be marshaled and -// unmarshaled from JSON. -type Credentials interface { - Matcher - - // Len returns the count of principals in this set. - Len() int - - // Get returns the Digest associated with the given Principal. - // This method returns false if the principal did not exist. - Get(principal string) (d Digest, exists bool) - - // Set associates a principal with a Digest. If the principal already - // exists, its digest is replaced. - // - // This method does not retain d. An copy is made and stored internally. - Set(principal string, d Digest) - - // Delete removes the principal from this set. If the principal did - // not exist, it returns false. - Delete(principal string) (d Digest, existed bool) - - // Update performs a bulk update of these credentials. Any existing - // principals are replaced. - Update(p Principals) -} diff --git a/basculehash/hasher_test.go b/basculehash/hasher_test.go deleted file mode 100644 index e1411f7..0000000 --- a/basculehash/hasher_test.go +++ /dev/null @@ -1,237 +0,0 @@ -// SPDX-FileCopyrightText: 2024 Comcast Cable Communications Management, LLC -// SPDX-License-Identifier: Apache-2.0 - -package basculehash - -import ( - "encoding/json" - "fmt" - "testing" - - "github.com/stretchr/testify/suite" - "golang.org/x/crypto/bcrypt" -) - -type HasherTestSuite struct { - TestSuite -} - -func (suite *HasherTestSuite) testMatches(cmp Comparer, d Digest) { - suite.Run("Success", func() { - suite.NoError( - Matches(cmp, suite.plaintext, d), - ) - }) - - suite.Run("Fail", func() { - suite.Error( - Matches(cmp, suite.plaintext, Digest("this will never match")), - ) - }) -} - -func (suite *HasherTestSuite) TestMatches() { - suite.Run("Default", func() { - suite.testMatches(nil, suite.goodHash( - Default().Hash(suite.plaintext)), - ) - }) - - suite.Run("Custom", func() { - custom := Bcrypt{Cost: 9} - suite.testMatches(custom, - suite.goodHash(custom.Hash(suite.plaintext)), - ) - }) -} - -func TestHasher(t *testing.T) { - suite.Run(t, new(HasherTestSuite)) -} - -// CredentialsTestSuite runs a standard battery of tests against -// a Credentials implementation. -// -// Tests of UnmarshalJSON need to be done in tests of concrete types -// due to the way unmarshalling works in golang. -type CredentialsTestSuite[C Credentials] struct { - TestSuite - - // Implementations should supply SetupTest and SetupSubTest - // methods that populate this member. Don't forget to call - // TestSuite.SetupTest and TestSuite.SetupSubTest! - credentials C - - hasher Hasher - comparer Comparer -} - -// SetupSuite initializes a hasher and comparer to use when verifying -// and creating digests. -func (suite *CredentialsTestSuite[C]) SetupSuite() { - suite.hasher = Bcrypt{Cost: bcrypt.MinCost} - suite.comparer = Bcrypt{Cost: bcrypt.MinCost} -} - -// assertLen asserts that the credentials under test have the given length. -func (suite *CredentialsTestSuite[C]) assertLen(expected int) { - suite.Equal(expected, suite.credentials.Len()) -} - -// exists asserts that a given principal exists with the given Digest. -func (suite *CredentialsTestSuite[C]) exists(principal string, expected Digest) { - d, ok := suite.credentials.Get(principal) - suite.Require().True(ok) - suite.Require().Equal(expected, d) -} - -// notExists asserts that the given principal did not exist. -func (suite *CredentialsTestSuite[C]) notExists(principal string) { - d, ok := suite.credentials.Get(principal) - suite.Require().False(ok) - suite.Require().Empty(d) -} - -// deleteExists deletes the given principal and asserts that the principal -// did exist. -func (suite *CredentialsTestSuite[C]) deleteExists(principal string, expected Digest) { - original := suite.credentials.Len() - d, ok := suite.credentials.Delete(principal) - suite.Require().True(ok) - suite.Equal(original, suite.credentials.Len()+1) - suite.Require().Equal(expected, d) -} - -// deleteNotExists deletes the given principal, asserting that the deletion did -// not modify the Principals because the principal did not exist. -func (suite *CredentialsTestSuite[C]) deleteNotExists(principal string) { - original := suite.credentials.Len() - d, ok := suite.credentials.Delete(principal) - suite.Require().False(ok) - suite.Empty(d) - suite.Equal(original, suite.credentials.Len()) -} - -// defaultHash creates a distinct hash of the suite plaintext for testing. -func (suite *CredentialsTestSuite[C]) defaultHash() Digest { - return suite.goodHash( - suite.hasher.Hash( - suite.plaintext, - ), - ) -} - -// defaultMatch asserts that the given principal matches with the defaults. -func (suite *CredentialsTestSuite[C]) defaultMatch(principal string) { - suite.NoError( - suite.credentials.Matches(suite.comparer, principal, suite.plaintext), - ) -} - -// defaultNoMatch tests if the principal does not match against the defaults. -// The match error is returned for further asserts. -func (suite *CredentialsTestSuite[C]) defaultNoMatch(principal string) error { - err := suite.credentials.Matches(suite.comparer, principal, suite.plaintext) - suite.Require().Error(err) - return err -} - -func (suite *CredentialsTestSuite[C]) TestGetSetDelete() { - suite.T().Log("empty") - suite.assertLen(0) - suite.deleteNotExists("joe") - - suite.T().Log("add") - joeDigest := suite.defaultHash() - suite.credentials.Set("joe", joeDigest) - suite.assertLen(1) - suite.exists("joe", joeDigest) - - suite.T().Log("add another") - fredDigest := suite.defaultHash() - suite.credentials.Set("fred", fredDigest) - suite.assertLen(2) - suite.exists("joe", joeDigest) - suite.exists("fred", fredDigest) - - suite.T().Log("replace") - newJoeDigest := suite.defaultHash() - suite.Require().NotEqual(newJoeDigest, joeDigest) // hashes should always generate salt to make them distinct - suite.credentials.Set("joe", newJoeDigest) - suite.assertLen(2) - suite.exists("joe", newJoeDigest) - suite.exists("fred", fredDigest) - - suite.T().Log("delete a principal") - suite.deleteExists("fred", fredDigest) - suite.assertLen(1) - suite.exists("joe", newJoeDigest) -} - -func (suite *CredentialsTestSuite[C]) TestMatches() { - // initial condition: - suite.defaultNoMatch("joe") - - suite.credentials.Set("joe", suite.defaultHash()) - suite.defaultMatch("joe") - - suite.credentials.Set("fred", suite.defaultHash()) - suite.defaultMatch("joe") - suite.defaultMatch("fred") - - suite.credentials.Set("joe", suite.goodHash(Default().Hash([]byte("a different password")))) - suite.defaultNoMatch("joe") - suite.defaultMatch("fred") -} - -func (suite *CredentialsTestSuite[C]) TestUpdate() { - suite.credentials.Update(nil) - suite.assertLen(0) - - joeDigest := suite.defaultHash() - fredDigest := suite.defaultHash() - suite.credentials.Update(Principals{ - "joe": joeDigest, - "fred": fredDigest, - }) - - suite.assertLen(2) - suite.exists("joe", joeDigest) - suite.exists("fred", fredDigest) - - joeDigest = suite.defaultHash() - moeDigest := suite.defaultHash() - suite.credentials.Update(Principals{ - "joe": joeDigest, - "moe": moeDigest, - }) - - suite.assertLen(3) - suite.exists("joe", joeDigest) - suite.exists("fred", fredDigest) - suite.exists("moe", moeDigest) -} - -// TestMarshalJSON can happen here since we can marshal things abstractly. -func (suite *CredentialsTestSuite[C]) TestMarshalJSON() { - var ( - joeDigest = suite.defaultHash() - fredDigest = suite.defaultHash() - - expectedJSON = fmt.Sprintf( - `{ - "joe": "%s", - "fred": "%s" - }`, - joeDigest, - fredDigest, - ) - ) - - suite.credentials.Set("joe", joeDigest) - suite.credentials.Set("fred", fredDigest) - - data, err := json.Marshal(suite.credentials) - suite.Require().NoError(err) - suite.JSONEq(expectedJSON, string(data)) -} diff --git a/basculehash/principals.go b/basculehash/principals.go index 325e827..5bbb061 100644 --- a/basculehash/principals.go +++ b/basculehash/principals.go @@ -3,7 +3,9 @@ package basculehash -import "fmt" +import ( + "context" +) // Principals is a Credentials implementation that is a simple map // of principals to digests. This type is not safe for concurrent @@ -13,54 +15,31 @@ import "fmt" // or protected from concurrent updates by some other means. type Principals map[string]Digest -var _ Matcher = Principals{} var _ Credentials = Principals{} -// Len returns the number of principals in this set. -func (p Principals) Len() int { - return len(p) -} - // Get returns the Digest associated with the principal. This method // returns false if the principal did not exist. -func (p Principals) Get(principal string) (d Digest, exists bool) { +func (p Principals) Get(_ context.Context, principal string) (d Digest, exists bool) { d, exists = p[principal] return } // Set adds or replaces the given principal and its associated digest. -func (p Principals) Set(principal string, d Digest) { +func (p Principals) Set(_ context.Context, principal string, d Digest) { p[principal] = d.Copy() } -// Delete removes the given principal from this set, returning any existing -// Digest and an indicator of whether it existed. -func (p Principals) Delete(principal string) (d Digest, existed bool) { - if d, existed = (p)[principal]; existed { - delete(p, principal) +// Delete removes the given principal(s) from this set. +func (p Principals) Delete(_ context.Context, principals ...string) { + for _, toDelete := range principals { + delete(p, toDelete) } - - return } // Update performs a bulk update of credentials. Each digest is copied // before storing in this instance. -func (p Principals) Update(more Principals) { +func (p Principals) Update(_ context.Context, more Principals) { for principal, digest := range more { p[principal] = digest.Copy() } } - -// Matches tests if a given principal's password matches the associated -// digest. If no such principal exists, this method returns bascule.ErrBadCredentials. -// -// If cmp is nil, DefaultComparer is used. -func (p Principals) Matches(cmp Comparer, principal string, plaintext []byte) (err error) { - if d, exists := p[principal]; exists { - err = Matches(cmp, plaintext, d) - } else { - err = fmt.Errorf("No such principal: %s", principal) - } - - return -} diff --git a/basculehash/principals_test.go b/basculehash/principals_test.go index 56a2444..fdf154a 100644 --- a/basculehash/principals_test.go +++ b/basculehash/principals_test.go @@ -4,8 +4,6 @@ package basculehash import ( - "encoding/json" - "fmt" "testing" "github.com/stretchr/testify/suite" @@ -24,28 +22,6 @@ func (suite *PrincipalsTestSuite) SetupTest() { suite.credentials = Principals{} } -func (suite *PrincipalsTestSuite) TestUnmarshalJSON() { - var ( - joeDigest = suite.defaultHash() - fredDigest = suite.defaultHash() - - jsonValue = fmt.Sprintf( - `{ - "joe": "%s", - "fred": "%s" - }`, - joeDigest, - fredDigest, - ) - ) - - err := json.Unmarshal([]byte(jsonValue), &suite.credentials) - suite.Require().NoError(err) - suite.Equal(2, suite.credentials.Len()) - suite.exists("joe", joeDigest) - suite.exists("fred", fredDigest) -} - func TestPrincipals(t *testing.T) { suite.Run(t, new(PrincipalsTestSuite)) } diff --git a/basculehash/store.go b/basculehash/store.go index 3533280..d8a9a3f 100644 --- a/basculehash/store.go +++ b/basculehash/store.go @@ -4,8 +4,8 @@ package basculehash import ( + "context" "encoding/json" - "fmt" "sync" ) @@ -19,28 +19,18 @@ type Store struct { principals Principals } -var _ Matcher = (*Store)(nil) var _ Credentials = (*Store)(nil) -// Len returns the number of principals contained in this Store. -func (s *Store) Len() (n int) { +// Get returns the Digest associated with the principal. +func (s *Store) Get(ctx context.Context, principal string) (d Digest, exists bool) { s.lock.RLock() - n = s.principals.Len() - s.lock.RUnlock() - return -} - -// Get returns the Digest associated with the principal. This method -// returns false to indicate that the principal did not exist. -func (s *Store) Get(principal string) (d Digest, exists bool) { - s.lock.RLock() - d, exists = s.principals.Get(principal) + d, exists = s.principals.Get(ctx, principal) s.lock.RUnlock() return } // Set adds or updates a principal's password. -func (s *Store) Set(principal string, d Digest) { +func (s *Store) Set(ctx context.Context, principal string, d Digest) { clone := d.Copy() s.lock.Lock() @@ -48,23 +38,24 @@ func (s *Store) Set(principal string, d Digest) { s.principals = make(Principals) } - s.principals.Set(principal, clone) + s.principals.Set(ctx, principal, clone) s.lock.Unlock() } -// Delete removes the principal from this Store. This method returns -// true if the deletion occurred, false if the principal didn't exist. -func (s *Store) Delete(principal string) (d Digest, existed bool) { +// Delete removes the principal(s) from this Store. +func (s *Store) Delete(_ context.Context, principals ...string) { s.lock.Lock() - d, existed = s.principals.Delete(principal) + + for _, toDelete := range principals { + delete(s.principals, toDelete) + } + s.lock.Unlock() - return } -// Update performs a bulk update to this Store. Copies are made of -// all digests before storing internally. -func (s *Store) Update(more Principals) { +// Update performs a bulk update to this Store. +func (s *Store) Update(_ context.Context, more Principals) { names := make([]string, 0, len(more)) digests := make([]Digest, 0, len(more)) for principal, digest := range more { @@ -85,24 +76,6 @@ func (s *Store) Update(more Principals) { s.lock.Unlock() } -// Matches tests if the given principal's hashed password matches the -// plaintext password. This method returns true if both (1) the principal -// exists, and (2) the password matches. If the principal does not exist, -// this method returns bascule.ErrBadCredentials. -func (s *Store) Matches(cmp Comparer, principal string, plaintext []byte) (err error) { - s.lock.RLock() - digest, exists := s.principals.Get(principal) - s.lock.RUnlock() - - if exists { - err = Matches(cmp, plaintext, digest) - } else { - err = fmt.Errorf("No such principal: %s", principal) - } - - return -} - // MarshalJSON writes the current state of this Store to JSON. func (s *Store) MarshalJSON() (data []byte, err error) { s.lock.RLock() diff --git a/basculehash/store_test.go b/basculehash/store_test.go index b7fd9d6..ef440bf 100644 --- a/basculehash/store_test.go +++ b/basculehash/store_test.go @@ -24,6 +24,29 @@ func (suite *StoreTestSuite) SetupTest() { suite.credentials = new(Store) } +func (suite *StoreTestSuite) TestMarshalJSON() { + var ( + joeDigest = suite.defaultHash() + fredDigest = suite.defaultHash() + + expectedJSON = fmt.Sprintf( + `{ + "joe": "%s", + "fred": "%s" + }`, + joeDigest, + fredDigest, + ) + ) + + suite.credentials.Set(suite.testCtx, "joe", joeDigest) + suite.credentials.Set(suite.testCtx, "fred", fredDigest) + actualJSON, err := json.Marshal(suite.credentials) + + suite.Require().NoError(err) + suite.JSONEq(expectedJSON, string(actualJSON)) +} + func (suite *StoreTestSuite) TestUnmarshalJSON() { var ( joeDigest = suite.defaultHash() @@ -41,7 +64,6 @@ func (suite *StoreTestSuite) TestUnmarshalJSON() { err := json.Unmarshal([]byte(jsonValue), suite.credentials) suite.Require().NoError(err) - suite.Equal(2, suite.credentials.Len()) suite.exists("joe", joeDigest) suite.exists("fred", fredDigest) } diff --git a/basculehash/testSuite_test.go b/basculehash/testSuite_test.go index 31bb93b..475d720 100644 --- a/basculehash/testSuite_test.go +++ b/basculehash/testSuite_test.go @@ -36,15 +36,3 @@ func (suite *TestSuite) badHash(d Digest, err error) (Digest, error) { suite.Require().Error(err) return d, err // hashers are not required to return empty digests on error } - -// match asserts that the result from a match operation is successful -func (suite *TestSuite) match(matched bool, err error) { - suite.Require().True(matched) - suite.Require().NoError(err) -} - -// noMatch asserts that the result from a match operation failed. -func (suite *TestSuite) noMatch(matched bool, err error) { - suite.Require().False(matched) - suite.Require().Error(err) -} diff --git a/basculehash/validator.go b/basculehash/validator.go index b28a937..4c70d8b 100644 --- a/basculehash/validator.go +++ b/basculehash/validator.go @@ -11,8 +11,8 @@ import ( ) type matcherValidator[S any] struct { - cmp Comparer - m Matcher + cmp Comparer + creds Credentials } func (mv *matcherValidator[S]) Validate(ctx context.Context, _ S, t bascule.Token) (next bascule.Token, err error) { @@ -22,8 +22,13 @@ func (mv *matcherValidator[S]) Validate(ctx context.Context, _ S, t bascule.Toke return } - if err = mv.m.Matches(mv.cmp, t.Principal(), []byte(password)); err != nil { - err = errors.Join(bascule.ErrBadCredentials, err) + if digest, exists := mv.creds.Get(ctx, t.Principal()); exists { + err = mv.cmp.Matches([]byte(password), digest) + if err != nil { + err = errors.Join(bascule.ErrBadCredentials, err) + } + } else { + err = bascule.ErrBadCredentials } return @@ -31,15 +36,13 @@ func (mv *matcherValidator[S]) Validate(ctx context.Context, _ S, t bascule.Toke // NewValidator returns a bascule.Validator that always uses the same hash // Comparer. The source S is unused, but conforms to the Validator interface. -func NewValidator[S any](cmp Comparer, m Matcher) bascule.Validator[S] { - v := &matcherValidator[S]{ - cmp: cmp, - m: m, +func NewValidator[S any](cmp Comparer, creds Credentials) bascule.Validator[S] { + if cmp == nil { + cmp = Default() } - if v.cmp == nil { - v.cmp = Default() + return &matcherValidator[S]{ + cmp: cmp, + creds: creds, } - - return v } From bee275c94c74d976bd3655df687be1fd16646d4b Mon Sep 17 00:00:00 2001 From: johnabass Date: Thu, 19 Sep 2024 13:12:55 -0700 Subject: [PATCH 7/8] chore: validator unit tests --- basculehash/validator_test.go | 109 ++++++++++++++++++++++++++++++++++ 1 file changed, 109 insertions(+) create mode 100644 basculehash/validator_test.go diff --git a/basculehash/validator_test.go b/basculehash/validator_test.go new file mode 100644 index 0000000..31cf802 --- /dev/null +++ b/basculehash/validator_test.go @@ -0,0 +1,109 @@ +// SPDX-FileCopyrightText: 2024 Comcast Cable Communications Management, LLC +// SPDX-License-Identifier: Apache-2.0 + +package basculehash + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/suite" + "github.com/xmidt-org/bascule" + "golang.org/x/crypto/bcrypt" +) + +type validatorTestToken struct { + principal, password string +} + +func (t validatorTestToken) Principal() string { return t.principal } + +func (t validatorTestToken) Password() string { return t.password } + +type ValidatorTestSuite struct { + TestSuite + + testCtx context.Context + request *http.Request +} + +func (suite *ValidatorTestSuite) SetupSubTest() { + suite.SetupTest() +} + +func (suite *ValidatorTestSuite) SetupTest() { + suite.TestSuite.SetupTest() + suite.testCtx = context.Background() + suite.request = httptest.NewRequest("GET", "/", nil) +} + +// newDefaultToken creates a password token using this suite's default plaintext. +func (suite *ValidatorTestSuite) newDefaultToken(principal string) bascule.Token { + return validatorTestToken{ + principal: principal, + password: string(suite.plaintext), + } +} + +// newCredentials builds a standard set of credentials using the given hasher. +func (suite *ValidatorTestSuite) newCredentials(h Hasher) Credentials { + return Principals{ + "joe": suite.goodHash(h.Hash(suite.plaintext)), + "fred": suite.goodHash(h.Hash(suite.plaintext)), + } +} + +func (suite *ValidatorTestSuite) newValidator(cmp Comparer, creds Credentials) bascule.Validator[*http.Request] { + v := NewValidator[*http.Request](cmp, creds) + suite.Require().NotNil(v) + return v +} + +func (suite *ValidatorTestSuite) testValidate(cmp Comparer, h Hasher) { + v := suite.newValidator(cmp, suite.newCredentials(h)) + + suite.Run("NonPasswordToken", func() { + t := bascule.StubToken("joe") + next, err := v.Validate(suite.testCtx, suite.request, t) + suite.Equal(t, next) + suite.NoError(err) + }) + + suite.Run("NoSuchPrincipal", func() { + t := suite.newDefaultToken("nosuch") + next, err := v.Validate(suite.testCtx, suite.request, t) + suite.Equal(t, next) + suite.ErrorIs(err, bascule.ErrBadCredentials) + }) + + suite.Run("BadPassword", func() { + t := validatorTestToken{principal: "joe", password: "bad"} + next, err := v.Validate(suite.testCtx, suite.request, t) + suite.Equal(t, next) + suite.ErrorIs(err, bascule.ErrBadCredentials) + }) + + suite.Run("Success", func() { + t := suite.newDefaultToken("joe") + next, err := v.Validate(suite.testCtx, suite.request, t) + suite.Equal(t, next) + suite.NoError(err) + }) +} + +func (suite *ValidatorTestSuite) TestValidate() { + suite.Run("DefaultComparer", func() { + suite.testValidate(nil, Default()) + }) + + suite.Run("CustomComparer", func() { + hc := Bcrypt{Cost: bcrypt.MinCost} + suite.testValidate(hc, hc) + }) +} + +func TestValidator(t *testing.T) { + suite.Run(t, new(ValidatorTestSuite)) +} From 315f214374e96d019bae2d97e64c7e9cf0ef03b2 Mon Sep 17 00:00:00 2001 From: johnabass Date: Thu, 19 Sep 2024 13:21:11 -0700 Subject: [PATCH 8/8] updated to the new Hasher interface --- cmd/hash/cli.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cmd/hash/cli.go b/cmd/hash/cli.go index 62be675..694428b 100644 --- a/cmd/hash/cli.go +++ b/cmd/hash/cli.go @@ -45,7 +45,11 @@ func (cmd *Bcrypt) Run(kong *kong.Kong) error { Cost: cmd.Cost, } - _, err := hasher.Hash(kong.Stdout, []byte(cmd.Plaintext)) + digest, err := hasher.Hash([]byte(cmd.Plaintext)) + if err == nil { + digest.WriteTo(kong.Stdout) + } + return err }