Skip to content
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

zscaler_zia: Fix improper escaping of backslash characters #9842

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

kcreddy
Copy link
Contributor

@kcreddy kcreddy commented May 13, 2024

Proposed commit message

See title

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

How to test this PR locally

elastic-package stack down && elastic-package build && elastic-package stack up -d -v && eval "$(elastic-package stack shellinit)" && elastic-package test pipeline --generate -v

--- Test results for package: zscaler_zia - START ---
╭─────────────┬─────────────┬───────────┬─────────────────────────────────┬────────┬──────────────╮
│ PACKAGE     │ DATA STREAM │ TEST TYPE │ TEST NAME                       │ RESULT │ TIME ELAPSED │
├─────────────┼─────────────┼───────────┼─────────────────────────────────┼────────┼──────────────┤
│ zscaler_zia │ alerts      │ pipeline  │ test-alerts.log                 │ PASS   │   2.356666ms │
│ zscaler_zia │ dns         │ pipeline  │ test-dns-http-endpoint.log      │ PASS   │   2.348167ms │
│ zscaler_zia │ dns         │ pipeline  │ test-dns.log                    │ PASS   │   3.147583ms │
│ zscaler_zia │ firewall    │ pipeline  │ test-firewall-http-endpoint.log │ PASS   │   2.246542ms │
│ zscaler_zia │ firewall    │ pipeline  │ test-firewall.log               │ PASS   │   2.812042ms │
│ zscaler_zia │ tunnel      │ pipeline  │ test-tunnel-http-endpoint.log   │ PASS   │   1.626791ms │
│ zscaler_zia │ tunnel      │ pipeline  │ test-tunnel.log                 │ PASS   │   3.076666ms │
│ zscaler_zia │ web         │ pipeline  │ test-web-http-endpoint.log      │ PASS   │   2.474125ms │
│ zscaler_zia │ web         │ pipeline  │ test-web.log                    │ PASS   │   5.092166ms │
╰─────────────┴─────────────┴───────────┴─────────────────────────────────┴────────┴──────────────╯
--- Test results for package: zscaler_zia - END   ---
Done

Screenshots

@kcreddy kcreddy marked this pull request as ready for review May 13, 2024 04:02
@kcreddy kcreddy requested a review from a team as a code owner May 13, 2024 04:02
@kcreddy kcreddy self-assigned this May 13, 2024
@elasticmachine
Copy link

Pinging @elastic/security-service-integrations (Team:Security-Service Integrations)

@elasticmachine
Copy link

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

@elasticmachine
Copy link

💚 Build Succeeded

History

cc @kcreddy

Copy link
Contributor

@efd6 efd6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm aware that this is fixing a customer issue, but it troubles me that we are going out of our way to transform invalid JSON into valid JSON in one location at the possible cost of transforming other valid JSON into syntactically correct, but semantically incorrect JSON at many other sites (any \x escape sequence anywhere else in the input will be converted to a litteral "\x" string). Why are we not asking the sender of the invalid message to fix their encoding?

@kcreddy kcreddy marked this pull request as draft May 14, 2024 02:10
@kcreddy
Copy link
Contributor Author

kcreddy commented May 14, 2024

@efd6 That does make sense especially when the ZIA documentation mentions how to overcome this issue with a configuration change.

Moving the PR to draft. I will close it once the user is satisfied with the solution to change the configuration instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants