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

tetragon: extract struct dentry member using CO:RE #2574

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

dwindsor
Copy link
Collaborator

@dwindsor dwindsor commented Jun 17, 2024

Thanks for contributing! Please ensure your pull request adheres to the following guidelines:

  • All commits contain a well written commit message and are signed-off (see Submitting a pull request).
  • All code is covered by unit and/or end-to-end tests where feasible.
  • All generated files are updated if needed (see Making changes).
  • Provide a title or release-note blurb suitable for the release notes (see guidelines).
  • Update documentation and write an upgrade note if needed (see guidelines).
  • Are you a user of Tetragon? Please add yourself to the Users doc in the Cilium repository.

Fixes: #2573

<!-- Enter the release note text here if needed or remove this section! -->

@dwindsor dwindsor requested a review from a team as a code owner June 17, 2024 16:13
@dwindsor dwindsor requested a review from kkourt June 17, 2024 16:13
Copy link

netlify bot commented Jun 17, 2024

Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit 6e29ad2
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/66992639e725e50008a84182
😎 Deploy Preview https://deploy-preview-2574--tetragon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@dwindsor
Copy link
Collaborator Author

Hi! I'm encountering the following error when attempting to build these changes, and am not quite sure why... 🤔

Following the same procedures as 9889f27, I'm getting this error:

shell time="2024-06-17T12:16:50-04:00" level=fatal msg="Failed to start tetragon" error="failed loading tracing policy file \"/home/dave/src/github.com/dwindsor/tetragon/examples/tracingpolicy/security_inode_follow_link.yaml\": validation failed: \"follow-symlink\": validation failure list:\nspec.kprobes[0].args[0].type in body should be one of [auto int int8 uint8 int16 uint16 uint32 int32 uint64 int64 char_buf char_iovec size_t skb sock string fd file filename path nop bpf_attr perf_event bpf_map user_namespace capability kiocb iov_iter cred load_info module syscall64 kernel_cap_t cap_inheritable cap_permitted cap_effective linux_binprm data_loc net_device]"

Using the following Tracing Policy:

apiVersion: cilium.io/v1alpha1
kind: TracingPolicy
metadata:
  name: "follow-symlink"
spec:
  kprobes:
  - call: "security_inode_follow_link"
    syscall: false
    args:
    - index: 0
      type: "dentry"
    returnArg:
      index: 0
      type: "int"
    selectors:
    - matchArgs:
      - index: 0
        operator: "Equal"
        values:
        - "/tmp/softlink"

It appears the procedure has changed at least slightly since 9889f27, as 577a2ac introduced some changes.

i've tried following the same procedure as 9268fd0, but haven't had luck so far. I'll keep trying but wanted to see if anyone had any pointers?

cc: @kevsecurity @mtardy @jrfastab

@dwindsor
Copy link
Collaborator Author

I forgot to run make crds; not sure if that target was around last time?

Either way, above errors are fixed =)

@dwindsor dwindsor added the area/bpf This is related to BPF code label Jun 17, 2024
@kkourt kkourt added the release-note/minor This PR introduces a minor user-visible change label Jun 20, 2024
@kkourt
Copy link
Contributor

kkourt commented Jun 20, 2024

I forgot to run make crds; not sure if that target was around last time?

It was renamed from make generate to make crds recently: c26e483

Either way, above errors are fixed =)

Thanks! I plan to review this tomorrow.

Could you also fix the static check failures? Running make check locally should perform the same tests. This is not required for the review, but it's something that needs to be fixed before merging.

Copy link
Contributor

@kkourt kkourt left a comment

Choose a reason for hiding this comment

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

Thanks, from a quick look this LGTM overall.

Can you please squash the fixes into the relevant original commits? (git rebase --interactive using the squash and fixup actions should help).

It would also be great if a test was added for this newly-introduced functionality. Happy to provide some pointers and help out in doing so.

@@ -7,4 +7,4 @@ package v1alpha1
// Used to determine if CRD needs to be updated in cluster
//
// Developers: Bump patch for each change in the CRD schema.
const CustomResourceDefinitionSchemaVersion = "1.2.0"
const CustomResourceDefinitionSchemaVersion = "1.3.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please keep the same minor number and just change the patch version since this follows the Tetragon version. So in this case, this should be 1.2.1.

@dwindsor dwindsor force-pushed the pr/dwindsor/add-struct-dentry branch 5 times, most recently from 56d6858 to 2b47d1c Compare June 27, 2024 21:13
@dwindsor
Copy link
Collaborator Author

dwindsor commented Jun 27, 2024

Thanks for the review @kkourt! I've responded to your comments and fixed the formatting issues, but I'm having trouble getting the test to work.

Running the test program on my system, I see the relevant event being produced in tetra:

{
  "process_kprobe": {
    "process": {
      "exec_id": "bGF0ZXJhbHVzOjI0ODQ0MzUwNjU1MzM4OjMyNjQ2MQ==",
      "pid": 326461,
      "uid": 1000,
      "cwd": "/home/dave/src/github.com/dwindsor/tetragon/contrib/tester-progs",
      "binary": "/home/dave/src/github.com/dwindsor/tetragon/contrib/tester-progs/symlink-tester",
      "flags": "execve clone",
      "start_time": "2024-06-27T21:39:16.896790982Z",
      "auid": 1000,
      "parent_exec_id": "bGF0ZXJhbHVzOjU2NTkyMTAwMDAwMDA6ODUyNjE=",
      "refcnt": 1,
      "tid": 326461
    },
    "parent": {
      "exec_id": "bGF0ZXJhbHVzOjU2NTkyMTAwMDAwMDA6ODUyNjE=",
      "pid": 85261,
      "uid": 1000,
      "cwd": "/tmp",
      "binary": "/usr/bin/bash",
      "flags": "procFS auid",
      "start_time": "2024-06-27T16:19:31.756135483Z",
      "auid": 1000,
      "parent_exec_id": "bGF0ZXJhbHVzOjU2NTkyMDAwMDAwMDA6ODUyNjA=",
      "tid": 85261
    },
    "function_name": "security_inode_follow_link",
    "args": [
      {
        "dentry_arg": {
          "name": "/tmp/id"
        }
      }
    ],
    "action": "KPROBE_ACTION_POST",
    "policy_name": "path-traversal-block",
    "return_action": "KPROBE_ACTION_POST"
  },
  "node_name": "lateralus",
  "time": "2024-06-27T21:39:16.897100809Z"
}

@dwindsor dwindsor requested a review from kkourt June 27, 2024 21:52
@dwindsor dwindsor changed the title DRAFT: tetragon: extract struct dentry member using CO:RE tetragon: extract struct dentry member using CO:RE Jun 28, 2024
@kkourt
Copy link
Contributor

kkourt commented Jul 3, 2024

Hi,

Thanks for the review @kkourt! I've responded to your comments and fixed the formatting issues, but I'm having trouble getting the test to work.

Thanks.

I think the issue might be that you are killing the test program before it has a chance to do the operation. I've tried the test locally, and this seems to work for me:

diff --git a/pkg/sensors/tracing/kprobe_test.go b/pkg/sensors/tracing/kprobe_test.go                                                                                                                                                                                                                                           
index db8a53f0b..55fcbbaa6 100644                                                                                                                                                                                                                                                                                              
--- a/pkg/sensors/tracing/kprobe_test.go                                                                                                                                                                                                                                                                                       
+++ b/pkg/sensors/tracing/kprobe_test.go                                                                                                                                                                                                                                                                                       
@@ -6049,7 +6049,6 @@ func TestDentryExtractPath(t *testing.T) {                                                                                                                                                                                                                                                               
        ops := func() {                                                                                                                                                                                                                                                                                                        
                err = command.Start()                                                                                                                                                                                                                                                                                          
                assert.NoError(t, err)                                                                                                                                                                                                                                                                                         
-               defer command.Process.Kill()                                                                                                                                                                                                                                                                                   
        }                                                                                                                                                                                                                                                                                                                      
                                                                                                                                                                                                                                                                                                                               
        events := perfring.RunTestEvents(t, ctx, ops)         
$ sudo $(which go) test ./pkg/sensors/tracing -bpf-lib $(pwd)/bpf/objs -test.run TestDentryExtractPath                                                                                                                                                                       
ok      github.com/cilium/tetragon/pkg/sensors/tracing  1.325s                                                                                                                                                                                                                                                                     

Could you also rebase to latest main? I believe it should address some of the e2e test failures.

@olsajiri
Copy link
Contributor

could you please split the change into separate logical parts? like at least bpf bits. tests, schema changes.. anything that makes sense to split, thnx

@kkourt kkourt added the needs-rebase This PR needs to be rebased because it has merge conflicts. label Jul 11, 2024
@dwindsor dwindsor force-pushed the pr/dwindsor/add-struct-dentry branch 2 times, most recently from 6c24409 to b450367 Compare July 18, 2024 13:58
Signed-off-by: David Windsor <dawindso@cisco.com>
@dwindsor dwindsor force-pushed the pr/dwindsor/add-struct-dentry branch from 438b3b5 to ad33396 Compare July 18, 2024 14:24
Signed-off-by: David Windsor <dawindso@cisco.com>
Signed-off-by: David Windsor <dawindso@cisco.com>
Signed-off-by: David Windsor <dawindso@cisco.com>
@dwindsor
Copy link
Collaborator Author

Hi @kkourt, thanks for the pointers! Your suggestion worked.

@olsajiri I've split the PR into logical patches. Some CI is failing now but for unrelated reasons.

Thanks!

@kkourt
Copy link
Contributor

kkourt commented Jul 22, 2024

Hi @kkourt, thanks for the pointers! Your suggestion worked.

@olsajiri I've split the PR into logical patches. Some CI is failing now but for unrelated reasons.

Thanks, can you also rebase? There seem to be conflicts. The conflicts seem to be in the auto-generated files, so it should be easy to resolve.

@dwindsor
Copy link
Collaborator Author

Hi @kkourt, thanks for the pointers! Your suggestion worked.
@olsajiri I've split the PR into logical patches. Some CI is failing now but for unrelated reasons.

Thanks, can you also rebase? There seem to be conflicts. The conflicts seem to be in the auto-generated files, so it should be easy to resolve.

Thanks! I've tried running make crds, but it shows no differences to push

@kkourt
Copy link
Contributor

kkourt commented Jul 26, 2024

Hi @kkourt, thanks for the pointers! Your suggestion worked.
@olsajiri I've split the PR into logical patches. Some CI is failing now but for unrelated reasons.

Thanks, can you also rebase? There seem to be conflicts. The conflicts seem to be in the auto-generated files, so it should be easy to resolve.

Thanks! I've tried running make crds, but it shows no differences to push

Have you tried running make protogen as well? The conflicts seem to be related to protobuf definitions.

@kkourt
Copy link
Contributor

kkourt commented Oct 7, 2024

Moving this to draft since there are many conflicts.

@kkourt kkourt marked this pull request as draft October 7, 2024 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/bpf This is related to BPF code needs-rebase This PR needs to be rebased because it has merge conflicts. release-note/minor This PR introduces a minor user-visible change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot track users attempting to follow symlinks outside of a directory root
3 participants