Use constant-time comparison for GitLab webhook secret - #692
Conversation
handleWebhook compared the configured WebhookSecret against the incoming X-Gitlab-Token header with a plain != check. Go's string inequality is not constant-time, so this leaks byte-position timing information about the secret on an endpoint that is intentionally unauthenticated apart from this check. Switch to subtle.ConstantTimeCompare, matching the pattern the sibling GitHub plugin already uses (hmac.Equal) for its webhook signature check.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughWebhook authentication now compares SHA-256 digests with constant-time comparison. Unauthorized requests still receive a 401 response. ChangesWebhook authentication
Estimated code review effort: 1 (Trivial) | ~3 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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}
|
|
||
| signature := r.Header.Get("X-Gitlab-Token") | ||
| if config.WebhookSecret != signature { | ||
| if subtle.ConstantTimeCompare([]byte(config.WebhookSecret), []byte(signature)) != 1 { |
There was a problem hiding this comment.
The remaining issue is that, unlike hmac.Equal(), the request's 'X-Gitlab-Token' can vary in it length, and subtle.ConstantTimeCompare returns 0 immediately if the two strings are different lengths. (And so you can at least determine the length of the secret.) So it seems we should keep going with this and compare digests of these two, i.e.:
secret := sha256.Sum256([]byte(config.WebhookSecret))
token := sha256.Sum256([]byte(signature))
if subtle.ConstantTimeCompare(secret[:], token[:]) != 1 {
http.Error(w, "Not authorized", http.StatusUnauthorized)
return
}
There was a problem hiding this comment.
You're right, ConstantTimeCompare returns 0 immediately on a length mismatch, which leaks the length of the configured secret. Switched to comparing fixed-length SHA-256 digests of the secret and the incoming token so the comparison is constant time regardless of input length. Pushed in 74dba17.
2 participants
Summary
handleWebhookcompares the configuredWebhookSecretagainst the incomingX-Gitlab-Tokenheader with a plain!=string comparison. Go's string inequality is not constant-time (it can return as soon as it hits a differing byte), which leaks timing information about the secret on an endpoint that has no other authentication.Fix: use
subtle.ConstantTimeCompare, matching the pattern the sibling GitHub plugin already uses (hmac.Equal) for its own webhook signature check.Test plan
TestHandleWebhookBadSecretand the valid-secret webhook tests inserver/webhook_test.gocover both branches; behavior is unchanged (only the comparison mechanism changed, not its outcome for equal/unequal inputs).Change Impact: 🟡 Medium
Reasoning: The change affects webhook authentication but remains isolated to one module. Existing tests cover valid and invalid secrets, and the unauthorized response remains unchanged.
Regression Risk: Low. The comparison uses fixed-length SHA-256 digests and preserves the existing validation result.
** QA Recommendation:** Focused manual QA is recommended for valid, invalid, and near-match
X-Gitlab-Tokenvalues. Skipping manual QA presents low functional risk.Generated by CodeRabbitAI