Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds support for the Argon2id password hashing algorithm as an alternative to the existing SHA256 and PBKDF2 implementations. Argon2id is a modern, memory-hard key derivation function that provides stronger security against brute-force attacks.
Changes:
- Adds Argon2id algorithm support to the password hashing service
- Extends configuration model to include Argon2id-specific parameters (memory, parallelism)
- Changes default password hashing algorithm from PBKDF2 to ARGON2ID in default configuration
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| backend/internal/system/crypto/hash/service.go | Implements argon2idHashProvider struct with Generate and Verify methods, adds default constants for Argon2id parameters |
| backend/internal/system/crypto/hash/model.go | Adds ARGON2ID constant and extends CredParameters struct with Memory and Parallelism fields |
| backend/internal/system/config/config.go | Extends PasswordHashingParamsConfig with Memory and Parallelism configuration fields |
| backend/cmd/server/repository/resources/conf/default.json | Changes default algorithm to ARGON2ID and adds Argon2id-specific parameter values |
| func newArgon2idProvider(saltSize, memory, iterations, parallelism, keySize int) *argon2idHashProvider { | ||
| if saltSize <= 0 { | ||
| saltSize = defaultSaltSize | ||
| } | ||
| if memory <= 0 { | ||
| memory = defaultArgon2idMemory | ||
| } | ||
| if iterations <= 0 { | ||
| iterations = defaultArgon2idIterations | ||
| } | ||
| if parallelism <= 0 { | ||
| parallelism = defaultArgon2idParallelism | ||
| } | ||
| if keySize <= 0 { | ||
| keySize = defaultArgon2idKeySize | ||
| } | ||
| return &argon2idHashProvider{ | ||
| SaltSize: saltSize, | ||
| Memory: memory, | ||
| Iterations: iterations, | ||
| Parallelism: parallelism, | ||
| KeySize: keySize, | ||
| } | ||
| } | ||
|
|
||
| // Generate Argon2idCredential generates an Argon2id hash of the input data using the provided salt. | ||
| func (a *argon2idHashProvider) Generate(credentialValue []byte) (Credential, error) { | ||
| credSalt, err := generateSalt(a.SaltSize) | ||
| if err != nil { | ||
| return Credential{}, err | ||
| } | ||
|
|
||
| hash := argon2.IDKey(credentialValue, credSalt, uint32(a.Iterations), uint32(a.Memory), uint8(a.Parallelism), uint32(a.KeySize)) | ||
|
|
||
| return Credential{ | ||
| Algorithm: ARGON2ID, | ||
| Hash: hex.EncodeToString(hash), | ||
| Parameters: CredParameters{ | ||
| Memory: a.Memory, | ||
| Iterations: a.Iterations, | ||
| Parallelism: a.Parallelism, | ||
| KeySize: a.KeySize, | ||
| Salt: hex.EncodeToString(credSalt), | ||
| }, | ||
| }, nil | ||
| } | ||
|
|
||
| // Verify Argon2idCredential checks if the Argon2id hash of the input data and salt matches the expected hash. | ||
| func (a *argon2idHashProvider) Verify(credentialValueToVerify []byte, referenceCredential Credential) (bool, error) { | ||
| memory := referenceCredential.Parameters.Memory | ||
| if memory <= 0 { | ||
| memory = defaultArgon2idMemory | ||
| } | ||
| iterations := referenceCredential.Parameters.Iterations | ||
| if iterations <= 0 { | ||
| iterations = defaultArgon2idIterations | ||
| } | ||
| parallelism := referenceCredential.Parameters.Parallelism | ||
| if parallelism <= 0 { | ||
| parallelism = defaultArgon2idParallelism | ||
| } | ||
| keySize := referenceCredential.Parameters.KeySize | ||
| if keySize <= 0 { | ||
| keySize = defaultArgon2idKeySize | ||
| } | ||
| saltBytes, err := hex.DecodeString(referenceCredential.Parameters.Salt) | ||
| if err != nil { | ||
| logger.Error("Error decoding salt: %v", log.Error(err)) | ||
| return false, err | ||
| } | ||
| hash := argon2.IDKey(credentialValueToVerify, saltBytes, uint32(iterations), uint32(memory), uint8(parallelism), uint32(keySize)) | ||
| return hex.EncodeToString(hash) == referenceCredential.Hash, nil | ||
| } |
There was a problem hiding this comment.
The new Argon2id password hashing implementation lacks test coverage. The existing test file has comprehensive tests for SHA256 and PBKDF2 algorithms (including generation, verification, custom parameters, and failure cases), but no tests have been added for the new ARGON2ID algorithm. You should add similar test coverage for Argon2id to maintain consistency with the project's testing standards and ensure at least 80% coverage as per the coding guidelines.
| }, | ||
| "password_hashing": { | ||
| "algorithm": "PBKDF2", | ||
| "algorithm": "ARGON2ID", |
There was a problem hiding this comment.
Changing the default algorithm from PBKDF2 to ARGON2ID in the default configuration is a breaking change that could affect existing deployments. Users who upgrade will suddenly have their password hashing algorithm changed, which could cause authentication issues if not properly documented and communicated. Consider keeping PBKDF2 as the default for backward compatibility, or clearly document this as a breaking change in the PR description and migration guide.
| "algorithm": "ARGON2ID", | |
| "algorithm": "PBKDF2", |
| } | ||
| } | ||
|
|
||
| // Generate Argon2idCredential generates an Argon2id hash of the input data using the provided salt. |
There was a problem hiding this comment.
The comment says "generates an Argon2id hash of the input data using the provided salt" but this is misleading - the function actually generates a new random salt internally (line 226), it doesn't use a "provided salt". The comment should be updated to accurately reflect that the function generates both the salt and the hash.
| // Generate Argon2idCredential generates an Argon2id hash of the input data using the provided salt. | |
| // Generate Argon2idCredential generates a new random salt and derives an Argon2id hash of the input data, | |
| // returning both the hash and the salt in the resulting Credential. |
| return Credential{}, err | ||
| } | ||
|
|
||
| hash := argon2.IDKey(credentialValue, credSalt, uint32(a.Iterations), uint32(a.Memory), uint8(a.Parallelism), uint32(a.KeySize)) |
There was a problem hiding this comment.
The parameters to argon2.IDKey are in the incorrect order. According to the golang.org/x/crypto/argon2 documentation, the function signature is: IDKey(password, salt []byte, time, memory uint32, threads uint8, keyLen uint32). The third parameter should be 'time' (iterations) but you're passing 'Memory', and the fourth parameter should be 'memory' but you're passing 'Iterations'.
The correct order should be: argon2.IDKey(credentialValue, credSalt, uint32(a.Iterations), uint32(a.Memory), uint8(a.Parallelism), uint32(a.KeySize))
This bug will cause passwords to be hashed with incorrect parameters, making password verification fail or use weaker security settings than intended.
There was a problem hiding this comment.
Suggestion and the existing code are same
| logger.Error("Error decoding salt: %v", log.Error(err)) | ||
| return false, err | ||
| } | ||
| hash := argon2.IDKey(credentialValueToVerify, saltBytes, uint32(iterations), uint32(memory), uint8(parallelism), uint32(keySize)) |
There was a problem hiding this comment.
The parameters to argon2.IDKey are in the incorrect order. The function signature is: IDKey(password, salt []byte, time, memory uint32, threads uint8, keyLen uint32). The third parameter should be 'time' (iterations) but you're passing 'Memory', and the fourth parameter should be 'memory' but you're passing 'Iterations'.
The correct order should be: argon2.IDKey(credentialValueToVerify, saltBytes, uint32(iterations), uint32(memory), uint8(parallelism), uint32(keySize))
This will cause password verification to fail even for correctly hashed passwords.
There was a problem hiding this comment.
Suggestion and the existing code are same
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (74.66%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1130 +/- ##
==========================================
+ Coverage 87.46% 89.41% +1.94%
==========================================
Files 543 555 +12
Lines 37092 37590 +498
Branches 1612 1636 +24
==========================================
+ Hits 32443 33611 +1168
+ Misses 3000 2330 -670
Partials 1649 1649
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ThaminduDilshan
removed
the
skip-changelog
2 participants
Purpose
Approach
Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks