Improve skyhigh secure web gateway #1887
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR enhances the Skyhigh Secure Web Gateway parser to support LEEF input by introducing a LEEF-specific grok stage and normalization to ECS fields, refactors timestamp parsing, overhauls ECS and McAfee-specific field mappings to use the new parsed_event structure, and adds LEEF-focused test cases. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider renaming the duplicated
parse_datesteps to unique identifiers (e.g.,parse_date_devTime) to avoid conflicts and improve readability. - Remove the redundant empty
description: ''andfilter: ''fields throughout the pipeline to streamline the YAML and reduce noise. - Double-check that the fallback filter (
parse_leef.message is not mapping) robustly routes partially matching LEEF or non-LEEF messages tokv.parse-kvso nothing slips through unparsed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider renaming the duplicated `parse_date` steps to unique identifiers (e.g., `parse_date_devTime`) to avoid conflicts and improve readability.
- Remove the redundant empty `description: ''` and `filter: ''` fields throughout the pipeline to streamline the YAML and reduce noise.
- Double-check that the fallback filter (`parse_leef.message is not mapping`) robustly routes partially matching LEEF or non-LEEF messages to `kv.parse-kv` so nothing slips through unparsed.
## Individual Comments
### Comment 1
<location> `SkyhighSecurity/skyhigh_secure_web_gateway/ingest/parser.yml:105` </location>
<code_context>
+ event.start: '{{parse_date_mcafee.datetime}}'
+ observer.ip: '{{parsed_event.message.serverIP}}'
+ source.port: '{{parsed_event.message.srcPort}}'
+ event.action: '{{parsed_event.message.result}}'
+ event.reason: >-
+ {{parsed_event.message.block_reason or
+ parsed_event.message.blockReason}}
</code_context>
<issue_to_address>
event.action assignment may be overwritten by later dictionary translation.
Since event.action is assigned from parsed_event.message.result and then potentially modified by dictionary actions, please ensure the assignment order is clear and event.action is only set once to avoid confusion.
Suggested implementation:
```
# Set event.action only once, combining result and dictionary translation if needed
event.action: >-
{% set action = parsed_event.message.result %}
{% if action in action_dict %}
{{ action_dict[action] }}
{% else %}
{{ action }}
{% endif %}
```
- You will need to ensure that `action_dict` is defined in the template context, mapping possible `result` values to their translated actions.
- Remove any other assignments to `event.action` elsewhere in the file to avoid overwriting.
- If dictionary translation is performed elsewhere, consolidate it into this single assignment.
</issue_to_address>
### Comment 2
<location> `SkyhighSecurity/skyhigh_secure_web_gateway/ingest/parser.yml:189` </location>
<code_context>
+ filter: '{{parsed_event.message.client_ip == null}}'
+ name: set
+ - set:
+ url.full: >-
+ {{parsed_event.message.uri_scheme}}://{{parsed_event.message.requested_host}}{{parsed_event.message.requested_path}}
+ url.original: '{{parsed_event.message.requested_path}}'
+ filter: >-
</code_context>
<issue_to_address>
url.full is set in multiple places with different sources.
Assigning url.full from different sources may cause inconsistencies. Please standardize the assignment to ensure consistent values.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+189
to
+190
| url.full: >- | ||
| {{parsed_event.message.uri_scheme}}://{{parsed_event.message.requested_host}}{{parsed_event.message.requested_path}} |
There was a problem hiding this comment.
issue: url.full is set in multiple places with different sources.
Assigning url.full from different sources may cause inconsistencies. Please standardize the assignment to ensure consistent values.
Corrected, bug on http.request.method and unified event.code with existing parser
Adapated test log
Adapted test log
Adapated test log
Adapated test log
Adapated test log
Adapated test log
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add leef handling to skyhigh format
Summary by Sourcery
Support LEEF format in the Skyhigh Secure Web Gateway integration by adding dedicated parsing stages, updating field mappings for parsed events, and adding test fixtures for LEEF cases
New Features:
Enhancements:
Tests: