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

Add support for TLS protocol tracing #2096

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ddelnano
Copy link
Member

Summary: Add support for TLS protocol tracing

This is the final change to wire up the tls protocol parser and stitcher into stirling. I've also filed #2095 to track supporting tracing TLS handshakes and the application data.

Relevant Issues: N/A

Type of change: /kind feature

Test Plan: New tests verify functionality works end to end

Changelog Message: Added support for tracing TLS handshakes. This can be enabled with --stirling_enable_tls_tracing=1 or through the PX_STIRLING_ENABLE_TLS_TRACING environment variable. Until #2095 is addressed, this will disable tracing the plaintext within encrypted connections.

@ddelnano ddelnano requested a review from a team as a code owner January 23, 2025 21:10
@ddelnano ddelnano force-pushed the ddelnano/wire-up-tls-protocol-parser-to-stirling branch from 7de9384 to b8ed611 Compare January 23, 2025 21:13
@ddelnano ddelnano requested a review from a team as a code owner January 24, 2025 15:09
@ddelnano ddelnano marked this pull request as draft January 27, 2025 15:33
@ddelnano ddelnano force-pushed the ddelnano/wire-up-tls-protocol-parser-to-stirling branch 2 times, most recently from 8c0cffd to 3e556d6 Compare January 29, 2025 16:02
@ddelnano ddelnano force-pushed the ddelnano/wire-up-tls-protocol-parser-to-stirling branch from 3e556d6 to 8ff4143 Compare January 29, 2025 16:03
Signed-off-by: Dom Del Nano <[email protected]>
@@ -195,7 +195,7 @@ struct Frame : public FrameBase {

HandshakeType handshake_type;

uint24_t handshake_length;
uint24_t handshake_length = uint24_t(0);
Copy link
Member Author

Choose a reason for hiding this comment

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

GCC fails if this isn't initialized (buildbuddy failure)

@ddelnano ddelnano marked this pull request as ready for review January 29, 2025 17:41
Comment on lines 40 to 51
{"req_type", "The type of request from the TLS record (Client/ServerHello, etc.)",
types::DataType::INT64,
types::SemanticType::ST_NONE,
types::PatternType::GENERAL_ENUM},
{"version", "Version of TLS record",
types::DataType::INT64,
types::SemanticType::ST_NONE,
types::PatternType::GENERAL_ENUM},
{"extensions", "Extensions in the TLS record",
types::DataType::STRING,
types::SemanticType::ST_NONE,
types::PatternType::GENERAL},
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have examples of the records we would see? Is version and extensions valid on all those records?

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 gist shows the output from stirling_wrapper (also copied below). Note: that the content type and version fields will have carnot function support that turn their integer representations into human readable strings. I already have that completed, but wanted to make that its own PR.

record=[
 req=[TLS Frame [len=216 content_type=22 legacy_version=769 handshake_version=771 handshake_type=1 extensions={"server_name":"[\"google.com\"]"}]
 resp=[TLS Frame [len=100 content_type=22 legacy_version=771 handshake_version=771 handshake_type=2 extensions={}]
]

As of now, the tls_events table inserts one record for each TLS handshake. Thus, the version and extensions columns will always have a value. I think it could be useful to support alert, heartbeat, or change cipher messages in the future. These messages don't contain extensions, and while they do have a "version" field, it is the legacy version used in TLS 1.2 and earlier.

Copy link
Member Author

@ddelnano ddelnano Feb 11, 2025

Choose a reason for hiding this comment

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

@oazizi000 after our discussion on the tls_events table schema, I made the following changes in 3f80935:

  • Renamed req_type to content_type: this better matches what was already inserted into the table (handshakes) and will map nicely to alerts, heartbeats, etc.
  • Removed the extensions column in favor of req_body and resp_body: this allows for storing extensions that exist on both the client and server side and means that the field is no longer specific to handshake records.

…TLS records. Update JSON parsing to better match new structure

Signed-off-by: Dom Del Nano <[email protected]>
@ddelnano ddelnano force-pushed the ddelnano/wire-up-tls-protocol-parser-to-stirling branch from f48ebb6 to 3f80935 Compare February 11, 2025 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants