[forescout] Initial release of the forescout#16426
[forescout] Initial release of the forescout#16426qcorporation merged 8 commits intoelastic:feature/forescout-0.1.0from
Conversation
|
Hi! We just realized that we haven't looked into this PR in a while. We're sorry! We're labeling this issue as |
| /packages/first_epss @elastic/security-service-integrations | ||
| /packages/fleet_server @elastic/fleet | ||
| /packages/forcepoint_web @elastic/security-service-integrations | ||
| /packages/forescout @elastic/security-service-integrations |
There was a problem hiding this comment.
@cpascale43 , @qcorporation from a quick glance this integration belongs to the integration experience team, using tcp/udp input
There was a problem hiding this comment.
@narph this makes a lot of sense.
@sharadcrest, can you update the owner to @elastic/integration-experience and assign it to us for review
| title: Collect logs from Forescout via UDP | ||
| description: Collecting logs from Forescout via UDP. | ||
| owner: | ||
| github: elastic/security-service-integrations |
There was a problem hiding this comment.
@sharadcrest , a team update is also necessary here
There was a problem hiding this comment.
Overall this looks great, just a few comments.
Edit: I missed the bigger issues with unparsed logs that @ilyannn pointed out. See my updated response below.
| - set: | ||
| tag: set_event_kind_to_event_de80643c | ||
| field: event.kind | ||
| value: event |
There was a problem hiding this comment.
Looks like we are missing the following ECS categorization fields:
event.categoryevent.type
There was a problem hiding this comment.
event.type can be a info but for event.category I couldn't identify suitable value. I’d really appreciate it if you could take a look and share your suggestions or guidance.
There was a problem hiding this comment.
Yeah this highly depends on the content of the log. To give you an idea of how varied this can be, this is a large script from cisco_asa that sets these fields. It's a bit "easier" in that integration as there are message IDs we can key off of to reliably set the fields. It doesn't look like we have that luxury with this integration.
I'm okay with this being deferred to a follow-up task, but is something that should be addressed before the integration is GA-ed.
| "message": "Accepted publickey for root from 172.20.10.101 port 46018 ssh2: RSA SHA256:WokXPUll0YJnJwbAFK1xYfYR+DaVN2RVFEyK6lMW78c" | ||
| } | ||
| }, | ||
| "message": "Accepted publickey for root from 172.20.10.101 port 46018 ssh2: RSA SHA256:WokXPUll0YJnJwbAFK1xYfYR+DaVN2RVFEyK6lMW78c", |
There was a problem hiding this comment.
This event is unparsed – is that expected?
| }, | ||
| "forescout": { | ||
| "event": { | ||
| "message": "New session 14906 of user root." |
There was a problem hiding this comment.
This is also unparsed; in fact there is only one DISSECT pattern and most of the test logs are not processed by it. Is this a conscious decision or an oversight?
| external: ecs | ||
| type: constant_keyword | ||
| value: RFC3164 | ||
| - name: observer.vendor |
There was a problem hiding this comment.
Should we also add the observer.product etc.?
There was a problem hiding this comment.
The syslog data we’re collecting is forwarded through the syslog plugin, which is a core module of Forescout. So we can either set observer.product to Forescout which will be same as observer.vendor or leave it unmapped as it is in the current implementation. I’m happy to go with whichever approach you think is more appropriate.
taylor-swanson
left a comment
There was a problem hiding this comment.
So looking at the example logs (particularly the system logs as they are the full syslog messages), it looks like we are receiving all of the system messages (see logs from sshd and systemd), not just the Forescout service (_fsservice).
To echo @ilyannn's question, is this intended? Looking at the syslog docs for Forescout, I suppose it is intentional, since you can enable system logs. It appears it just forwards regular systemd and other service logs.
This is problematic since in order to properly parse these, not only do we have to write parsers for Forescout-specific logs, we also have to write parsers for regular linux-type logs. However, if we just want to generically ingest them, then the current solution is "okay", although I would prefer if the dissect keys off of the syslog app name, rather than blindly running the dissect and relying on ignore_failure to recover it. This is bad because a legitimate failure will be silenced by ignore_failure.
Since it sounds like there are other types of events to handle (of which I don't think the current dissect can handle?), a better solution needs to be found to handle other log types.
|
Regarding the comments #16426 and #16426, I agree with @taylor-swanson(#16426). It was intentionally left unparsed. According to RFC 3164 log format, the MSG portion of a syslog message contains the tag and message text, and it doesn’t enforce a structured format. Because of that, even if we extract important values like IP or port, it’s not always possible to determine their semantic role (e.g., source.ip vs destination.ip vs host.ip) from the free-form content. |
taylor-swanson
left a comment
There was a problem hiding this comment.
Regarding the comments #16426 and #16426, I agree with @taylor-swanson(#16426). It was intentionally left unparsed.
According to RFC 3164 log format, the MSG portion of a syslog message contains the tag and message text, and it doesn’t enforce a structured format. Because of that, even if we extract important values like IP or port, it’s not always possible to determine their semantic role (e.g., source.ip vs destination.ip vs host.ip) from the free-form content.
I would think that exporting system logs (or really anything non-Forescout) should be turned off, but it's still a good idea to gracefully handle these logs coming through, even if it's just indexing them as-is.
I was going to mention looking at log.syslog.appname again, but then I looked closer at the example logs. It seems like all of these were run through sudo, which isn't a bad thing, it's just that the appname is going to be sudo instead of _fsservice. I was going to suggest something like: "If the appname is NOT _fsservice, then maybe think about dropping the log," but that may not be as easy as I think. It's probably safer right now to just index everything. If a user doesn't want those logs, then the best course of action would be to turn off the export of those logs.
|
/test |
🚀 Benchmarks reportTo see the full report comment with |
💚 Build Succeeded
|
|
Hey @taylor-swanson, @ilyannn, Could you please let me know if anything is still pending from my side? If everything looks good, can we proceed with the merge? |
ilyannn
left a comment
There was a problem hiding this comment.
Since it seems to be fine that we just pass through and not parse the other logs, LGTM
I think it would be good to add this information to the README. |
taylor-swanson
left a comment
There was a problem hiding this comment.
LGTM as well
@elastic/integrations-triaging, could someone take a look (@qcorporation perhaps)?
qcorporation
left a comment
There was a problem hiding this comment.
@sharadcrest
can you please update ./.github/integrations_bug.yml and integration_feature_request.yml please
@sharadcrest it would be also nice if you used the LLM docs generation - to generate the docs. @narph have we introduced this tooling to @sharadcrest ? |
|
@qcorporation in the |
@sharadcrest Have you talked to @narph about docs generation? We have an LLM based docs generation workflow that I feel could be applied to this new integration - are you aware of it and do you think you should be using it to update the docs instead of writing it manually? |
I haven’t connected with @narph yet regarding the docs generation workflow. I’ll reach out and get guidance on the LLM-based docs generation process. If it fits well for this integration, I’ll use that approach instead of writing the documentation manually. |
|
@narph Does LLM based doc generation tool is ready to use? |
|
@qcorporation — I noticed that we are currently waiting for the LLM-based documentation generation tool. I connected with @ShourieG , and it appears that it is still in progress. Would it be possible to proceed with this PR for now? If there are any changes required later, we can address them as enhancements. This would help us avoid blocking other PR that are currently in draft cause on this. |
sound good |
Proposed commit message
Checklist
changelog.ymlfile.How to test this PR locally
To test the forescout package:
Related issues
Screenshots
Go Code for Ingest Pipeline Generation
The event data stream pipeline is generated using Go code with the help of Dispear library.