-
Notifications
You must be signed in to change notification settings - Fork 430
tetragon: add matchCurrentCred filter to match current tasks based on their process credentials #3845
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
base: main
Are you sure you want to change the base?
Conversation
7725d54
to
8e55321
Compare
✅ Deploy Preview for tetragon ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
69da199
to
96c8c80
Compare
82ce367
to
d14a5d3
Compare
6160277
to
4bb933b
Compare
4bb933b
to
212e198
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Please find some comments below.
|
||
/* Here we are at the beginning of matching credentials */ | ||
|
||
if (flags & PF_CRED_RUID) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems like the code below this condition and the code under PF_CRED_EUID could be in the function that has the uid argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I tried but it propagated PFILTER_ERROR inside, let me know what you think? since we still need to handle in that generic calls the PFILTER_ERROR, IIRC right now PFILTER_ERROR could be PFILTER_ACCEPT in top callers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olsajiri ok pushed let's see if tests pass, but also what you think about above
Adds matchCurrentCred selector to match against credentials of current running task. For now we support only real and effective uids [1] on current task. matchCurrentCred is against current task credentials, it will always be able to track the real current credentials. Tetragon shadow state of tasks as of today does not save full uids/gids credentials. They are fetched during execve but cached in userspace when we report process.process_credentials fields in json, even if they are cached right now we are more interested in the real current credentials. So how matchCurrentCred will work: 1. When a process executes Tetragon captures full creds and send it to userspace process cache. These are the cached process.process_credentials that are reported for process_exec, process_kprobe and so on, they never change since they are cached in userspace. 2. If a process changes its credentials since the execve snapshot, tetragon userspace cache won't change. 3. matchCurrentCred will always match against current task real time credentials. So if a task changes its credentials in kernel matchCurrentCred will always be able to track that change. Changes: make crds && make -C install/kubernetes [1] https://tetragon.io/docs/reference/grpc-api/#processcredentials Signed-off-by: Djalal Harouni <[email protected]>
Add first matchCurrentCred selector logic to match real and effective UIDs. matchCurrentCred is against current task credentials, it will always be able to track the real current credentials. Tetragon shadow state of tasks as of today does not save full uids/gids credentials. They are fetched during execve but cached in userspace when we report process.process_credentials fields in json, even if they are cached right now we are more interested in the real current credentials. So how matchCurrentCred will work: 1. When a process executes Tetragon captures full creds and send it to userspace process cache. These are the cached process.process_credentials that are reported for process_exec, process_kprobe and so on, they never change since they are cached in userspace. 2. If a process changes its credentials since the execve snapshot, tetragon userspace cache won't change. 3. matchCurrentCred will always match against current task real time credentials. So if a task changes its credentials in kernel matchCurrentCred will always be able to track that change. Signed-off-by: Djalal Harouni <[email protected]>
Signed-off-by: Djalal Harouni <[email protected]>
212e198
to
81f576b
Compare
Much appreciated for the review, all comments handled let's see if tests pass. |
Signed-off-by: Djalal Harouni <[email protected]>
1. Tests starts with root uid 0 2. Tests executes drop-privileges, in Tetragon userspace cached process credentials are euid == 0 3. drop-privileges changes its reuid to 1879048188 but cached state is still uid/euid == 0 since execve snapshot. But in kernel, current credentials are uid/euid == 1879048188. These are the ones that we use for matching. 4. drop-privileges executes /usr/bin/echo In tetragon userspace uid == 0, but in kernel current uid == 1879048188 In tracing policy we match against current credentials 1879048188 In Process json output checker we match against cached userspace state credentials uid/euid == 0. 5. /usr/bin/echo starts with uid/euid == 1879048188 In tetragon uid == 1879048188 same in kernel current uid == 1879048188 Signed-off-by: Djalal Harouni <[email protected]>
Signed-off-by: Djalal Harouni <[email protected]>
81f576b
to
dba3bdf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Please find some comments below.
type CredentialsSelector struct { | ||
// +kubebuilder:validation:Optional | ||
// Real UIDs to match. | ||
RUIDs []CredIDValues `json:"uid,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what's the point of this being a list.
For example, if a user specifies:
- uid:
- operator: Equal
values:
- "10"
- "30"
- operator: Equal
values:
- "20"
Then this either means: (uid == 10 OR uid == 30) AND (uid == 20) which will never match if the values are different.
Or it means (uid == 10 OR uid == 30) OR (uid == 20) which can just be written as a single selector:
- uid:
- operator: Equal
values:
- "10"
- "20"
- "30"
Also, is there a reason why the JSON field different? uid
vs RUIDs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I was thinking an AND , I intended to disable it for now and open it later. To combine Equal and NotEqual
- uid:
- operator: Equal
values:
- "0:999" # system users
- operator: NotEqual
values:
- "102" # syslog
- "995:998" # systemd tools on ubuntu
Or let's say Equal "1000:65535" users but NotEqual "2000" . Of course we can make it in single Equal and use ranges but it is also easier if you use one or two single ranges, and exclude the special ones, so some flexibility for the future, especially if you have cases running with UIDs ranges...
I already check if we have already parsed uid field, let me add another check to ensure we have only one operator, and we can activate later. Will add code comments for it.
Also, is there a reason why the JSON field different? uid vs RUIDs?
To be consistent with https://tetragon.io/docs/reference/grpc-api/#processcredentials
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or let's say Equal "1000:65535" users but NotEqual "2000"
OK, above makes sense to me, thanks!. Let's document that it's an AND, add a test-case for it.
To be consistent with https://tetragon.io/docs/reference/grpc-api/#processcredentials
They are consistent at the JSON level, but not at the gRPC level. I would suggest to name the JSON field ruid
to make the mapping between gRPC and JSON fields obvious as we do in all other cases.
RUIDs []CredIDValues `json:"uid,omitempty"` | ||
// +kubebuilder:validation:Optional | ||
// Effective UIDs to match. | ||
EUIDs []CredIDValues `json:"euid,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
} __attribute__((packed)); | ||
|
||
struct cred_filter { | ||
__u64 ty; /* credentials type flags */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the values of these flags?
Are they used somewhere?
The only thing I'm seeing is:
if (!cred_f->ty)
return PFILTER_REJECT;
so it seems to me that this can be omitted.
If they are used at a later patch, please introduce them there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, it should be used since we can't predict the order of passed credentials filters uid,euid,x,y or x,uid,y,euid... in the tracing policy, so every time we iterate to the next one we should check it to know which field we are parsing... I intended to use it for that, will fix it, thanks!
Operator: "Equal", | ||
Values: []string{"0:0"}, | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is hard to read.
I would suggest an array of test cases, each with the filter and the expected result to make it easier to read and understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure will do, missed it.
cred []v1alpha1.CredentialsSelector | ||
expected []byte | ||
err error | ||
}{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
}) | ||
} | ||
|
||
/* Extra tests */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please use a similar structure as above? That is, something like:
tests := map[string]struct {
cred []v1alpha1.CredentialsSelector
expected []byte
err error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will try otherwise I separate them in different test, since they are a bit special...
// Restore all gids to 0 | ||
if err := syscall.Setgid(0); err != nil { | ||
t.Fatalf("setgid(0) error: %s", err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a fix for an existing test? Please have it as a separate patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok thanks
// Restore all gids to 0 | ||
if err := syscall.Setgid(0); err != nil { | ||
t.Fatalf("setgid(0) error: %s", err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar as above: if this is a fix on an existing test, please add it as a separate patch.
Description
Adds new matchCurrentCred selector to match current running task credentials. For now it supports real and effective UIDs.
Changelog