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

Use process.name and event.outcome in filebeat system auth module #11231

Merged
merged 1 commit into from
Mar 18, 2019

Conversation

jsoriano
Copy link
Member

Before migration to ECS (#9138), we could rely on the presence of specific
fields to know the process originating the events, but this is not so reliable
after some of these fields have been moved to common places. Add
process.name also for known messages so we keep this info in a known
place.

Also use event.outcome instead of event.action for the result of the
logged action.

@jsoriano jsoriano added module review Filebeat Filebeat needs_backport PR is waiting to be backported to other branches. v7.0.0 ecs labels Mar 13, 2019
@jsoriano jsoriano self-assigned this Mar 13, 2019
@jsoriano jsoriano requested review from webmat and ruflin March 13, 2019 15:14
@jsoriano jsoriano requested a review from a team as a code owner March 13, 2019 15:14
@jsoriano
Copy link
Member Author

Not adding changelog entry as this was not released yet and there was already one for ECS migration included in #9138.

@webmat
Copy link
Contributor

webmat commented Mar 14, 2019

Good stuff! Looks a bit like work I had started here but haven't had time to get back to yet.

Notice that over in my PR, I'm doing grok twice. Once for the syslog header (instead of repeating in every single pattern) and then a second grok for the various kinds of messages. No big deal if we keep as a single grok definition, though (I suspect it's faster this way).

One thing that worries me is that in this PR we're putting the source's raw values in event.outcome, when in fact we were hoping to make this a field an enumerated field, to ensure we have a very precise list of expected keywords, with regular capitalization.

On the other hand, nobody's had time to do significant work on building these lists of expected values yet. Perhaps it's ok to keep the raw values for now, and change around to the normalized values when these are ready.

Thoughts, @ruflin, @elastic/secops ?

@jsoriano
Copy link
Member Author

jsoriano commented Mar 15, 2019

@webmat your PR looks much more complete 🙂 if you plan to finish it for 7.0 I am fine with closing this PR.

If not I think that we could merge this as it addresses the main issues I see (loss of process information after migration to ECS and use of event.action for a result), also it doesn't change the result regarding the raw values stored as outcome, and you could still continue improving this in your PR.

So as you prefer.

@ruflin
Copy link
Member

ruflin commented Mar 15, 2019

I'm good with both options as long as we hit 7.0 with it :-)

@webmat
Copy link
Contributor

webmat commented Mar 15, 2019

My PR will definitely not make 7.0, it was an experiment. Please proceed with yours.

@jsoriano jsoriano merged commit a2e6d3a into elastic:master Mar 18, 2019
jsoriano added a commit to jsoriano/beats that referenced this pull request Mar 18, 2019
@jsoriano jsoriano removed the needs_backport PR is waiting to be backported to other branches. label Mar 18, 2019
@jsoriano jsoriano deleted the filebeat-auth-process-name branch March 18, 2019 15:51
jsoriano added a commit that referenced this pull request Mar 19, 2019
Before migration to ECS (#9138), we could rely on the presence of specific
fields to know the process originating the events, but this is not so reliable
after some of these fields have been moved to common places. Add
process.name also for known messages so we keep this info in a known
place.

Also use event.outcome instead of event.action for the result of the
logged action.

(cherry picked from commit a2e6d3a)
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.

3 participants