-
Notifications
You must be signed in to change notification settings - Fork 343
Implementing basic authentication for webhook auditlog sink #5792
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Extends the webhook audit log sink to support HTTP Basic Authentication by reading username and password from the configuration settings. When credentials are configured, the sink generates a properly formatted Authorization header with Base64-encoded credentials and includes it in all webhook HTTP requests. This feature enables webhook endpoints to require authentication, addressing issue opensearch-project#5738 where the username and password configuration settings were previously only functional for external_opensearch sinks. The implementation maintains full backward compatibility - webhooks without configured credentials continue to work as before without any Authorization header. Changes: - Modified WebhookSink to read username/password from settings - Generate Base64-encoded Basic Auth header when credentials are present - Add Authorization header to both POST and GET HTTP requests - Enhanced TestHttpHandler to capture request headers for testing - Added unit tests for webhook authentication scenarios Signed-off-by: Sander van de Geijn <[email protected]>
src/main/java/org/opensearch/security/auditlog/sink/WebhookSink.java
Outdated
Show resolved
Hide resolved
|
Looks quite good! I think we also need to update the docs at https://docs.opensearch.org/latest/security/audit-logs/storage-types/ ... would you also be willing to file a PR for that? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5792 +/- ##
=======================================
Coverage 73.51% 73.51%
=======================================
Files 438 438
Lines 26710 26722 +12
Branches 3956 3959 +3
=======================================
+ Hits 19637 19646 +9
- Misses 5195 5197 +2
- Partials 1878 1879 +1
🚀 New features to boost your workflow:
|
Renamed SECURITY_AUDIT_EXTERNAL_OPENSEARCH_USERNAME and SECURITY_AUDIT_EXTERNAL_OPENSEARCH_PASSWORD to SECURITY_AUDIT_CONFIG_USERNAME and SECURITY_AUDIT_CONFIG_PASSWORD to reflect their generic usage across both external OpenSearch and webhook audit sinks. Signed-off-by: Sander van de Geijn <[email protected]>
Signed-off-by: Sander van de Geijn <[email protected]>
|
@nibix are the CI jobs stuck by any chance? Was waiting to see if my fixes were ok :) |
DarshitChanpura
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sandervandegeijn thank you for adding this feature!
Would you please run ./gradlew spotlessApply to fix errors from spotless scan?
src/test/java/org/opensearch/security/auditlog/sink/WebhookAuditLogTest.java
Show resolved
Hide resolved
Signed-off-by: Sander van de Geijn <[email protected]>
Signed-off-by: Sander van de Geijn <[email protected]>
Remove fallback message size assertion that can fail due to connection issues with test server. The existing GET test at line 330-346 also doesn't check fallback messages. Signed-off-by: Sander van de Geijn <[email protected]>
|
The test suite doesn't seen entirely stable, look like the changes are ok? |
A maintainer needs to approve them to run for first time contributors. After your first PR is merged, they then run automatically on push. |
src/test/java/org/opensearch/security/auditlog/sink/WebhookAuditLogTest.java
Show resolved
Hide resolved
|
Thank you for the PR @sandervandegeijn ! The changes LGTM. Claude code knows this code base quite well at this point..one of the giveaways that a code block (particularly a test) may have been autogenerated is the use of full qualified classnames inline rather than top of file imports. In this case, it added some good tests that cover all of Webhook POST, GET and when auth is not configured at all which is what I would expect for a contribution like this. |
|
CI fails dues to changes from here: opensearch-project/OpenSearch#19803 |
Description
This change adds HTTP Basic Authentication support to the webhook audit log sink, enabling webhook endpoints to require authentication when receiving audit logs from OpenSearch.
In OpenSearch 2.x, users could embed credentials in webhook URLs (e.g., https://user:[email protected]), but this approach was deprecated in 3.x due to the Apache HttpComponents Client5 upgrade. The configuration parameters
plugins.security.audit.config.username and plugins.security.audit.config.password were available but only functional for external_opensearch sinks. Webhook sinks had no way to authenticate with secured endpoints, limiting their
use in production environments where webhook endpoints require authentication.
Old Behavior:
New Behavior:
Issues Resolved
Closes #5738
Is this a backport? No
Do these changes introduce new permission(s) to be displayed in the static dropdown on the front-end? No
Testing
Unit Testing:
the credentials match the configured values.
Integration Testing:
Manual Testing:
Test Results:
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check https://github.com/opensearch-project/security/blob/main/CONTRIBUTING.md#developer-certificate-of-origin.
Configuration Example:
plugins.security.audit.type: webhook
plugins.security.audit.config.webhook.url: "https://your-webhook-endpoint.com/audit"
plugins.security.audit.config.webhook.format: "json"
plugins.security.audit.config.username: "audit_user"
plugins.security.audit.config.password: "secure_password"