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

[Filebeat]Azure module - activity logs #13776

Merged
merged 24 commits into from
Oct 10, 2019
Merged

Conversation

narph
Copy link
Contributor

@narph narph commented Sep 24, 2019

PR is a POC for the azure module in filebeat.
The azure module will contain:

  • the activitylogs fileset which will retrieve the Azure activity logs.
    Requirements are exporting these logs to an event hub (with Kafka feature enabled), steps here
  • the signinlogs fileset which will retrieve the Sign in logs
  • the auditlogs fileset which will retrieve the AD audit logs
    Requirements are exporting these 2 AD reports to an event hub (with Kafka feature enabled), steps here

GH issue #13385

filebeat/input/kafka/config.go Outdated Show resolved Hide resolved
filebeat/input/kafka/config.go Outdated Show resolved Hide resolved
@narph narph self-assigned this Sep 24, 2019
@narph narph added Team:Integrations Label for the Integrations team x-pack Issues and pull requests for X-Pack features. Filebeat Filebeat [zube]: In Progress labels Sep 24, 2019
filebeat/input/kafka/config.go Outdated Show resolved Hide resolved
filebeat/input/kafka/config.go Outdated Show resolved Hide resolved
filebeat/input/kafka/azure_log_patterns.go Outdated Show resolved Hide resolved
filebeat/input/kafka/azure_log_patterns.go Outdated Show resolved Hide resolved
filebeat/input/kafka/azure_log_patterns.go Outdated Show resolved Hide resolved
filebeat/input/kafka/config.go Outdated Show resolved Hide resolved
filebeat/input/kafka/config.go Outdated Show resolved Hide resolved
filebeat/input/kafka/azure_log_patterns.go Outdated Show resolved Hide resolved
filebeat/input/kafka/azure_log_patterns.go Outdated Show resolved Hide resolved
filebeat/input/kafka/azure_log_patterns.go Outdated Show resolved Hide resolved
filebeat/input/kafka/azure_log_patterns.go Outdated Show resolved Hide resolved
filebeat/input/kafka/azure_log_patterns.go Outdated Show resolved Hide resolved
filebeat/input/kafka/azure_log_patterns.go Outdated Show resolved Hide resolved
filebeat/input/kafka/azure_log_patterns.go Outdated Show resolved Hide resolved
filebeat/input/kafka/azure_log_patterns.go Outdated Show resolved Hide resolved
@narph narph requested a review from faec October 4, 2019 13:19
Copy link
Contributor

@faec faec left a comment

Choose a reason for hiding this comment

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

This looks mostly good and I will do a more thorough review after I return from vacation on Tuesday... a question in addition to the other comments: what specifically is restricted to x-pack here? It looks like ~all the code is in OSS and only module config is in x-pack. This would still leave the functionality in OSS, right? If there hasn't been a conversation about where the division lies, we should probably have one

@@ -50,6 +50,7 @@ type kafkaInputConfig struct {
TLS *tlscommon.Config `config:"ssl"`
Username string `config:"username"`
Password string `config:"password"`
AzureLogs string `config:"azure_logs"`
Copy link
Contributor

Choose a reason for hiding this comment

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

This field should be documented and validated -- consider following the pattern used for enums like kafka.Version, which is validated by its Unpack function (see libbeat/common/kafka/version.go) and can thereafter be used as an enum constant rather than a string match.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@faec , I have replaced the AzureLogs with YieldEventsFromField property. This property value, if entered will have the json object name that will contain multiple events.
Ex for azure:
{ "records": [ {event1}, {event2}]}
The YieldEventsFromField property value will be "records".
I have also removed any check for the azure type logs and added some validation inside the actual pipeline.
Let me know if you think this property should be handled the same way as "kafka.Version".

handler := &groupHandler{
version: input.config.Version,
outlet: input.outlet,
logType: input.config.AzureLogs,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I follow what's happening here (in generic kafka config there is no AzureLogs field so this gets set to the empty string -> no special processing), but a comment would be nice to clarify that this is a no-op if there's no Azure-specific configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AzureLogs have been replaced with a more generic option "YieldEventsFromField" and added some comments there

event := beat.Event{
Timestamp: timestamp,
Fields: common.MapStr{
"message": msg,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work ok? My recollection is that the explicit string conversion (string(message.Value) in the old code) was necessary for a lot of things to work, since otherwise it got interpreted as raw bytes by the backend, which messed up the logs and the indexing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had no issues with it so far, let me know if you know a more secure option, happy to change it

// if azure input, then a check for the message is done regarding the list of events

var events []beat.Event
messages := h.parseMultipleMessages(message.Value)
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that you need azure specific code in order to parse a message into multiple events. The rest could be done with a json processor in a pipeline, isn't it?

If that's the case, perhaps it would be better to have a generic way to say: spawn events from list under this JSON field: "records"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately that is not possible, I could separate the json elements but they will still be generated as one event. The workaround would have been to create a new processor and a new interface for processors that can return multiple events.

"module" : "azure",
"dataset" : "azure.activitylogs"
},
"resultType" : "Failure",
Copy link
Member

Choose a reason for hiding this comment

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

I think some of these fields could be mapped into ECS like

  • resultType -> event.result
  • callerIpAddress -> source.ip

I'm sure they're are more and maybe you are already planning for some of these mappings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andrewkroh , thanks, in the course of identifying the ecs fields. I could not find event.result in https://www.elastic.co/guide/en/beats/filebeat/master/exported-fields-ecs.html. Is it event.outcome or I am not looking in the right list.

Copy link
Member

Choose a reason for hiding this comment

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

You're right. It's event.outcome. I was working from memory on that example.

@narph
Copy link
Contributor Author

narph commented Oct 8, 2019

hi @faec , regarding:

This looks mostly good and I will do a more thorough review after I return from vacation on Tuesday... a question in addition to the other comments: what specifically is restricted to x-pack here? It looks like ~all the code is in OSS and only module config is in x-pack. This would still leave the functionality in OSS, right? If there hasn't been a conversation about where the division lies, we should probably have one

I assumed the reason for adding the kafka input inside the oss is to be reused by other non x-pack modules as well.
I have tried to remove any azure specific logic from the input as I think the input should not know/care about it.
The azure module is in the same location (x-pack) as the rest as it's "type" , aws etc.
Was there any other reason the kafka input is inside the oss?

@narph narph marked this pull request as ready for review October 9, 2019 09:37
@narph narph requested a review from a team as a code owner October 9, 2019 09:37
Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

Did a first pass, I like how this is looking!

CHANGELOG.next.asciidoc Outdated Show resolved Hide resolved
x-pack/filebeat/module/azure/_meta/fields.yml Outdated Show resolved Hide resolved
x-pack/filebeat/module/azure/_meta/fields.yml Outdated Show resolved Hide resolved
x-pack/filebeat/module/azure/activitylogs/_meta/fields.yml Outdated Show resolved Hide resolved
x-pack/filebeat/module/azure/auditlogs/_meta/fields.yml Outdated Show resolved Hide resolved
x-pack/filebeat/module/azure/module.yml Outdated Show resolved Hide resolved
x-pack/filebeat/module/azure/signinlogs/_meta/fields.yml Outdated Show resolved Hide resolved
x-pack/filebeat/module/azure/_meta/config.yml Show resolved Hide resolved
@narph narph requested a review from exekias October 9, 2019 15:19
Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

I think this is good to go if CI is happy. Thank you for implementing the changes!

Something that would be great to see here (in a different PR) is cloud.* metadata matching the Metricbeat module. That should improve correlation opportunities

@narph
Copy link
Contributor Author

narph commented Oct 10, 2019

jenkins test this

@narph narph merged commit 0aea418 into elastic:master Oct 10, 2019
@narph narph deleted the azure-filebeat branch October 10, 2019 13:31
@narph narph added the v7.5.0 label Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Filebeat Filebeat Team:Integrations Label for the Integrations team v7.5.0 x-pack Issues and pull requests for X-Pack features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants