[Lang-Akshay]

Summary

• Webhook HMAC Replay Prevention: Binds webhook HMAC signatures to timestamps and unique delivery IDs to prevent replay attacks. The new timestamp binding scheme is opt-in via hmac_include_timestamp (defaults to false for backward compatibility) and includes HMAC algorithm allowlisting (sha256, sha384, sha512) and secret validation.
• Per-Gateway Identity Propagation: Honors per-gateway identity_propagation settings on tool and resource execution paths, with cache fallback to global configuration.

Closes internal issue 1149

[msureshkumar88]

Review: request changes

The timestamp binding is the right fix for HMAC replay, and the identity-propagation change aligns tool and resource calls with the documented per-gateway override. Both are backend-only. Neither needs an Alembic migration. Tests pass locally and CI is green. The items below block merge.

severity | path:line | problem | fix

Blocking

blocking | plugins/webhook_notification/webhook_notification.py:251 | The code computes timestamp and the signature once, before the retry loop. Retries send a stale timestamp. At retry_delay up to 60000 ms and retry_attempts up to 10, a receiver that enforces freshness rejects the retries. | Compute the timestamp and signature inside the for attempt loop. Add a test that each attempt sends a fresh timestamp.

blocking | plugins/webhook_notification/webhook_notification.py:169-188 | Breaking consumer contract. X-Signature was HMAC(body) and is now HMAC("{ts}.{body}"). Every existing HMAC receiver fails verification after upgrade. Alerting webhooks go quiet and retries multiply load on the receiver. There is no flag, no versioning, and no CHANGELOG entry. | Make the new scheme opt-in for one release (for example hmac_include_timestamp), or version the signature value. Add a CHANGELOG "Breaking" entry.

blocking | plugins/webhook_notification/README.md:62-70, TESTING.md:315-328 | Docs describe the old scheme. TESTING.md shows a verifier that fails against the new signature. README omits hmac_timestamp_header, the signed format, and the receiver freshness check. | Document the format. Add a receiver example with hmac.compare_digest and a freshness window.

blocking | mcpgateway/services/tool_service.py:5439-5441 | The "fail closed" default {"enabled": False} applies when the cache payload lacks the key. A cache entry from an older worker (rolling upgrade, shared Redis L2) then disables identity headers for up to the cache TTL, even when global propagation is on. A key with value None falls back to global. The two "unset" states behave differently. | Treat a missing key like None. Or bump the cache payload version. Add a cache-hit test for a payload without the key.

blocking | PR scope | The PR carries two unrelated fixes. The identity-propagation commit (0c7f9ad80) has no issue reference and does not match the title. The A2A test edits duplicate #7331, which is on main. This branch is 1 commit behind main, so a rebase conflicts. | Move the identity-propagation fix to its own PR and issue, or retitle this PR to cover both. Drop the A2A test edits. Rebase on main. Squash the fixup: commit.

Functionally-impacting

functionally-impacting | webhook_notification.py:251 | The timestamp is the only freshness input. No delivery ID is signed, so replay inside the window succeeds, and webhooks that share a secret accept each other's deliveries (CWE-294). | Add an X-Delivery-Id header and include it in the signed material. Document that receivers must reject duplicate IDs.

functionally-impacting | webhook_notification.py:185 | getattr(hashlib, algorithm, hashlib.sha256) accepts any hashlib attribute and silently falls back to sha256 while the header keeps the configured name (CWE-327, CWE-20). | Validate hmac_algorithm against sha256, sha384, sha512 in a Pydantic validator.

functionally-impacting | webhook_notification.py:250 | type: hmac without hmac_secret sends the webhook unsigned and logs nothing (CWE-306). | Add a model validator that requires the secret for hmac. Log a warning at plugin init.

functionally-impacting | tests | Test gaps:

• No known-answer HMAC test. Current tests check only the prefix and header presence.
• No test that retries re-sign.
• No cache-hit test for identity propagation.
• The MCP identity test lacks the None case.
• The direct-proxy path (tool_service.py:5097) has no behaviour test.
• No live-gateway test under tests/live_gateway/, which the repo requires for behaviour changes.
• No deny-path test for the identity change. | Add the tests listed.

functionally-impacting | PR body | The body says "Security hardening PR". The issue link points to github.ibm.com, so closes does not apply. The text has a typo ("inernal"). The body gives no change summary or breaking-change note. | Describe the change. Link a public issue or state the internal ID with its acceptance criteria. Add the breaking-change note and migration path.

Suggestions

suggestion | webhook_notification.py:86 | hmac_timestamp_header accepts an empty value or the same value as hmac_header, which overwrites the signature header. | Add a validator.

suggestion | tool_service.py:5440-5441 | SimpleNamespace stands in for a gateway so _resolve_config() can call getattr. | Make build_identity_headers() and build_identity_meta() take identity_propagation: Optional[dict].

suggestion | tool_service.py:1432 vs :5097 | Two forms of the same lookup (getattr(...) and direct attribute). | Use one form.

Security note. The identity change closes an over-disclosure path. A gateway set to enabled: false still received user identity headers when the global setting was on (CWE-200). Call it out as a security fix in the release notes. The blast radius is deployments with global propagation on and a gateway override of off, or the reverse.

Full analysis with line references is available on request.

[msureshkumar88]

Re-review: request changes

The fixes since the last review land well. Retries now sign a fresh timestamp. The new HMAC scheme is opt-in through hmac_include_timestamp, with a CHANGELOG entry. The README and TESTING.md describe the format and give a receiver example. A cache payload without identity_propagation falls back to the global setting. The A2A test edits are gone, and the branch is rebased on main. The new known-answer, retry, cache-hit, deny-path and live-gateway tests pass locally.

Four items remain from the previous review.

severity | path:line | problem | fix

blocking | PR title and body | The PR carries two fixes (webhook HMAC and identity propagation). The title names only the webhook fix. The body says "Security hardening PR" and has the typo "inernal". The PR still claims less than it delivers. | Retitle to cover both fixes. Replace the body with a summary of both changes. State that hmac_include_timestamp is opt-in. Fix the typo.

functionally-impacting | commit dbc7c5f49 | The fixup: test and coverage commit is still in the history. | Squash it into its parent commit.

functionally-impacting | plugins/webhook_notification/webhook_notification.py:85 | hmac_algorithm has no validation. getattr(hashlib, algorithm, hashlib.sha256) accepts any hashlib attribute. An unknown name falls back to sha256 while the header keeps the configured name (CWE-327, CWE-20). | Check hmac_algorithm against sha256, sha384, sha512 in _validate_hmac_headers. Add a test for an invalid name.

functionally-impacting | plugins/webhook_notification/webhook_notification.py:275 | type: hmac without hmac_secret sends the webhook unsigned and logs nothing (CWE-306). | Require a non-empty hmac_secret for type: hmac in the same validator. Add a test.

The last two items are about ten lines in the existing validator. If the author defers them, state the waiver in the PR body.

CI still has pending jobs, including pytest (py3.12), pylint, vulture and conformance. I will approve when the four items are done and those jobs pass.

[msureshkumar88]

Re-review: request changes

All four items from the last review are done. The title and body cover both fixes and state that hmac_include_timestamp is opt-in. The fixup: commit is gone. hmac_algorithm is checked against sha256, sha384 and sha512. type: hmac requires a non-empty hmac_secret. The webhook plugin tests and test_identity_propagation.py pass locally.

The new algorithm check adds one defect.

severity | path:line | problem | fix

blocking | plugins/webhook_notification/webhook_notification.py:104 | The validator checks hmac_algorithm.lower(), but _create_hmac_signature() (line 213) calls getattr(hashlib, algorithm, hashlib.sha256) with the original case. hmac_algorithm: SHA512 passes validation, signs with sha256 and sends SHA512=<sha256 hex>. A receiver that verifies with sha512 rejects every delivery. test_hmac_validation_accepts_valid_algorithms asserts that SHA256 and SHA512 are kept unchanged, so the test locks in the defect. | Normalize the value with a field_validator("hmac_algorithm") that returns value.lower(), or reject uppercase. Replace the getattr(..., hashlib.sha256) fallback with a direct lookup so an unknown name cannot fall back silently. Change the test to assert the lowercase value. Add a known-answer check for SHA512.

Repro: WebhookNotificationPlugin._create_hmac_signature(None, "body", "k", "SHA512") returns SHA512= and a 64-character digest. A sha512 digest has 128 characters.

minor | commits f626f13af, 6d3a1dadf | f626f13af edits test_a2a_jsonrpc_passthrough.py and 6d3a1dadf reverts the edit, so the net diff has no A2A change. 6d3a1dadf says "refresh secrets baseline" but does not change .secrets.baseline. | Squash on merge, or reword the commit messages.

CI still has 16 pending jobs, including pylint, conformance and Full CI decision. I will approve when the blocking item is fixed and CI passes.

[msureshkumar88]

Thanks for the quick fix. The blocking item from my last review is resolved.

• hmac_algorithm is now lowercased by a field_validator before the allowlist check, so SHA512 stores sha512.
• _create_hmac_signature() no longer falls back to sha256 and raises on an unknown name.
• I checked locally: SHA512 now yields a digest equal to hmac.new(key, body, hashlib.sha512).hexdigest(). md5, SHA1 and new are rejected. The webhook plugin tests and test_identity_propagation.py pass (151 tests).

Approving. One item must land before merge:

functionally-impacting | .secrets.baseline | `Run pre-commit hooks` fails because `detect-secrets` rewrites the baseline. The PR adds 6 lines to `CHANGELOG.md`, so the entries at lines 251 and 313 move to 257 and 319. This is line-number drift only; there is no new secret. `main` also changed the baseline in `e9ca21f1a`. | Rebase on `main`, run `make detect-secrets-scan`, and commit `.secrets.baseline`.

Non-blocking follow-ups:

minor | tests/unit/mcpgateway/plugins/plugins/webhook_notification/test_webhook_notification.py | `test_hmac_signature_sha512_known_answer` checks the digest length only. | Compare against `hmac.new(b"key", b"body", hashlib.sha512).hexdigest()`.
minor | commits f626f13af, 6d3a1dadf | The commit messages do not match their changes (from my last review). | Squash on merge.