flow: SYN/FIN detection #382

vperelmax · 2017-08-13T16:22:03Z

Added fixes after first round of reviews

masco · 2017-08-14T16:29:01Z

ok to test

nplanel · 2017-08-15T05:23:47Z

flow/flow.go

@@ -240,7 +243,7 @@ func networkID(p *gopacket.Packet) int64 {
 	return id
 }

-// NewFlow creates a new empty flow
+// NewFlow returns extended with memory-only attributes Flow instance


"// NewFlow creates a new empty flow" is enough

nplanel · 2017-08-15T05:27:05Z

etc/skydive.yml.default

@@ -104,6 +104,8 @@ agent:
    # stats_update: 1
  metadata:
    info: This is compute node
+  tcp_len_by_seq: true


I understand that your patch for tcp_len_by_seq, counting flow bytes based on the tcp len and avoid counting the retransmission. Why leaving the flow bytes as it's done today and add a TCPRetransmissionLen in TCPFlowMetric
as we can store these datas and see number of retransmitted packets and bytes in a separated TCP only statistics

I would rather prefer to have TCP stack counters like, overlap, retransmission that we could add in TCP Metric, and enable them by default. Basically add counter like wireshark TCP analysis.
Another interesting subject would be IP fragmentation

What do you think ?

My patch doesn't deal so much with retransmissions. It refers more to the case when for example we capture only partial traffic (such as SYN/FIN) and then using sequence numbers we calculate traffic length.
Thing that you propose looks like good thing to have, but I see it as rather big and independent feature. So, I propose opening it as a new open issue and starting discussing it via our meetings.

Ok I understand your point
But if you don't take in account overlapping, retransmission, reordering packets, you flow tcp size metric would not be correct.

So let's do this patch step by step, and just focus on the syn/fin/rst flag and ttl only, this part sound good for me except few exception.
Could you remove the "unrelated" parts to tcp len calculation, I will prefer to see that in a separate patch

It's a pity that we have a big timezone difference, I will try catch you on IRC tonight (morning for you)

nplanel · 2017-08-15T05:31:07Z

flow/flow.go

@@ -499,6 +545,167 @@ func (f *Flow) updateMetricsWithNetworkLayer(packet *gopacket.Packet) error {
 	return errors.New("Unable to decode the IP layer")
 }

+func (f *Flow) updateMetricsSynCapture(packet *gopacket.Packet) error {


updateTCPMetrics() name will fit nicer

nplanel · 2017-08-15T05:34:31Z

flow/flow.go

+	}
+
+	if transportPacket.FIN {
+		logging.GetLogger().Notice("FIN", f.state.aBExpectedSeq, f.state.bAExpectedSeq, f.Metric.ABBytes, f.Metric.BABytes)


Notice is too verbose, you will see 1 log per tcp flow
As you don't dump the flowid I think you can remove this line

nplanel · 2017-08-15T05:34:56Z

flow/flow.go

@@ -528,6 +735,13 @@ func (f *Flow) newTransportLayer(packet *gopacket.Packet) error {
 		transportPacket, _ := transportLayer.(*layers.TCP)
 		f.Transport.A = strconv.Itoa(int(transportPacket.SrcPort))
 		f.Transport.B = strconv.Itoa(int(transportPacket.DstPort))
+		if config.GetConfig().GetBool("agent.capture_syn") {
+			f.TCPFlowMetric = &TCPMetric{
+				ABSynStart: int64(0),


don't need to initialize at 0, it's already the default in go

nplanel · 2017-08-15T05:36:42Z

flow/flow.go

+	}
+
+	switch field {
+	case "ABSynData":


This is already addressed by @safchain patch #299

OK, u r right, I will remove it

nplanel · 2017-08-15T05:41:19Z

flow/flow.go

+
+	length := uint32Distance(transportPacket.Seq, expectedSeq)
+
+	if transportPacket.Seq < TCPMaxSegmentSize {


I would prefer to see a logic that track the TCP segment, that will address the TCP partial segment retransmission, etc ...
Add this sliding window logic tracking in flowState
With these information, we could count number of restransmitted and overlaped packets and bytes easily and correctly
In the current version you only retransmission when the restransmit packet if old by TCPMaxSegmentSize, that it different than TCP window size.

It looks like additional feature...
In this feature we don't detect retransmittings and with BPF it is even impossible to detect them this way, we just calculate length of data that passes in the session in a more exact way using sequence numbers, rather than just calculating passed bytes, because not always we look at all the packets that pass (only SYN/FIN/RST, if BPF is used).
Its my fault that I have added retransmit to the title, I have removed it.
I think that thing that you propose to calculate is more complex to do and I propose to open it as an issue (plan for the next feature).

nplanel · 2017-08-15T05:41:50Z

flow/flow.go

+		logging.GetLogger().Notice("Overlap", transportPacket.Seq, expectedSeq)
+	}
+
+	if length > 0x80010000 {


why 0x80010000 ? -TCPMaxSegmentSize ?

It wont pass compilation in go

yes, the idea was have a signed distance

nplanel · 2017-08-16T03:51:16Z

flow/flow.proto

@@ -131,7 +145,7 @@ message Flow {
  string BNodeTID = 35;

 /* raw packets, will not be exported, see Makefile */
-  repeated RawPacket LastRawPackets = 36;
+  repeated RawPacket LastRawPackets = 38;


don't change the protobuf ID from other field

nplanel · 2017-08-16T03:52:23Z

flow/flow_test.go

@@ -79,6 +79,12 @@ func TestFlowJSON(t *testing.T) {
 		},
 		Start: 1111,
 		Last:  222,
+		LastUpdateMetric: &FlowMetric{


don't declare field that are 0 or "", golang guarantee that by design, just LastUpdateMetric: &FlowMetric{}

nplanel · 2017-08-16T03:53:28Z

flow/flow_test.go

@@ -79,6 +79,12 @@ func TestFlowJSON(t *testing.T) {
 		},


you should add a test based based on simple pcap trace like tests : TestFlowSimpleIPv4 and TestFlowSimpleIPv6

nplanel · 2017-08-16T03:55:01Z

flow/flow_test.go

@@ -314,6 +339,12 @@ func TestPCAP1(t *testing.T) {
 				BAPackets: 1,
 				BABytes:   42,
 			},
+			LastUpdateMetric: &FlowMetric{


nplanel · 2017-08-16T04:00:35Z

flow/storage/elasticsearch/elasticsearch.go

@@ -81,6 +81,54 @@ const flowMapping = `
 					"type": "date", "format": "epoch_millis"
 				}
 			}
+		},
+		{


just declare these with simple regex like match by modify the "start"
like this :
{
"start": {
"match": "*Start",
"mapping": {
"type": "date", "format": "epoch_millis"
}
}
},

nplanel · 2017-08-16T04:10:04Z

flow/storage/orientdb/orientdb.go

+			"LastUpdateMetric":   lastMetricDoc,
+			"Metric":             metricDoc,
+			"TCPFlowMetric":      tcpMetric,
+			"Start":              flow.Start,


same here "TCPMetrics don't have Start and Last"

nplanel · 2017-08-16T04:10:47Z

flow/storage/orientdb/orientdb.go

+		class := orient.ClassDefinition{
+			Name: "TCPMetric",
+			Properties: []orient.Property{
+				{Name: "Start", Type: "LONG", Mandatory: true, NotNull: true},


same here "TCPMetrics don't have Start and Last"

OK, uploading change

nplanel · 2017-08-16T04:15:07Z

flow/flow.go

+	if f.state.lenBySeq {
+		f.updateMetricsBySeq(packet)
+	} else if updated := f.updateMetricsWithLinkLayer(packet, length); !updated {
+		logging.GetLogger().Error("Update metrics with network\n")


please remove this log

nplanel · 2017-08-16T04:32:14Z

etc/skydive.yml.default

@@ -104,6 +104,8 @@ agent:
    # stats_update: 1
  metadata:
    info: This is compute node
+  tcp_len_by_seq: true


Ok I understand your point
But if you don't take in account overlapping, retransmission, reordering packets, you flow tcp size metric would not be correct.

So let's do this patch step by step, and just focus on the syn/fin/rst flag and ttl only, this part sound good for me except few exception.
Could you remove the "unrelated" parts to tcp len calculation, I will prefer to see that in a separate patch

It's a pity that we have a big timezone difference, I will try catch you on IRC tonight (morning for you)

nplanel · 2017-08-16T04:37:34Z

flow/flow.go

+		logging.GetLogger().Notice("Overlap", transportPacket.Seq, expectedSeq)
+	}
+
+	if length > 0x80010000 {


yes, the idea was have a signed distance

nplanel · 2017-08-19T07:52:36Z

flow/flow.go

+
+	}
+
+	var captureTime int64


you already test metadata != nil a the begining already, reuse it and replace the whole section by 👍
captureTime := common.UnixMillis(metadata.CaptureInfo.Timestamp)

nplanel · 2017-08-19T07:53:39Z

flow/flow.go

+func (f *Flow) updateTCPMetrics(packet *gopacket.Packet) error {
+	// capture content of SYN packets
+	//bypass if not TCP
+	if f.Transport == nil || f.Transport.Protocol != FlowProtocol_TCPPORT {


may add "|| f.Network == nil"

nplanel · 2017-08-19T07:58:24Z

flow/flow.go

@@ -607,6 +698,7 @@ func PacketsFromGoPacket(packet *gopacket.Packet, outerLength int64, t int64, bp

 	if len(flowPackets.Packets) > 0 {
 		p := gopacket.NewPacket(packetData[start:], topLayer.LayerType(), gopacket.NoCopy)
+		p.Metadata().CaptureInfo = packet_capture_info


Why do it all at once here (and check metadata)
if metadata := (*packet).Metadata(); metadata != nil {
p.Metadata().CaptureInfo = metadata.CaptureInfo
}

I do it here because, p is a packet copy that is created and scoped only here and NewPacket is not our function (we cant change it) and it doesn't copy the metadata that we need (the timestemps), so we loose here the data. But you are right, there is some mess in the code, I will fix it and also add check for metadata.

nplanel · 2017-08-19T08:00:04Z

flow/flow.go

@@ -770,6 +890,10 @@ func (f *Flow) GetFieldInt64(field string) (_ int64, err error) {
 		return f.Metric.GetFieldInt64(fields[1])
 	case "LastUpdateMetric":
 		return f.LastUpdateMetric.GetFieldInt64(fields[1])
+	case "TCPFlowMetric":
+		if f.TCPFlowMetric != nil {
+			return f.TCPFlowMetric.GetFieldInt64(fields[1])


you can call directly the return f.TCPFlowMetric.GetFieldInt64( .... as you do the test already line 794

nplanel · 2017-08-19T08:00:45Z

flow/flow.proto

@@ -80,6 +80,17 @@ message RawPacket {
  bytes Data = 3;
 }

+message TCPMetric {
+	int64 ABSynStart = 7;


why not start protobuf ID at 1

nplanel · 2017-08-19T08:09:06Z

flow/storage/orientdb/orientdb.go

-		"RawPacketsCaptured": flow.RawPacketsCaptured,
+	tcpMetric := flowTCPMetricToDocument(flow, flow.TCPFlowMetric)
+	var flowDoc orient.Document
+	if tcpMetric != nil {


don't do a big if here, just do after flowDoc declaration
if tcpMetric != nil {
flowDoc["TCPFlowMetric"] = tcpMetric
}

nplanel · 2017-08-19T08:10:10Z

flow/storage/orientdb/orientdb.go

+		class := orient.ClassDefinition{
+			Name: "TCPMetric",
+			Properties: []orient.Property{
+				{Name: "ABSynStart", Type: "LONG", Mandatory: false, NotNull: false},


put the NotNull to true for all TCPMetric

nplanel · 2017-08-19T08:16:07Z

flow/flow_test.go

@@ -273,9 +285,8 @@ func validateAllParentChains(t *testing.T, table *Table) {

 func flowsFromPCAP(t *testing.T, filename string, linkType layers.LinkType, bpf *BPF) []*Flow {
 	table := NewTable(nil, nil, NewEnhancerPipeline(), "", TableOpts{})
-
+	table.SetSynCapture(true)


as the flag is global to the agent, and will be set only by the user, set the config flag directly here for the purpose of the test, so place this code in your unit test (TestFlowSimpleSynFin)

config.GetConfig().Set("agent.capture_syn", true)
defer config.GetConfig().Set("agent.capture_syn", false)

nplanel · 2017-08-19T08:17:22Z

flow/table.go

@@ -349,6 +349,11 @@ func (ft *Table) flowPacketToFlow(packet *Packet, parentUUID string, t int64, L2
 	return flow
 }

+// Enable/Disable capturing SYN packets
+func (ft *Table) SetSynCapture(enableSynCapture bool) {


remove this class method, see comment above

OK, I removed it.
The bug was that I used two different folders of config, now, everything works well.
Thanks!

nplanel · 2017-08-19T08:23:02Z

flow/flow_test.go

 			if p.ErrorLayer() != nil {
 				t.Fatalf("Failed to decode packet with layer path '%s': %s\n", layerPathFromGoPacket(&p), p.ErrorLayer().Error())
 			}
 			pcapPacketNB++
 			if strings.Contains(layerPathFromGoPacket(&p), "DecodeFailure") {
 				t.Fatalf("GoPacket decode this pcap packet %d as DecodeFailure :\n%s", pcapPacketNB, p.Dump())
 			}
-
+			config.GetConfig().Set("agent.capture_syn", true)


why forcing the flag here, old debuging trace I assume ...

nplanel

LGTM except few typo/cosmetic glitch

nplanel · 2017-08-21T06:26:39Z

flow/flow.proto

@@ -80,6 +80,17 @@ message RawPacket {
  bytes Data = 3;
 }

+message TCPMetric {
+	int64 ABSynStart = 1;


ident this block like others, prefix with 2 spaces

nplanel · 2017-08-21T06:26:53Z

flow/flow.proto

@@ -106,6 +117,9 @@ message Flow {
 /* Total amount of data for the whole flow duration */
 	FlowMetric Metric = 32;

+/* Floe metric specific to the TCP Protocol only and optional */
+  TCPMetric TCPFlowMetric = 38;


NodeTID is 33, I think it is impossible in the same struct to have two elements with the same ID

Beforehand you asked not to change any existing IDs, and I tried to chenge ID to 33, as I thought it is impossible...
I also didnt want to move this filed down in the list of fileds, because it is its logical location...

nplanel · 2017-08-21T06:27:22Z

flow/flow_test.go

+func TestFlowSimpleSynFin(t *testing.T) {
+	flows := flowsFromPCAP(t, "pcaptraces/simple-tcpv4.pcap", layers.LinkTypeEthernet, nil)
+	// In test pcap SYNs happen at 2017-03-21 10:58:23.768977 +0200 IST
+	synTimestemp := int64(1490086703768)


s/Timestemp/Timestamp/g

nplanel · 2017-08-21T06:27:31Z

flow/metrics.go

+// Copy extended metric
+func (fm *TCPMetric) Copy() *TCPMetric {
+	return &TCPMetric{
+


remove this line

nplanel · 2017-08-21T06:27:45Z

flow/storage/orientdb/orientdb.go

@@ -67,11 +67,32 @@ func flowMetricToDocument(flow *flow.Flow, metric *flow.FlowMetric) orient.Docum
 	}
 }

+func flowTCPMetricToDocument(flow *flow.Flow, tcp_metric *flow.TCPMetric) orient.Document {
+


remove this line

nplanel · 2017-08-21T06:28:23Z

flow/table.go

@@ -208,7 +208,6 @@ func (ft *Table) updateMetric(f *Flow, start, last int64) {
 	} else {
 		f.LastUpdateStart = f.Start
 	}
-


don't remove this line, cosmetic

Capturing of SYN/FIN/RST timestamps Additional detailes that are captured: 1. Add support for elastic search scheme in date format for ABSynStart 2. Use accurate clock from the packet info (and not from flow-table) 3. Make extended capturing configurable via agent paramewqters in .yml 4. Change tests to reflect changes in the tables that we create Related issue: skydive-project#323

vperelmax changed the title ~~SYN/FIN/Retransmit detection~~ SYN/FIN/Retransmition detection Aug 14, 2017

vperelmax changed the title ~~SYN/FIN/Retransmition detection~~ flow: SYN/FIN/Retransmition detection Aug 14, 2017

nplanel reviewed Aug 15, 2017

View reviewed changes

nplanel mentioned this pull request Aug 15, 2017

flow: Enable SYN/FIN/RST capturing #329

Closed

vperelmax force-pushed the master branch from 0de71ef to cd2b05e Compare August 15, 2017 13:58

vperelmax changed the title ~~flow: SYN/FIN/Retransmition detection~~ flow: SYN/FIN detection Aug 15, 2017

vperelmax force-pushed the master branch from cd2b05e to 846f068 Compare August 15, 2017 15:36

nplanel reviewed Aug 16, 2017

View reviewed changes

vperelmax force-pushed the master branch 4 times, most recently from 62ce494 to 78d5b53 Compare August 18, 2017 19:09

nplanel reviewed Aug 19, 2017

View reviewed changes

vperelmax force-pushed the master branch 3 times, most recently from 8f8d3b7 to 4e01253 Compare August 20, 2017 11:10

nplanel reviewed Aug 21, 2017

View reviewed changes

vperelmax force-pushed the master branch from 4e01253 to 14fd01e Compare August 21, 2017 07:04

nplanel merged commit 5a4a7e1 into skydive-project:master Aug 21, 2017


		length := uint32Distance(transportPacket.Seq, expectedSeq)

		if transportPacket.Seq < TCPMaxSegmentSize {

flow: SYN/FIN detection #382

flow: SYN/FIN detection #382

Conversation

vperelmax commented Aug 13, 2017

masco commented Aug 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nplanel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment