TLS module for packetbeat #5476

adriansr · 2017-10-30T11:52:07Z

This patch adds TLS protocol support to Packetbeat.

It will parse the initial handshake and send an event to Elasticsearch for every TLS connection.

Events contain:

ciphers supported by client
cipher selected by server
extensions
client and server certificate chain (if any)
- key algorithm
- signing algorithm
- subject alternative name (SAN)
- validity dates
whether the session is resumed using session IDs or tickets
alert (if any)

There is support for Application Layer Protocol Negotiation (ALPN) extension to TLS. It parses extension data found in hello messages. This involves a list of protocols in the client hello:

"application_layer_protocol_negotiation": "h2,http/1.1"

And the selected protocol in the server hello.

"application_layer_protocol_negotiation": "h2"

References

ALPN: https://en.wikipedia.org/wiki/Application-Layer_Protocol_Negotiation

adriansr · 2017-10-30T11:53:08Z

Please review when you have time. It is a work in progress (working on the tests now)

andrewkroh

You are making good progress. Things look good so far to me. I'll take another look when you get closer to done.

andrewkroh · 2017-10-31T14:25:58Z

packetbeat/_meta/fields.yml

+    - name: tls
+      type: group
+      fields:
+        - name: negotiated cypher


This field name has a space in it. Probably you want an underscore here. But it looks like this file is progress because I don't see this field in the code so I'm sure you be updating all of this.

urso · 2017-11-02T12:29:16Z

packetbeat/protos/tls/parse.go

+	return m
+}
+
+func (parser *parser) parse(buf *streambuf.Buffer, conn *tlsConnectionData, plugin *tlsPlugin) (bool, bool) {


Try not to introduce circular dependencies in any way. At optimium the parser must be testable without requiring an instance of tlsPlugin. This simplifies potential unit and/or fuzzy testing of the parser only.

The TCP code generate creates different types with different responsibilities on purpose. One type per task like: manage connections, parse, correlate messages, create events, publish events

urso · 2017-11-02T12:31:45Z

packetbeat/protos/tls/parse.go

+
+func (parser *parser) parse(buf *streambuf.Buffer, conn *tlsConnectionData, plugin *tlsPlugin) (bool, bool) {
+
+	if conn.handshakeCompleted {


Abstraction leak. The parser should be about parsing messages from receive buffer only. The parser should not have to care about the connection state if possible.

if connection is 'completed' and there is nothing new to learn during the lifetime of the connect, clear the buffers more early and consider not appending to the buffer again. This will make the module much more lightweight.

urso · 2017-11-02T12:34:11Z

packetbeat/protos/tls/parse.go

+		switch header.recordType {
+		case recordTypeChangeCipherSpec: // single message of size 1 (byte 1)
+			conn.handshakeCompleted = true
+			plugin.sendEvent(conn)


Abstraction leak. Parser should not directly trigger an event, depending on potential connection state. Instead parser should return/report messages only.

urso · 2017-11-02T12:36:15Z

packetbeat/protos/tls/parse.go

+	return true
+}
+
+func (parser *parser) processHandshake(conn *tlsConnectionData, handshakeType handshakeType, reader bufferView) bool {


This looks like a task for the message 'correlator'. Note, depending on deployment (e.g. port forwarding), packetbeat might potentially see packets out of order.

urso · 2017-11-02T12:38:30Z

Nice progress. Can't wait for finally having TLS envelope support 🎉

adriansr · 2017-11-06T17:45:43Z

Will update with correct fields.yml

houndci-bot · 2017-11-06T22:36:46Z

packetbeat/protos/tls/parse.go

+		return nil
+	}
+
+	certs := make([]*x509.Certificate, 0)


can probably use "var certs []*x509.Certificate" instead

houndci-bot · 2017-11-06T22:36:46Z

packetbeat/protos/tls/parse.go

+		return nil
+	}
+
+	certs := make([]*x509.Certificate, 0)


can probably use "var certs []*x509.Certificate" instead

andrewkroh · 2017-11-09T15:37:48Z

packetbeat/_meta/fields.yml

+            - name: timestamp
+              type: long
+              description: >
+                The current time and date in standard UNIX 32-bit format


Is this seconds since Unix epoch? Either way I suggest converting it to a date rather than a long so that’s easier to use in ES.

Right. Will do

andrewkroh · 2017-11-09T15:40:54Z

packetbeat/_meta/fields.yml

+              fields:
+                - name: server_name_indication
+                  type: string
+                  description: Comma-separated list of server names


Instead of comma separated, I think a list would be better for searching and aggregating.

andrewkroh · 2017-11-09T15:44:55Z

packetbeat/_meta/fields.yml

+                - name: session_ticket
+                  type: string
+                  description: >
+                    Length of the session ticket, if provided, or empty


“or empty” - does this mean the field is not sent when a value is not present?

andrewkroh · 2017-11-09T15:46:52Z

packetbeat/_meta/fields.yml

+
+                - name: session_ticket
+                  type: string
+                  description: Empty


Could you please clarify here.

andrewkroh · 2017-11-09T15:50:21Z

packetbeat/_meta/fields.yml

+              description: X509 format version.
+
+            - name: serial_number
+              type: keyword


Are the some benefits to sending this as a keyword rather than a long? Will users be expecting to see hex? Maybe we should have a field formater in Kibana for hex (possibly there is one; I didn’t check).

The serial number can be up to 160 bits long so it doesn't fit on a numeric type.

I have changed the format to decimal. The most common way of representation seems to be decimal up to 64 bit in length and then hexadecimal number or hex dump of individual bytes. I will stick with decimal in a keyword field.

This patch adds TLS protocol support for packetbeat. It will parse the initial handshake and send to Elasticsearch an event for every TLS connection. This event contains: - ciphers supported by client - cipher selected by server - extensions - client and server certificate chain (if any) - whether the session is resumed using session IDs or tickets

This parses the Application Layer Protocol Negotiation extension in hello messages. This involves a list of protocols in the client hello: "application_layer_protocol_negotiation": "h2,http/1.1" And the selected protocol in the server hello. "application_layer_protocol_negotiation": "h2"

Both decimal and hexadecimal are used interchangeably. Unfortunately long datatype cannot be used for the serial number as it can have a length of up to 160 bits.

andrewkroh

LGTM

monicasarbu · 2017-12-11T09:53:18Z

@adriansr I just realized that this PR doesn't have a changelog. I think the easiest would be if @tsg adds it to the #5853.

adriansr requested a review from andrewkroh October 30, 2017 11:52

adriansr added in progress Pull request is currently in progress. Packetbeat labels Oct 30, 2017

adriansr force-pushed the tls branch from 50fc8e5 to e98a280 Compare October 30, 2017 16:20

andrewkroh requested a review from urso October 30, 2017 21:02

andrewkroh reviewed Oct 31, 2017

View reviewed changes

urso reviewed Nov 2, 2017

View reviewed changes

adriansr force-pushed the tls branch from 46358b1 to ab00950 Compare November 6, 2017 17:43

adriansr added review and removed in progress Pull request is currently in progress. labels Nov 6, 2017

adriansr force-pushed the tls branch 3 times, most recently from 93b394f to b3f8885 Compare November 6, 2017 22:36

houndci-bot reviewed Nov 6, 2017

View reviewed changes

adriansr force-pushed the tls branch 4 times, most recently from 3e7e9cb to e46b850 Compare November 7, 2017 10:55

adriansr added the module label Nov 7, 2017

andrewkroh reviewed Nov 9, 2017

View reviewed changes

adriansr added 3 commits November 10, 2017 02:33

Added fields description to fields.yml

428c5a6

Support for Subject Alternative Names in certificates

1e8401a

adriansr added 9 commits November 10, 2017 02:33

Golint fixes

079ed4b

Fix bad formatting in fields.yml

5312b8a

Updated fields.asciidoc for packetbeat

1a72ee9

Missing extensions fields

3794223

Report timestamp field as a date

b0edb88

Use array fields for string list extensions

4cad5f9

Better descriptions for ticket extension

3066128

Use decimal representation for certs serial number

05603c8

Both decimal and hexadecimal are used interchangeably. Unfortunately long datatype cannot be used for the serial number as it can have a length of up to 160 bits.

adriansr force-pushed the tls branch from ee60744 to 05603c8 Compare November 10, 2017 15:18

Fix spurious warning message

c585045

andrewkroh approved these changes Nov 13, 2017

View reviewed changes

andrewkroh merged commit 4ca792f into elastic:master Nov 14, 2017

tsg mentioned this pull request Nov 20, 2017

new protocol: SSL envelope #3604

Closed

5 tasks

monicasarbu changed the title ~~TLS plugin for packetbeat~~ TLS module for packetbeat Nov 20, 2017

adriansr deleted the tls branch November 22, 2017 09:37

tbragin mentioned this pull request Dec 29, 2017

Packetbeat flows enhancements #3444

Open

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TLS module for packetbeat #5476

TLS module for packetbeat #5476

adriansr commented Oct 30, 2017 •

edited by andrewkroh

Loading

adriansr commented Oct 30, 2017

andrewkroh left a comment

andrewkroh Oct 31, 2017

urso Nov 2, 2017

urso Nov 2, 2017

urso Nov 2, 2017

urso Nov 2, 2017

urso commented Nov 2, 2017

adriansr commented Nov 6, 2017

houndci-bot Nov 6, 2017

houndci-bot Nov 6, 2017

andrewkroh Nov 9, 2017

adriansr Nov 10, 2017

andrewkroh Nov 9, 2017

andrewkroh Nov 9, 2017

andrewkroh Nov 9, 2017

andrewkroh Nov 9, 2017

adriansr Nov 10, 2017

andrewkroh left a comment

monicasarbu commented Dec 11, 2017


		func (parser parser) parse(buf streambuf.Buffer, conn tlsConnectionData, plugin tlsPlugin) (bool, bool) {

		if conn.handshakeCompleted {

TLS module for packetbeat #5476

TLS module for packetbeat #5476

Conversation

adriansr commented Oct 30, 2017 • edited by andrewkroh Loading

adriansr commented Oct 30, 2017

andrewkroh 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

urso commented Nov 2, 2017

adriansr commented Nov 6, 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

andrewkroh left a comment

Choose a reason for hiding this comment

monicasarbu commented Dec 11, 2017

adriansr commented Oct 30, 2017 •

edited by andrewkroh

Loading