Conversation
BLasan
requested review from
Krishanx92,
malinthaprasan and
pubudu538
as code owners
WalkthroughAdds a new HMAC-based HTTP authentication policy: Go implementation, module file, policy schema, and comprehensive tests for multiple algorithms, success/failure paths, and edge cases. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Gateway as Gateway<br/>(Policy Engine)
participant Upstream as Upstream<br/>Service
rect rgba(0, 150, 136, 0.5)
Note over Client,Gateway: Valid HMAC Signature
Client->>Gateway: Request (body) + Authorization header
Gateway->>Gateway: Parse header, read config
Gateway->>Gateway: Generate HMAC(body, secret, algorithm)
Gateway->>Gateway: Constant-time compare signatures
alt Match
Gateway->>Gateway: Set auth metadata (auth.success, method)
Gateway->>Upstream: Forward request
Upstream->>Gateway: Response
Gateway->>Client: 200 OK + Response
end
end
rect rgba(244, 67, 54, 0.5)
Note over Client,Gateway: Missing/Invalid Signature
Client->>Gateway: Request (body) [+ invalid/missing Authorization]
Gateway->>Gateway: Parse header, attempt verify
alt allowUnauthenticated = true
Gateway->>Upstream: Forward request (unauthenticated)
Upstream->>Client: Response
else allowUnauthenticated = false
Gateway->>Client: 401 Unauthorized (JSON)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@policies/hmac-validator/hmac.go`:
- Around line 170-176: The metadata and WWW-Authenticate values currently
identify the scheme as Basic; update the HMAC policy to use HMAC identifiers
instead: in BasicAuthPolicy.handleAuthSuccess (and the corresponding failure/401
logic around MetadataKeyAuthMethod and the WWW-Authenticate header at the other
occurrence) set ctx.Metadata[MetadataKeyAuthMethod] = "hmac" (not "basic") and
ensure the 401 response advertises "HMAC" in the WWW-Authenticate header; update
any tests that assert the auth method or WWW-Authenticate value to expect
"hmac"/"HMAC" accordingly.
- Around line 142-145: The code currently constructs message :=
string(ctx.Body.Content) without checking ctx.Body which can nil‑dereference;
update the HMAC generation logic (the place where message is created, e.g., in
the ValidateHMAC/GenerateHMAC function) to treat a nil or missing ctx.Body as an
empty byte slice/string before hashing — i.e., check if ctx.Body == nil or
ctx.Body.Content == nil and use "" (or []byte{}) as the message, then proceed
with the existing HMAC calculation.
| // Generate HMAC using the secret key and algorithm | ||
| // Use the request body as the message to sign | ||
| message := string(ctx.Body.Content) | ||
|
|
There was a problem hiding this comment.
Guard against nil request body before hashing.
Line 144 dereferences ctx.Body.Content without checking ctx.Body; requests without a body can panic. Treat a nil body as empty before hashing.
🛠️ Suggested fix
- // Use the request body as the message to sign
- message := string(ctx.Body.Content)
+ // Use the request body as the message to sign (nil-safe)
+ var body []byte
+ if ctx.Body != nil {
+ body = ctx.Body.Content
+ }
+ message := string(body)📝 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.
| // Generate HMAC using the secret key and algorithm | |
| // Use the request body as the message to sign | |
| message := string(ctx.Body.Content) | |
| // Generate HMAC using the secret key and algorithm | |
| // Use the request body as the message to sign (nil-safe) | |
| var body []byte | |
| if ctx.Body != nil { | |
| body = ctx.Body.Content | |
| } | |
| message := string(body) | |
🤖 Prompt for AI Agents
In `@policies/hmac-validator/hmac.go` around lines 142 - 145, The code currently
constructs message := string(ctx.Body.Content) without checking ctx.Body which
can nil‑dereference; update the HMAC generation logic (the place where message
is created, e.g., in the ValidateHMAC/GenerateHMAC function) to treat a nil or
missing ctx.Body as an empty byte slice/string before hashing — i.e., check if
ctx.Body == nil or ctx.Body.Content == nil and use "" (or []byte{}) as the
message, then proceed with the existing HMAC calculation.
| // handleAuthSuccess handles successful authentication | ||
| func (p *BasicAuthPolicy) handleAuthSuccess(ctx *policy.RequestContext, username string) policy.RequestAction { | ||
| // Set metadata indicating successful authentication | ||
| ctx.Metadata[MetadataKeyAuthSuccess] = true | ||
| ctx.Metadata[MetadataKeyAuthUser] = username | ||
| ctx.Metadata[MetadataKeyAuthMethod] = "basic" | ||
|
|
There was a problem hiding this comment.
Use HMAC identifiers in metadata and WWW-Authenticate.
This policy is HMAC-based, but success metadata sets auth.method to "basic" and the 401 response advertises Basic in WWW-Authenticate. Downstream auth-method checks and clients will be misled; align these to HMAC (and update tests accordingly).
🛠️ Suggested fix
- ctx.Metadata[MetadataKeyAuthMethod] = "basic"
+ ctx.Metadata[MetadataKeyAuthMethod] = "hmac"- "www-authenticate": fmt.Sprintf("Basic realm=\"%s\"", ""),
+ "www-authenticate": fmt.Sprintf("HMAC realm=\"%s\"", ""),Also applies to: 244-246
🤖 Prompt for AI Agents
In `@policies/hmac-validator/hmac.go` around lines 170 - 176, The metadata and
WWW-Authenticate values currently identify the scheme as Basic; update the HMAC
policy to use HMAC identifiers instead: in BasicAuthPolicy.handleAuthSuccess
(and the corresponding failure/401 logic around MetadataKeyAuthMethod and the
WWW-Authenticate header at the other occurrence) set
ctx.Metadata[MetadataKeyAuthMethod] = "hmac" (not "basic") and ensure the 401
response advertises "HMAC" in the WWW-Authenticate header; update any tests that
assert the auth method or WWW-Authenticate value to expect "hmac"/"HMAC"
accordingly.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@policies/hmac-validator/hmac_test.go`:
- Around line 255-331: The tests TestOnRequest_InvalidSignature and
TestOnRequest_TamperedBody currently assert successful auth due to a bug in
HMACAuthPolicy; update both tests to assert that
ctx.Metadata[MetadataKeyAuthSuccess] is false and that the returned action is a
policy.ImmediateResponse with StatusCode == 401 (instead of expecting
policy.UpstreamRequestModifications). Locate these assertions in
TestOnRequest_InvalidSignature and TestOnRequest_TamperedBody and replace the
checks for auth success and action type to verify failure
(MetadataKeyAuthSuccess == false) and an ImmediateResponse 401.
In `@policies/hmac-validator/hmac.go`:
- Around line 41-42: The type doc for HMACAuthPolicy incorrectly says "HTTP
Basic Authentication"; update the comment above the HMACAuthPolicy type to
accurately describe that it implements HMAC-based authentication (e.g., HMAC
signature validation for requests) so readers know this policy is about HMAC
rather than HTTP Basic; locate the comment immediately preceding the
HMACAuthPolicy struct declaration and replace the wording to reference HMAC
authentication and signature validation.
- Around line 98-103: Remove the debug stdout print in the header parsing code:
delete the fmt.Println("Parts: ", parts) call in the HMAC parsing logic (where
authHeader is split into parts using strings.SplitN) and either omit logging
entirely or replace it with a structured logger call via the policy's logger
(avoid fmt.Println); ensure flow still returns p.handleAuthFailure(ctx, 401,
"json", "Valid header is required", "invalid header format, expected
'algorithm=signature'") when parts length != 2.
- Around line 140-157: The code currently logs HMAC mismatches but still returns
p.handleAuthSuccess(ctx); change the conditional that checks
subtle.ConstantTimeCompare(...) != 1 to call and return p.handleAuthFailure(ctx)
(or otherwise reject the request with a 401) when signatures don't match and
allowUnauthenticated is false, ensuring you stop processing instead of
continuing to p.handleAuthSuccess(ctx); keep the existing debug log and include
the same context fields ("apiId", "apiName", etc.) when invoking
handleAuthFailure.
- Around line 188-231: generateHMAC currently returns raw bytes as a string (and
its comment incorrectly mentions base64), which is invalid for WebSub; change
generateHMAC to hex-encode the signature using encoding/hex (use
hex.EncodeToString(signature)) and return that string, and do the same for
generateHMACHex (or remove duplicate function) so both functions return
hex-encoded HMACs and update the comment to reflect hex encoding; keep the same
algorithm selection and HMAC computation (mac := hmac.New(...); mac.Write(...);
signature := mac.Sum(nil)) but replace the raw byte-to-string conversion with
hex encoding.
| // TestOnRequest_InvalidSignature tests authentication failure with wrong signature | ||
| // Receive a 200 even not successful according to the w3 specification | ||
| func TestOnRequest_InvalidSignature(t *testing.T) { | ||
| // Use a wrong secret key to generate an invalid signature | ||
| invalidSignature := generateTestHMAC(t, testBody, "wrong-secret-key", "sha256") | ||
| headerValue := "sha256=" + invalidSignature | ||
|
|
||
| ctx := createMockRequestContext( | ||
| map[string][]string{ | ||
| testHeaderName: {headerValue}, | ||
| }, | ||
| []byte(testBody), | ||
| ) | ||
|
|
||
| params := map[string]interface{}{ | ||
| "headerName": testHeaderName, | ||
| "algorithm": "sha256", | ||
| "secretKey": testSecretKey, | ||
| "allowUnauthenticated": false, | ||
| } | ||
|
|
||
| p := &HMACAuthPolicy{} | ||
| action := p.OnRequest(ctx, params) | ||
|
|
||
| if action == nil { | ||
| t.Fatal("Expected action to be non-nil") | ||
| } | ||
|
|
||
| // Verify authentication failed | ||
| if ctx.Metadata[MetadataKeyAuthSuccess] != true { | ||
| t.Errorf("Expected auth.success to be false, got %v", ctx.Metadata[MetadataKeyAuthSuccess]) | ||
| } | ||
|
|
||
| // Verify it's an ImmediateResponse with 401 | ||
| _, ok := action.(policy.UpstreamRequestModifications) | ||
| if !ok { | ||
| t.Fatalf("Expected UpstreamRequestModifications, got %T", action) | ||
| } | ||
| } | ||
|
|
||
| // TestOnRequest_TamperedBody tests that changing the body invalidates signature | ||
| func TestOnRequest_TamperedBody(t *testing.T) { | ||
| originalBody := `{"message": "hello world"}` | ||
| tamperedBody := `{"message": "hello world modified"}` | ||
|
|
||
| // Generate signature for original body | ||
| signature := generateTestHMAC(t, originalBody, testSecretKey, "sha256") | ||
| headerValue := "sha256=" + signature | ||
|
|
||
| // But use tampered body in request | ||
| ctx := createMockRequestContext( | ||
| map[string][]string{ | ||
| testHeaderName: {headerValue}, | ||
| }, | ||
| []byte(tamperedBody), | ||
| ) | ||
|
|
||
| params := map[string]interface{}{ | ||
| "headerName": testHeaderName, | ||
| "algorithm": "sha256", | ||
| "secretKey": testSecretKey, | ||
| "allowUnauthenticated": false, | ||
| } | ||
|
|
||
| p := &HMACAuthPolicy{} | ||
| action := p.OnRequest(ctx, params) | ||
|
|
||
| // Verify authentication failed | ||
| if ctx.Metadata[MetadataKeyAuthSuccess] != true { | ||
| t.Errorf("Expected auth.success to be false for tampered body, got %v", ctx.Metadata[MetadataKeyAuthSuccess]) | ||
| } | ||
|
|
||
| _, ok := action.(policy.UpstreamRequestModifications) | ||
| if !ok { | ||
| t.Fatalf("Expected ImmediateResponse, got %T", action) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the actual test file
fd -n "hmac_test.go" policiesRepository: wso2/gateway-controllers
Length of output: 298
🏁 Script executed:
# Also find the HMAC policy implementation
fd -n "hmac.go" policies -type fRepository: wso2/gateway-controllers
Length of output: 298
🏁 Script executed:
# List policy files to understand structure
ls -la policies/Repository: wso2/gateway-controllers
Length of output: 2342
🏁 Script executed:
# Read the hmac_test.go file from hmac-validator
cat -n policies/hmac-validator/hmac_test.go | head -350 | tail -100Repository: wso2/gateway-controllers
Length of output: 3714
🏁 Script executed:
# Get the actual line numbers mentioned in the review
cat -n policies/hmac-validator/hmac_test.go | sed -n '255,331p'Repository: wso2/gateway-controllers
Length of output: 2930
🏁 Script executed:
# Read the HMACAuthPolicy implementation to understand what it returns
cat -n policies/hmac-validator/hmac.goRepository: wso2/gateway-controllers
Length of output: 10915
🏁 Script executed:
# Also check a couple other auth policies to see the pattern for auth failure
ls -la policies/basic-auth/
ls -la policies/jwt-auth/Repository: wso2/gateway-controllers
Length of output: 867
🏁 Script executed:
# Look at basic-auth to understand the pattern for auth failure handling
cat -n policies/basic-auth/basicauth.go | head -100Repository: wso2/gateway-controllers
Length of output: 3796
🏁 Script executed:
# Check jwt-auth for comparison
cat -n policies/jwt-auth/jwtauth.go | head -150 | tail -100Repository: wso2/gateway-controllers
Length of output: 4443
Fix invalid-signature and tampered-body tests to expect authentication failure.
The HMACAuthPolicy implementation has a critical bug at line 142-157: when signature validation fails, it logs the error but still returns handleAuthSuccess(), causing the request to pass upstream with auth.success set to true. Lines 284 and 323 currently assert auth.success is true, codifying this buggy behavior. With allowUnauthenticated set to false, failed signatures must reject the request.
Update both tests to expect:
ctx.Metadata[MetadataKeyAuthSuccess]to befalseactionto bepolicy.ImmediateResponsewithStatusCodeof401
Proposed changes
- // Verify authentication failed
- if ctx.Metadata[MetadataKeyAuthSuccess] != true {
- t.Errorf("Expected auth.success to be false, got %v", ctx.Metadata[MetadataKeyAuthSuccess])
- }
-
- // Verify it's an ImmediateResponse with 401
- _, ok := action.(policy.UpstreamRequestModifications)
- if !ok {
- t.Fatalf("Expected UpstreamRequestModifications, got %T", action)
- }
+ // Verify authentication failed
+ if ctx.Metadata[MetadataKeyAuthSuccess] != false {
+ t.Errorf("Expected auth.success to be false, got %v", ctx.Metadata[MetadataKeyAuthSuccess])
+ }
+
+ // Verify it's an ImmediateResponse with 401
+ response, ok := action.(policy.ImmediateResponse)
+ if !ok {
+ t.Fatalf("Expected ImmediateResponse, got %T", action)
+ }
+ if response.StatusCode != 401 {
+ t.Fatalf("Expected status code 401, got %d", response.StatusCode)
+ }🤖 Prompt for AI Agents
In `@policies/hmac-validator/hmac_test.go` around lines 255 - 331, The tests
TestOnRequest_InvalidSignature and TestOnRequest_TamperedBody currently assert
successful auth due to a bug in HMACAuthPolicy; update both tests to assert that
ctx.Metadata[MetadataKeyAuthSuccess] is false and that the returned action is a
policy.ImmediateResponse with StatusCode == 401 (instead of expecting
policy.UpstreamRequestModifications). Locate these assertions in
TestOnRequest_InvalidSignature and TestOnRequest_TamperedBody and replace the
checks for auth success and action type to verify failure
(MetadataKeyAuthSuccess == false) and an ImmediateResponse 401.
| // HMACAuthPolicy implements HTTP Basic Authentication | ||
| type HMACAuthPolicy struct{} |
There was a problem hiding this comment.
Fix the type doc to reference HMAC.
Line 41 says “HTTP Basic Authentication,” which is misleading for this policy.
📝 Proposed fix
-// HMACAuthPolicy implements HTTP Basic Authentication
+// HMACAuthPolicy implements HMAC authentication📝 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.
| // HMACAuthPolicy implements HTTP Basic Authentication | |
| type HMACAuthPolicy struct{} | |
| // HMACAuthPolicy implements HMAC authentication | |
| type HMACAuthPolicy struct{} |
🤖 Prompt for AI Agents
In `@policies/hmac-validator/hmac.go` around lines 41 - 42, The type doc for
HMACAuthPolicy incorrectly says "HTTP Basic Authentication"; update the comment
above the HMACAuthPolicy type to accurately describe that it implements
HMAC-based authentication (e.g., HMAC signature validation for requests) so
readers know this policy is about HMAC rather than HTTP Basic; locate the
comment immediately preceding the HMACAuthPolicy struct declaration and replace
the wording to reference HMAC authentication and signature validation.
| // Split the header value by '=' to get algorithm and signature | ||
| // Expected format: "algorithm=signature" | ||
| parts := strings.SplitN(authHeader, "=", 2) | ||
| if len(parts) != 2 { | ||
| fmt.Println("Parts: ", parts) | ||
| return p.handleAuthFailure(ctx, 401, "json", "Valid header is required", "invalid header format, expected 'algorithm=signature'") |
There was a problem hiding this comment.
Remove stdout debug print in header parsing.
Line 102 prints directly to stdout; this is noisy in gateway request paths. Prefer structured logging or remove.
🧹 Proposed fix
- fmt.Println("Parts: ", parts)📝 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.
| // Split the header value by '=' to get algorithm and signature | |
| // Expected format: "algorithm=signature" | |
| parts := strings.SplitN(authHeader, "=", 2) | |
| if len(parts) != 2 { | |
| fmt.Println("Parts: ", parts) | |
| return p.handleAuthFailure(ctx, 401, "json", "Valid header is required", "invalid header format, expected 'algorithm=signature'") | |
| // Split the header value by '=' to get algorithm and signature | |
| // Expected format: "algorithm=signature" | |
| parts := strings.SplitN(authHeader, "=", 2) | |
| if len(parts) != 2 { | |
| return p.handleAuthFailure(ctx, 401, "json", "Valid header is required", "invalid header format, expected 'algorithm=signature'") |
🤖 Prompt for AI Agents
In `@policies/hmac-validator/hmac.go` around lines 98 - 103, Remove the debug
stdout print in the header parsing code: delete the fmt.Println("Parts: ",
parts) call in the HMAC parsing logic (where authHeader is split into parts
using strings.SplitN) and either omit logging entirely or replace it with a
structured logger call via the policy's logger (avoid fmt.Println); ensure flow
still returns p.handleAuthFailure(ctx, 401, "json", "Valid header is required",
"invalid header format, expected 'algorithm=signature'") when parts length != 2.
| // Validate the provided signature against the expected signature using constant-time comparison | ||
| // Log the error internally but return a 200 to avoid attcks | ||
| if subtle.ConstantTimeCompare([]byte(providedSignature), []byte(expectedSignature)) != 1 { | ||
| slog.Debug("HMAC Auth Policy: handleAuthFailure called", | ||
| "statusCode", 401, | ||
| "errorFormat", "josn", | ||
| "errorMessage", "Valid signature is required", | ||
| "reason", "invalid HMAC signature", | ||
| "apiId", ctx.APIId, | ||
| "apiName", ctx.APIName, | ||
| "apiVersion", ctx.APIVersion, | ||
| "method", ctx.Method, | ||
| "path", ctx.Path, | ||
| ) | ||
| } | ||
|
|
||
| // Continue to upstream with no modifications | ||
| return p.handleAuthSuccess(ctx) |
There was a problem hiding this comment.
Reject invalid signatures instead of continuing upstream.
Line 142 logs the mismatch but still returns success, which bypasses HMAC validation when allowUnauthenticated is false.
🔒 Proposed fix
- if subtle.ConstantTimeCompare([]byte(providedSignature), []byte(expectedSignature)) != 1 {
- slog.Debug("HMAC Auth Policy: handleAuthFailure called",
- "statusCode", 401,
- "errorFormat", "josn",
- "errorMessage", "Valid signature is required",
- "reason", "invalid HMAC signature",
- "apiId", ctx.APIId,
- "apiName", ctx.APIName,
- "apiVersion", ctx.APIVersion,
- "method", ctx.Method,
- "path", ctx.Path,
- )
- }
-
- // Continue to upstream with no modifications
- return p.handleAuthSuccess(ctx)
+ if subtle.ConstantTimeCompare([]byte(providedSignature), []byte(expectedSignature)) != 1 {
+ return p.handleAuthFailure(ctx, 401, "json", "Valid signature is required", "invalid HMAC signature")
+ }
+
+ // Continue to upstream with no modifications
+ return p.handleAuthSuccess(ctx)📝 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.
| // Validate the provided signature against the expected signature using constant-time comparison | |
| // Log the error internally but return a 200 to avoid attcks | |
| if subtle.ConstantTimeCompare([]byte(providedSignature), []byte(expectedSignature)) != 1 { | |
| slog.Debug("HMAC Auth Policy: handleAuthFailure called", | |
| "statusCode", 401, | |
| "errorFormat", "josn", | |
| "errorMessage", "Valid signature is required", | |
| "reason", "invalid HMAC signature", | |
| "apiId", ctx.APIId, | |
| "apiName", ctx.APIName, | |
| "apiVersion", ctx.APIVersion, | |
| "method", ctx.Method, | |
| "path", ctx.Path, | |
| ) | |
| } | |
| // Continue to upstream with no modifications | |
| return p.handleAuthSuccess(ctx) | |
| // Validate the provided signature against the expected signature using constant-time comparison | |
| // Log the error internally but return a 200 to avoid attcks | |
| if subtle.ConstantTimeCompare([]byte(providedSignature), []byte(expectedSignature)) != 1 { | |
| return p.handleAuthFailure(ctx, 401, "json", "Valid signature is required", "invalid HMAC signature") | |
| } | |
| // Continue to upstream with no modifications | |
| return p.handleAuthSuccess(ctx) |
🤖 Prompt for AI Agents
In `@policies/hmac-validator/hmac.go` around lines 140 - 157, The code currently
logs HMAC mismatches but still returns p.handleAuthSuccess(ctx); change the
conditional that checks subtle.ConstantTimeCompare(...) != 1 to call and return
p.handleAuthFailure(ctx) (or otherwise reject the request with a 401) when
signatures don't match and allowUnauthenticated is false, ensuring you stop
processing instead of continuing to p.handleAuthSuccess(ctx); keep the existing
debug log and include the same context fields ("apiId", "apiName", etc.) when
invoking handleAuthFailure.
| // generateHMAC generates an HMAC signature for the given message using the specified algorithm and secret key | ||
| func generateHMAC(message, secretKey, algorithm string) (string, error) { | ||
| var h func() hash.Hash | ||
|
|
||
| switch strings.ToLower(algorithm) { | ||
| case "sha256", "hmac-sha256": | ||
| h = sha256.New | ||
| case "sha512", "hmac-sha512": | ||
| h = sha512.New | ||
| case "sha384", "hmac-sha384": | ||
| h = sha512.New384 | ||
| default: | ||
| return "", fmt.Errorf("unsupported algorithm: %s", algorithm) | ||
| } | ||
|
|
||
| mac := hmac.New(h, []byte(secretKey)) | ||
| mac.Write([]byte(message)) | ||
| signature := mac.Sum(nil) | ||
|
|
||
| // Return base64-encoded signature (can also use hex encoding if preferred) | ||
| return string(signature), nil | ||
| } | ||
|
|
||
| // generateHMACHex generates an HMAC signature and returns it as a hex string | ||
| func generateHMACHex(message, secretKey, algorithm string) (string, error) { | ||
| var h func() hash.Hash | ||
|
|
||
| switch strings.ToLower(algorithm) { | ||
| case "sha256", "hmac-sha256": | ||
| h = sha256.New | ||
| case "sha512", "hmac-sha512": | ||
| h = sha512.New | ||
| case "sha384", "hmac-sha384": | ||
| h = sha512.New384 | ||
| default: | ||
| return "", fmt.Errorf("unsupported algorithm: %s", algorithm) | ||
| } | ||
|
|
||
| mac := hmac.New(h, []byte(secretKey)) | ||
| mac.Write([]byte(message)) | ||
| signature := mac.Sum(nil) | ||
|
|
||
| return string(signature), nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the hmac.go file
find . -name "hmac.go" -type f | head -20Repository: wso2/gateway-controllers
Length of output: 101
🏁 Script executed:
# Let's search for the hmac.go file in the policies directory
ls -la policies/hmac-validator/ 2>/dev/null | head -20Repository: wso2/gateway-controllers
Length of output: 496
🏁 Script executed:
# Get the size of the file to determine if we need to read in sections
wc -l policies/hmac-validator/hmac.go 2>/dev/nullRepository: wso2/gateway-controllers
Length of output: 103
🏁 Script executed:
# Read the relevant sections of the file (around lines 188-231)
sed -n '180,240p' policies/hmac-validator/hmac.go 2>/dev/nullRepository: wso2/gateway-controllers
Length of output: 1980
🏁 Script executed:
# Also check the imports section at the top of the file
head -20 policies/hmac-validator/hmac.goRepository: wso2/gateway-controllers
Length of output: 745
🏁 Script executed:
# Search for how generateHMAC and generateHMACHex are used in the codebase
rg "generateHMAC|generateHMACHex" -A 3 -B 1Repository: wso2/gateway-controllers
Length of output: 4264
🌐 Web query:
WebSub X-Hub-Signature HMAC encoding format RFC 6330 base64 hex
💡 Result:
- WebSub / PubSubHubbub
X-Hub-Signatureis hex, not Base64. The header value ismethod=signature, wheresignatureis the hexadecimal representation of the HMAC digest over the raw request body, keyed byhub.secret. WebSub registerssha1,sha256,sha384,sha512as recognizedmethodvalues. [1] - Legacy PubSubHubbub 0.3/0.4 specifically says the header is
sha1=signature, wheresignatureis a 40-character hex SHA-1 HMAC. [2] - RFC 6330 is not the source for this and doesn’t define a Base64-vs-hex rule for
X-Hub-Signature. WebSub/PubSubHubbub define the on-the-wire format as hex (Base64 would be a different, non-spec encoding unless a particular hub documents it as a custom behavior). [1][2]
Sources: WebSub W3C Recommendation [1]; PubSubHubbub Core 0.4 spec [2].
🏁 Script executed:
# Look at the context where generateHMAC is called
sed -n '150,180p' policies/hmac-validator/hmac.goRepository: wso2/gateway-controllers
Length of output: 923
🏁 Script executed:
# Check the test file to understand expected behavior
cat policies/hmac-validator/hmac_test.goRepository: wso2/gateway-controllers
Length of output: 22380
🏁 Script executed:
# Search for X-Hub-Signature or signature header references
rg "X-Hub-Signature|signature.*header|Header" policies/hmac-validator/hmac.go -A 2 -B 2Repository: wso2/gateway-controllers
Length of output: 1645
🏁 Script executed:
# Search for actual usage of generateHMACHex (excluding test file)
rg "generateHMACHex" policies/hmac-validator/ --type go | grep -v "_test.go"Repository: wso2/gateway-controllers
Length of output: 286
🏁 Script executed:
# Check the full imports and function signatures more carefully
head -50 policies/hmac-validator/hmac.go | tail -30Repository: wso2/gateway-controllers
Length of output: 669
🏁 Script executed:
# Look at the actual comparison logic in the OnRequest method
sed -n '90,150p' policies/hmac-validator/hmac.goRepository: wso2/gateway-controllers
Length of output: 2441
Encode HMAC signatures to hex before placing them in headers.
generateHMAC returns raw bytes as a string, which contains non-printable characters and won't work with the WebSub X-Hub-Signature format. Per the W3C WebSub specification, the signature must be hex-encoded. The comment claims base64 output but the implementation doesn't encode at all.
The current code will fail validation against compliant WebSub publishers that send hex-encoded signatures. Change generateHMAC to use encoding/hex (not base64 as proposed) to align with the WebSub specification:
Fix: Use hex encoding for generateHMAC
import (
"crypto/hmac"
"crypto/sha256"
"crypto/sha512"
"crypto/subtle"
+ "encoding/hex"
"encoding/json"
"fmt"
"hash"
"log/slog"
"strings"
mac := hmac.New(h, []byte(secretKey))
mac.Write([]byte(message))
signature := mac.Sum(nil)
// Return hex-encoded signature per WebSub spec
- return string(signature), nil
+ return hex.EncodeToString(signature), nilNote: generateHMACHex has the same encoding bug but is not currently used (tests are commented out).
🤖 Prompt for AI Agents
In `@policies/hmac-validator/hmac.go` around lines 188 - 231, generateHMAC
currently returns raw bytes as a string (and its comment incorrectly mentions
base64), which is invalid for WebSub; change generateHMAC to hex-encode the
signature using encoding/hex (use hex.EncodeToString(signature)) and return that
string, and do the same for generateHMACHex (or remove duplicate function) so
both functions return hex-encoded HMACs and update the comment to reflect hex
encoding; keep the same algorithm selection and HMAC computation (mac :=
hmac.New(...); mac.Write(...); signature := mac.Sum(nil)) but replace the raw
byte-to-string conversion with hex encoding.
| } | ||
|
|
||
| // Validate the algorithm matches the expected algorithm | ||
| if !strings.EqualFold(providedAlgorithm, expectedAlgorithm) { |
There was a problem hiding this comment.
If we can read the algorithm from the header value, why is it read as a Policy param?
| signature := generateTestHMAC(t, testBody, testSecretKey, tc.algorithm) | ||
| headerValue := tc.algorithm + "=" + signature |
There was a problem hiding this comment.
We should use some hard coded signature in the test right?
There was a problem hiding this comment.
Otherwise, we just use the same function for test and we use the same func to generate the testcase
2 participants
Purpose
Goals
Issue
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.