Add support for authentication in custom webhooks#6907
Conversation
WalkthroughThe changes introduce support for an optional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant System
participant WebhookReceiver
User->>System: Define Custom Webhook with optional shared_key
System->>System: Store shared_key in schema and model
System->>WebhookReceiver: Send webhook request
alt shared_key is set
System->>System: Sign request using HMAC with shared_key
System->>WebhookReceiver: Include signature headers
else shared_key is not set
System->>WebhookReceiver: Send request without signature
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
Bot
added
the
group/backend
CodSpeed Performance ReportMerging #6907 will not alter performanceComparing Summary
|
ogenstad
force-pushed
the
pog-authenticated-custom-webhooks-IFC-1532
branch
from
02ac187 to
dd71ded
Compare
ogenstad
force-pushed
the
pog-authenticated-custom-webhooks-IFC-1532
branch
from
dd71ded to
2c10007
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
backend/tests/functional/webhook/test_task.py (1)
245-268: Add test coverage for custom webhooks with authentication.The current test only covers custom webhooks without a
shared_key. Consider adding a test case that validates the behavior when a custom webhook has ashared_keyconfigured.Would you like me to generate a test case that covers custom webhooks with authentication enabled?
🧹 Nitpick comments (3)
changelog/6521.added.md (1)
1-1: Enhance the changelog entry with more specific details.The changelog should be more explicit about the authentication method being implemented (HMAC-SHA256 signing) and clarify whether JSON-only support for transforms is a current limitation or a permanent design decision.
-Added support for authentication / signing to custom webhooks. If using a transform it is assumed that the transform renders JSON data. +Added support for HMAC-SHA256 authentication to custom webhooks via an optional `shared_key` attribute. When using transforms with authenticated webhooks, the transform output must be JSON-formatted (currently the only supported format).backend/infrahub/webhook/models.py (2)
151-155: Remove unnecessary error handling in signing_key property.The
signing_keyproperty raises an exception whenshared_keyis not set, but this property is only accessed whenshared_keyexists (line 135 checks this). The error case is unreachable.@property def signing_key(self) -> str: """Return the signing key for the webhook.""" - if self.shared_key: - return self.shared_key - raise ValueError("Shared key is not set for the webhook") + return self.shared_key # type: ignore[return-value]
217-236: Consider security implications of transform output in authenticated webhooks.When using transforms with authenticated webhooks, the transform completely replaces the payload that gets signed. This means the transform has full control over what data is sent and signed. Ensure this is documented and that users understand the security implications.
Consider adding validation or restrictions on transform output when used with authenticated webhooks to prevent potential security issues where a compromised transform could send arbitrary signed data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/infrahub/core/protocols.py(1 hunks)backend/infrahub/core/schema/definitions/core/webhook.py(1 hunks)backend/infrahub/webhook/models.py(4 hunks)backend/tests/functional/webhook/test_task.py(1 hunks)changelog/6521.added.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
backend/**/*
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/core/protocols.pybackend/infrahub/core/schema/definitions/core/webhook.pybackend/tests/functional/webhook/test_task.pybackend/infrahub/webhook/models.py
backend/tests/**/*
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Place backend tests in
backend/tests/
Files:
backend/tests/functional/webhook/test_task.py
🔇 Additional comments (4)
backend/infrahub/core/protocols.py (1)
322-322: Verifyshared_keyoptionality in webhook models
I didn’t find any documentation or in-code comments explaining whyCoreStandardWebhookrequiresshared_keywhileCoreCustomWebhookmakes it optional. Please confirm that this difference is intentional (e.g., for backward-compatibility or differing security needs) or update the definitions accordingly.• backend/infrahub/core/protocols.py
– Line 322:class CoreCustomWebhook→shared_key: StringOptional
– Line 531:class CoreStandardWebhook→shared_key: Stringbackend/infrahub/core/schema/definitions/core/webhook.py (1)
123-125: LGTM! Schema implementation is consistent and secure.The
shared_keyattribute is properly implemented with:
- Appropriate
Passwordkind for sensitive data handling- Consistent order weight (4000) matching
StandardWebhook- Correct optional flag aligning with the protocol definition
backend/infrahub/webhook/models.py (2)
191-191: Ensure consistent handling of optional shared_key.Both
CustomWebhookandTransformWebhookpassobj.shared_key.valuedirectly, which could beNonefor custom webhooks. This is handled correctly by the base class, but consider adding a comment to clarify this is intentional.Also applies to: 253-253
129-143: Fix potential issues in the webhook signing implementation.The signing implementation has a few issues that should be addressed:
- When
uuidisNone, the code generates a new UUID but the logic seems incorrect (line 136)- The JSON serialization of an empty payload as
{}might cause signature mismatches- message_id = f"msg_{uuid.hex}" if uuid else f"msg_{uuid4().hex}" - timestamp = str(at.to_timestamp()) if at else str(Timestamp().to_timestamp()) + message_id = f"msg_{uuid.hex if uuid else uuid4().hex}" + timestamp = str(at.to_timestamp() if at else Timestamp().to_timestamp())Also consider handling the edge case where
_payloadisNone:- payload = json.dumps(self._payload or {}) + payload = json.dumps(self._payload if self._payload is not None else {})Likely an incorrect or invalid review comment.
2 participants
This PR adds support for an optional shared_key to custom webhooks.
Most of the work consists of just moving some data from the standard webhooks to the generic parent class so that they work in a similar way.
It currently assumes that the content_type of the transforms is JSON which is the only option now but will change in the future: opsmill/infrahub-sdk-python#282
Fixes #6521
Summary by CodeRabbit
New Features
Bug Fixes
Documentation