Moved RuntimeCryptoProvider to thunderidengine#3836
Open
anushasunkada wants to merge 1 commit into
Open
Conversation
anushasunkada
requested review from
ThaminduDilshan,
hwupathum and
thiva-k
as code owners
Contributor
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (61)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (32)
📝 WalkthroughWalkthroughThe PR moves runtime cryptography contracts into ChangesRuntime crypto provider migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/internal/system/cryptolib/model.go (1)
107-114: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd AES-GCM Key Wrap to the param-map round trip
AlgorithmParams.ToParamsMapdropsA128GCMKW/A192GCMKW/A256GCMKW, andAlgorithmParamsFromMaphas no matching case. ThroughRuntimeCryptoProvider, that losesAESGCMKW.IV/Tagon encrypt and rejects AES-GCM Key Wrap on decrypt.🤖 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/model.go` around lines 107 - 114, AlgorithmParams.ToParamsMap is omitting AES-GCM Key Wrap values, and AlgorithmParamsFromMap has no branch to restore them. Update the AlgorithmParams round-trip so the AESGCMKW case is handled alongside RSAOAEP256, RSAOAEP, ECDHES, and AESKW, preserving AESGCMKW.IV and AESGCMKW.Tag for A128GCMKW, A192GCMKW, and A256GCMKW in RuntimeCryptoProvider.
🧹 Nitpick comments (2)
backend/internal/system/jose/jwe/service_test.go (1)
66-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate decrypt-mock closure across three tests.
The same
RunAndReturnclosure (convertalgorithm/paramsMapviaAlgorithmParamsFromMap, thencryptolib.Decrypt) is repeated in the RSA, ECDH, and CBC tests, differing only in which private key is used.♻️ Proposed helper to consolidate duplicated mock closures
+func mockDecryptFn(privKey crypto.PrivateKey) func( + ctx context.Context, keyRef *providers.KeyRef, algorithm string, + paramsMap map[string]interface{}, content []byte, +) ([]byte, error) { + return func( + ctx context.Context, keyRef *providers.KeyRef, algorithm string, + paramsMap map[string]interface{}, content []byte, + ) ([]byte, error) { + params, err := cryptolib.AlgorithmParamsFromMap(algorithm, paramsMap) + if err != nil { + return nil, err + } + return cryptolib.Decrypt(privKey, params, content) + } +}Then replace each inline closure with
mockDecryptFn(suite.testRSAPrivateKey)/mockDecryptFn(suite.testECPrivateKey).Also applies to: 116-126, 406-416
🤖 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/jose/jwe/service_test.go` around lines 66 - 76, The same Decrypt mock closure is duplicated in the RSA, ECDH, and CBC tests in service_test.go. Extract the shared RunAndReturn logic into a reusable helper such as mockDecryptFn that takes the private key and handles AlgorithmParamsFromMap plus cryptolib.Decrypt, then replace each inline closure in the affected test cases with that helper using the appropriate key (for example, suite.testRSAPrivateKey or suite.testECPrivateKey).backend/internal/system/cmodels/property.go (1)
78-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid new callers of the deprecated crypto accessor
configkm.GetConfigCryptoService()is marked deprecated, butProperty.GetValueandProperty.Encryptnow depend on it. Use a non-deprecated init/injection path here, or drop the deprecation if this is meant to remain the supported API.🤖 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/cmodels/property.go` at line 78, Property.GetValue and Property.Encrypt are now calling the deprecated configkm.GetConfigCryptoService accessor; update cmodels.Property to use a non-deprecated crypto initialization/injection path instead of creating a new caller here. Locate the crypto setup in Property.GetValue/Property.Encrypt and refactor it to receive or reuse the cryptoProvider from the supported initialization flow, or remove the deprecation only if GetConfigCryptoService is still intended to be the public API.
🤖 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.
Outside diff comments:
In `@backend/internal/system/cryptolib/model.go`:
- Around line 107-114: AlgorithmParams.ToParamsMap is omitting AES-GCM Key Wrap
values, and AlgorithmParamsFromMap has no branch to restore them. Update the
AlgorithmParams round-trip so the AESGCMKW case is handled alongside RSAOAEP256,
RSAOAEP, ECDHES, and AESKW, preserving AESGCMKW.IV and AESGCMKW.Tag for
A128GCMKW, A192GCMKW, and A256GCMKW in RuntimeCryptoProvider.
---
Nitpick comments:
In `@backend/internal/system/cmodels/property.go`:
- Line 78: Property.GetValue and Property.Encrypt are now calling the deprecated
configkm.GetConfigCryptoService accessor; update cmodels.Property to use a
non-deprecated crypto initialization/injection path instead of creating a new
caller here. Locate the crypto setup in Property.GetValue/Property.Encrypt and
refactor it to receive or reuse the cryptoProvider from the supported
initialization flow, or remove the deprecation only if GetConfigCryptoService is
still intended to be the public API.
In `@backend/internal/system/jose/jwe/service_test.go`:
- Around line 66-76: The same Decrypt mock closure is duplicated in the RSA,
ECDH, and CBC tests in service_test.go. Extract the shared RunAndReturn logic
into a reusable helper such as mockDecryptFn that takes the private key and
handles AlgorithmParamsFromMap plus cryptolib.Decrypt, then replace each inline
closure in the affected test cases with that helper using the appropriate key
(for example, suite.testRSAPrivateKey or suite.testECPrivateKey).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b7ade0f-e8cd-4470-b3d5-697c2dc00389
⛔ Files ignored due to path filters (2)
backend/tests/mocks/crypto/cryptomock/RuntimeCryptoProvider_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/crypto/cryptomock/TLSMaterialProvider_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (40)
backend/cmd/server/main.gobackend/internal/authn/openid4vp/init.gobackend/internal/authn/openid4vp/service.gobackend/internal/authn/openid4vp/service_test.gobackend/internal/flow/flowexec/init.gobackend/internal/flow/flowexec/service.gobackend/internal/flow/flowexec/service_test.gobackend/internal/oauth/init.gobackend/internal/oauth/jwks/init.gobackend/internal/oauth/jwks/init_test.gobackend/internal/oauth/jwks/service.gobackend/internal/oauth/jwks/service_test.gobackend/internal/oauth/oauth2/discovery/discovery_test.gobackend/internal/oauth/oauth2/discovery/init.gobackend/internal/oauth/oauth2/discovery/service.gobackend/internal/openid4vci/init.gobackend/internal/openid4vci/service.gobackend/internal/openid4vci/service_test.gobackend/internal/system/cmodels/property.gobackend/internal/system/cryptolib/model.gobackend/internal/system/jose/init.gobackend/internal/system/jose/init_test.gobackend/internal/system/jose/jwe/init.gobackend/internal/system/jose/jwe/service.gobackend/internal/system/jose/jwe/service_test.gobackend/internal/system/jose/jwt/init.gobackend/internal/system/jose/jwt/init_test.gobackend/internal/system/jose/jwt/service.gobackend/internal/system/jose/jwt/service_test.gobackend/internal/system/kmprovider/common/interface.gobackend/internal/system/kmprovider/configkm/config_crypto_provider.gobackend/internal/system/kmprovider/configkm/config_crypto_provider_test.gobackend/internal/system/kmprovider/configkm/init.gobackend/internal/system/kmprovider/configkm/model.gobackend/internal/system/kmprovider/defaultkm/init.gobackend/internal/system/kmprovider/defaultkm/runtime_crypto_provider.gobackend/internal/system/kmprovider/defaultkm/runtime_crypto_provider_test.gobackend/internal/system/kmprovider/init.gobackend/pkg/thunderidengine/providers/interface.gobackend/pkg/thunderidengine/providers/model.go
senthalan
reviewed
Jul 8, 2026
| @@ -16,7 +16,7 @@ | |||
| * under the License. | |||
| */ | |||
|
|
|||
| package defaultkm | |||
Member
There was a problem hiding this comment.
Why we need to bring the config crypto files to a different package? what is the need for this
Contributor
Author
There was a problem hiding this comment.
it was necessary to avoid a cyclic import.
- defaultkm imports pkg/thunderidengine/providers
- pkg/thunderidengine/providers imports internal/system/cmodels (for the Property/PropertyDTO type)
- internal/system/cmodels imports kmprovider/defaultkm — to encrypt secret property values via ConfigCryptoProvider.
another way is move Property/PropertyDTO to providers. and this will be a bigger change list
Contributor
Author
There was a problem hiding this comment.
Renamed defaultkm to runtimekm as discussed.
Comment on lines
+170
to
+171
| algName, paramsMap := params.ToParamsMap() | ||
| cek, err := js.cryptoProvider.Decrypt(ctx, &js.keyRef, algName, paramsMap, encryptedKey) |
Member
There was a problem hiding this comment.
Here we are using
params.ToParamsMap() to converts AlgorithmParams into the algorithm string and generic params map and but the buildDecryptParams[1] function is validating whether the provided algorithm is supported cryptolib (which is the default crypto providers' lib). So we can't be able to plug any other crypto provider which supports additional algorithms. So ideally the algorithm and other paramMap should be passed to the js.cryptoProvider.Decrypt() function and the buildDecryptParams should be moved to the default crypto provider. Then only we can achieve the level of customisation we need
Contributor
Author
There was a problem hiding this comment.
You are right @senthalan, we have to discuss change in JWE service. Both Encrypt and Decrypt method will need change. just moving buildDecryptParams will not serve the purpose.
Encrypt never touches cryptoProvider at all — it calls cryptolib.Encrypt directly. Separately, encryptContent/decryptContent (AES-GCM/CBC-HMAC in utils.go) never go through the provider either.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/internal/system/kmprovider/configkm/init_test.go (1)
52-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting the specific error message for consistency.
All other error test cases use
assert.EqualErrorto verify the exact error message, butTestInitConfigProvider_InvalidHexuses onlyassert.Error. If the hex decode error message is stable, asserting it would improve diagnostic value when the test fails. If the message is intentionally left unasserted to avoid coupling toencoding/hexinternals, a brief comment explaining that choice would help future readers.🤖 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/kmprovider/configkm/init_test.go` around lines 52 - 56, In TestInitConfigProvider_InvalidHex, the error is only checked with assert.Error while the other InitConfigProvider tests verify exact messages. Update this test to either use assert.EqualError on the InitConfigProvider return error, or add a short comment explaining why the exact hex decode message is intentionally not asserted; use the TestInitConfigProvider_InvalidHex and InitConfigProvider symbols to locate the case.
🤖 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.
Nitpick comments:
In `@backend/internal/system/kmprovider/configkm/init_test.go`:
- Around line 52-56: In TestInitConfigProvider_InvalidHex, the error is only
checked with assert.Error while the other InitConfigProvider tests verify exact
messages. Update this test to either use assert.EqualError on the
InitConfigProvider return error, or add a short comment explaining why the exact
hex decode message is intentionally not asserted; use the
TestInitConfigProvider_InvalidHex and InitConfigProvider symbols to locate the
case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 80d87580-202a-439a-b598-59327d3257d3
⛔ Files ignored due to path filters (2)
backend/tests/mocks/crypto/cryptomock/RuntimeCryptoProvider_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/crypto/cryptomock/TLSMaterialProvider_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (42)
backend/cmd/server/main.gobackend/internal/authn/openid4vp/init.gobackend/internal/authn/openid4vp/service.gobackend/internal/authn/openid4vp/service_test.gobackend/internal/flow/flowexec/init.gobackend/internal/flow/flowexec/service.gobackend/internal/flow/flowexec/service_test.gobackend/internal/oauth/init.gobackend/internal/oauth/jwks/init.gobackend/internal/oauth/jwks/init_test.gobackend/internal/oauth/jwks/service.gobackend/internal/oauth/jwks/service_test.gobackend/internal/oauth/oauth2/discovery/discovery_test.gobackend/internal/oauth/oauth2/discovery/init.gobackend/internal/oauth/oauth2/discovery/service.gobackend/internal/openid4vci/init.gobackend/internal/openid4vci/service.gobackend/internal/openid4vci/service_test.gobackend/internal/system/cmodels/property.gobackend/internal/system/cryptolib/model.gobackend/internal/system/cryptolib/model_test.gobackend/internal/system/jose/init.gobackend/internal/system/jose/init_test.gobackend/internal/system/jose/jwe/init.gobackend/internal/system/jose/jwe/service.gobackend/internal/system/jose/jwe/service_test.gobackend/internal/system/jose/jwt/init.gobackend/internal/system/jose/jwt/init_test.gobackend/internal/system/jose/jwt/service.gobackend/internal/system/jose/jwt/service_test.gobackend/internal/system/kmprovider/common/interface.gobackend/internal/system/kmprovider/configkm/config_crypto_provider.gobackend/internal/system/kmprovider/configkm/config_crypto_provider_test.gobackend/internal/system/kmprovider/configkm/init.gobackend/internal/system/kmprovider/configkm/init_test.gobackend/internal/system/kmprovider/configkm/model.gobackend/internal/system/kmprovider/defaultkm/init.gobackend/internal/system/kmprovider/defaultkm/runtime_crypto_provider.gobackend/internal/system/kmprovider/defaultkm/runtime_crypto_provider_test.gobackend/internal/system/kmprovider/init.gobackend/pkg/thunderidengine/providers/interface.gobackend/pkg/thunderidengine/providers/model.go
✅ Files skipped from review due to trivial changes (2)
- backend/internal/system/kmprovider/configkm/config_crypto_provider.go
- backend/internal/system/kmprovider/configkm/config_crypto_provider_test.go
🚧 Files skipped from review as they are similar to previous changes (39)
- backend/internal/system/jose/init.go
- backend/internal/system/jose/jwt/init.go
- backend/internal/system/kmprovider/configkm/model.go
- backend/internal/openid4vci/init.go
- backend/internal/system/kmprovider/init.go
- backend/internal/system/jose/jwe/init.go
- backend/internal/system/cmodels/property.go
- backend/internal/oauth/oauth2/discovery/init.go
- backend/internal/oauth/jwks/service.go
- backend/internal/oauth/jwks/init.go
- backend/pkg/thunderidengine/providers/model.go
- backend/cmd/server/main.go
- backend/internal/flow/flowexec/init.go
- backend/internal/system/kmprovider/defaultkm/init.go
- backend/internal/oauth/oauth2/discovery/service.go
- backend/internal/system/jose/jwe/service.go
- backend/internal/system/jose/jwt/service.go
- backend/pkg/thunderidengine/providers/interface.go
- backend/internal/authn/openid4vp/service_test.go
- backend/internal/system/jose/init_test.go
- backend/internal/system/kmprovider/configkm/init.go
- backend/internal/flow/flowexec/service.go
- backend/internal/oauth/jwks/init_test.go
- backend/internal/system/jose/jwt/init_test.go
- backend/internal/openid4vci/service.go
- backend/internal/oauth/init.go
- backend/internal/openid4vci/service_test.go
- backend/internal/system/jose/jwe/service_test.go
- backend/internal/authn/openid4vp/service.go
- backend/internal/system/cryptolib/model_test.go
- backend/internal/system/kmprovider/common/interface.go
- backend/internal/oauth/oauth2/discovery/discovery_test.go
- backend/internal/flow/flowexec/service_test.go
- backend/internal/oauth/jwks/service_test.go
- backend/internal/system/jose/jwt/service_test.go
- backend/internal/authn/openid4vp/init.go
- backend/internal/system/cryptolib/model.go
- backend/internal/system/kmprovider/defaultkm/runtime_crypto_provider.go
- backend/internal/system/kmprovider/defaultkm/runtime_crypto_provider_test.go
Signed-off-by: anushasunkada <anushasunkada@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/internal/system/jose/jwt/service.go (1)
309-334: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPreserve the request context during public-key verification.
DPoP starts with a request context, but this API cannot receive it and invokes the provider with
context.Background(). Custom providers performing remote operations therefore cannot honor request cancellation or deadlines.Proposed context propagation
-VerifyJWTSignatureWithPublicKey(jwtToken string, +VerifyJWTSignatureWithPublicKey(ctx context.Context, jwtToken string, jwtPublicKey crypto.PublicKey) *tidcommon.ServiceError { @@ - err = js.cryptoProvider.Verify(context.Background(), providers.KeyRef{PublicKey: jwtPublicKey}, + err = js.cryptoProvider.Verify(ctx, providers.KeyRef{PublicKey: jwtPublicKey}, algStr, []byte(signingInput), signature)Update the interface, DPoP caller, and mocks accordingly.
🤖 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/jose/jwt/service.go` around lines 309 - 334, The public-key verification path in jwtService.VerifyJWTSignatureWithPublicKey is dropping the caller’s request context by using context.Background(), so remote crypto providers can’t respect cancellation or deadlines. Update the verification flow to accept and thread through a context from the DPoP caller into VerifyJWTSignatureWithPublicKey, pass that context into js.cryptoProvider.Verify, and adjust the related interface plus any mocks/tests that reference VerifyJWTSignatureWithPublicKey or cryptoProvider.Verify accordingly.
🧹 Nitpick comments (1)
backend/pkg/thunderidengine/providers/constants.go (1)
496-503: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale
"kmprovider:"prefix on package-level errors now owned byproviders.These sentinels were moved into the generic
thunderidengine/providerspackage (used by any embedder-suppliedRuntimeCryptoProvider, not justkmprovider), but the error text still says"kmprovider: ...", which will mislabel errors surfaced by non-kmprovider implementations.♻️ Proposed fix
-var ErrKeyNotFound = errors.New("kmprovider: no key found matching the requested identifier") +var ErrKeyNotFound = errors.New("providers: no key found matching the requested identifier") -var ErrUnsupportedAlgorithm = errors.New("kmprovider: unsupported signature algorithm") +var ErrUnsupportedAlgorithm = errors.New("providers: unsupported signature algorithm")🤖 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/pkg/thunderidengine/providers/constants.go` around lines 496 - 503, The package-level sentinels ErrKeyNotFound and ErrUnsupportedAlgorithm still hardcode the stale “kmprovider:” prefix even though they now live in the generic providers package. Update the error text in constants.go to use package-agnostic wording that fits any RuntimeCryptoProvider implementation, and keep the sentinel names unchanged so callers can still match them reliably.
🤖 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/oauth/oauth2/tokenservice/builder.go`:
- Around line 138-143: The token claim merge order in build/claims assembly is
wrong: subject attributes can overwrite authoritative protocol claims like
scope, client_id, and grant_type. Update the logic in the token builder around
ctx.SubjectAttributes so those attributes are merged first, then set the
protocol/system claims afterward in the same claim-building path, ensuring the
named claims always win over any colliding subject keys.
In `@backend/internal/system/kmprovider/runtimekm/runtime_crypto_provider.go`:
- Around line 210-220: Update runtimeCryptoService.Verify so it only requires
s.pkiService when it actually needs to resolve a key via KeyID, matching the
behavior of getRSAPublicKey and getECPublicKey. If keyRef.PublicKey is already
present, allow verification to proceed without initializing PKI service; only
fail with "PKI service not initialized" on the key-resolution path. Use Verify,
getRSAPublicKey, and getECPublicKey as the reference points for the control-flow
change.
- Around line 248-252: `runtimeCryptoService.IsSupportedEncAlgorithm` is too
permissive because it delegates to `cryptolib.EncryptionAlgorithmFor`, which
reports support for key-wrap algorithms that `Encrypt` and `Decrypt` do not
actually handle. Narrow the check in `IsSupportedEncAlgorithm` to only return
true for the algorithms that `runtimeCryptoService.Encrypt` and
`runtimeCryptoService.Decrypt` explicitly support, so callers do not get a false
positive and fail later with unsupported algorithm errors.
In `@backend/pkg/thunderidengine/engine.go`:
- Line 46: `Engine.New` always initializes the bundled PKI path, which blocks
embedders from supplying their own crypto implementation. Add a
`WithRuntimeCryptoProvider` option on the engine constructor path and thread
that value through `New` so it is used when present, falling back to the
existing `pki`-based setup only when no provider is supplied. Update the
`Engine` construction logic and any related option wiring so the runtime crypto
provider can be injected by callers.
---
Outside diff comments:
In `@backend/internal/system/jose/jwt/service.go`:
- Around line 309-334: The public-key verification path in
jwtService.VerifyJWTSignatureWithPublicKey is dropping the caller’s request
context by using context.Background(), so remote crypto providers can’t respect
cancellation or deadlines. Update the verification flow to accept and thread
through a context from the DPoP caller into VerifyJWTSignatureWithPublicKey,
pass that context into js.cryptoProvider.Verify, and adjust the related
interface plus any mocks/tests that reference VerifyJWTSignatureWithPublicKey or
cryptoProvider.Verify accordingly.
---
Nitpick comments:
In `@backend/pkg/thunderidengine/providers/constants.go`:
- Around line 496-503: The package-level sentinels ErrKeyNotFound and
ErrUnsupportedAlgorithm still hardcode the stale “kmprovider:” prefix even
though they now live in the generic providers package. Update the error text in
constants.go to use package-agnostic wording that fits any RuntimeCryptoProvider
implementation, and keep the sentinel names unchanged so callers can still match
them reliably.
🪄 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: 437caf75-7a63-4388-a4c1-d5171a514e1f
⛔ Files ignored due to path filters (3)
backend/tests/mocks/crypto/cryptomock/RuntimeCryptoProvider_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/crypto/cryptomock/TLSMaterialProvider_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/jose/jwemock/JWEServiceInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (61)
backend/.mockery.public.ymlbackend/cmd/server/main.gobackend/cmd/server/servicemanager.gobackend/internal/authn/openid4vp/init.gobackend/internal/authn/openid4vp/service.gobackend/internal/authn/openid4vp/service_test.gobackend/internal/flow/flowexec/init.gobackend/internal/flow/flowexec/service.gobackend/internal/flow/flowexec/service_test.gobackend/internal/oauth/init.gobackend/internal/oauth/jwks/init.gobackend/internal/oauth/jwks/init_test.gobackend/internal/oauth/jwks/service.gobackend/internal/oauth/jwks/service_test.gobackend/internal/oauth/oauth2/discovery/discovery_test.gobackend/internal/oauth/oauth2/discovery/init.gobackend/internal/oauth/oauth2/discovery/service.gobackend/internal/oauth/oauth2/dpop/init.gobackend/internal/oauth/oauth2/dpop/verifier.gobackend/internal/oauth/oauth2/dpop/verifier_test.gobackend/internal/oauth/oauth2/tokenservice/builder.gobackend/internal/oauth/oauth2/tokenservice/builder_test.gobackend/internal/oauth/oauth2/userinfo/service.gobackend/internal/oauth/oauth2/userinfo/service_jwe_test.gobackend/internal/openid4vci/init.gobackend/internal/openid4vci/service.gobackend/internal/openid4vci/service_test.gobackend/internal/system/cmodels/property.gobackend/internal/system/cryptolib/encrypt.gobackend/internal/system/cryptolib/encrypt_decrypt_test.gobackend/internal/system/cryptolib/model.gobackend/internal/system/cryptolib/model_test.gobackend/internal/system/jose/init.gobackend/internal/system/jose/init_test.gobackend/internal/system/jose/jwe/init.gobackend/internal/system/jose/jwe/service.gobackend/internal/system/jose/jwe/service_test.gobackend/internal/system/jose/jwt/init.gobackend/internal/system/jose/jwt/init_test.gobackend/internal/system/jose/jwt/service.gobackend/internal/system/jose/jwt/service_test.gobackend/internal/system/kmprovider/common/interface.gobackend/internal/system/kmprovider/configkm/config_crypto_provider.gobackend/internal/system/kmprovider/configkm/config_crypto_provider_test.gobackend/internal/system/kmprovider/configkm/init.gobackend/internal/system/kmprovider/configkm/init_test.gobackend/internal/system/kmprovider/configkm/model.gobackend/internal/system/kmprovider/init.gobackend/internal/system/kmprovider/init_test.gobackend/internal/system/kmprovider/runtimekm/init.gobackend/internal/system/kmprovider/runtimekm/init_test.gobackend/internal/system/kmprovider/runtimekm/pki/init.gobackend/internal/system/kmprovider/runtimekm/pki/model.gobackend/internal/system/kmprovider/runtimekm/pki/service.gobackend/internal/system/kmprovider/runtimekm/pki/utils.gobackend/internal/system/kmprovider/runtimekm/runtime_crypto_provider.gobackend/internal/system/kmprovider/runtimekm/runtime_crypto_provider_test.gobackend/pkg/thunderidengine/engine.gobackend/pkg/thunderidengine/providers/constants.gobackend/pkg/thunderidengine/providers/interface.gobackend/pkg/thunderidengine/providers/model.go
✅ Files skipped from review due to trivial changes (2)
- backend/internal/system/kmprovider/configkm/config_crypto_provider.go
- backend/internal/system/kmprovider/configkm/model.go
🚧 Files skipped from review as they are similar to previous changes (32)
- backend/internal/oauth/jwks/init.go
- backend/cmd/server/main.go
- backend/internal/system/jose/init.go
- backend/internal/system/jose/jwe/init.go
- backend/internal/oauth/oauth2/discovery/init.go
- backend/internal/system/jose/jwt/init.go
- backend/internal/flow/flowexec/init.go
- backend/internal/authn/openid4vp/service.go
- backend/internal/oauth/init.go
- backend/internal/oauth/jwks/init_test.go
- backend/internal/oauth/jwks/service.go
- backend/internal/oauth/oauth2/discovery/service.go
- backend/internal/system/cmodels/property.go
- backend/pkg/thunderidengine/providers/model.go
- backend/internal/system/kmprovider/init.go
- backend/internal/openid4vci/service_test.go
- backend/internal/system/jose/init_test.go
- backend/internal/oauth/jwks/service_test.go
- backend/internal/authn/openid4vp/init.go
- backend/internal/openid4vci/init.go
- backend/internal/system/kmprovider/configkm/config_crypto_provider_test.go
- backend/internal/openid4vci/service.go
- backend/internal/system/kmprovider/common/interface.go
- backend/internal/oauth/oauth2/discovery/discovery_test.go
- backend/internal/system/cryptolib/model_test.go
- backend/internal/authn/openid4vp/service_test.go
- backend/internal/system/cryptolib/model.go
- backend/internal/flow/flowexec/service.go
- backend/internal/system/jose/jwt/init_test.go
- backend/internal/system/kmprovider/configkm/init.go
- backend/internal/system/jose/jwe/service_test.go
- backend/internal/flow/flowexec/service_test.go
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/internal/system/jose/jwt/service.go (1)
309-334: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPreserve the request context during public-key verification.
DPoP starts with a request context, but this API cannot receive it and invokes the provider with
context.Background(). Custom providers performing remote operations therefore cannot honor request cancellation or deadlines.Proposed context propagation
-VerifyJWTSignatureWithPublicKey(jwtToken string, +VerifyJWTSignatureWithPublicKey(ctx context.Context, jwtToken string, jwtPublicKey crypto.PublicKey) *tidcommon.ServiceError { @@ - err = js.cryptoProvider.Verify(context.Background(), providers.KeyRef{PublicKey: jwtPublicKey}, + err = js.cryptoProvider.Verify(ctx, providers.KeyRef{PublicKey: jwtPublicKey}, algStr, []byte(signingInput), signature)Update the interface, DPoP caller, and mocks accordingly.
🤖 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/jose/jwt/service.go` around lines 309 - 334, The public-key verification path in jwtService.VerifyJWTSignatureWithPublicKey is dropping the caller’s request context by using context.Background(), so remote crypto providers can’t respect cancellation or deadlines. Update the verification flow to accept and thread through a context from the DPoP caller into VerifyJWTSignatureWithPublicKey, pass that context into js.cryptoProvider.Verify, and adjust the related interface plus any mocks/tests that reference VerifyJWTSignatureWithPublicKey or cryptoProvider.Verify accordingly.
🧹 Nitpick comments (1)
backend/pkg/thunderidengine/providers/constants.go (1)
496-503: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale
"kmprovider:"prefix on package-level errors now owned byproviders.These sentinels were moved into the generic
thunderidengine/providerspackage (used by any embedder-suppliedRuntimeCryptoProvider, not justkmprovider), but the error text still says"kmprovider: ...", which will mislabel errors surfaced by non-kmprovider implementations.♻️ Proposed fix
-var ErrKeyNotFound = errors.New("kmprovider: no key found matching the requested identifier") +var ErrKeyNotFound = errors.New("providers: no key found matching the requested identifier") -var ErrUnsupportedAlgorithm = errors.New("kmprovider: unsupported signature algorithm") +var ErrUnsupportedAlgorithm = errors.New("providers: unsupported signature algorithm")🤖 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/pkg/thunderidengine/providers/constants.go` around lines 496 - 503, The package-level sentinels ErrKeyNotFound and ErrUnsupportedAlgorithm still hardcode the stale “kmprovider:” prefix even though they now live in the generic providers package. Update the error text in constants.go to use package-agnostic wording that fits any RuntimeCryptoProvider implementation, and keep the sentinel names unchanged so callers can still match them reliably.
🤖 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/oauth/oauth2/tokenservice/builder.go`:
- Around line 138-143: The token claim merge order in build/claims assembly is
wrong: subject attributes can overwrite authoritative protocol claims like
scope, client_id, and grant_type. Update the logic in the token builder around
ctx.SubjectAttributes so those attributes are merged first, then set the
protocol/system claims afterward in the same claim-building path, ensuring the
named claims always win over any colliding subject keys.
In `@backend/internal/system/kmprovider/runtimekm/runtime_crypto_provider.go`:
- Around line 210-220: Update runtimeCryptoService.Verify so it only requires
s.pkiService when it actually needs to resolve a key via KeyID, matching the
behavior of getRSAPublicKey and getECPublicKey. If keyRef.PublicKey is already
present, allow verification to proceed without initializing PKI service; only
fail with "PKI service not initialized" on the key-resolution path. Use Verify,
getRSAPublicKey, and getECPublicKey as the reference points for the control-flow
change.
- Around line 248-252: `runtimeCryptoService.IsSupportedEncAlgorithm` is too
permissive because it delegates to `cryptolib.EncryptionAlgorithmFor`, which
reports support for key-wrap algorithms that `Encrypt` and `Decrypt` do not
actually handle. Narrow the check in `IsSupportedEncAlgorithm` to only return
true for the algorithms that `runtimeCryptoService.Encrypt` and
`runtimeCryptoService.Decrypt` explicitly support, so callers do not get a false
positive and fail later with unsupported algorithm errors.
In `@backend/pkg/thunderidengine/engine.go`:
- Line 46: `Engine.New` always initializes the bundled PKI path, which blocks
embedders from supplying their own crypto implementation. Add a
`WithRuntimeCryptoProvider` option on the engine constructor path and thread
that value through `New` so it is used when present, falling back to the
existing `pki`-based setup only when no provider is supplied. Update the
`Engine` construction logic and any related option wiring so the runtime crypto
provider can be injected by callers.
---
Outside diff comments:
In `@backend/internal/system/jose/jwt/service.go`:
- Around line 309-334: The public-key verification path in
jwtService.VerifyJWTSignatureWithPublicKey is dropping the caller’s request
context by using context.Background(), so remote crypto providers can’t respect
cancellation or deadlines. Update the verification flow to accept and thread
through a context from the DPoP caller into VerifyJWTSignatureWithPublicKey,
pass that context into js.cryptoProvider.Verify, and adjust the related
interface plus any mocks/tests that reference VerifyJWTSignatureWithPublicKey or
cryptoProvider.Verify accordingly.
---
Nitpick comments:
In `@backend/pkg/thunderidengine/providers/constants.go`:
- Around line 496-503: The package-level sentinels ErrKeyNotFound and
ErrUnsupportedAlgorithm still hardcode the stale “kmprovider:” prefix even
though they now live in the generic providers package. Update the error text in
constants.go to use package-agnostic wording that fits any RuntimeCryptoProvider
implementation, and keep the sentinel names unchanged so callers can still match
them reliably.
🪄 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: 437caf75-7a63-4388-a4c1-d5171a514e1f
⛔ Files ignored due to path filters (3)
backend/tests/mocks/crypto/cryptomock/RuntimeCryptoProvider_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/crypto/cryptomock/TLSMaterialProvider_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/jose/jwemock/JWEServiceInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (61)
backend/.mockery.public.ymlbackend/cmd/server/main.gobackend/cmd/server/servicemanager.gobackend/internal/authn/openid4vp/init.gobackend/internal/authn/openid4vp/service.gobackend/internal/authn/openid4vp/service_test.gobackend/internal/flow/flowexec/init.gobackend/internal/flow/flowexec/service.gobackend/internal/flow/flowexec/service_test.gobackend/internal/oauth/init.gobackend/internal/oauth/jwks/init.gobackend/internal/oauth/jwks/init_test.gobackend/internal/oauth/jwks/service.gobackend/internal/oauth/jwks/service_test.gobackend/internal/oauth/oauth2/discovery/discovery_test.gobackend/internal/oauth/oauth2/discovery/init.gobackend/internal/oauth/oauth2/discovery/service.gobackend/internal/oauth/oauth2/dpop/init.gobackend/internal/oauth/oauth2/dpop/verifier.gobackend/internal/oauth/oauth2/dpop/verifier_test.gobackend/internal/oauth/oauth2/tokenservice/builder.gobackend/internal/oauth/oauth2/tokenservice/builder_test.gobackend/internal/oauth/oauth2/userinfo/service.gobackend/internal/oauth/oauth2/userinfo/service_jwe_test.gobackend/internal/openid4vci/init.gobackend/internal/openid4vci/service.gobackend/internal/openid4vci/service_test.gobackend/internal/system/cmodels/property.gobackend/internal/system/cryptolib/encrypt.gobackend/internal/system/cryptolib/encrypt_decrypt_test.gobackend/internal/system/cryptolib/model.gobackend/internal/system/cryptolib/model_test.gobackend/internal/system/jose/init.gobackend/internal/system/jose/init_test.gobackend/internal/system/jose/jwe/init.gobackend/internal/system/jose/jwe/service.gobackend/internal/system/jose/jwe/service_test.gobackend/internal/system/jose/jwt/init.gobackend/internal/system/jose/jwt/init_test.gobackend/internal/system/jose/jwt/service.gobackend/internal/system/jose/jwt/service_test.gobackend/internal/system/kmprovider/common/interface.gobackend/internal/system/kmprovider/configkm/config_crypto_provider.gobackend/internal/system/kmprovider/configkm/config_crypto_provider_test.gobackend/internal/system/kmprovider/configkm/init.gobackend/internal/system/kmprovider/configkm/init_test.gobackend/internal/system/kmprovider/configkm/model.gobackend/internal/system/kmprovider/init.gobackend/internal/system/kmprovider/init_test.gobackend/internal/system/kmprovider/runtimekm/init.gobackend/internal/system/kmprovider/runtimekm/init_test.gobackend/internal/system/kmprovider/runtimekm/pki/init.gobackend/internal/system/kmprovider/runtimekm/pki/model.gobackend/internal/system/kmprovider/runtimekm/pki/service.gobackend/internal/system/kmprovider/runtimekm/pki/utils.gobackend/internal/system/kmprovider/runtimekm/runtime_crypto_provider.gobackend/internal/system/kmprovider/runtimekm/runtime_crypto_provider_test.gobackend/pkg/thunderidengine/engine.gobackend/pkg/thunderidengine/providers/constants.gobackend/pkg/thunderidengine/providers/interface.gobackend/pkg/thunderidengine/providers/model.go
✅ Files skipped from review due to trivial changes (2)
- backend/internal/system/kmprovider/configkm/config_crypto_provider.go
- backend/internal/system/kmprovider/configkm/model.go
🚧 Files skipped from review as they are similar to previous changes (32)
- backend/internal/oauth/jwks/init.go
- backend/cmd/server/main.go
- backend/internal/system/jose/init.go
- backend/internal/system/jose/jwe/init.go
- backend/internal/oauth/oauth2/discovery/init.go
- backend/internal/system/jose/jwt/init.go
- backend/internal/flow/flowexec/init.go
- backend/internal/authn/openid4vp/service.go
- backend/internal/oauth/init.go
- backend/internal/oauth/jwks/init_test.go
- backend/internal/oauth/jwks/service.go
- backend/internal/oauth/oauth2/discovery/service.go
- backend/internal/system/cmodels/property.go
- backend/pkg/thunderidengine/providers/model.go
- backend/internal/system/kmprovider/init.go
- backend/internal/openid4vci/service_test.go
- backend/internal/system/jose/init_test.go
- backend/internal/oauth/jwks/service_test.go
- backend/internal/authn/openid4vp/init.go
- backend/internal/openid4vci/init.go
- backend/internal/system/kmprovider/configkm/config_crypto_provider_test.go
- backend/internal/openid4vci/service.go
- backend/internal/system/kmprovider/common/interface.go
- backend/internal/oauth/oauth2/discovery/discovery_test.go
- backend/internal/system/cryptolib/model_test.go
- backend/internal/authn/openid4vp/service_test.go
- backend/internal/system/cryptolib/model.go
- backend/internal/flow/flowexec/service.go
- backend/internal/system/jose/jwt/init_test.go
- backend/internal/system/kmprovider/configkm/init.go
- backend/internal/system/jose/jwe/service_test.go
- backend/internal/flow/flowexec/service_test.go
🛑 Comments failed to post (4)
backend/internal/oauth/oauth2/tokenservice/builder.go (1)
138-143: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate the file and inspect the relevant region with line numbers. git ls-files backend/internal/oauth/oauth2/tokenservice/builder.go wc -l backend/internal/oauth/oauth2/tokenservice/builder.go sed -n '1,260p' backend/internal/oauth/oauth2/tokenservice/builder.go | cat -n # Find related claim-building code and any other uses of SubjectAttributes in this package. rg -n "SubjectAttributes|scope|client_id|grant_type|claims\\[" backend/internal/oauth/oauth2/tokenservice -SRepository: thunder-id/thunderid
Length of output: 37137
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the build context/model and any code that populates or filters SubjectAttributes. sed -n '1,220p' backend/internal/oauth/oauth2/tokenservice/model.go | cat -n rg -n "SubjectAttributes|reserved.*scope|reserved.*client_id|reserved.*grant_type|grant handler|filtered by" backend/internal/oauth/oauth2 -S # Inspect the tests around claim collisions to see expected behavior. sed -n '1180,1320p' backend/internal/oauth/oauth2/tokenservice/builder_test.go | cat -n sed -n '1,220p' backend/internal/oauth/oauth2/tokenservice/utils.go | cat -nRepository: thunder-id/thunderid
Length of output: 27252
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the attribute filtering logic and the exact reserved keys. sed -n '400,520p' backend/internal/oauth/oauth2/tokenservice/utils.go | cat -n # Inspect where the filtered attributes are passed into the builder for access-token grants. sed -n '140,240p' backend/internal/oauth/oauth2/granthandlers/authorization_code.go | cat -n sed -n '140,230p' backend/internal/oauth/oauth2/granthandlers/client_credentials.go | cat -n sed -n '180,235p' backend/internal/oauth/oauth2/granthandlers/refresh_token.go | cat -n sed -n '190,235p' backend/internal/oauth/oauth2/granthandlers/ciba.go | cat -n sed -n '200,245p' backend/internal/oauth/oauth2/granthandlers/token_exchange.go | cat -nRepository: thunder-id/thunderid
Length of output: 18829
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect standard claim reservations and how the allow-list applies to access-token attributes. rg -n "func getStandardJWTClaims|getStandardJWTClaims\\(|ReservedAccessTokenClaimNames|scopeClaims|client_id" backend/internal/oauth/oauth2/tokenservice/utils.go backend/internal/oauth/oauth2/tokenservice -S sed -n '220,320p' backend/internal/oauth/oauth2/tokenservice/utils.go | cat -n sed -n '1,140p' backend/internal/oauth/oauth2/tokenservice/utils_test.go | cat -nRepository: thunder-id/thunderid
Length of output: 15526
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the validation that rejects reserved access-token claim names in app config. sed -n '520,590p' backend/internal/oauth/oauth2/tokenservice/utils.go | cat -n # Inspect any tests covering reserved claim-name rejection. rg -n "ReservedAccessTokenClaimNames|reserved access-token|scope.*client_id|client_id.*scope" backend/internal/oauth/oauth2/tokenservice -SRepository: thunder-id/thunderid
Length of output: 2138
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the allow-list validation that uses ReservedAccessTokenClaimNames. sed -n '540,580p' backend/internal/oauth/oauth2/tokenservice/utils.go | cat -n # Inspect tests for rejection of reserved names in access-token attribute allow-lists. rg -n "reserved.*claim|ReservedAccessTokenClaimNames|allow-list.*reserved|scope.*reserved|client_id.*reserved|grant_type.*reserved" backend/internal/oauth/oauth2/tokenservice/utils_test.go backend/internal/oauth/oauth2/granthandlers -SRepository: thunder-id/thunderid
Length of output: 912
🏁 Script executed:
#!/bin/bash set -euo pipefail # Search for validation of access-token attribute allow-lists against reserved claims. rg -n "ReservedAccessTokenClaimNames|allow-list.*reserved|clientAccessTokenConfig|userAccessTokenConfig|AccessTokenSubConfig|Attributes" backend/internal/oauth -S # Inspect the OAuth client config helpers that expose the access-token sub-config. sed -n '1,260p' backend/internal/oauth/config/*.go | cat -nRepository: thunder-id/thunderid
Length of output: 50376
Keep protocol claims authoritative in
backend/internal/oauth/oauth2/tokenservice/builder.go:124-145. Mergectx.SubjectAttributesbefore settingscope,client_id, andgrant_type; otherwise colliding subject attributes can overwrite those token claims.🤖 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/oauth/oauth2/tokenservice/builder.go` around lines 138 - 143, The token claim merge order in build/claims assembly is wrong: subject attributes can overwrite authoritative protocol claims like scope, client_id, and grant_type. Update the logic in the token builder around ctx.SubjectAttributes so those attributes are merged first, then set the protocol/system claims afterward in the same claim-building path, ensuring the named claims always win over any colliding subject keys.backend/internal/system/kmprovider/runtimekm/runtime_crypto_provider.go (2)
210-220: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Verifyrequires a PKI service even when only an inlinePublicKeyis supplied.Unlike
getRSAPublicKey/getECPublicKey, which only requires.pkiServicewhenkeyRef.KeyID != "",Verifyfails immediately with"PKI service not initialized"regardless of whetherkeyRef.PublicKeyis already set — blocking a legitimate inline-key verification path (used e.g. for DPoP/JWT verification with an explicit public key) whenever a provider instance has no PKI service wired.🐛 Proposed fix
func (s *runtimeCryptoService) Verify( ctx context.Context, keyRef providers.KeyRef, alg string, content []byte, signature []byte, ) error { - if s.pkiService == nil { - return errors.New("PKI service not initialized") - } signAlg, err := cryptolib.SignAlgorithmFor(cryptolib.Algorithm(alg)) if err != nil { return fmt.Errorf("%w: %q", providers.ErrUnsupportedAlgorithm, alg) } publicKey := keyRef.PublicKey if keyRef.KeyID != "" { + if s.pkiService == nil { + return errors.New("PKI service not initialized") + } keys, err := s.GetPublicKeys(ctx, providers.PublicKeyFilter{})📝 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.func (s *runtimeCryptoService) Verify( ctx context.Context, keyRef providers.KeyRef, alg string, content []byte, signature []byte, ) error { signAlg, err := cryptolib.SignAlgorithmFor(cryptolib.Algorithm(alg)) if err != nil { return fmt.Errorf("%w: %q", providers.ErrUnsupportedAlgorithm, alg) } publicKey := keyRef.PublicKey if keyRef.KeyID != "" { if s.pkiService == nil { return errors.New("PKI service not initialized") } keys, err := s.GetPublicKeys(ctx, providers.PublicKeyFilter{})🤖 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/kmprovider/runtimekm/runtime_crypto_provider.go` around lines 210 - 220, Update runtimeCryptoService.Verify so it only requires s.pkiService when it actually needs to resolve a key via KeyID, matching the behavior of getRSAPublicKey and getECPublicKey. If keyRef.PublicKey is already present, allow verification to proceed without initializing PKI service; only fail with "PKI service not initialized" on the key-resolution path. Use Verify, getRSAPublicKey, and getECPublicKey as the reference points for the control-flow change.
248-252: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Confirm AlgorithmParamsFromMap and the Encrypt/Decrypt switch really lack AES-KW/GCMKW handling. fd model.go backend/internal/system/cryptolib --exec cat -n {}Repository: thunder-id/thunderid
Length of output: 13435
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the provider and caller before reading targeted sections. ast-grep outline backend/internal/system/kmprovider/runtimekm/runtime_crypto_provider.go --view expanded ast-grep outline backend/internal/system/jose/jwe/service.go --view expanded # Read the relevant sections with line numbers. sed -n '1,340p' backend/internal/system/kmprovider/runtimekm/runtime_crypto_provider.go | cat -n sed -n '1,220p' backend/internal/system/jose/jwe/service.go | cat -nRepository: thunder-id/thunderid
Length of output: 24436
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the encryption-algorithm whitelist and the JWE key-management validation path. ast-grep outline backend/internal/system/cryptolib/model.go --view expanded ast-grep outline backend/internal/system/jose/jwe/service.go --view expanded sed -n '1,320p' backend/internal/system/cryptolib/model.go | cat -n sed -n '220,320p' backend/internal/system/jose/jwe/service.go | cat -nRepository: thunder-id/thunderid
Length of output: 19715
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate and read the algorithm resolver used by IsSupportedEncAlgorithm. rg -n "func .*EncryptionAlgorithmFor|EncryptionAlgorithmFor\(" backend/internal/system/cryptolib backend/internal/system/kmprovider/runtimekm/runtime_crypto_provider.goRepository: thunder-id/thunderid
Length of output: 898
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '300,380p' backend/internal/system/cryptolib/encrypt.go | cat -nRepository: thunder-id/thunderid
Length of output: 948
Narrow
IsSupportedEncAlgorithmto the algorithms this provider actually handles.
EncryptionAlgorithmForacceptsA128KW/A192KW/A256KWandA128GCMKW/A192GCMKW/A256GCMKW, butruntimeCryptoService.Encrypt/Decryptnever switch on those values. Callers that gate on this method will get a false positive and then fail later withunsupported algorithm.🤖 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/kmprovider/runtimekm/runtime_crypto_provider.go` around lines 248 - 252, `runtimeCryptoService.IsSupportedEncAlgorithm` is too permissive because it delegates to `cryptolib.EncryptionAlgorithmFor`, which reports support for key-wrap algorithms that `Encrypt` and `Decrypt` do not actually handle. Narrow the check in `IsSupportedEncAlgorithm` to only return true for the algorithms that `runtimeCryptoService.Encrypt` and `runtimeCryptoService.Decrypt` explicitly support, so callers do not get a false positive and fail later with unsupported algorithm errors.backend/pkg/thunderidengine/engine.go (1)
46-46: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash ast-grep outline backend/pkg/thunderidengine --items all \ --match 'Option|engineContext|RuntimeCryptoProvider' rg -nP -C4 'runtimeCryptoSvc\s*=|RuntimeCryptoProvider|With.*Crypto|func\s+New\s*\(' \ backend/pkg/thunderidengineRepository: thunder-id/thunderid
Length of output: 3605
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,220p' backend/pkg/thunderidengine/engine.go printf '\n==== TESTS ====\n' sed -n '1,260p' backend/pkg/thunderidengine/engine_test.go printf '\n==== RUNTIME CRYPTO INTERFACE ====\n' sed -n '200,280p' backend/pkg/thunderidengine/providers/interface.goRepository: thunder-id/thunderid
Length of output: 17582
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '==== runtimeCryptoSvc references ====\n' rg -n --hidden --glob '!**/vendor/**' 'runtimeCryptoSvc|RuntimeCryptoProvider|WithRuntimeCrypto|CryptoProvider' backend/pkg/thunderidengine backend/pkg/thunderidengine/providers printf '\n==== option definitions ====\n' sed -n '1,260p' backend/pkg/thunderidengine/engine.goRepository: thunder-id/thunderid
Length of output: 11413
Allow
Engine.Newto accept an embedder-suppliedRuntimeCryptoProvider.
Newstill always builds the bundled PKI path here, so embedders have no way to plug in their own crypto implementation. Add aWithRuntimeCryptoProvideroption and use it when provided.🤖 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/pkg/thunderidengine/engine.go` at line 46, `Engine.New` always initializes the bundled PKI path, which blocks embedders from supplying their own crypto implementation. Add a `WithRuntimeCryptoProvider` option on the engine constructor path and thread that value through `New` so it is used when present, falling back to the existing `pki`-based setup only when no provider is supplied. Update the `Engine` construction logic and any related option wiring so the runtime crypto provider can be injected by callers.
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
3 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
Moved RuntimeCryptoProvider to thunderidengine, so that embedder application can provide their own keystore implmentation.
Approach
Refer #2776 (comment)
Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
New Features
Bug Fixes
Tests