More comments to be more explicit

Signed-off-by: Andrew Thornton <[email protected]>

Author: zeripath

modules/auth/password/hash/argon2.go

@@ -41,6 +41,9 @@ func NewArgon2Hasher(config string) *Argon2Hasher {
 	// This default configuration uses the following parameters:
 	// time=2, memory=64*1024, threads=8, keyLen=50.
 	// It will make two passes through the memory, using 64MiB in total.
+	// This matches the original configuration for `argon2` prior to storing hash parameters
+	// in the database.
+	// THESE VALUES MUST NOT BE CHANGED OR BACKWARDS COMPATIBILITY WILL BREAK
 	hasher := &Argon2Hasher{
 		time:    2,
 		memory:  1 << 16,

modules/auth/password/hash/bcrypt.go

@@ -34,6 +34,9 @@ func (hasher *BcryptHasher) VerifyPassword(password, hashedPassword, salt string
 // The provided config should be either empty or the string representation of the "<cost>"
 // as an integer
 func NewBcryptHasher(config string) *BcryptHasher {
+	// This matches the original configuration for `bcrypt` prior to storing hash parameters
+	// in the database.
+	// THESE VALUES MUST NOT BE CHANGED OR BACKWARDS COMPATIBILITY WILL BREAK
 	hasher := &BcryptHasher{
 		cost: 10, // cost=10. i.e. 2^10 rounds of key expansion.
 	}

modules/auth/password/hash/hash.go

@@ -35,7 +35,7 @@ type PasswordVerifier interface {
 // PasswordHashAlgorithms are named PasswordSaltHashers with a default verifier and hash function
 type PasswordHashAlgorithm struct {
 	PasswordSaltHasher
-	Name string
+	Specification string // The specification that is used to create the internal PasswordSaltHasher
 }
 
 // Hash the provided password with the salt and return the hash
@@ -61,16 +61,16 @@ func (algorithm *PasswordHashAlgorithm) Hash(password, salt string) (string, err
 
 // Verify the provided password matches the hashPassword when hashed with the salt
 func (algorithm *PasswordHashAlgorithm) VerifyPassword(providedPassword, hashedPassword, salt string) bool {
-	// The bcrypt package has its own specialized compare function that takes into
-	// account the stored password's bcrypt parameters.
+	// Some PasswordSaltHashers have their own specialised compare function that takes into
+	// account the stored parameters within the hash. e.g. bcrypt
 	if verifier, ok := algorithm.PasswordSaltHasher.(PasswordVerifier); ok {
 		return verifier.VerifyPassword(providedPassword, hashedPassword, salt)
 	}
 
 	// Compute the hash of the password.
 	providedPasswordHash, err := algorithm.Hash(providedPassword, salt)
 	if err != nil {
-		log.Error("passwordhash: %v.Hash(): %v", algorithm.Name, err)
+		log.Error("passwordhash: %v.Hash(): %v", algorithm.Specification, err)
 		return false
 	}
 
@@ -84,7 +84,7 @@ var (
 )
 
 // Register registers a PasswordSaltHasher with the availableHasherFactories
-// This is not thread safe.
+// Caution: This is not thread safe.
 func Register[T PasswordSaltHasher](name string, newFn func(config string) T) {
 	if _, has := availableHasherFactories[name]; has {
 		panic(fmt.Errorf("duplicate registration of password salt hasher: %s", name))
@@ -96,49 +96,82 @@ func Register[T PasswordSaltHasher](name string, newFn func(config string) T) {
 	}
 }
 
-// In early versions of gitea the password hash algorithm field could be empty
-// At that point the default was `pbkdf2` without configuration values
-// Please note this is not the same as the DefaultAlgorithm
-const defaultEmptyHashAlgorithmName = "pbkdf2"
-
-func Parse(algorithm string) *PasswordHashAlgorithm {
-	if algorithm == "" {
-		algorithm = defaultEmptyHashAlgorithmName
+// In early versions of gitea the password hash algorithm field of a user could be
+// empty. At that point the default was `pbkdf2` without configuration values
+//
+// Please note this is not the same as the DefaultAlgorithm which is used
+// to determine what an empty PASSWORD_HASH_ALGO setting in the app.ini means.
+// These are not the same even if they have the same apparent value and they mean different things.
+//
+// DO NOT COALESCE THESE VALUES
+const defaultEmptyHashAlgorithmSpecification = "pbkdf2"
+
+// Parse will convert the provided algorithm specification in to a PasswordHashAlgorithm
+// If the provided specification matches the DefaultHashAlgorithm Specification it will be
+// used.
+// In addition the last non-default hasher will be cached to help reduce the load from
+// parsing specifications.
+//
+// NOTE: No de-aliasing is done in this function, thus any specification which does not
+// contain a configuration will use the default values for that hasher. These are not
+// necessarily the same values as those obtained by dealiasing. This allows for
+// seamless backwards compatibility with the original configuration.
+//
+// To further labour this point, running `Parse("pbkdf2")` does not obtain the
+// same algorithm as setting `PASSWORD_HASH_ALGO=pbkdf2` in app.ini, nor is it intended to.
+// A user that has `password_hash_algo='pbkdf2'` in the db means get the original, unconfigured algorithm
+// Users will be migrated automatically as they log-in to have the complete specification stored
+// in their `password_hash_algo` fields by other code.
+func Parse(algorithmSpec string) *PasswordHashAlgorithm {
+	if algorithmSpec == "" {
+		algorithmSpec = defaultEmptyHashAlgorithmSpecification
 	}
 
-	if DefaultHashAlgorithm != nil && algorithm == DefaultHashAlgorithm.Name {
+	if DefaultHashAlgorithm != nil && algorithmSpec == DefaultHashAlgorithm.Specification {
 		return DefaultHashAlgorithm
 	}
 
 	ptr := lastNonDefaultAlgorithm.Load()
 	if ptr != nil {
 		hashAlgorithm, ok := ptr.(*PasswordHashAlgorithm)
-		if ok && hashAlgorithm.Name == algorithm {
+		if ok && hashAlgorithm.Specification == algorithmSpec {
 			return hashAlgorithm
 		}
 	}
 
-	vals := strings.SplitN(algorithm, "$", 2)
-	var name string
+	// Now convert the provided specification in to a hasherType +/- some configuration parameters
+	vals := strings.SplitN(algorithmSpec, "$", 2)
+	var hasherType string
 	var config string
+
 	if len(vals) == 0 {
+		// This should not happen as algorithmSpec should not be empty
+		// due to it being assigned to defaultEmptyHashAlgorithmSpecification above
+		// but we should be absolutely cautious here
 		return nil
 	}
-	name = vals[0]
+
+	hasherType = vals[0]
 	if len(vals) > 1 {
 		config = vals[1]
 	}
-	newFn, has := availableHasherFactories[name]
+
+	newFn, has := availableHasherFactories[hasherType]
 	if !has {
+		// unknown hasher type
 		return nil
 	}
+
 	ph := newFn(config)
 	if ph == nil {
+		// The provided configuration is likely invalid - it will have been logged already
+		// but we cannot hash safely
 		return nil
 	}
+
 	hashAlgorithm := &PasswordHashAlgorithm{
 		PasswordSaltHasher: ph,
-		Name:               algorithm,
+		Specification:      algorithmSpec,
 	}
 
 	lastNonDefaultAlgorithm.Store(hashAlgorithm)

modules/auth/password/hash/pbkdf2.go

@@ -36,6 +36,11 @@ func (hasher *PBKDF2Hasher) HashWithSaltBytes(password string, salt []byte) stri
 // "<iter>$<keyLen>", where <x> is the string representation
 // of an integer
 func NewPBKDF2Hasher(config string) *PBKDF2Hasher {
+	// This default configuration uses the following parameters:
+	// iter=10000, keyLen=50.
+	// This matches the original configuration for `pbkdf2` prior to storing parameters
+	// in the database.
+	// THESE VALUES MUST NOT BE CHANGED OR BACKWARDS COMPATIBILITY WILL BREAK
 	hasher := &PBKDF2Hasher{
 		iter:   10_000,
 		keyLen: 50,

modules/auth/password/hash/scrypt.go

@@ -36,6 +36,9 @@ func (hasher *ScryptHasher) HashWithSaltBytes(password string, salt []byte) stri
 // "<n>$<r>$<p>$<keyLen>", where <x> is the string representation
 // of an integer
 func NewScryptHasher(config string) *ScryptHasher {
+	// This matches the original configuration for `scrypt` prior to storing hash parameters
+	// in the database.
+	// THESE VALUES MUST NOT BE CHANGED OR BACKWARDS COMPATIBILITY WILL BREAK
 	hasher := &ScryptHasher{
 		n:      1 << 16,
 		r:      16,

modules/auth/password/hash/setting.go

@@ -3,10 +3,22 @@
 
 package hash
 
+// DefaultHashAlgorithmName represents the defualt value of PASSWORD_HASH_ALGO
+// configured in app.ini.
+//
+// It is NOT the same and does NOT map to the defaultEmptyHashAlgorithmSpecification.
+//
+// It will be dealiased as per aliasAlgorithmNames whereas
+// defaultEmptyHashAlgorithmSpecification does not undergo dealiasing.
 const DefaultHashAlgorithmName = "pbkdf2"
 
 var DefaultHashAlgorithm *PasswordHashAlgorithm
 
+// aliasAlgorithNames provides a mapping between the value of PASSWORD_HASH_ALGO
+// configured in the app.ini and the parameters used within the hashers internally.
+//
+// If it is necessary to change the default parameters for any hasher in future you
+// should change these values and not those in argon2.go etc.
 var aliasAlgorithmNames = map[string]string{
 	"argon2":    "argon2$2$65536$8$50",
 	"bcrypt":    "bcrypt$10",
@@ -29,6 +41,8 @@ var RecommendedHashAlgorithms = []string{
 	"pbkdf2_hi",
 }
 
+// SetDefaultPasswordHashAlgorithm will take a provided algorithmName and dealias it to
+// a complete algorithm specification.
 func SetDefaultPasswordHashAlgorithm(algorithmName string) (string, *PasswordHashAlgorithm) {
 	if algorithmName == "" {
 		algorithmName = DefaultHashAlgorithmName
@@ -38,6 +52,9 @@ func SetDefaultPasswordHashAlgorithm(algorithmName string) (string, *PasswordHas
 		algorithmName = alias
 		alias, has = aliasAlgorithmNames[algorithmName]
 	}
+
+	// algorithmName should now be a full algorithm specification
+	// e.g. pbkdf2$50000$50 rather than pbdkf2
 	DefaultHashAlgorithm = Parse(algorithmName)
 
 	return algorithmName, DefaultHashAlgorithm

modules/auth/password/hash/setting_test.go

@@ -15,7 +15,7 @@ func TestCheckSettingPasswordHashAlgorithm(t *testing.T) {
 		pbkdf2Config, pbkdf2Algo := SetDefaultPasswordHashAlgorithm("pbkdf2")
 
 		assert.Equal(t, pbkdf2v2Config, pbkdf2Config)
-		assert.Equal(t, pbkdf2v2Algo.Name, pbkdf2Algo.Name)
+		assert.Equal(t, pbkdf2v2Algo.Specification, pbkdf2Algo.Specification)
 	})
 
 	for a, b := range aliasAlgorithmNames {
@@ -24,7 +24,7 @@ func TestCheckSettingPasswordHashAlgorithm(t *testing.T) {
 			bConfig, bAlgo := SetDefaultPasswordHashAlgorithm(b)
 
 			assert.Equal(t, bConfig, aConfig)
-			assert.Equal(t, aAlgo.Name, bAlgo.Name)
+			assert.Equal(t, aAlgo.Specification, bAlgo.Specification)
 		})
 	}
 
@@ -33,6 +33,6 @@ func TestCheckSettingPasswordHashAlgorithm(t *testing.T) {
 		pbkdf2v2Config, pbkdf2v2Algo := SetDefaultPasswordHashAlgorithm("pbkdf2_v2")
 
 		assert.Equal(t, pbkdf2v2Config, emptyConfig)
-		assert.Equal(t, pbkdf2v2Algo.Name, emptyAlgo.Name)
+		assert.Equal(t, pbkdf2v2Algo.Specification, emptyAlgo.Specification)
 	})
 }