Skip to content

Commit c0ff63a

Browse files
ddelnanoddelnano
authored
Remove legacy TLS tracing feature toggle and transitional code (#1530)
Summary: Remove legacy TLS tracing feature toggle and transitional code The new style of TLS tracing has been rolled out since April 11th (vizier release v0.12.19). We believe that it is performing well and it has already allowed Pixie's TLS tracing to cover more libraries (OpenSSL v3) with BoringSSL coming soon. This includes changes from #1518 and must be rebased once that is merged. Relevant Issues: #692 Type of change: /kind cleanup Test Plan: Existing test coverage --------- Signed-off-by: Dom Del Nano <ddelnano@pixielabs.ai>
1 parent d7a1f67 commit c0ff63a

File tree

6 files changed

+54
-94
lines changed

6 files changed

+54
-94
lines changed

‎src/cloud/config_manager/controllers/vizier_feature_flags.go‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -42,11 +42,6 @@ var availableFeatureFlags = []*featureFlag{
4242
VizierFlagName: "PL_PROFILER_JAVA_SYMBOLS",
4343
DefaultValue: true,
4444
},
45-
{
46-
FeatureFlagName: "access-tls-socket-fd-via-syscall",
47-
VizierFlagName: "PL_ACCESS_TLS_SOCKET_FD_VIA_SYSCALL",
48-
DefaultValue: false,
49-
},
5045
}
5146

5247
// NewVizierFeatureFlagClient creates a LaunchDarkly feature flag client if the SDK key is provided,

‎src/stirling/source_connectors/socket_tracer/bcc_bpf/socket_trace.c‎

Lines changed: 12 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1058,9 +1058,7 @@ int syscall__probe_ret_write(struct pt_regs* ctx) {
10581058
// Syscalls that aren't exclusively used for networking must be
10591059
// validated to be a sock_event before propagating a socket fd to the
10601060
// tls tracing probes
1061-
if (ACCESS_TLS_SK_FD_VIA_ACTIVE_SYSCALL) {
1062-
propagate_fd_to_user_space_call(id, write_args->fd);
1063-
}
1061+
propagate_fd_to_user_space_call(id, write_args->fd);
10641062
process_syscall_data(ctx, id, kEgress, write_args, bytes_count);
10651063
}
10661064

@@ -1072,9 +1070,7 @@ int syscall__probe_ret_write(struct pt_regs* ctx) {
10721070
int syscall__probe_entry_send(struct pt_regs* ctx, int sockfd, char* buf, size_t len) {
10731071
uint64_t id = bpf_get_current_pid_tgid();
10741072

1075-
if (ACCESS_TLS_SK_FD_VIA_ACTIVE_SYSCALL) {
1076-
propagate_fd_to_user_space_call(id, sockfd);
1077-
}
1073+
propagate_fd_to_user_space_call(id, sockfd);
10781074

10791075
// Stash arguments.
10801076
struct data_args_t write_args = {};
@@ -1124,9 +1120,7 @@ int syscall__probe_ret_read(struct pt_regs* ctx) {
11241120
// Syscalls that aren't exclusively used for networking must be
11251121
// validated to be a sock_event before propagating a socket fd to the
11261122
// tls tracing probes
1127-
if (ACCESS_TLS_SK_FD_VIA_ACTIVE_SYSCALL) {
1128-
propagate_fd_to_user_space_call(id, read_args->fd);
1129-
}
1123+
propagate_fd_to_user_space_call(id, read_args->fd);
11301124
process_syscall_data(ctx, id, kIngress, read_args, bytes_count);
11311125
}
11321126

@@ -1138,9 +1132,7 @@ int syscall__probe_ret_read(struct pt_regs* ctx) {
11381132
int syscall__probe_entry_recv(struct pt_regs* ctx, int sockfd, char* buf, size_t len) {
11391133
uint64_t id = bpf_get_current_pid_tgid();
11401134

1141-
if (ACCESS_TLS_SK_FD_VIA_ACTIVE_SYSCALL) {
1142-
propagate_fd_to_user_space_call(id, sockfd);
1143-
}
1135+
propagate_fd_to_user_space_call(id, sockfd);
11441136

11451137
// Stash arguments.
11461138
struct data_args_t read_args = {};
@@ -1172,9 +1164,7 @@ int syscall__probe_entry_sendto(struct pt_regs* ctx, int sockfd, char* buf, size
11721164
const struct sockaddr* dest_addr, socklen_t addrlen) {
11731165
uint64_t id = bpf_get_current_pid_tgid();
11741166

1175-
if (ACCESS_TLS_SK_FD_VIA_ACTIVE_SYSCALL) {
1176-
propagate_fd_to_user_space_call(id, sockfd);
1177-
}
1167+
propagate_fd_to_user_space_call(id, sockfd);
11781168

11791169
// Stash arguments.
11801170
if (dest_addr != NULL) {
@@ -1235,9 +1225,7 @@ int syscall__probe_entry_recvfrom(struct pt_regs* ctx, int sockfd, char* buf, si
12351225
struct sockaddr* src_addr, socklen_t* addrlen) {
12361226
uint64_t id = bpf_get_current_pid_tgid();
12371227

1238-
if (ACCESS_TLS_SK_FD_VIA_ACTIVE_SYSCALL) {
1239-
propagate_fd_to_user_space_call(id, sockfd);
1240-
}
1228+
propagate_fd_to_user_space_call(id, sockfd);
12411229

12421230
// Stash arguments.
12431231
if (src_addr != NULL) {
@@ -1283,9 +1271,7 @@ int syscall__probe_entry_sendmsg(struct pt_regs* ctx, int sockfd,
12831271
const struct user_msghdr* msghdr) {
12841272
uint64_t id = bpf_get_current_pid_tgid();
12851273

1286-
if (ACCESS_TLS_SK_FD_VIA_ACTIVE_SYSCALL) {
1287-
propagate_fd_to_user_space_call(id, sockfd);
1288-
}
1274+
propagate_fd_to_user_space_call(id, sockfd);
12891275

12901276
if (msghdr != NULL) {
12911277
// Stash arguments.
@@ -1333,9 +1319,7 @@ int syscall__probe_entry_sendmmsg(struct pt_regs* ctx, int sockfd, struct mmsghd
13331319
unsigned int vlen) {
13341320
uint64_t id = bpf_get_current_pid_tgid();
13351321

1336-
if (ACCESS_TLS_SK_FD_VIA_ACTIVE_SYSCALL) {
1337-
propagate_fd_to_user_space_call(id, sockfd);
1338-
}
1322+
propagate_fd_to_user_space_call(id, sockfd);
13391323

13401324
// TODO(oazizi): Right now, we only trace the first message in a sendmmsg() call.
13411325
if (msgvec != NULL && vlen >= 1) {
@@ -1389,9 +1373,7 @@ int syscall__probe_ret_sendmmsg(struct pt_regs* ctx) {
13891373
int syscall__probe_entry_recvmsg(struct pt_regs* ctx, int sockfd, struct user_msghdr* msghdr) {
13901374
uint64_t id = bpf_get_current_pid_tgid();
13911375

1392-
if (ACCESS_TLS_SK_FD_VIA_ACTIVE_SYSCALL) {
1393-
propagate_fd_to_user_space_call(id, sockfd);
1394-
}
1376+
propagate_fd_to_user_space_call(id, sockfd);
13951377

13961378
if (msghdr != NULL) {
13971379
// Stash arguments.
@@ -1441,9 +1423,7 @@ int syscall__probe_entry_recvmmsg(struct pt_regs* ctx, int sockfd, struct mmsghd
14411423
unsigned int vlen) {
14421424
uint64_t id = bpf_get_current_pid_tgid();
14431425

1444-
if (ACCESS_TLS_SK_FD_VIA_ACTIVE_SYSCALL) {
1445-
propagate_fd_to_user_space_call(id, sockfd);
1446-
}
1426+
propagate_fd_to_user_space_call(id, sockfd);
14471427

14481428
// TODO(oazizi): Right now, we only trace the first message in a recvmmsg() call.
14491429
if (msgvec != NULL && vlen >= 1) {
@@ -1518,9 +1498,7 @@ int syscall__probe_ret_writev(struct pt_regs* ctx) {
15181498
// Syscalls that aren't exclusively used for networking must be
15191499
// validated to be a sock_event before propagating a socket fd to the
15201500
// tls tracing probes
1521-
if (ACCESS_TLS_SK_FD_VIA_ACTIVE_SYSCALL) {
1522-
propagate_fd_to_user_space_call(id, write_args->fd);
1523-
}
1501+
propagate_fd_to_user_space_call(id, write_args->fd);
15241502
process_syscall_data_vecs(ctx, id, kEgress, write_args, bytes_count);
15251503
}
15261504

@@ -1553,9 +1531,7 @@ int syscall__probe_ret_readv(struct pt_regs* ctx) {
15531531
// Syscalls that aren't exclusively used for networking must be
15541532
// validated to be a sock_event before propagating a socket fd to the
15551533
// tls tracing probes
1556-
if (ACCESS_TLS_SK_FD_VIA_ACTIVE_SYSCALL) {
1557-
propagate_fd_to_user_space_call(id, read_args->fd);
1558-
}
1534+
propagate_fd_to_user_space_call(id, read_args->fd);
15591535
process_syscall_data_vecs(ctx, id, kIngress, read_args, bytes_count);
15601536
}
15611537

‎src/stirling/source_connectors/socket_tracer/openssl_trace_bpf_test.cc‎

Lines changed: 10 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -97,10 +97,10 @@ struct TraceRecords {
9797
std::vector<std::string> remote_address;
9898
};
9999

100-
template <typename TServerContainer, bool UseNestedSyscallFD>
101-
class BaseOpenSSLTraceTest : public SocketTraceBPFTestFixture</* TClientSideTracing */ false> {
100+
template <typename TServerContainer>
101+
class OpenSSLTraceTest : public SocketTraceBPFTestFixture</* TClientSideTracing */ false> {
102102
protected:
103-
BaseOpenSSLTraceTest() {
103+
OpenSSLTraceTest() {
104104
// Run the nginx HTTPS server.
105105
// The container runner will make sure it is in the ready state before unblocking.
106106
// Stirling will run after this unblocks, as part of SocketTraceBPFTest SetUp().
@@ -111,12 +111,6 @@ class BaseOpenSSLTraceTest : public SocketTraceBPFTestFixture</* TClientSideTrac
111111
sleep(1);
112112
}
113113

114-
void SetUp() override {
115-
FLAGS_access_tls_socket_fd_via_syscall = UseNestedSyscallFD;
116-
117-
SocketTraceBPFTestFixture::SetUp();
118-
}
119-
120114
// Returns the trace records of the process specified by the input pid.
121115
TraceRecords GetTraceRecords(int pid) {
122116
std::vector<TaggedRecordBatch> tablets =
@@ -178,26 +172,12 @@ http::Record GetExpectedHTTPRecord() {
178172

179173
using OpenSSLServerImplementations =
180174
Types<NginxOpenSSL_1_1_0_ContainerWrapper, NginxOpenSSL_1_1_1_ContainerWrapper,
175+
NginxOpenSSL_3_0_8_ContainerWrapper, Python310ContainerWrapper,
181176
Node12_3_1ContainerWrapper, Node14_18_1AlpineContainerWrapper>;
182-
using OpenSSLServerNestedSyscallFDImplementations =
183-
Types<Python310ContainerWrapper, NginxOpenSSL_1_1_1_ContainerWrapper,
184-
NginxOpenSSL_3_0_8_ContainerWrapper>;
185-
186-
template <typename T>
187-
using OpenSSLTraceTest = BaseOpenSSLTraceTest<T, false>;
188-
189-
template <typename T>
190-
using OpenSSLTraceNestedSyscallFD = BaseOpenSSLTraceTest<T, true>;
191-
192-
#define OPENSSL_TYPED_TEST(TestCase, CodeBlock) \
193-
TYPED_TEST(OpenSSLTraceTest, TestCase) \
194-
CodeBlock TYPED_TEST(OpenSSLTraceNestedSyscallFD, TestCase) \
195-
CodeBlock
196177

197178
TYPED_TEST_SUITE(OpenSSLTraceTest, OpenSSLServerImplementations);
198-
TYPED_TEST_SUITE(OpenSSLTraceNestedSyscallFD, OpenSSLServerNestedSyscallFDImplementations);
199179

200-
OPENSSL_TYPED_TEST(ssl_capture_curl_client, {
180+
TYPED_TEST(OpenSSLTraceTest, ssl_capture_curl_client) {
201181
this->StartTransferDataThread();
202182

203183
// Make an SSL request with curl.
@@ -218,9 +198,9 @@ OPENSSL_TYPED_TEST(ssl_capture_curl_client, {
218198

219199
EXPECT_THAT(records.http_records, UnorderedElementsAre(EqHTTPRecord(expected_record)));
220200
EXPECT_THAT(records.remote_address, UnorderedElementsAre(StrEq("127.0.0.1")));
221-
})
201+
}
222202

223-
OPENSSL_TYPED_TEST(ssl_capture_ruby_client, {
203+
TYPED_TEST(OpenSSLTraceTest, ssl_capture_ruby_client) {
224204
this->StartTransferDataThread();
225205

226206
// Make multiple requests and make sure we capture all of them.
@@ -261,9 +241,9 @@ OPENSSL_TYPED_TEST(ssl_capture_ruby_client, {
261241
EqHTTPRecord(expected_record)));
262242
EXPECT_THAT(records.remote_address,
263243
UnorderedElementsAre(StrEq("127.0.0.1"), StrEq("127.0.0.1"), StrEq("127.0.0.1")));
264-
})
244+
}
265245

266-
OPENSSL_TYPED_TEST(ssl_capture_node_client, {
246+
TYPED_TEST(OpenSSLTraceTest, ssl_capture_node_client) {
267247
this->StartTransferDataThread();
268248

269249
// Make an SSL request with the client.
@@ -280,7 +260,7 @@ OPENSSL_TYPED_TEST(ssl_capture_node_client, {
280260

281261
EXPECT_THAT(records.http_records, UnorderedElementsAre(EqHTTPRecord(expected_record)));
282262
EXPECT_THAT(records.remote_address, UnorderedElementsAre(StrEq("127.0.0.1")));
283-
})
263+
}
284264

285265
} // namespace stirling
286266
} // namespace px

‎src/stirling/source_connectors/socket_tracer/socket_trace_connector.cc‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -147,13 +147,6 @@ DEFINE_uint32(datastream_buffer_retention_size,
147147
DEFINE_uint64(max_body_bytes, gflags::Uint64FromEnv("PL_STIRLING_MAX_BODY_BYTES", 512),
148148
"The maximum number of bytes in the body of protocols like HTTP");
149149

150-
DEFINE_bool(
151-
access_tls_socket_fd_via_syscall,
152-
gflags::BoolFromEnv("PL_ACCESS_TLS_SOCKET_FD_VIA_SYSCALL", true),
153-
"If true, stirling will identify a socket's fd based on the underlying syscall (read, write, "
154-
"etc) while a user space tls function call occurs. When false, stirling attempts to access the "
155-
"socket fd by walking user space data structures which may be brittle.");
156-
157150
OBJ_STRVIEW(socket_trace_bcc_script, socket_trace);
158151

159152
namespace px {
@@ -418,8 +411,6 @@ Status SocketTraceConnector::InitBPF() {
418411
absl::StrCat("-DENABLE_NATS_TRACING=", protocol_transfer_specs_[kProtocolNATS].enabled),
419412
absl::StrCat("-DENABLE_AMQP_TRACING=", protocol_transfer_specs_[kProtocolAMQP].enabled),
420413
absl::StrCat("-DENABLE_MONGO_TRACING=", "true"),
421-
absl::StrCat("-DACCESS_TLS_SK_FD_VIA_ACTIVE_SYSCALL=",
422-
FLAGS_access_tls_socket_fd_via_syscall),
423414
};
424415
PX_RETURN_IF_ERROR(InitBPFProgram(socket_trace_bcc_script, defines));
425416

‎src/stirling/source_connectors/socket_tracer/uprobe_manager.cc‎

Lines changed: 32 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -276,6 +276,15 @@ StatusOr<std::vector<std::filesystem::path>> FindHostPathForPIDLibs(
276276
HostPathForPIDPathSearchType::kSearchTypeEndsWith);
277277
}
278278

279+
enum class SSLSocketFDAccess {
280+
// Specifies that a connection's socket fd will be identified by accessing struct members
281+
// of the SSL struct exposed by OpenSSL's API when the SSL_write/SSL_read functions are called.
282+
kUserSpaceOffsets,
283+
// Specifies that a connection's socket fd will be identified based on the underlying syscall
284+
// (read, write, etc) while a user space tls function is on the stack.
285+
kNestedSyscall,
286+
};
287+
279288
// SSLLibMatcher allows customizing the search of shared object files
280289
// that need to be traced with the SSL_write and SSL_read uprobes.
281290
// In dynamically linked cases, it's likely that there are two
@@ -285,6 +294,7 @@ struct SSLLibMatcher {
285294
std::string_view libssl;
286295
std::string_view libcrypto;
287296
HostPathForPIDPathSearchType search_type;
297+
SSLSocketFDAccess socket_fd_access;
288298
};
289299

290300
constexpr char kLibSSL_1_1[] = "libssl.so.1.1";
@@ -296,23 +306,29 @@ static constexpr const auto kLibSSLMatchers = MakeArray<SSLLibMatcher>({
296306
.libssl = kLibSSL_1_1,
297307
.libcrypto = "libcrypto.so.1.1",
298308
.search_type = HostPathForPIDPathSearchType::kSearchTypeEndsWith,
309+
.socket_fd_access = SSLSocketFDAccess::kNestedSyscall,
299310
},
300311
SSLLibMatcher{
301312
.libssl = kLibSSL_3,
302313
.libcrypto = "libcrypto.so.3",
303314
.search_type = HostPathForPIDPathSearchType::kSearchTypeEndsWith,
315+
.socket_fd_access = SSLSocketFDAccess::kNestedSyscall,
304316
},
305317
SSLLibMatcher{
306318
// This must match independent of python version and INSTSONAME suffix
307319
// (e.g. libpython3.10.so.0.1).
308320
.libssl = kLibPython,
309321
.libcrypto = kLibPython,
310322
.search_type = HostPathForPIDPathSearchType::kSearchTypeContains,
323+
.socket_fd_access = SSLSocketFDAccess::kNestedSyscall,
311324
},
325+
// non BIO native TLS applications cannot be probed by accessing the socket fd
326+
// within the underlying syscall.
312327
SSLLibMatcher{
313328
.libssl = kLibNettyTcnativePrefix,
314329
.libcrypto = kLibNettyTcnativePrefix,
315330
.search_type = HostPathForPIDPathSearchType::kSearchTypeContains,
331+
.socket_fd_access = SSLSocketFDAccess::kUserSpaceOffsets,
316332
},
317333
});
318334

@@ -332,19 +348,26 @@ ssl_source_t SSLSourceFromLib(std::string_view libssl) {
332348
return kSSLUnspecified;
333349
}
334350

351+
std::string ProbeFuncForSocketAccessMethod(std::string_view probe_fn,
352+
SSLSocketFDAccess socket_fd_access) {
353+
std::string probe_suffix = "";
354+
switch (socket_fd_access) {
355+
case SSLSocketFDAccess::kUserSpaceOffsets:
356+
break;
357+
case SSLSocketFDAccess::kNestedSyscall:
358+
probe_suffix = "_syscall_fd_access";
359+
}
360+
361+
return absl::StrCat(probe_fn, probe_suffix);
362+
}
363+
335364
// Return error if something unexpected occurs.
336365
// Return 0 if nothing unexpected, but there is nothing to deploy (e.g. no OpenSSL detected).
337366
StatusOr<int> UProbeManager::AttachOpenSSLUProbesOnDynamicLib(uint32_t pid) {
338367
for (auto ssl_library_match : kLibSSLMatchers) {
339368
const auto libssl = ssl_library_match.libssl;
340369
const auto libcrypto = ssl_library_match.libcrypto;
341370

342-
// TODO(ddelnano): The legacy tls tracing implementation does not support OpenSSL v3.
343-
// Remove this once that implementation is removed in addition to the feature toggle.
344-
if (!FLAGS_access_tls_socket_fd_via_syscall && absl::EndsWith(libssl, "so.3")) {
345-
continue;
346-
}
347-
348371
const std::vector<std::string_view> lib_names = {libssl, libcrypto};
349372
const auto search_type = ssl_library_match.search_type;
350373

@@ -373,7 +396,7 @@ StatusOr<int> UProbeManager::AttachOpenSSLUProbesOnDynamicLib(uint32_t pid) {
373396
return error::Internal("libcrypto not found [path = $0]", container_libcrypto.string());
374397
}
375398

376-
if (!FLAGS_access_tls_socket_fd_via_syscall || libssl == kLibNettyTcnativePrefix) {
399+
if (libssl == kLibNettyTcnativePrefix) {
377400
auto fptr_manager = std::make_unique<obj_tools::RawFptrManager>(container_libcrypto);
378401

379402
PX_RETURN_IF_ERROR(UpdateOpenSSLSymAddrs(fptr_manager.get(), container_libcrypto, pid));
@@ -393,12 +416,8 @@ StatusOr<int> UProbeManager::AttachOpenSSLUProbesOnDynamicLib(uint32_t pid) {
393416
openssl_source_map_->UpdateValue(pid, ssl_source);
394417
for (auto spec : kOpenSSLUProbes) {
395418
spec.binary_path = container_libssl.string();
396-
397-
// TODO(ddelnano): Remove this conditional logic once the new tls tracing
398-
// implementation is the default.
399-
if (FLAGS_access_tls_socket_fd_via_syscall && libssl != kLibNettyTcnativePrefix) {
400-
spec.probe_fn = absl::Substitute("$0_syscall_fd_access", spec.probe_fn);
401-
}
419+
spec.probe_fn =
420+
ProbeFuncForSocketAccessMethod(spec.probe_fn, ssl_library_match.socket_fd_access);
402421

403422
PX_RETURN_IF_ERROR(LogAndAttachUProbe(spec));
404423
}

‎src/stirling/source_connectors/socket_tracer/uprobe_manager.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,6 @@
4545
DECLARE_bool(stirling_rescan_for_dlopen);
4646
DECLARE_bool(stirling_enable_grpc_c_tracing);
4747
DECLARE_double(stirling_rescan_exp_backoff_factor);
48-
DECLARE_bool(access_tls_socket_fd_via_syscall);
4948

5049
namespace px {
5150
namespace stirling {

0 commit comments

Comments
 (0)