Skip to content

fix(tracers): detect TTY on /dev/pts/0 - #977

Open
ANAMASGARD wants to merge 1 commit into
kubescape:mainfrom
ANAMASGARD:fix/975-trace-exec-tty-gadget-bump
Open

ANAMASGARD wants to merge 1 commit into
kubescape:mainfrom
ANAMASGARD:fix/975-trace-exec-tty-gadget-bump

Conversation

@ANAMASGARD

@ANAMASGARD ANAMASGARD commented Sep 18, 2026

Copy link
Copy Markdown
Member

Related issues/PRs

Overview

trace_exec:v0.48.1 reports only the driver-local TTY index. This makes /dev/pts/0 indistinguishable from a process without a terminal because both report tty=0, causing event.hasTty to incorrectly evaluate to false.

This change pins only the trace_exec gadget to v0.55.0, which emits the terminal device major and minor numbers.

After this change:

  • event.hasTty is false for processes without a TTY.
  • event.hasTty is true for /dev/pts/0.
  • event.hasTty is true for nonzero PTY indices.
  • has(event.ttyMajor) is true for all emitted exec events, including no-TTY events where the major value is zero.

The strict exec schema test, component rules, acceptance assertions, and documentation were updated accordingly. No public CEL fields or Go module dependencies were changed.

Additional Information

  • advise_seccomp, trace_capabilities, trace_dns, and trace_open remain on their existing versions.
  • The schema test now includes all five fields introduced by trace_exec:v0.55.0:
    ctime, fctime, pctime, tty_major, and tty_minor.
  • Mixed-version guidance remains documented for agents bundling older gadgets.
  • go.mod, go.sum, and workflows are unchanged.

How to Test

make gadgets

GOTOOLCHAIN=auto GOMAXPROCS=2 \
go test -count=1 -p=1 ./pkg/containerwatcher/v2/tracers \
-run '^TestExecFields$'

GOTOOLCHAIN=auto GOMAXPROCS=2 \
go test -count=1 -p=1 \
./pkg/utils \
./pkg/rulemanager/cel \
./pkg/rulemanager/types/v1

GOTOOLCHAIN=auto GOMAXPROCS=2 \
go test -race -count=1 -p=1 \
./pkg/utils \
./pkg/rulemanager/cel \
./pkg/rulemanager/types/v1

GOOS=linux GOARCH=arm64 CGO_ENABLED=0 \
go build -o /tmp/node-agent-975-arm64 ./cmd/main.go

make docker-build \
  IMAGE=quay.io/kubescape/node-agent \
  TAG=test-975

Summary by CodeRabbit

  • Bug Fixes

    • Improved exec TTY detection, including /dev/pts/0 and other pseudo-terminals.
    • TTY-related alerts now correctly distinguish events with and without terminal information.
  • Enhancements

    • Updated exec monitoring to include creation-time and TTY detail fields.
    • Updated the bundled exec gadget to version 0.55.0 for improved metadata reporting.
  • Documentation

    • Updated TTY behavior and validation guidance to reflect the latest exec monitoring capabilities.

Signed-off-by: Gaurav Chaudhary <chaudharygaurav2004@gmail.com>
@coderabbitai

coderabbitai Bot commented Sep 18, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: d8b63c69-9cd2-4623-a9df-afa650758aa1

📥 Commits

Reviewing files that changed from the base of the PR and between 5acac56 and 489d5e2.

📒 Files selected for processing (6)
  • Makefile
  • docs/features/exec-tty-field.md
  • pkg/containerwatcher/v2/tracers/exec.go
  • pkg/containerwatcher/v2/tracers/exec_test.go
  • tests/component_test.go
  • tests/resources/exec-tty-rules.yaml

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The change upgrades trace_exec to v0.55.0, adds new exec metadata fields, and updates TTY documentation, rules, and component tests for consistent tty_major behavior.

Changes

Exec TTY metadata

Layer / File(s) Summary
Pin trace_exec and tracer image
Makefile, pkg/containerwatcher/v2/tracers/exec.go
The Makefile pulls and exports trace_exec:v0.55.0 separately. The exec tracer loads the same image version.
Update exec metadata fields
pkg/containerwatcher/v2/tracers/exec_test.go
The expected exec fields now include ctime, fctime, pctime, tty_major, and tty_minor.
Revise TTY rules and documentation
docs/features/exec-tty-field.md, tests/resources/exec-tty-rules.yaml
The documentation and rule descriptions define v0.55.0 field presence and require a minimum node-agent version for negative TTY predicates.
Validate TTY behavior
tests/component_test.go
The component test waits for positive rule results and checks /dev/pts/0, nonzero PTY indices, R9903 for all exec events, and no R9904.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Suggested reviewers: matthyx, slashben

Merge Risk: ⚪ Minimal · up to 489d5

The TTY metadata update has no remaining actionable risk identified in the changed behavior.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files. (3 skipped: 3… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: fixing TTY detection for processes using /dev/pts/0.
Full details: Docstring Coverage

Explanation

Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files. (3 skipped: 3 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@matthyx matthyx left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review: request changes

Reviewed head 489d5e26a5fa81826b3f58afa2c8b3fd678726a5 against main (5acac56a).

The TTY fix itself worksTest_35_ExecTTYFieldTest passes on CI with the new gadget, and I confirmed locally that ig 0.48.1 can pull/export trace_exec:v0.55.0 and that TestExecFields passes against the pinned IG runtime (kubescape/inspektor-gadget@06b0d12, based on upstream ~v0.48.0). Bumping the gadget is the right way to close #975.

Blocker: the bump silently changes args semantics and breaks R0040 / existing profiles.

Between v0.48.1 and v0.55.0, trace_exec also picked up inspektor-gadget commits 06323f6d ("read args from the new process address space"), c759b4e1 ("report argv[0] instead of the exec path for failed execs") and 03c0f590 (lazy arg parsing). Net effect: args[0] used to be the pathname passed to execve (e.g. /bin/sh); it is now the committed argv[0] (e.g. sh). For anything launched through a shell or via PATH lookup those differ.

node-agent stores the full args vector (including args[0]) in container profiles and compares it verbatim in cp.was_executed_with_args (rule R0040), and utils.GetExecPathFromEvent returns args[0]. Consequences:

  • CI already shows it: component-tests (Test_32_UnexpectedProcessArguments) fails on this PR (run 35366394715, job 105671995732), while it passes on other recent PRs (e.g. runs 35364356926, 34937021951). Example: sh -c "echo hi" → runtime argv [sh, -c, echo hi] no longer matches profile [/bin/sh, -c, ⋯⋯], so R0040 fires ("must stay silent", expected 2 got 3). The workflow is green only because component-tests has continue-on-error: true.
  • Fleet impact: every already-learned/authored profile encodes the old args[0] form, so after upgrade R0040 would alert on shell-launched execs across the board, and freshly learned profiles would be in a different format than existing ones (mixed-version fleets get both).
  • pkg/malwaremanager/v1 builds /proc/<pid>/root/<args[0]> via GetHostFilePathFromEvent; with a relative argv[0] that path no longer resolves. Alert CommandLine/exec fields in ruleadapters/adapters/exec.go change shape too.

Suggested remedies (either is fine; the first is the smaller blast radius):

  1. Keep the gadget at v0.48.1 and vendor trace_exec under pkg/ebpf/gadgets/ with only upstream c6fc831a (the tty_major/tty_minor change) applied, built via the existing BUILT_GADGETS mechanism exactly like trace_open already is.
  2. Stay on v0.55.0 but make the args change explicit: restore path-style args[0] (e.g. from exepath) in execoperator.go or adjust profile recording/matching to be robust to both forms, update Test_32/fixtures, and document the profile-compatibility implication. This needs a real migration story for existing profiles, so it's more than a follow-up.

Non-blocking:

  • docs/features/exec-tty-field.md: "the regression fixed by #975" — #975 is the issue; this PR is the fix.
  • Makefile: the trailing sudo ig image pull for trace_exec is fine, but consider folding it into the existing $(foreach …) with a per-gadget version map if more gadgets end up pinned separately.

Validation performed: go build ./... OK; go test ./pkg/utils ./pkg/rulemanager/cel/... ./pkg/rulemanager/types/v1 ./pkg/containerprofilemanager/... OK; TestExecFields OK against a locally exported trace_exec:v0.55.0; CI Test_35 pass / Test_32 fail as above. I did not run the component suite locally (needs kind).

Search notes: related history is #868 (introduced the two-phase TTY fields and Test_35), #695 (IG v0.48.1 upgrade), #800 (exec path recording symmetry — relevant to the args[0] convention). No prior PR attempts a trace_exec bump; nothing superseding this one.


const (
execImageName = "ghcr.io/inspektor-gadget/gadget/trace_exec:v0.48.1"
execImageName = "ghcr.io/inspektor-gadget/gadget/trace_exec:v0.55.0"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Blocker. trace_exec v0.55.0 also includes upstream 06323f6d/c759b4e1: args[0] changes from the execve pathname (/bin/sh) to the committed argv[0] (sh). node-agent records the full args vector in profiles and R0040 (cp.was_executed_with_args) compares it verbatim, so existing profiles stop matching — Test_32_UnexpectedProcessArguments already fails on this PR (job 105671995732) while passing on other current PRs. GetExecPathFromEvent/malware-manager /proc/<pid>/root/<args[0]> are affected too. Suggest vendoring trace_exec at v0.48.1 + only the tty commit (c6fc831a) via BUILT_GADGETS like trace_open, or explicitly restoring/handling the args[0] convention and its profile migration before bumping.

@matthyx matthyx moved this to Waiting on Author in KS PRs tracking Sep 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Waiting on Author

Development

Successfully merging this pull request may close these issues.

hasTty is false on /dev/pts/0: bump trace_exec gadget to v0.55.0+

2 participants