Add GetTLSMaterial impl in RuntimeCryptoProvider#3031
Open
JeethJJ wants to merge 1 commit into
Open
Conversation
JeethJJ
requested review from
ThaminduDilshan,
hwupathum,
senthalan and
thiva-k
as code owners
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 (2)
📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughThis PR migrates TLS material loading from file-path construction in ChangesTLS Material Loading Migration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/internal/oauth/oauth2/authz/service.go (1)
428-437:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDifferentiate context failures from invalid assertion failures.
On Line 579, all
VerifyJWTerrors are collapsed into"invalid assertion signature", and on Line 428 that becomes aninvalid_requestresponse. With context-aware verification,context.Canceled/context.DeadlineExceededshould be treated as server-side failures, not client assertion errors.Suggested fix
@@ - if err := as.verifyAssertion(ctx, assertion); err != nil { + if err := as.verifyAssertion(ctx, assertion); err != nil { + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + authErr = &AuthorizationError{ + Code: oauth2const.ErrorServerError, + Message: "Failed to process authorization request", + } + return err + } as.logger.Debug("Assertion verification failed", log.Error(err)) @@ func (as *authorizeService) verifyAssertion(ctx context.Context, assertion string) error { if err := as.jwtService.VerifyJWT(ctx, assertion, "", ""); err != nil { + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return err + } as.logger.Debug("Invalid assertion signature", log.String("error", err.Error.DefaultValue)) return errors.New("invalid assertion signature") } return nil }Also applies to: 578-582
🤖 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/authz/service.go` around lines 428 - 437, The verification path currently treats all VerifyJWT errors as assertion failures and turns them into an invalid_request AuthorizationError in as.verifyAssertion (seen where as.verifyAssertion is called and the AuthorizationError is constructed); change this so context errors are recognized and handled as server-side failures: in VerifyJWT (and/or as.verifyAssertion) check errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) and return or propagate a distinct server error (so the caller does not map it to oauth2const.ErrorInvalidRequest) — ensure the caller that builds the AuthorizationError (the block that logs "Assertion verification failed" and sets Code to ErrorInvalidRequest) treats context.Canceled/DeadlineExceeded as an internal/server error path (or bubbles up the context error) rather than marking the client assertion invalid.backend/internal/oauth/oauth2/introspect/service.go (1)
60-64:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid treating context timeout/cancel as an inactive token result.
With Line 82 now using request context,
context.Canceledandcontext.DeadlineExceededcan occur. The current bool-only path (Line 60) maps these toactive=false, which masks transient/server-side failures as token invalidation.Suggested fix
@@ - if !s.validateToken(ctx, logger, token) { + valid, validationErr := s.validateToken(ctx, logger, token) + if validationErr != nil { + return nil, validationErr + } + if !valid { return &IntrospectResponse{ Active: false, }, nil } @@ -func (s *tokenIntrospectionService) validateToken(ctx context.Context, logger *log.Logger, token string) bool { +func (s *tokenIntrospectionService) validateToken(ctx context.Context, logger *log.Logger, token string) (bool, error) { if err := s.jwtService.VerifyJWT(ctx, token, "", ""); err != nil { + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return false, err + } logger.Debug("Failed to verify refresh token", log.String("error", err.Error.DefaultValue)) - return false + return false, nil } - return true + return true, nil }Also applies to: 81-84
🤖 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/introspect/service.go` around lines 60 - 64, The current call to s.validateToken(ctx, logger, token) treats any failure (including context.Canceled or context.DeadlineExceeded) as an inactive token; change the flow so context cancellations/timeouts are propagated as errors instead of mapping to Active=false: update s.validateToken to return (bool, error) or return a distinct error for context issues, then in the caller (the introspect path that builds IntrospectResponse) check the error and if ctx.Err() or an error equal to context.Canceled/context.DeadlineExceeded is present, return that error (or a 5xx/internal error) rather than returning &IntrospectResponse{Active:false}, otherwise use the boolean to set Active. Ensure you reference s.validateToken and the IntrospectResponse Active branch when making the change.backend/internal/system/security/jwt_authenticator_test.go (1)
288-288:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHardcoded brand string literal found
Line 288 contains
"ThunderID"in a raw string literal. Is this hardcoded brand name intentional? If not, please replace it with a named constant (or runtime/config-sourced value where appropriate) instead of embedding the brand directly.As per coding guidelines:
**/*.go: Scan for hardcoded occurrences of the string literalsThunderorThunderIDin this file.🤖 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/security/jwt_authenticator_test.go` at line 288, The test contains a hardcoded brand string "ThunderID" (seen in the test case name for the JWT authenticator tests); replace this literal with a named constant (e.g., ThunderIDBrand) defined near the top of the test file or in a test constants block and use that constant in the test case `name` and any other occurrences in `jwt_authenticator_test.go` to avoid embedding the brand directly; ensure the constant is used consistently (instead of the raw string) so future scans for `Thunder`/`ThunderID` detect the centralized symbol.backend/internal/oauth/oauth2/tokenservice/validator_test.go (1)
529-535:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix
AssertNotCalledargument list forVerifyJWTSignature
VerifyJWTSignatureis mocked asVerifyJWTSignature(ctx, jwtToken), but the untrusted-issuer assertion usesAssertNotCalled(..., "VerifyJWTSignature", tokenInvalid)(missing thectxarg). Change it tosuite.mockJWTService.AssertNotCalled(suite.T(), "VerifyJWTSignature", mock.Anything, tokenInvalid)(or assert the method is never called at all).🤖 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/validator_test.go` around lines 529 - 535, The test currently asserts the JWT verifier wasn't called using suite.mockJWTService.AssertNotCalled(suite.T(), "VerifyJWTSignature", tokenInvalid) but VerifyJWTSignature is defined as VerifyJWTSignature(ctx, jwtToken); update the assertion in the ValidateSubjectToken test to include the context placeholder so it matches the mocked signature (e.g., use mock.Anything for the ctx), i.e. call suite.mockJWTService.AssertNotCalled(suite.T(), "VerifyJWTSignature", mock.Anything, tokenInvalid) or alternatively assert the method was never called at all for mockJWTService to ensure VerifyJWTSignature(ctx, jwtToken) was not invoked for tokenInvalid.
🤖 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/cmd/server/main.go`:
- Around line 167-170: The loadCertConfig function currently calls
runtimeSvc.GetTLSMaterial with context.Background(), which can block
indefinitely; change it to use a bounded context (e.g., context.WithTimeout)
when calling runtimeSvc.GetTLSMaterial inside loadCertConfig, create the timeout
context with a reasonable startup deadline, defer the cancel function, use that
context in the GetTLSMaterial call, and ensure error handling detects and
surfaces context deadline/cancellation errors appropriately so startup won't
hang.
In `@backend/internal/system/jose/jwe/service.go`:
- Around line 167-170: The current handling of Decrypt errors treats all
provider errors as ErrorJWEDecryptionFailed; change the error branch after
calling js.cryptoProvider.Decrypt(ctx, &js.keyRef, params, encryptedKey) to
detect context cancellation/timeouts and propagate them instead of converting to
ErrorJWEDecryptionFailed: if errors.Is(err, context.Canceled) || errors.Is(err,
context.DeadlineExceeded) (or simply if ctx.Err() != nil) return nil, ctx.Err()
(or return the original err) so request aborts are surfaced, otherwise log and
return &ErrorJWEDecryptionFailed; reference the call to
js.cryptoProvider.Decrypt, the js.logger.Error line, and the
ErrorJWEDecryptionFailed symbol when making the change.
In `@backend/internal/system/jose/jwt/service.go`:
- Around line 231-233: The code currently maps every error from
VerifyJWTSignature to ErrorInvalidTokenSignature; instead, change the error
handling in the caller (the block around VerifyJWTSignature in service.go) to
preserve operational/context errors and only convert true signature-failure
errors to ErrorInvalidTokenSignature: call js.VerifyJWTSignature(ctx, jwtToken),
and if it returns an error, check for context.Canceled/context.DeadlineExceeded
(errors.Is) and any provider/lookup errors (the specific sentinel or typed
errors returned by VerifyJWTSignature) and return those errors as-is (or
wrapped) while only returning &ErrorInvalidTokenSignature when the error
indicates a real signature/validation failure; reference VerifyJWTSignature and
ErrorInvalidTokenSignature to locate and update the conditional.
In `@backend/internal/system/kmprovider/defaultkm/runtime_crypto_provider.go`:
- Around line 200-207: The code returns kmprovider.TLSMaterial using
tlsCfg.Certificates[0] without nil/length checks; modify the code around the
call to s.pkiService.GetTLSConfig() (the tlsCfg variable) to validate that
tlsCfg != nil and that len(tlsCfg.Certificates) > 0 before indexing, and if
either check fails return a clear error (e.g., "no TLS config" or "no
certificates available") instead of risking a panic when constructing
kmprovider.TLSMaterial.
---
Outside diff comments:
In `@backend/internal/oauth/oauth2/authz/service.go`:
- Around line 428-437: The verification path currently treats all VerifyJWT
errors as assertion failures and turns them into an invalid_request
AuthorizationError in as.verifyAssertion (seen where as.verifyAssertion is
called and the AuthorizationError is constructed); change this so context errors
are recognized and handled as server-side failures: in VerifyJWT (and/or
as.verifyAssertion) check errors.Is(err, context.Canceled) || errors.Is(err,
context.DeadlineExceeded) and return or propagate a distinct server error (so
the caller does not map it to oauth2const.ErrorInvalidRequest) — ensure the
caller that builds the AuthorizationError (the block that logs "Assertion
verification failed" and sets Code to ErrorInvalidRequest) treats
context.Canceled/DeadlineExceeded as an internal/server error path (or bubbles
up the context error) rather than marking the client assertion invalid.
In `@backend/internal/oauth/oauth2/introspect/service.go`:
- Around line 60-64: The current call to s.validateToken(ctx, logger, token)
treats any failure (including context.Canceled or context.DeadlineExceeded) as
an inactive token; change the flow so context cancellations/timeouts are
propagated as errors instead of mapping to Active=false: update s.validateToken
to return (bool, error) or return a distinct error for context issues, then in
the caller (the introspect path that builds IntrospectResponse) check the error
and if ctx.Err() or an error equal to context.Canceled/context.DeadlineExceeded
is present, return that error (or a 5xx/internal error) rather than returning
&IntrospectResponse{Active:false}, otherwise use the boolean to set Active.
Ensure you reference s.validateToken and the IntrospectResponse Active branch
when making the change.
In `@backend/internal/oauth/oauth2/tokenservice/validator_test.go`:
- Around line 529-535: The test currently asserts the JWT verifier wasn't called
using suite.mockJWTService.AssertNotCalled(suite.T(), "VerifyJWTSignature",
tokenInvalid) but VerifyJWTSignature is defined as VerifyJWTSignature(ctx,
jwtToken); update the assertion in the ValidateSubjectToken test to include the
context placeholder so it matches the mocked signature (e.g., use mock.Anything
for the ctx), i.e. call suite.mockJWTService.AssertNotCalled(suite.T(),
"VerifyJWTSignature", mock.Anything, tokenInvalid) or alternatively assert the
method was never called at all for mockJWTService to ensure
VerifyJWTSignature(ctx, jwtToken) was not invoked for tokenInvalid.
In `@backend/internal/system/security/jwt_authenticator_test.go`:
- Line 288: The test contains a hardcoded brand string "ThunderID" (seen in the
test case name for the JWT authenticator tests); replace this literal with a
named constant (e.g., ThunderIDBrand) defined near the top of the test file or
in a test constants block and use that constant in the test case `name` and any
other occurrences in `jwt_authenticator_test.go` to avoid embedding the brand
directly; ensure the constant is used consistently (instead of the raw string)
so future scans for `Thunder`/`ThunderID` detect the centralized symbol.
🪄 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: d1ac276e-a409-4bdb-ae51-fe83430c8a5f
⛔ Files ignored due to path filters (6)
backend/tests/mocks/crypto/cryptomock/RuntimeCryptoProvider_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/crypto/pki/pkimock/PKIServiceInterface_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/jose/jwemock/JWEServiceInterface_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/jose/jwtmock/JWTServiceInterface_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/oauth/oauth2/tokenservicemock/TokenBuilderInterface_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/oauth/oauth2/tokenservicemock/TokenValidatorInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (49)
backend/.mockery.public.ymlbackend/cmd/server/main.gobackend/cmd/server/servicemanager.gobackend/internal/authn/consent/service.gobackend/internal/authn/consent/service_test.gobackend/internal/authn/magiclink/service.gobackend/internal/authn/magiclink/service_test.gobackend/internal/authn/service.gobackend/internal/authn/service_test.gobackend/internal/notification/otp_service.gobackend/internal/notification/otp_service_test.gobackend/internal/oauth/jwks/JWKSServiceInterface_mock_test.gobackend/internal/oauth/jwks/handler.gobackend/internal/oauth/jwks/handler_test.gobackend/internal/oauth/jwks/service.gobackend/internal/oauth/jwks/service_test.gobackend/internal/oauth/oauth2/authz/service.gobackend/internal/oauth/oauth2/authz/service_test.gobackend/internal/oauth/oauth2/dpop/context.gobackend/internal/oauth/oauth2/granthandlers/authorization_code.gobackend/internal/oauth/oauth2/granthandlers/authorization_code_test.gobackend/internal/oauth/oauth2/granthandlers/client_credentials.gobackend/internal/oauth/oauth2/granthandlers/client_credentials_test.gobackend/internal/oauth/oauth2/granthandlers/refresh_token.gobackend/internal/oauth/oauth2/granthandlers/refresh_token_test.gobackend/internal/oauth/oauth2/granthandlers/token_exchange.gobackend/internal/oauth/oauth2/granthandlers/token_exchange_test.gobackend/internal/oauth/oauth2/introspect/service.gobackend/internal/oauth/oauth2/introspect/service_test.gobackend/internal/oauth/oauth2/tokenservice/builder.gobackend/internal/oauth/oauth2/tokenservice/builder_test.gobackend/internal/oauth/oauth2/tokenservice/model.gobackend/internal/oauth/oauth2/tokenservice/validator.gobackend/internal/oauth/oauth2/tokenservice/validator_test.gobackend/internal/oauth/oauth2/userinfo/service.gobackend/internal/oauth/oauth2/userinfo/service_test.gobackend/internal/system/jose/jwe/service.gobackend/internal/system/jose/jwe/service_test.gobackend/internal/system/jose/jwt/service.gobackend/internal/system/jose/jwt/service_test.gobackend/internal/system/kmprovider/common/interface.gobackend/internal/system/kmprovider/defaultkm/pki/service.gobackend/internal/system/kmprovider/defaultkm/runtime_crypto_provider.gobackend/internal/system/kmprovider/defaultkm/runtime_crypto_provider_test.gobackend/internal/system/kmprovider/init.gobackend/internal/system/mcp/auth/token_verifier.gobackend/internal/system/mcp/auth/token_verifier_test.gobackend/internal/system/security/jwt_authenticator.gobackend/internal/system/security/jwt_authenticator_test.go
💤 Files with no reviewable changes (2)
- backend/internal/oauth/oauth2/tokenservice/model.go
- backend/internal/oauth/oauth2/dpop/context.go
Comment on lines
+167
to
170
| // loadCertConfig loads the TLS material via the runtime crypto provider. | ||
| func loadCertConfig(logger *log.Logger, runtimeSvc kmprovider.RuntimeCryptoProvider) *tls.Config { | ||
| mat, err := runtimeSvc.GetTLSMaterial(context.Background()) | ||
| if err != nil { |
Contributor
There was a problem hiding this comment.
Use a bounded context for TLS material loading at startup.
Line 169 uses context.Background(), so TLS material fetch can block startup indefinitely. Prefer a timeout-bound context.
Proposed fix
const shutdownTimeout = 5 * time.Second
+const tlsMaterialLoadTimeout = 10 * time.Second
@@
func loadCertConfig(logger *log.Logger, runtimeSvc kmprovider.RuntimeCryptoProvider) *tls.Config {
- mat, err := runtimeSvc.GetTLSMaterial(context.Background())
+ ctx, cancel := context.WithTimeout(context.Background(), tlsMaterialLoadTimeout)
+ defer cancel()
+ mat, err := runtimeSvc.GetTLSMaterial(ctx)
if err != nil {
logger.Fatal("Failed to load TLS material", log.Error(err))
}📝 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
| // loadCertConfig loads the TLS material via the runtime crypto provider. | |
| func loadCertConfig(logger *log.Logger, runtimeSvc kmprovider.RuntimeCryptoProvider) *tls.Config { | |
| mat, err := runtimeSvc.GetTLSMaterial(context.Background()) | |
| if err != nil { | |
| // loadCertConfig loads the TLS material via the runtime crypto provider. | |
| func loadCertConfig(logger *log.Logger, runtimeSvc kmprovider.RuntimeCryptoProvider) *tls.Config { | |
| ctx, cancel := context.WithTimeout(context.Background(), tlsMaterialLoadTimeout) | |
| defer cancel() | |
| mat, err := runtimeSvc.GetTLSMaterial(ctx) | |
| if err != nil { |
🤖 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/cmd/server/main.go` around lines 167 - 170, The loadCertConfig
function currently calls runtimeSvc.GetTLSMaterial with context.Background(),
which can block indefinitely; change it to use a bounded context (e.g.,
context.WithTimeout) when calling runtimeSvc.GetTLSMaterial inside
loadCertConfig, create the timeout context with a reasonable startup deadline,
defer the cancel function, use that context in the GetTLSMaterial call, and
ensure error handling detects and surfaces context deadline/cancellation errors
appropriately so startup won't hang.
Comment on lines
+167
to
170
| cek, err := js.cryptoProvider.Decrypt(ctx, &js.keyRef, params, encryptedKey) | ||
| if err != nil { | ||
| js.logger.Error("Failed to decrypt CEK: " + err.Error()) | ||
| return nil, &ErrorJWEDecryptionFailed |
Contributor
There was a problem hiding this comment.
Don't report canceled decrypts as bad JWE payloads.
Passing ctx into cryptoProvider.Decrypt is correct, but this now makes context.Canceled / deadline failures reachable here. Returning ErrorJWEDecryptionFailed for every provider error hides request aborts and makes callers treat them like malformed tokens.
Suggested fix
import (
"context"
"crypto"
"encoding/base64"
"encoding/json"
+ "errors"
"fmt"
@@
cek, err := js.cryptoProvider.Decrypt(ctx, &js.keyRef, params, encryptedKey)
if err != nil {
+ if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
+ return nil, &serviceerror.InternalServerError
+ }
js.logger.Error("Failed to decrypt CEK: " + err.Error())
return nil, &ErrorJWEDecryptionFailed
}🤖 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.go` around lines 167 - 170, The
current handling of Decrypt errors treats all provider errors as
ErrorJWEDecryptionFailed; change the error branch after calling
js.cryptoProvider.Decrypt(ctx, &js.keyRef, params, encryptedKey) to detect
context cancellation/timeouts and propagate them instead of converting to
ErrorJWEDecryptionFailed: if errors.Is(err, context.Canceled) || errors.Is(err,
context.DeadlineExceeded) (or simply if ctx.Err() != nil) return nil, ctx.Err()
(or return the original err) so request aborts are surfaced, otherwise log and
return &ErrorJWEDecryptionFailed; reference the call to
js.cryptoProvider.Decrypt, the js.logger.Error line, and the
ErrorJWEDecryptionFailed symbol when making the change.
Comment on lines
231
to
233
| // First verify signature using the configured server key and algorithm | ||
| if err := js.VerifyJWTSignature(jwtToken); err != nil { | ||
| if err := js.VerifyJWTSignature(ctx, jwtToken); err != nil { | ||
| return &ErrorInvalidTokenSignature |
Contributor
There was a problem hiding this comment.
Preserve non-signature failures from VerifyJWTSignature.
Now that VerifyJWTSignature uses the caller's ctx, provider lookup failures and canceled/deadline-exceeded requests are reachable here. Collapsing every error to ErrorInvalidTokenSignature turns those operational failures into bad-token responses for access/refresh-token validation.
Suggested fix
// First verify signature using the configured server key and algorithm
if err := js.VerifyJWTSignature(ctx, jwtToken); err != nil {
+ if err == &serviceerror.InternalServerError {
+ return err
+ }
return &ErrorInvalidTokenSignature
}🤖 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 231 - 233, The code
currently maps every error from VerifyJWTSignature to
ErrorInvalidTokenSignature; instead, change the error handling in the caller
(the block around VerifyJWTSignature in service.go) to preserve
operational/context errors and only convert true signature-failure errors to
ErrorInvalidTokenSignature: call js.VerifyJWTSignature(ctx, jwtToken), and if it
returns an error, check for context.Canceled/context.DeadlineExceeded
(errors.Is) and any provider/lookup errors (the specific sentinel or typed
errors returned by VerifyJWTSignature) and return those errors as-is (or
wrapped) while only returning &ErrorInvalidTokenSignature when the error
indicates a real signature/validation failure; reference VerifyJWTSignature and
ErrorInvalidTokenSignature to locate and update the conditional.
Comment on lines
+200
to
+207
| tlsCfg, err := s.pkiService.GetTLSConfig() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to load TLS config: %w", err) | ||
| } | ||
| return &kmprovider.TLSMaterial{ | ||
| Certificate: tlsCfg.Certificates[0], | ||
| MinVersion: tlsCfg.MinVersion, | ||
| }, nil |
Contributor
There was a problem hiding this comment.
Guard tlsCfg and certificate slice before indexing.
Line 205 dereferences tlsCfg.Certificates[0] without validating tlsCfg != nil and len(Certificates) > 0, which can panic.
Proposed fix
tlsCfg, err := s.pkiService.GetTLSConfig()
if err != nil {
return nil, fmt.Errorf("failed to load TLS config: %w", err)
}
+ if tlsCfg == nil {
+ return nil, errors.New("received nil TLS config")
+ }
+ if len(tlsCfg.Certificates) == 0 {
+ return nil, errors.New("TLS config has no certificates")
+ }
return &kmprovider.TLSMaterial{
Certificate: tlsCfg.Certificates[0],
MinVersion: tlsCfg.MinVersion,
}, nil📝 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
| tlsCfg, err := s.pkiService.GetTLSConfig() | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to load TLS config: %w", err) | |
| } | |
| return &kmprovider.TLSMaterial{ | |
| Certificate: tlsCfg.Certificates[0], | |
| MinVersion: tlsCfg.MinVersion, | |
| }, nil | |
| tlsCfg, err := s.pkiService.GetTLSConfig() | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to load TLS config: %w", err) | |
| } | |
| if tlsCfg == nil { | |
| return nil, errors.New("received nil TLS config") | |
| } | |
| if len(tlsCfg.Certificates) == 0 { | |
| return nil, errors.New("TLS config has no certificates") | |
| } | |
| return &kmprovider.TLSMaterial{ | |
| Certificate: tlsCfg.Certificates[0], | |
| MinVersion: tlsCfg.MinVersion, | |
| }, nil |
🤖 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/defaultkm/runtime_crypto_provider.go`
around lines 200 - 207, The code returns kmprovider.TLSMaterial using
tlsCfg.Certificates[0] without nil/length checks; modify the code around the
call to s.pkiService.GetTLSConfig() (the tlsCfg variable) to validate that
tlsCfg != nil and that len(tlsCfg.Certificates) > 0 before indexing, and if
either check fails return a clear error (e.g., "no TLS config" or "no
certificates available") instead of risking a panic when constructing
kmprovider.TLSMaterial.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
JeethJJ
force-pushed
the
thunder-km-8
branch
3 times, most recently
from
b46bf09 to
0e9369d
Compare
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
1 participant
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
Implements
GetTLSMaterialin the default key manager.The implementation wires through the existing
LoadTLSConfigutility and exposes the result via theRuntimeCryptoProviderinterface so that main.go no longer calls PKI internals directly.Approach
TLSMaterialwith aMinVersion uint16field so callers receive a ready-to-use TLS config without needing to know the minimum version policy.*KeyRefparameter fromGetTLSMaterial— TLS cert identity is not key-ref-based; it comes from server config paths.GetTLSConfig() (*tls.Config, error)toPKIServiceInterface, implemented by delegating toLoadTLSConfigwith the server'scfg.TLSpaths.runtimeCryptoService.GetTLSMaterialnow callspkiService.GetTLSConfig()and maps the result toTLSMaterial.loadCertConfigin main.go updated to accept aRuntimeCryptoProviderinstead of calling PKI internals directly;registerServicesnow returns the provider alongsideJWTServiceInterface.PKIServiceInterfaceMock,RuntimeCryptoProviderMock) to reflect interface changes.Related Issues
Summary by CodeRabbit