From e9cee3d3b15dc05282449539fcc8e9c91c4e04e1 Mon Sep 17 00:00:00 2001 From: Guy Arbitman Date: Mon, 9 Sep 2024 15:21:09 +0300 Subject: [PATCH] Revert "[USMON-417] gotls: Restore support on fedora (#27437)" (#29128) --- .../ebpf/c/protocols/tls/go-tls-maps.h | 4 - pkg/network/ebpf/c/protocols/tls/https.h | 46 +++++++++--- .../protocols/tls/gotls/testutil/helpers.go | 27 ++++++- pkg/network/usm/ebpf_gotls.go | 73 +++++-------------- pkg/network/usm/kafka_monitor_test.go | 6 +- pkg/network/usm/monitor_tls_test.go | 12 +-- pkg/network/usm/postgres_monitor_test.go | 2 +- .../usm/tests/tracer_usm_linux_test.go | 4 +- pkg/network/usm/usm_grpc_monitor_test.go | 2 +- pkg/network/usm/usm_http2_monitor_test.go | 2 +- 10 files changed, 95 insertions(+), 83 deletions(-) diff --git a/pkg/network/ebpf/c/protocols/tls/go-tls-maps.h b/pkg/network/ebpf/c/protocols/tls/go-tls-maps.h index 58f3ce3ac771f..1c66dee1d0abc 100644 --- a/pkg/network/ebpf/c/protocols/tls/go-tls-maps.h +++ b/pkg/network/ebpf/c/protocols/tls/go-tls-maps.h @@ -9,10 +9,6 @@ // offsets_data map contains the information about the locations of structs in the inspected binary, mapped by the binary's inode number. BPF_HASH_MAP(offsets_data, go_tls_offsets_data_key_t, tls_offsets_data_t, 1024) -// Maps PID to the - tuple, that is used to find the offsets_data map for the binary. -// Size is a 10 times the size of the offsets_data map, to have enough space for all the binaries. -BPF_HASH_MAP(pid_to_device_inode, u32, go_tls_offsets_data_key_t, 10240) - /* go_tls_read_args is used to get the read function info when running in the read-return uprobe. The key contains the go routine id and the pid. */ BPF_LRU_MAP(go_tls_read_args, go_tls_function_args_key_t, go_tls_read_args_data_t, 2048) diff --git a/pkg/network/ebpf/c/protocols/tls/https.h b/pkg/network/ebpf/c/protocols/tls/https.h index ccb9550f3602c..db485ab14403c 100644 --- a/pkg/network/ebpf/c/protocols/tls/https.h +++ b/pkg/network/ebpf/c/protocols/tls/https.h @@ -3,6 +3,10 @@ #ifdef COMPILE_CORE #include "ktypes.h" +#define MINORBITS 20 +#define MINORMASK ((1U << MINORBITS) - 1) +#define MAJOR(dev) ((unsigned int) ((dev) >> MINORBITS)) +#define MINOR(dev) ((unsigned int) ((dev) & MINORMASK)) #else #include #include @@ -283,19 +287,41 @@ static __always_inline void map_ssl_ctx_to_sock(struct sock *skp) { bpf_map_update_with_telemetry(ssl_sock_by_ctx, &ssl_ctx, &ssl_sock, BPF_ANY); } - -// Retrieves the result of binary analysis for the current task binary's inode number. -// For the current PID, we retrieve the inode number of the binary and then we look up the binary's analysis result. +/** + * get_offsets_data retrieves the result of binary analysis for the + * current task binary's inode number. + */ static __always_inline tls_offsets_data_t* get_offsets_data() { - u64 pid_tgid = bpf_get_current_pid_tgid(); - u32 pid = pid_tgid >> 32; - go_tls_offsets_data_key_t *key = bpf_map_lookup_elem(&pid_to_device_inode, &pid); - if (key == NULL) { - log_debug("get_offsets_data: could not find key for pid %u", pid); + struct task_struct *t = (struct task_struct *) bpf_get_current_task(); + struct inode *inode; + go_tls_offsets_data_key_t key; + dev_t dev_id; + + inode = BPF_CORE_READ(t, mm, exe_file, f_inode); + if (!inode) { + log_debug("get_offsets_data: could not read f_inode field"); + return NULL; + } + + int err; + err = BPF_CORE_READ_INTO(&key.ino, inode, i_ino); + if (err) { + log_debug("get_offsets_data: could not read i_ino field"); return NULL; } - go_tls_offsets_data_key_t key_copy = *key; - return bpf_map_lookup_elem(&offsets_data, &key_copy); + + err = BPF_CORE_READ_INTO(&dev_id, inode, i_sb, s_dev); + if (err) { + log_debug("get_offsets_data: could not read s_dev field"); + return NULL; + } + + key.device_id_major = MAJOR(dev_id); + key.device_id_minor = MINOR(dev_id); + + log_debug("get_offsets_data: task binary inode number: %llu; device ID %x:%x", key.ino, key.device_id_major, key.device_id_minor); + + return bpf_map_lookup_elem(&offsets_data, &key); } #endif diff --git a/pkg/network/protocols/tls/gotls/testutil/helpers.go b/pkg/network/protocols/tls/gotls/testutil/helpers.go index fcf418ae505a9..2315779aa184a 100644 --- a/pkg/network/protocols/tls/gotls/testutil/helpers.go +++ b/pkg/network/protocols/tls/gotls/testutil/helpers.go @@ -8,11 +8,34 @@ package testutil import ( + "slices" + "testing" + + "github.com/stretchr/testify/require" + "github.com/DataDog/datadog-agent/pkg/network/config" usmconfig "github.com/DataDog/datadog-agent/pkg/network/usm/config" + "github.com/DataDog/datadog-agent/pkg/util/kernel" +) + +const ( + fedoraPlatform = "fedora" ) +var fedoraUnsupportedVersions = []string{"35", "36", "37", "38"} + +// isFedora returns true if the current OS is Fedora. +// go-tls does not work correctly on Fedora 35, 36, 37 and 38. +func isFedora(t *testing.T) bool { + platform, err := kernel.Platform() + require.NoError(t, err) + platformVersion, err := kernel.PlatformVersion() + require.NoError(t, err) + + return platform == fedoraPlatform && slices.Contains(fedoraUnsupportedVersions, platformVersion) +} + // GoTLSSupported returns true if GO-TLS monitoring is supported on the current OS. -func GoTLSSupported(cfg *config.Config) bool { - return usmconfig.TLSSupported(cfg) && (cfg.EnableRuntimeCompiler || cfg.EnableCORE) +func GoTLSSupported(t *testing.T, cfg *config.Config) bool { + return usmconfig.TLSSupported(cfg) && (cfg.EnableRuntimeCompiler || cfg.EnableCORE) && !isFedora(t) } diff --git a/pkg/network/usm/ebpf_gotls.go b/pkg/network/usm/ebpf_gotls.go index b7e75f7c3c964..e0090e3ba889c 100644 --- a/pkg/network/usm/ebpf_gotls.go +++ b/pkg/network/usm/ebpf_gotls.go @@ -42,7 +42,6 @@ import ( const ( offsetsDataMap = "offsets_data" - pidToDeviceInodeMap = "pid_to_device_inode" goTLSReadArgsMap = "go_tls_read_args" goTLSWriteArgsMap = "go_tls_write_args" connectionTupleByGoTLSMap = "conn_tup_by_go_tls_conn" @@ -116,11 +115,6 @@ type goTLSProgram struct { // inodes. offsetsDataMap *ebpf.Map - // eBPF map holding the mapping of PIDs to device/inode numbers. - // On some filesystems (like btrfs), the device-id in the task-struct can be different from the device-id extracted - // in the user-mode. This map is used to ensure the eBPF probes are getting the correct device/inode numbers. - pidToDeviceInodeMap *ebpf.Map - // binAnalysisMetric handles telemetry on the time spent doing binary // analysis binAnalysisMetric *libtelemetry.Counter @@ -137,7 +131,6 @@ var _ utils.Attacher = &goTLSProgram{} var goTLSSpec = &protocols.ProtocolSpec{ Maps: []*manager.Map{ {Name: offsetsDataMap}, - {Name: pidToDeviceInodeMap}, {Name: goTLSReadArgsMap}, {Name: goTLSWriteArgsMap}, {Name: connectionTupleByGoTLSMap}, @@ -223,10 +216,6 @@ func (p *goTLSProgram) PreStart(m *manager.Manager) error { if err != nil { return fmt.Errorf("could not get offsets_data map: %s", err) } - p.pidToDeviceInodeMap, _, err = m.GetMap(pidToDeviceInodeMap) - if err != nil { - return fmt.Errorf("could not get %s map: %s", pidToDeviceInodeMap, err) - } procMonitor := monitor.GetProcessMonitor() cleanupExec := procMonitor.SubscribeExec(p.handleProcessStart) @@ -253,7 +242,7 @@ func (p *goTLSProgram) PreStart(m *manager.Manager) error { processSet := p.registry.GetRegisteredProcesses() deletedPids := monitor.FindDeletedProcesses(processSet) for deletedPid := range deletedPids { - _ = p.DetachPID(deletedPid) + _ = p.registry.Unregister(deletedPid) } } } @@ -289,7 +278,6 @@ var ( // DetachPID detaches the provided PID from the eBPF program. func (p *goTLSProgram) DetachPID(pid uint32) error { - _ = p.pidToDeviceInodeMap.Delete(unsafe.Pointer(&pid)) return p.registry.Unregister(pid) } @@ -350,13 +338,12 @@ func (p *goTLSProgram) AttachPID(pid uint32) error { // Check go process probeList := make([]manager.ProbeIdentificationPair, 0) - return p.registry.Register(binPath, pid, - registerCBCreator(p.manager, p.offsetsDataMap, p.pidToDeviceInodeMap, &probeList, p.binAnalysisMetric, p.binNoSymbolsMetric), - unregisterCBCreator(p.manager, &probeList, p.offsetsDataMap, p.pidToDeviceInodeMap), - alreadyCBCreator(p.pidToDeviceInodeMap)) + return p.registry.Register(binPath, pid, registerCBCreator(p.manager, p.offsetsDataMap, &probeList, p.binAnalysisMetric, p.binNoSymbolsMetric), + unregisterCBCreator(p.manager, &probeList, p.offsetsDataMap), + utils.IgnoreCB) } -func registerCBCreator(mgr *manager.Manager, offsetsDataMap, pidToDeviceInodeMap *ebpf.Map, probeIDs *[]manager.ProbeIdentificationPair, binAnalysisMetric, binNoSymbolsMetric *libtelemetry.Counter) func(path utils.FilePath) error { +func registerCBCreator(mgr *manager.Manager, offsetsDataMap *ebpf.Map, probeIDs *[]manager.ProbeIdentificationPair, binAnalysisMetric, binNoSymbolsMetric *libtelemetry.Counter) func(path utils.FilePath) error { return func(filePath utils.FilePath) error { start := time.Now() @@ -379,13 +366,13 @@ func registerCBCreator(mgr *manager.Manager, offsetsDataMap, pidToDeviceInodeMap return fmt.Errorf("error extracting inspectoin data from %s: %w", filePath.HostPath, err) } - if err := addInspectionResultToMap(offsetsDataMap, pidToDeviceInodeMap, filePath, inspectionResult); err != nil { + if err := addInspectionResultToMap(offsetsDataMap, filePath.ID, inspectionResult); err != nil { return fmt.Errorf("failed adding inspection rules: %w", err) } pIDs, err := attachHooks(mgr, inspectionResult, filePath.HostPath, filePath.ID) if err != nil { - removeInspectionResultFromMap(offsetsDataMap, pidToDeviceInodeMap, filePath) + removeInspectionResultFromMap(offsetsDataMap, filePath.ID) return fmt.Errorf("error while attaching hooks to %s: %w", filePath.HostPath, err) } *probeIDs = pIDs @@ -398,21 +385,6 @@ func registerCBCreator(mgr *manager.Manager, offsetsDataMap, pidToDeviceInodeMap } } -// alreadyCBCreator handles the case where a binary is already registered. In such a case the registry callback won't -// be called, so we need to add a mapping from the PID to the device/inode of the binary. -func alreadyCBCreator(pidToDeviceInodeMap *ebpf.Map) func(utils.FilePath) error { - return func(filePath utils.FilePath) error { - if filePath.PID == 0 { - return nil - } - return pidToDeviceInodeMap.Put(unsafe.Pointer(&filePath.PID), unsafe.Pointer(&gotls.TlsBinaryId{ - Id_major: unix.Major(filePath.ID.Dev), - Id_minor: unix.Minor(filePath.ID.Dev), - Ino: filePath.ID.Inode, - })) - } -} - func (p *goTLSProgram) handleProcessExit(pid pid) { _ = p.DetachPID(pid) } @@ -423,39 +395,32 @@ func (p *goTLSProgram) handleProcessStart(pid pid) { // addInspectionResultToMap runs a binary inspection and adds the result to the // map that's being read by the probes, indexed by the binary's inode number `ino`. -func addInspectionResultToMap(offsetsDataMap, pidToDeviceInodeMap *ebpf.Map, filePath utils.FilePath, result *bininspect.Result) error { +func addInspectionResultToMap(offsetsDataMap *ebpf.Map, binID utils.PathIdentifier, result *bininspect.Result) error { offsetsData, err := inspectionResultToProbeData(result) if err != nil { return fmt.Errorf("error while parsing inspection result: %w", err) } key := &gotls.TlsBinaryId{ - Id_major: unix.Major(filePath.ID.Dev), - Id_minor: unix.Minor(filePath.ID.Dev), - Ino: filePath.ID.Inode, + Id_major: unix.Major(binID.Dev), + Id_minor: unix.Minor(binID.Dev), + Ino: binID.Inode, } if err := offsetsDataMap.Put(unsafe.Pointer(key), unsafe.Pointer(&offsetsData)); err != nil { - return fmt.Errorf("could not write binary inspection result to map for binID %v (pid %v): %w", filePath.ID, filePath.PID, err) + return fmt.Errorf("could not write binary inspection result to map for binID %v: %w", binID, err) } - if err := pidToDeviceInodeMap.Put(unsafe.Pointer(&filePath.PID), unsafe.Pointer(key)); err != nil { - return fmt.Errorf("could not write pid to device/inode (%s) map for pid %v: %w", filePath.ID.String(), filePath.PID, err) - } return nil } -func removeInspectionResultFromMap(offsetsDataMap, pidToDeviceInodeMap *ebpf.Map, filePath utils.FilePath) { +func removeInspectionResultFromMap(offsetsDataMap *ebpf.Map, binID utils.PathIdentifier) { key := &gotls.TlsBinaryId{ - Id_major: unix.Major(filePath.ID.Dev), - Id_minor: unix.Minor(filePath.ID.Dev), - Ino: filePath.ID.Inode, - } - if filePath.PID != 0 { - _ = pidToDeviceInodeMap.Delete(unsafe.Pointer(&filePath.PID)) + Id_major: unix.Major(binID.Dev), + Id_minor: unix.Minor(binID.Dev), + Ino: binID.Inode, } if err := offsetsDataMap.Delete(unsafe.Pointer(key)); err != nil { - log.Errorf("could not remove inspection result from map for ino %v: %s", filePath.ID, err) - return + log.Errorf("could not remove inspection result from map for ino %v: %s", binID, err) } } @@ -510,12 +475,12 @@ func attachHooks(mgr *manager.Manager, result *bininspect.Result, binPath string return probeIDs, nil } -func unregisterCBCreator(mgr *manager.Manager, probeIDs *[]manager.ProbeIdentificationPair, offsetsDataMap, pidToDeviceInodeMap *ebpf.Map) func(path utils.FilePath) error { +func unregisterCBCreator(mgr *manager.Manager, probeIDs *[]manager.ProbeIdentificationPair, offsetsDataMap *ebpf.Map) func(path utils.FilePath) error { return func(path utils.FilePath) error { if len(*probeIDs) == 0 { return nil } - removeInspectionResultFromMap(offsetsDataMap, pidToDeviceInodeMap, path) + removeInspectionResultFromMap(offsetsDataMap, path.ID) for _, probeID := range *probeIDs { err := mgr.DetachHook(probeID) if err != nil { diff --git a/pkg/network/usm/kafka_monitor_test.go b/pkg/network/usm/kafka_monitor_test.go index c5ac048be3a80..bbcedaecb2be6 100644 --- a/pkg/network/usm/kafka_monitor_test.go +++ b/pkg/network/usm/kafka_monitor_test.go @@ -148,7 +148,7 @@ func (s *KafkaProtocolParsingSuite) TestKafkaProtocolParsing() { for mode, name := range map[bool]string{false: "without TLS", true: "with TLS"} { t.Run(name, func(t *testing.T) { - if mode && !gotlsutils.GoTLSSupported(config.New()) { + if mode && !gotlsutils.GoTLSSupported(t, config.New()) { t.Skip("GoTLS not supported for this setup") } for _, version := range versions { @@ -1244,7 +1244,7 @@ func (s *KafkaProtocolParsingSuite) TestKafkaFetchRaw() { }) t.Run("with TLS", func(t *testing.T) { - if !gotlsutils.GoTLSSupported(config.New()) { + if !gotlsutils.GoTLSSupported(t, config.New()) { t.Skip("GoTLS not supported for this setup") } @@ -1470,7 +1470,7 @@ func (s *KafkaProtocolParsingSuite) TestKafkaProduceRaw() { }) t.Run("with TLS", func(t *testing.T) { - if !gotlsutils.GoTLSSupported(config.New()) { + if !gotlsutils.GoTLSSupported(t, config.New()) { t.Skip("GoTLS not supported for this setup") } diff --git a/pkg/network/usm/monitor_tls_test.go b/pkg/network/usm/monitor_tls_test.go index b9df53c92dd16..c706b5fe874d4 100644 --- a/pkg/network/usm/monitor_tls_test.go +++ b/pkg/network/usm/monitor_tls_test.go @@ -548,9 +548,11 @@ func (s *tlsSuite) TestJavaInjection() { } func TestHTTPGoTLSAttachProbes(t *testing.T) { + t.Skip("skipping GoTLS tests while we investigate their flakiness") + modes := []ebpftest.BuildMode{ebpftest.RuntimeCompiled, ebpftest.CORE} ebpftest.TestBuildModes(t, modes, "", func(t *testing.T) { - if !gotlstestutil.GoTLSSupported(config.New()) { + if !gotlstestutil.GoTLSSupported(t, config.New()) { t.Skip("GoTLS not supported for this setup") } @@ -569,7 +571,7 @@ func testHTTP2GoTLSAttachProbes(t *testing.T, cfg *config.Config) { if !http2.Supported() { t.Skip("HTTP2 not supported for this setup") } - if !gotlstestutil.GoTLSSupported(cfg) { + if !gotlstestutil.GoTLSSupported(t, cfg) { t.Skip("GoTLS not supported for this setup") } @@ -601,7 +603,7 @@ func TestHTTPSGoTLSAttachProbesOnContainer(t *testing.T) { t.Skip("Skipping a flaky test") modes := []ebpftest.BuildMode{ebpftest.RuntimeCompiled, ebpftest.CORE} ebpftest.TestBuildModes(t, modes, "", func(t *testing.T) { - if !gotlstestutil.GoTLSSupported(config.New()) { + if !gotlstestutil.GoTLSSupported(t, config.New()) { t.Skip("GoTLS not supported for this setup") } @@ -619,7 +621,7 @@ func TestOldConnectionRegression(t *testing.T) { modes := []ebpftest.BuildMode{ebpftest.RuntimeCompiled, ebpftest.CORE} ebpftest.TestBuildModes(t, modes, "", func(t *testing.T) { - if !gotlstestutil.GoTLSSupported(config.New()) { + if !gotlstestutil.GoTLSSupported(t, config.New()) { t.Skip("GoTLS not supported for this setup") } @@ -694,7 +696,7 @@ func TestOldConnectionRegression(t *testing.T) { func TestLimitListenerRegression(t *testing.T) { modes := []ebpftest.BuildMode{ebpftest.RuntimeCompiled, ebpftest.CORE} ebpftest.TestBuildModes(t, modes, "", func(t *testing.T) { - if !gotlstestutil.GoTLSSupported(config.New()) { + if !gotlstestutil.GoTLSSupported(t, config.New()) { t.Skip("GoTLS not supported for this setup") } diff --git a/pkg/network/usm/postgres_monitor_test.go b/pkg/network/usm/postgres_monitor_test.go index 47a891cd4040b..a74ef203f3721 100644 --- a/pkg/network/usm/postgres_monitor_test.go +++ b/pkg/network/usm/postgres_monitor_test.go @@ -137,7 +137,7 @@ func (s *postgresProtocolParsingSuite) TestDecoding() { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if tt.isTLS && !gotlstestutil.GoTLSSupported(config.New()) { + if tt.isTLS && !gotlstestutil.GoTLSSupported(t, config.New()) { t.Skip("GoTLS not supported for this setup") } testDecoding(t, tt.isTLS) diff --git a/pkg/network/usm/tests/tracer_usm_linux_test.go b/pkg/network/usm/tests/tracer_usm_linux_test.go index 9399e6b1cb210..2da79eadbf80a 100644 --- a/pkg/network/usm/tests/tracer_usm_linux_test.go +++ b/pkg/network/usm/tests/tracer_usm_linux_test.go @@ -118,7 +118,7 @@ func skipIfUsingNAT(t *testing.T, ctx testContext) { // skipIfGoTLSNotSupported skips the test if GoTLS is not supported. func skipIfGoTLSNotSupported(t *testing.T, _ testContext) { - if !gotlstestutil.GoTLSSupported(config.New()) { + if !gotlstestutil.GoTLSSupported(t, config.New()) { t.Skip("GoTLS is not supported") } } @@ -183,7 +183,7 @@ func (s *USMSuite) TestProtocolClassification() { cfg.EnableNativeTLSMonitoring = true cfg.EnableHTTPMonitoring = true cfg.EnablePostgresMonitoring = true - cfg.EnableGoTLSSupport = gotlstestutil.GoTLSSupported(cfg) + cfg.EnableGoTLSSupport = gotlstestutil.GoTLSSupported(t, cfg) cfg.BypassEnabled = true tr, err := tracer.NewTracer(cfg, nil) require.NoError(t, err) diff --git a/pkg/network/usm/usm_grpc_monitor_test.go b/pkg/network/usm/usm_grpc_monitor_test.go index 73e3a5de28f2e..35ae34a9d46d4 100644 --- a/pkg/network/usm/usm_grpc_monitor_test.go +++ b/pkg/network/usm/usm_grpc_monitor_test.go @@ -71,7 +71,7 @@ func TestGRPCScenarios(t *testing.T) { }, } { t.Run(tc.name, func(t *testing.T) { - if tc.isTLS && !gotlsutils.GoTLSSupported(config.New()) { + if tc.isTLS && !gotlsutils.GoTLSSupported(t, config.New()) { t.Skip("GoTLS not supported for this setup") } suite.Run(t, &usmGRPCSuite{isTLS: tc.isTLS}) diff --git a/pkg/network/usm/usm_http2_monitor_test.go b/pkg/network/usm/usm_http2_monitor_test.go index 825c842fe68cd..46c3188b09833 100644 --- a/pkg/network/usm/usm_http2_monitor_test.go +++ b/pkg/network/usm/usm_http2_monitor_test.go @@ -108,7 +108,7 @@ func TestHTTP2Scenarios(t *testing.T) { }, } { t.Run(tc.name, func(t *testing.T) { - if tc.isTLS && !gotlsutils.GoTLSSupported(config.New()) { + if tc.isTLS && !gotlsutils.GoTLSSupported(t, config.New()) { t.Skip("GoTLS not supported for this setup") } suite.Run(t, &usmHTTP2Suite{isTLS: tc.isTLS})