🛡️ Sentinel: [MEDIUM] Add VerifyHMACSha256 for secure HMAC verification#32
🛡️ Sentinel: [MEDIUM] Add VerifyHMACSha256 for secure HMAC verification#32
Conversation
Co-authored-by: Laisky <4532436+Laisky@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
There was a problem hiding this comment.
Pull request overview
Adds a dedicated HMAC-SHA256 verification helper to the crypto package to encourage constant-time signature comparison and reduce timing-attack risk.
Changes:
- Introduces
VerifyHMACSha256incrypto/sign.gousingcrypto/hmac.Equal. - Adds
TestVerifyHMACSha256incrypto/sign_test.gocovering success and mismatch scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
crypto/sign.go |
Adds a new public verification helper for HMAC-SHA256 signatures. |
crypto/sign_test.go |
Adds unit tests for the new HMAC verification helper. |
You can also share your feedback on Copilot code review. Take the survey.
| // VerifyHMACSha256 verify HMAC by sha256 | ||
| // | ||
| // # Args: | ||
| // - key: secure key, no limit on length | ||
| // - data: raw data to verify HMAC | ||
| // - signature: HMAC signature to verify | ||
| // | ||
| // # Returns: | ||
| // - err: nil if signature match | ||
| func VerifyHMACSha256(key, signature []byte, data io.Reader) error { |
There was a problem hiding this comment.
The doc comment for VerifyHMACSha256 lists args in the order key, data, signature, but the function signature is func VerifyHMACSha256(key, signature []byte, data io.Reader). Please update the comment (or reorder parameters) so the documented argument order matches the actual API and avoids misuse by callers.
| if !hmac.Equal(h, signature) { | ||
| return errors.New("signature not match") |
There was a problem hiding this comment.
VerifyHMACSha256 accepts any signature length and relies on hmac.Equal. hmac.Equal returns immediately when lengths differ, which undermines the goal of constant-time verification and also differs from other codepaths that enforce a 32-byte HMAC (e.g., SM4 decrypt validates HMAC length). Consider explicitly validating that signature is exactly 32 bytes (sha256 size) and returning an error otherwise.
| } | ||
|
|
||
| if !hmac.Equal(h, signature) { | ||
| return errors.New("signature not match") |
There was a problem hiding this comment.
The error string "signature not match" is both grammatically incorrect and inconsistent with other verification helpers in this package (e.g., VerifyByEd25519WithSHA512 returns "invalid signature", and SM4 HMAC verification returns "hmac not match"). Consider using a consistent message (and update tests accordingly) so callers can reliably match/handle verification failures.
| return errors.New("signature not match") | |
| return errors.New("hmac not match") |
🚨 Severity: MEDIUM
💡 Vulnerability: Lack of a dedicated HMAC verification function often leads developers to use non-constant-time comparison methods (like bytes.Equal), making the system vulnerable to timing attacks.
🎯 Impact: Attackers might exploit timing differences to recover valid HMAC signatures bit by bit.
🔧 Fix: Implemented VerifyHMACSha256 in crypto/sign.go using crypto/hmac.Equal for secure, constant-time verification.
✅ Verification: Added TestVerifyHMACSha256 in crypto/sign_test.go to verify success and failure cases.
PR created automatically by Jules for task 15182121104108134464 started by @Laisky