Merge hash service into a single file#3022
Open
JeethJJ wants to merge 1 commit into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (22)
💤 Files with no reviewable changes (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (15)
📝 WalkthroughWalkthroughConsolidates hashing code from the former cryptolib/hash subpackage into top-level ChangesCryptolib API Consolidation and Consumer Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
JeethJJ
requested review from
ThaminduDilshan,
darshanasbg and
thiva-k
as code owners
JeethJJ
force-pushed
the
hash-service
branch
2 times, most recently
from
dbdc4d6 to
c4aa39a
Compare
JeethJJ
added
Type/Improvement
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/internal/entity/service.go (1)
529-536:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve full hash parameters during store/verify.
Line 532 and Line 927 only copy
Salt,Iterations, andKeySize.MemoryandParallelismare dropped, which can make Argon2 credentials fail verification.💡 Suggested fix
ref := cryptolib.Credential{ Algorithm: stored.StorageAlgo, Hash: stored.Value, Parameters: cryptolib.CredParameters{ Salt: stored.StorageAlgoParams.Salt, Iterations: stored.StorageAlgoParams.Iterations, + Memory: stored.StorageAlgoParams.Memory, + Parallelism: stored.StorageAlgoParams.Parallelism, KeySize: stored.StorageAlgoParams.KeySize, }, } @@ StorageAlgo: credHash.Algorithm, StorageAlgoParams: cryptolib.CredParameters{ Salt: credHash.Parameters.Salt, Iterations: credHash.Parameters.Iterations, + Memory: credHash.Parameters.Memory, + Parallelism: credHash.Parameters.Parallelism, KeySize: credHash.Parameters.KeySize, }, Value: credHash.Hash,Also applies to: 927-931
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/entity/service.go` around lines 529 - 536, The cryptographic parameter mapping drops Argon2-specific fields causing verification failures: when building cryptolib.Credential/CredParameters (the code that sets ref := cryptolib.Credential and the other occurrence around the stored.StorageAlgoParams usage), include all fields from stored.StorageAlgoParams — add Memory and Parallelism into the CredParameters construction in the same places where Salt, Iterations, and KeySize are set so Argon2 parameters are preserved for store/verify.backend/internal/user/declarative_resource.go (1)
453-455:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep Argon2 pre-hashed params intact when parsing YAML.
Line 453-Line 455 only read
iterations,keySize, andsalt.memoryandparallelismare dropped for pre-hashed credentials.💡 Suggested fix
iterations, _ := paramsMap["iterations"].(int) + memory, _ := paramsMap["memory"].(int) + parallelism, _ := paramsMap["parallelism"].(int) keySize, _ := paramsMap["keySize"].(int) salt, _ := paramsMap["salt"].(string) return Credential{ StorageType: storageType, StorageAlgo: cryptolib.CredAlgorithm(storageAlgo), StorageAlgoParams: cryptolib.CredParameters{ Iterations: iterations, + Memory: memory, + Parallelism: parallelism, KeySize: keySize, Salt: salt, }, Value: value, }, nilAlso applies to: 460-464
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/user/declarative_resource.go` around lines 453 - 455, The YAML parsing currently extracts only iterations, keySize, and salt from paramsMap (variables named iterations, keySize, salt), which drops Argon2's memory and parallelism fields; update the parsing to also read and preserve memory and parallelism from paramsMap (e.g., memory, parallelism) with the same type assertions used for the other fields, and make the same change in the other parsing block referenced (the block at lines ~460-464) so pre-hashed Argon2 params remain intact when unmarshalling.
🧹 Nitpick comments (2)
backend/internal/system/cryptolib/hash_test.go (2)
1164-1175: ⚡ Quick winRemove the unused runtime-config bootstrap from this test.
Initializeonly uses theHashConfigpassed in, so this setup does not affect the assertion and just adds global-state coupling to a unit test.Proposed cleanup
func (suite *InitTestSuite) TestInitialize() { - testConfig := &config.Config{ - Crypto: config.CryptoConfig{ - PasswordHashing: config.PasswordHashingConfig{ - Algorithm: string(SHA256), - SHA256: config.SHA256Config{ - SaltSize: 32, - }, - }, - }, - } - config.ResetServerRuntime() - _ = config.InitializeServerRuntime("/test/thunderid/home", testConfig) - service, err := Initialize(HashConfig{ Algorithm: SHA256, SaltSize: 32, })As per coding guidelines,
**/*.{go,js,ts,tsx}: Delete dead code cleanly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/system/cryptolib/hash_test.go` around lines 1164 - 1175, The test includes an unnecessary global runtime bootstrap: remove the calls to config.ResetServerRuntime() and config.InitializeServerRuntime("/test/thunderid/home", testConfig) (and any related runtime setup) because Initialize only uses the local HashConfig passed via testConfig; keep the testConfig definition and use it directly with the hashing Initialize function, eliminating the global-state coupling introduced by ResetServerRuntime and InitializeServerRuntime.
1247-1290: ⚡ Quick winAssert algorithm-specific outputs here.
Hashreturning the wrong algorithm for one or more enums would still pass these tests, andGetHashonly proves that some hash was returned. Please check known digests or at least the expected output sizes (32,48,64) so the new mapping logic is actually covered.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/system/cryptolib/hash_test.go` around lines 1247 - 1290, The tests currently only check that Hash and GetHash return non-empty values; update TestHash and TestGetHash to assert algorithm-specific outputs by verifying the returned digest lengths and at least one known digest: for Hash(data, GenericSHA256) assert len(hashed) == 32 (and optionally compare to the known SHA-256 digest of "hello world"), for GenericSHA384 assert len == 48, and for GenericSHA512 assert len == 64; likewise in TestGetHash, after obtaining h from GetHash(tc.alg) either compute h.Sum(nil) or use h to hash known input and assert output sizes (32/48/64) and optionally compare one known digest to ensure the mapping logic in Hash, GetHash, and HashAlgorithm (including GenericSHA256/GenericSHA384/GenericSHA512) is correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/internal/system/cryptolib/hash.go`:
- Around line 335-343: The GenerateThumbprint and GenerateThumbprintFromString
helpers currently use base64.StdEncoding which produces '+' '/' and '=' padding;
change them to use base64.RawURLEncoding (or base64.URLEncoding without padding)
so the SHA-256 thumbprint is encoded as base64url with no padding (e.g., replace
base64.StdEncoding.EncodeToString(h[:]) with
base64.RawURLEncoding.EncodeToString(h[:]) and ensure
GenerateThumbprintFromString still delegates to GenerateThumbprint).
---
Outside diff comments:
In `@backend/internal/entity/service.go`:
- Around line 529-536: The cryptographic parameter mapping drops Argon2-specific
fields causing verification failures: when building
cryptolib.Credential/CredParameters (the code that sets ref :=
cryptolib.Credential and the other occurrence around the
stored.StorageAlgoParams usage), include all fields from
stored.StorageAlgoParams — add Memory and Parallelism into the CredParameters
construction in the same places where Salt, Iterations, and KeySize are set so
Argon2 parameters are preserved for store/verify.
In `@backend/internal/user/declarative_resource.go`:
- Around line 453-455: The YAML parsing currently extracts only iterations,
keySize, and salt from paramsMap (variables named iterations, keySize, salt),
which drops Argon2's memory and parallelism fields; update the parsing to also
read and preserve memory and parallelism from paramsMap (e.g., memory,
parallelism) with the same type assertions used for the other fields, and make
the same change in the other parsing block referenced (the block at lines
~460-464) so pre-hashed Argon2 params remain intact when unmarshalling.
---
Nitpick comments:
In `@backend/internal/system/cryptolib/hash_test.go`:
- Around line 1164-1175: The test includes an unnecessary global runtime
bootstrap: remove the calls to config.ResetServerRuntime() and
config.InitializeServerRuntime("/test/thunderid/home", testConfig) (and any
related runtime setup) because Initialize only uses the local HashConfig passed
via testConfig; keep the testConfig definition and use it directly with the
hashing Initialize function, eliminating the global-state coupling introduced by
ResetServerRuntime and InitializeServerRuntime.
- Around line 1247-1290: The tests currently only check that Hash and GetHash
return non-empty values; update TestHash and TestGetHash to assert
algorithm-specific outputs by verifying the returned digest lengths and at least
one known digest: for Hash(data, GenericSHA256) assert len(hashed) == 32 (and
optionally compare to the known SHA-256 digest of "hello world"), for
GenericSHA384 assert len == 48, and for GenericSHA512 assert len == 64; likewise
in TestGetHash, after obtaining h from GetHash(tc.alg) either compute h.Sum(nil)
or use h to hash known input and assert output sizes (32/48/64) and optionally
compare one known digest to ensure the mapping logic in Hash, GetHash, and
HashAlgorithm (including GenericSHA256/GenericSHA384/GenericSHA512) is correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 858f69e1-0819-4b5e-8908-cd88b65bc7d8
⛔ Files ignored due to path filters (1)
backend/tests/mocks/crypto/hashmock/HashServiceInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (22)
backend/.mockery.public.ymlbackend/cmd/server/servicemanager.gobackend/internal/entity/declarative_resource_test.gobackend/internal/entity/init.gobackend/internal/entity/model.gobackend/internal/entity/service.gobackend/internal/entity/service_test.gobackend/internal/notification/otp_service.gobackend/internal/notification/otp_service_test.gobackend/internal/oauth/jwks/service.gobackend/internal/system/cryptolib/hash.gobackend/internal/system/cryptolib/hash/init.gobackend/internal/system/cryptolib/hash/init_test.gobackend/internal/system/cryptolib/hash/model.gobackend/internal/system/cryptolib/hash/utils.gobackend/internal/system/cryptolib/hash/utils_test.gobackend/internal/system/cryptolib/hash_test.gobackend/internal/system/kmprovider/defaultkm/config_crypto_provider.gobackend/internal/system/kmprovider/defaultkm/pki/service.gobackend/internal/user/declarative_resource.gobackend/internal/user/declarative_resource_test.gobackend/internal/user/model.go
💤 Files with no reviewable changes (5)
- backend/internal/system/cryptolib/hash/init.go
- backend/internal/system/cryptolib/hash/model.go
- backend/internal/system/cryptolib/hash/utils.go
- backend/internal/system/cryptolib/hash/init_test.go
- backend/internal/system/cryptolib/hash/utils_test.go
Comment on lines
+335
to
+343
| // GenerateThumbprint generates a SHA-256 thumbprint for the given data. | ||
| func GenerateThumbprint(data []byte) string { | ||
| h := sha256.Sum256(data) | ||
| return base64.StdEncoding.EncodeToString(h[:]) | ||
| } | ||
|
|
||
| // GenerateThumbprintFromString generates a SHA-256 thumbprint for the given string data. | ||
| func GenerateThumbprintFromString(data string) string { | ||
| return GenerateThumbprint([]byte(data)) |
Contributor
There was a problem hiding this comment.
Use base64url, not standard Base64, for thumbprints.
StdEncoding emits +, /, and = padding, but the downstream JWKS x5t#S256 path needs base64url without padding. As written, this helper will generate non-compliant thumbprints for that contract.
Proposed fix
func GenerateThumbprint(data []byte) string {
h := sha256.Sum256(data)
- return base64.StdEncoding.EncodeToString(h[:])
+ return base64.RawURLEncoding.EncodeToString(h[:])
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // GenerateThumbprint generates a SHA-256 thumbprint for the given data. | |
| func GenerateThumbprint(data []byte) string { | |
| h := sha256.Sum256(data) | |
| return base64.StdEncoding.EncodeToString(h[:]) | |
| } | |
| // GenerateThumbprintFromString generates a SHA-256 thumbprint for the given string data. | |
| func GenerateThumbprintFromString(data string) string { | |
| return GenerateThumbprint([]byte(data)) | |
| // GenerateThumbprint generates a SHA-256 thumbprint for the given data. | |
| func GenerateThumbprint(data []byte) string { | |
| h := sha256.Sum256(data) | |
| return base64.RawURLEncoding.EncodeToString(h[:]) | |
| } | |
| // GenerateThumbprintFromString generates a SHA-256 thumbprint for the given string data. | |
| func GenerateThumbprintFromString(data string) string { | |
| return GenerateThumbprint([]byte(data)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/internal/system/cryptolib/hash.go` around lines 335 - 343, The
GenerateThumbprint and GenerateThumbprintFromString helpers currently use
base64.StdEncoding which produces '+' '/' and '=' padding; change them to use
base64.RawURLEncoding (or base64.URLEncoding without padding) so the SHA-256
thumbprint is encoded as base64url with no padding (e.g., replace
base64.StdEncoding.EncodeToString(h[:]) with
base64.RawURLEncoding.EncodeToString(h[:]) and ensure
GenerateThumbprintFromString still delegates to GenerateThumbprint).
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
rajithacharith
approved these changes
May 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
The hash service is currently consist of unnecessary utils and models which can be combined into a single PR. Here we have merged all into a single hash.go and hash_test.go file.
Summary by CodeRabbit
Refactor
Tests