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.

Sign up for GitHub

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

Jump to bottom

Update ICMP protocol to use ECS fields #10062

Merged
merged 2 commits into from
Jan 16, 2019
Merged

Update ICMP protocol to use ECS fields #10062

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
af9f7c9
Update ICMP protocol to use ECS fields
andrewkroh Jan 14, 2019
4d681b5
Change NNNN to 10062
andrewkroh Jan 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.next.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d
- Changed DNS protocol fields to align with ECS. {pull}9941[9941]
- Removed trailing dot from domain names reported by the DNS protocol. {pull}9941[9941]
- Changed TLS protocol fields to align with ECS. {pull}9980[9980]
- Changed ICMP protocol fields to align with ECS. {pull}NNNN[NNNN]
andrewkroh marked this conversation as resolved.
Show resolved Hide resolved

*Winlogbeat*

Expand Down
9 changes: 6 additions & 3 deletions packetbeat/pb/event.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ type Fields struct {
SourceProcess *ecs.Process `ecs:"source.process"`
DestinationProcess *ecs.Process `ecs:"destination.process"`
Process *ecs.Process `ecs:"process"`

ICMPType uint8 // ICMP message type for use in computing network.community_id.
ICMPCode uint8 // ICMP message code for use in computing network.community_id.
}

// NewFields returns a new Fields value.
Expand Down Expand Up @@ -151,11 +154,11 @@ func (f *Fields) ComputeValues(localIPs []net.IP) error {
flow.Protocol = 6
case f.Network.Transport == "icmp":
flow.Protocol = 1
// TODO: Populate the ICMP type/code.
case f.Network.Transport == "ipv6-icmp":
flow.Protocol = 58
// TODO: Populate the ICMP type/code.
}
flow.ICMP.Type = f.ICMPType
flow.ICMP.Code = f.ICMPCode
if flow.Protocol > 0 && len(flow.SourceIP) > 0 && len(flow.DestinationIP) > 0 {
f.Network.CommunityID = flowhash.CommunityID.Hash(flow)
}
Expand Down Expand Up @@ -241,7 +244,7 @@ func (f *Fields) MarshalMapStr(m common.MapStr) error {
structField := typ.Field(i)
tag := structField.Tag.Get("ecs")
if tag == "" {
panic(errors.Errorf("missing tag on field %v", structField.Name))
continue
Copy link
Member

Choose a reason for hiding this comment

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

Change in behaviour? Did not follow the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

This file is "new" code as part of the parent issue #7968. Yes, this is a behavior change. It needed to be less restrictive to allow for fields in the object that are not to be marshaled. These are helper fields used to derive and make decisions about other fields that are marshaled.

Copy link
Member

Choose a reason for hiding this comment

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

Should this be mentioned in the changelog?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this is new code that's never been released and it has no impact on the user.

}

fieldValue := val.Field(i)
Expand Down
101 changes: 42 additions & 59 deletions packetbeat/protos/icmp/icmp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,13 @@ import (
"net"
"time"

"github.com/elastic/beats/libbeat/beat"
"github.com/elastic/beats/libbeat/common"
"github.com/elastic/beats/libbeat/logp"
"github.com/elastic/beats/libbeat/monitoring"
"github.com/elastic/ecs/code/go/ecs"

"github.com/elastic/beats/packetbeat/flows"
"github.com/elastic/beats/packetbeat/pb"
"github.com/elastic/beats/packetbeat/protos"

"github.com/tsg/gopacket/layers"
Expand Down Expand Up @@ -283,86 +284,68 @@ func (icmp *icmpPlugin) publishTransaction(trans *icmpTransaction) {

logp.Debug("icmp", "Publishing transaction. %s", &trans.tuple)

// common fields - group "env"
fields := common.MapStr{
"client": common.MapStr{
"ip": trans.tuple.srcIP,
},
"server": common.MapStr{
"ip": trans.tuple.dstIP,
},
}
evt, pbf := pb.NewBeatEvent(trans.ts)
pbf.Source = &ecs.Source{IP: trans.tuple.srcIP.String()}
pbf.Destination = &ecs.Destination{IP: trans.tuple.dstIP.String()}
pbf.Event.Dataset = "icmp"

// common fields - group "event"
fields["type"] = "icmp" // protocol name
fields := evt.Fields
fields["type"] = pbf.Event.Dataset
fields["path"] = trans.tuple.dstIP // what is requested (dst ip)
if trans.HasError() {
fields["status"] = common.ERROR_STATUS
} else {
fields["status"] = common.OK_STATUS
}
if len(trans.notes) > 0 {
fields["notes"] = trans.notes
}

// common fields - group "measurements"
responsetime, hasResponseTime := trans.ResponseTimeMillis()
if hasResponseTime {
fields["responsetime"] = responsetime
}
switch icmp.direction(trans) {
case directionFromInside:
if trans.request != nil {
fields["bytes_out"] = trans.request.length
}
if trans.response != nil {
fields["bytes_in"] = trans.response.length
}
case directionFromOutside:
if trans.request != nil {
fields["bytes_in"] = trans.request.length
}
if trans.response != nil {
fields["bytes_out"] = trans.response.length
}
icmpEvent := common.MapStr{
"version": trans.tuple.icmpVersion,
}

// event fields - group "icmp"
icmpEvent := common.MapStr{}
fields["icmp"] = icmpEvent

icmpEvent["version"] = trans.tuple.icmpVersion
pbf.Network.Transport = pbf.Event.Dataset
if trans.tuple.icmpVersion == 6 {
pbf.Network.Transport = "ipv6-icmp"
}

if trans.request != nil {
request := common.MapStr{}
icmpEvent["request"] = request
pbf.Event.Start = trans.request.ts
pbf.Source.Bytes = int64(trans.request.length)

request["message"] = humanReadable(&trans.tuple, trans.request)
request["type"] = trans.request.Type
request["code"] = trans.request.code
request := common.MapStr{
"message": humanReadable(&trans.tuple, trans.request),
"type": trans.request.Type,
"code": trans.request.code,
}
icmpEvent["request"] = request

// TODO: Add more info. The IPv4/IPv6 payload could be interesting.
// if icmp.SendRequest {
// request["payload"] = ""
// }
pbf.ICMPType = trans.request.Type
pbf.ICMPCode = trans.request.code
}

if trans.response != nil {
response := common.MapStr{}
pbf.Event.End = trans.response.ts
pbf.Destination.Bytes = int64(trans.response.length)

response := common.MapStr{
"message": humanReadable(&trans.tuple, trans.response),
"type": trans.response.Type,
"code": trans.response.code,
}
icmpEvent["response"] = response

response["message"] = humanReadable(&trans.tuple, trans.response)
response["type"] = trans.response.Type
response["code"] = trans.response.code
if trans.request == nil {
pbf.ICMPType = trans.response.Type
pbf.ICMPCode = trans.response.code
}
}

// TODO: Add more info. The IPv4/IPv6 payload could be interesting.
// if icmp.SendResponse {
// response["payload"] = ""
// }
if len(trans.notes) == 1 {
evt.PutValue("error.message", trans.notes[0])
} else if len(trans.notes) > 1 {
evt.PutValue("error.message", trans.notes)
Copy link
Member

Choose a reason for hiding this comment

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

From and ES perspective, I would say both are the same as message in Lucene is always an array. Having said that, do we really want to put an array into message or flatten it first?

Copy link
Member Author

Choose a reason for hiding this comment

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

If there's more than one note/error then I'd rather sent them as discrete strings that are part of an array. As for whether to always send an array or not that doesn't really matter to me (or to ES). I'll probably refactor this when I get to the end of #7968, because I'm finding that each protocol had slightly different behavior w.r.t. notes. So if you have a preference for whether to always send an array or not let me know.

Copy link
Member

Choose a reason for hiding this comment

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

No strong preference either.

}

icmp.results(beat.Event{
Timestamp: trans.ts,
Fields: fields,
})
icmp.results(evt)
}
7 changes: 0 additions & 7 deletions packetbeat/protos/icmp/transaction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,3 @@ func (t *icmpTransaction) HasError() bool {
(t.response != nil && isError(&t.tuple, t.response)) ||
(t.request != nil && t.response == nil && requiresCounterpart(&t.tuple, t.request))
}

func (t *icmpTransaction) ResponseTimeMillis() (int32, bool) {
if t.request != nil && t.response != nil {
return int32(t.response.ts.Sub(t.request.ts).Nanoseconds() / 1e6), true
}
return 0, false
}
21 changes: 0 additions & 21 deletions packetbeat/protos/icmp/transaction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ package icmp

import (
"testing"
"time"

"github.com/tsg/gopacket/layers"

Expand Down Expand Up @@ -65,23 +64,3 @@ func TestIcmpTransactionHasErrorICMPv6(t *testing.T) {
trans5 := icmpTransaction{tuple: tuple, request: &icmpMessage{Type: layers.ICMPv6TypeRedirect}, response: nil}
assert.False(t, trans5.HasError(), "non-transactional request without response")
}

func TestIcmpTransactionResponseTimeMillis(t *testing.T) {
reqTime := time.Now()
resTime := reqTime.Add(time.Duration(1) * time.Second)

trans1 := icmpTransaction{request: &icmpMessage{ts: reqTime}, response: &icmpMessage{ts: resTime}}
time1, hasTime1 := trans1.ResponseTimeMillis()
assert.Equal(t, int32(1000), time1, "request with response")
assert.True(t, hasTime1, "request with response")

trans2 := icmpTransaction{request: &icmpMessage{ts: reqTime}}
time2, hasTime2 := trans2.ResponseTimeMillis()
assert.Equal(t, int32(0), time2, "request without response")
assert.False(t, hasTime2, "request without response")

trans3 := icmpTransaction{response: &icmpMessage{ts: resTime}}
time3, hasTime3 := trans3.ResponseTimeMillis()
assert.Equal(t, int32(0), time3, "response without request")
assert.False(t, hasTime3, "response without request")
}
19 changes: 11 additions & 8 deletions packetbeat/tests/system/test_0050_icmp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@ def test_2_pings(self):
assert len(objs) == 2
assert all([o["icmp.version"] == 4 for o in objs])
assert objs[0]["@timestamp"] == "2015-10-19T21:47:49.900Z"
assert objs[0]["responsetime"] == 12
assert objs[0]["event.duration"] == 12152000
assert objs[1]["@timestamp"] == "2015-10-19T21:47:49.924Z"
assert objs[1]["responsetime"] == 11
assert objs[1]["event.duration"] == 11935000
self.assert_common_fields(objs)
self.assert_common_icmp4_fields(objs[0])
self.assert_common_icmp4_fields(objs[1])
Expand All @@ -26,7 +26,7 @@ def test_icmp4_ping(self):
assert len(objs) == 1
assert objs[0]["icmp.version"] == 4
assert objs[0]["@timestamp"] == "2015-10-19T20:49:23.817Z"
assert objs[0]["responsetime"] == 20
assert objs[0]["event.duration"] == 20130000
self.assert_common_fields(objs)
self.assert_common_icmp4_fields(objs[0])

Expand All @@ -38,7 +38,7 @@ def test_icmp4_ping_over_vlan(self):
assert len(objs) == 1
assert objs[0]["icmp.version"] == 4
assert objs[0]["@timestamp"] == "2015-10-19T20:49:23.849Z"
assert objs[0]["responsetime"] == 12
assert objs[0]["event.duration"] == 12192000
self.assert_common_fields(objs)
self.assert_common_icmp4_fields(objs[0])

Expand All @@ -50,7 +50,7 @@ def test_icmp6_ping(self):
assert len(objs) == 1
assert objs[0]["icmp.version"] == 6
assert objs[0]["@timestamp"] == "2015-10-19T20:49:23.872Z"
assert objs[0]["responsetime"] == 16
assert objs[0]["event.duration"] == 16439000
self.assert_common_fields(objs)
self.assert_common_icmp6_fields(objs[0])

Expand All @@ -62,18 +62,20 @@ def test_icmp6_ping_over_vlan(self):
assert len(objs) == 1
assert objs[0]["icmp.version"] == 6
assert objs[0]["@timestamp"] == "2015-10-19T20:49:23.901Z"
assert objs[0]["responsetime"] == 12
assert objs[0]["event.duration"] == 12333000
self.assert_common_fields(objs)
self.assert_common_icmp6_fields(objs[0])

def assert_common_fields(self, objs):
assert all([o["type"] == "icmp" for o in objs])
assert all([o["bytes_in"] == 4 for o in objs])
assert all([o["bytes_out"] == 4 for o in objs])
assert all([o["event.dataset"] == "icmp" for o in objs])
assert all([o["source.bytes"] == 4 for o in objs])
assert all([o["destination.bytes"] == 4 for o in objs])
assert all([("server.port" in o) == False for o in objs])
assert all([("transport" in o) == False for o in objs])

def assert_common_icmp4_fields(self, obj):
assert obj["network.transport"] == "icmp"
assert obj["server.ip"] == "10.0.0.2"
assert obj["client.ip"] == "10.0.0.1"
assert obj["path"] == "10.0.0.2"
Expand All @@ -86,6 +88,7 @@ def assert_common_icmp4_fields(self, obj):
assert obj["icmp.response.code"] == 0

def assert_common_icmp6_fields(self, obj):
assert obj["network.transport"] == "ipv6-icmp"
assert obj["server.ip"] == "::2"
assert obj["client.ip"] == "::1"
assert obj["path"] == "::2"
Expand Down