Fix mysql parser handling of connection phase #8173
Conversation
packetbeat/protos/mysql/mysql.go
Outdated
| @@ -275,7 +277,6 @@ func mysqlMessageParser(s *mysqlStream) (bool, bool) { | |||
| m.ignoreMessage = true | |||
| s.parseState = mysqlStateEatMessage | |||
| } | |||
|
|
|||
| } else { | |||
There was a problem hiding this comment.
This branch can now no longer be entered since if s.client and else if !s.isClient cover all options. It should be removed. I wonder though if we now should have debug statements in the else branches where we currently ignore commands?
packetbeat/protos/mysql/mysql.go
Outdated
| @@ -500,9 +501,14 @@ func (mysql *mysqlPlugin) Parse(pkt *protos.Packet, tcptuple *common.TCPTuple, | |||
| } | |||
|
|
|||
| if priv.data[dir] == nil { | |||
| dstPort := tcptuple.DstPort | |||
| if dir == 0 { | |||
There was a problem hiding this comment.
Can we use tcp.TCPDirectionReverse instead of 0 if that's what its meaning is?
| @@ -540,17 +543,17 @@ func Test_gap_in_response(t *testing.T) { | |||
|
|
|||
| private := protos.ProtocolData(new(mysqlPrivateData)) | |||
|
|
|||
| private = mysql.Parse(&req, tcptuple, 0, private) | |||
| private = mysql.Parse(&resp, tcptuple, 1, private) | |||
| private = mysql.Parse(&req, tcptuple, 1, private) | |||
There was a problem hiding this comment.
Same in this file, can we use tcp.TCPDirectionReverse and tcp.TCPDirectionOriginal instead of 0 and 1 so we don't have magic numbers? :)
| self.run_packetbeat(pcap="mysql_connection.pcap") | ||
|
|
||
| objs = self.read_output() | ||
| assert len(objs) > 0 |
There was a problem hiding this comment.
Great that we have a test for this now. Can we add at least one more specific assert, e.g. assert objs[0]["query"] == "SELECT DATABASE()"?
There was a problem hiding this comment.
I didn't want to make the test too specific as I anticipate that further changes will cause it to generate more events, but it can be updated then
|
The server greeting packet("connection phase") is a special packet that be sent by server but seq is 0, so it will be ignored by the preview version code. Because the server greeting packet and login packet are not support yet, and the login transaction will be lost, but I don't think it will cause the following transactions be lost. But the tcp.direction is decided by the first packet captured by beats, and the program will be started at any time, so tcp.direction sometimes is reverse. So it's a good way to determine the direction by looking at the destination port. |
The mysql protocol parser assumes that transactions can only be started
by the client. This is true once the connection has been negotiated
("command phase"), but not during initial handshake ("connection phase").
This causes parsing problems when a connection is monitored from the
start, as sometimes the connection phase leaves the parser confused on
which side is client.
This patch modifies how client-side is detected, which can only be done
by looking at the destination port.
adriansr
force-pushed
the
fix/pb/mysql_conn_phase
branch
from
c430524
to
bce039a
Compare
The mysql protocol parser assumes that transactions can only be started by the client. This is true once the connection has been negotiated ("command phase"), but not during initial handshake ("connection phase").
This causes parsing problems when a connection is monitored from the beginning, as sometimes the connection phase leaves the parser confused on which side is client. As a result, transactions can be lost.
This patch modifies how client-side is detected, which can only be done by looking at the destination port.
Related to #8139
Blocks #8084