Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for HMAC-signed HTTP health checks by passing additional configuration through the Guardian HttpEvent payload (and updating the pinned http-check Lambda version), plus accompanying documentation updates.
Changes:
- Bump gem version to
0.12.1. - Extend
HttpEventto optionally include HMAC/SSM configuration (and expose the SSM parameter for IAM policy generation). - Update HTTP custom check documentation with HMAC signing guidance and examples.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| lib/cfnguardian/version.rb | Version bump for the release containing the new HTTP check behavior. |
| lib/cfnguardian/models/event.rb | Adds HMAC/SSM-related fields into HttpEvent payload and surfaces SSM parameter paths. |
| lib/cfnguardian/models/check.rb | Updates the pinned http-check Lambda version to a newer commit. |
| docs/custom_checks/http.md | Documents HMAC-signed request support and application-side verification guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @hmac_key_id = resource.fetch('HmacKeyId','default') | ||
| @hmac_header_prefix = resource.fetch('HmacHeaderPrefix','X-Health') |
There was a problem hiding this comment.
resource.fetch('HmacKeyId', 'default') / fetch('HmacHeaderPrefix', ...) won’t apply the default if the YAML key is present but set to null (it will propagate nil into the payload as JSON null). To match the documented defaults and avoid generating invalid/empty header names, coerce nil/blank values back to the defaults (and consider validating HmacHeaderPrefix is non-empty when HMAC is enabled).
| @hmac_key_id = resource.fetch('HmacKeyId','default') | |
| @hmac_header_prefix = resource.fetch('HmacHeaderPrefix','X-Health') | |
| raw_hmac_key_id = resource.fetch('HmacKeyId', nil) | |
| @hmac_key_id = raw_hmac_key_id.to_s.strip | |
| @hmac_key_id = 'default' if @hmac_key_id.empty? | |
| raw_hmac_header_prefix = resource.fetch('HmacHeaderPrefix', nil) | |
| @hmac_header_prefix = raw_hmac_header_prefix.to_s.strip | |
| @hmac_header_prefix = 'X-Health' if @hmac_header_prefix.empty? |
| | `HmacHeaderPrefix` | No | Prefix for all HMAC header names. Default: `X-Health` (yields `X-Health-Signature`, `X-Health-Key-Id`, etc.). | | ||
|
|
There was a problem hiding this comment.
ReportResponseBody is supported by HttpEvent (see event model changes) but isn’t documented here alongside the other HTTP options. Please add it to this configuration table and clarify what data is emitted/recorded when enabled so users can assess sensitivity.
| | `HmacHeaderPrefix` | No | Prefix for all HMAC header names. Default: `X-Health` (yields `X-Health-Signature`, `X-Health-Key-Id`, etc.). | | |
| | `HmacHeaderPrefix` | No | Prefix for all HMAC header names. Default: `X-Health` (yields `X-Health-Signature`, `X-Health-Key-Id`, etc.). | | |
| | `ReportResponseBody` | No | When `true`, the HTTP check includes the raw HTTP response body in the emitted `HttpEvent` for the request. This body content is stored with other Guardian events/logs and may contain sensitive data (such as application data, PII, or secrets), so enable only for endpoints whose responses are safe to record. | | |
| **Security note:** `ReportResponseBody` causes the checker to record the raw HTTP response body as part of each `HttpEvent`. This may include any data your endpoint returns, including personal data or credentials, so use this option only on endpoints whose responses are explicitly designed to be non-sensitive. |
| - `QUERY` – Raw query string (e.g. `foo=bar` or empty). | ||
| - `BODY_HASH` – SHA-256 hex digest of the raw request body (empty string for GET; for POST/PUT, hash the body as sent). | ||
|
|
There was a problem hiding this comment.
The BODY_HASH description says it is an empty string for GET, but the pseudo code later computes a SHA-256 hex digest even when the body is empty. This inconsistency could cause implementers to compute a different signature; please align the text with the actual algorithm (e.g., always hash the raw body, using an empty body for requests without one).
| # Replay: reject old timestamps | ||
| if abs(int(timestamp) - time.time()) > max_age_seconds: | ||
| return False |
There was a problem hiding this comment.
The Python verification example uses time.time() but doesn’t import time, so the snippet won’t run as written. Add the missing import (or adjust the example to avoid referencing time).
No description provided.