-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Fix webhook errror messages to be more specific #87044
Conversation
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.
LGTM
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
@elasticmachine merge upstream |
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.
LGTM!
@kobelb @elastic/kibana-security I'm trying to find the right balance between exposing useful error messages and keeping the risk of exposing secrets low. Right now the Webhook failures are very generic (mostly status code / generic error message) and we have to guess what may be wrong (certificates, proxy configuration, domain not found, bad request body, missing headers, etc). This causes SDHs where we can't easily support them unless we had more information in the error message. The risk with the approach in my PR is that it relays error messages from a 3rd party services. If ever that service decides to include a secret (ex: password) in the response error message, we would relay that to the user. Though, if ever that was the case, it sounds like it would be something wrong on the 3rd party to not return such information? |
I think that's a reasonable assumption. |
++ This feels ok to me as well. Thanks for checking! |
* Initial work * Fix variables to pull from * Rename some variables Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
* Initial work * Fix variables to pull from * Rename some variables Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@pmuellr I wonder if it would be worth doing something similar for email connector? To help with the SDHs. |
💔 Build Failed
Failed CI Steps
Test FailuresChrome X-Pack UI Functional Tests.x-pack/test/functional/apps/api_keys/home_page·ts.API Keys app Home page Loads the appStandard Out
Stack Trace
X-Pack API Integration Tests (Security Basic).x-pack/test/api_integration/apis/security/api_keys·ts.security (basic license) API Keys GET /internal/security/api_key/_enabled should indicate that API Keys are enabledStandard Out
Stack Trace
X-Pack EPM API Integration Tests.x-pack/test/fleet_api_integration/apis/agents/unenroll·ts.Fleet Endpoints fleet_unenroll_agent should allow to unenroll single agentStandard Out
Stack Trace
and 3 more failures, only showing the first 3. Metrics [docs]Distributable file count
History
To update your PR or re-run it, just comment with: |
In this PR, I'm improving the error message the Webhook connector gives when the execution fails. We should discuss whether this exposes too much about the error or just the right amount of information. I feel the previous behaviour is too limited when it comes to debugging.
Screenshots
Provides more details on some previous "unexpected error"
Previously it was only telling users
error calling webhook, unexpected error
and nothing else.Provides more details when the response contains an error
Previously it would only have
[400] Bad Request
in the details section.Shouldn't expose passwords (maybe username depending on the Webhook response) when authorization fails
Previously it would only have
[401] Unauthorized
in the details section.