TracingPolicy: add global selectors syntactic sugar

[kobrineli]

Is there an existing issue for this?

• I have searched the existing issues

Is your feature request related to a problem?

No response

Describe the feature you would like

Sometimes in tetragon policies we need to write same selectors for each hook we want to intercept and report. For example, we may want to filter out all events, where process binary has specific prefix, e.g. we consider that all events from some directory are false positives.

For instance, we want to intercept call_1, call_2, call_3, and we always want to filter out events where binary locates in dir.

The policy would look like:

spec:
  kprobes:
  - call: <call_1>
    selectors:
    - matchBinaries:
      - operator: "NotPrefix"
        values:
        - "dir"
  - call: <call_2>
    selectors:
    - matchBinaries:
      - operator: "NotPrefix"
        values:
        - "dir"
  - call: <call_3>
    selectors:
    - matchBinaries:
      - operator: "NotPrefix"
        values:
        - "dir"

To make such policies prettier and to improve experience of writing new policies, we could add global selectors - selectors that are declared in the beginning of specification and that are appended to every selector in the policy.

With global selectors, the policy would look like this:

spec:
  globalSelectors:
    matchBinaries:
	- operator: "NotPrefix"
	  values:
	  - "dir"
  kprobes:
  - call: <call_1>
  - call: <call_2>
  - call: <call_3>

Currently in tetragon we have an ability to create lists of calls and to have same selectors for every call in the list (https://tetragon.io/docs/concepts/tracing-policy/hooks/#lists), and it can solve the described problem as well, but if we want to have specific selectors for different calls in the list besides common selectors, it is not sufficient.

For example, we want to intercept system calls open and openat, which have different signatures: open system call has pathname as first argument, and openat has pathname as second argument. We want to catch event when pathname ends with passwd, but we want to filter out events where binary has prefix /opt/myapp, because we consider that our application can read passwd file safely. Because of the fact that these system calls signatures are different, we cannot create a list and have same matchArgs selector, because indices of pathname are different.

With global selectors, the policy would look like this:

spec:
  globalSelectors:
    matchBinaries:
      - operator: "NotPrefix"
        values: "/opt/myapp"
  kprobes:
  - call: "sys_open"
    args:
    - index: 0
      type: "string"
    selectors:
    - matchArgs:
      - index: 0
        operator: "Postfix"
        values:
        - "passwd"
  - call: "sys_openat"
    args:
    - index: 1
      type: "string"
    selectors:
    - matchArgs:
      - index: 1
        operator: "Postfix"
        values:
        - "passwd" 

Describe your proposed solution

To solve the problem and to add such global selectors, we could add global selectors field GlobalSelectors to TracingPolicySpec with type KProbeSelector.

Every time we parse selectors from the specification, we will append to currently parsed selectors selectors from GlobalSelectors field, and parse them as if they were specified together in same field. E.g.:

func ParseMatchArgs(k *KernelSelectorState, matchArgs []v1alpha1.ArgSelector, matchData []v1alpha1.ArgSelector,
	args []v1alpha1.KProbeArg, data []v1alpha1.KProbeArg, globalSelectors *KProbeSelector) error {

  matchArgs = append(matchArgs, globalSelectors.MatchArgs)
  matchData = append(matchData, globalSelectors.MatchData)
  ...
}

Code of Conduct

• I agree to follow this project's Code of Conduct

[kobrineli]

@kkourt
Hi! Could you check this proposal?

If it is useful and makes sense to be implemented, I would like to implement it by myself, seems not very difficult.

[kkourt]

Hi,

Thanks!

From a first look it makes sense to me.

Would it make sense to make things more explicit?

For example, have something like:

spec:
  globalSelectors:
  - myapp:
      matchBinaries:
      - operator: "NotPrefix"
        values:
        - "dir"
  kprobes:
  - call: "sys_open"
    args:
    - index: 0
      type: "string"
    selectors:
    - matchArgs:
      - index: 0
        operator: "Postfix"
        values:
        - "passwd"
      globalSelectors: "myapp"

That is, define a key for the global selector and use it to explicitly add it to the individual selectors.
If we proceed, we should define what happens if the global selectors and the local selectors collide (and the answer might be that there is an error and the policy fails to load).

[kobrineli]

That is, define a key for the global selector and use it to explicitly add it to the individual selectors.
This makes sense to me, but in some complicated big policies, where there might be several combinations of selectors per one call, we will still need to specify the key of global selector in each of them.

What do you think about adding special field then, which will make selector apply to all selectors (maybe something like applyToAll, not sure about this idea actually)?

So it will be something like:

spec:
  globalSelectors:
  - myapp:
      applyToAll: true
      matchBinaries:
      - operator: "NotPrefix"
        values:
        - "dir"
  kprobes:
  - call: "sys_open"
    args:
    - index: 0
      type: "string"
    selectors:
    - matchArgs:
      - index: 0
        operator: "Postfix"
        values:
        - "passwd"

and we will have both possibilities to specify global selector explicitly and implicitly.

If we proceed, we should define what happens if the global selectors and the local selectors collide (and the answer might be that there is an error and the policy fails to load).

I think we should process global selectors in the same way as if they were written in each selector manually. So for example if we specify matchBinaries in global selectors, and then specify matchBinaries in local selector, we will have an error that we specified more than one matchBinaries, as if we written them near each other. I hope I've understood what you meant correctly.

[kkourt]

What do you think about adding special field then, which will make selector apply to all selectors (maybe something like applyToAll, not sure about this idea actually)?

I would personally prefer having things stated only explicitly. I think it makes the policies easier to grok and results in fewer surprises. We even have warnings if a globalSelector is not used if that's a concern. I'll mention this issue on slack to see whether other folk have opinions.

I think we should process global selectors in the same way as if they were written in each selector manually. So for example if we specify matchBinaries in global selectors, and then specify matchBinaries in local selector, we will have an error that we specified more than one matchBinaries, as if we written them near each other. I hope I've understood what you meant correctly.

👍🏼

[mtardy]

Hey 👋 , thanks for taking the time to write this issue. While I agree that having this kind of macro mechanism for syntactic sugar could be useful, I think we should be careful on the naming. For example, right now we use the spec.podSelector as a way to tell "this policy applies only the Pods selected by these labels". Which means that those selectors affect the way the policy is enforced. By reusing the same kind of globalSelectors naming convention, I think it would be maybe confusing and not clear this is not about selecting the targets of the policies and we could end up with stuff like that:

spec:
  podSelector:
    matchLabels:
      app: "prod"
  globalSelectors:
    matchBinaries:
      - operator: "NotPrefix"
         values:
          - "dir"
  kprobes:
  - call: <call_1>
  - call: <call_2>
  - call: <call_3>

Maybe we should find a name to make it clear that those two have very different intents.

[kobrineli]

Maybe we should find a name to make it clear that those two have very different intents.

Maybe we could name it like globalKprobeSelectors, but this might be confusing as well, because it will not be obvious that they can be used not only in kprobes, but in lsm hooks and tracepoints.

If we settle on @kkourt idea of using global selectors explicitly with giving them names, maybe we should name them macros?
Then it will be something like that:

spec:
  podSelector:
    matchLabels:
      app: "prod"
  selectorsMacros:
    macro_1:
      matchBinaries:
       - operator: "NotPrefix"
         values:
         - "dir"
  kprobes:
  - call: <call_1>
    selectors:
      macros:
      - macro_1
  - call: <call_2>
    selectors:
      macros:
      - macro_1
  - call: <call_3>
    selectors:
      macros:
      - macro_1

So selectors section would have macros field, where we can list used macros from declared selectorsMacros above.

And personally I would like to add global parameter to macros to apply them to all selectors in policy implicitly, so the policy would look like:

spec:
  podSelector:
    matchLabels:
      app: "prod"
  selectorsMacros:
    macro_1:
      matchBinaries:
       - operator: "NotPrefix"
         values:
         - "dir"
      global: true
  kprobes:
  - call: <call_1>
  - call: <call_2>
  - call: <call_3>

But I don't insist on adding this global option, I like explicit only variant as well.

Seems like now podSelector and selectorMacros have different naming conventions and are not so confusing.

[kkourt]

From my side, after thinking about it, I think I would still prefer having it explicit.
Is your motivation for suggesting the implicit approach purely ergonomic? Or is there something else?

[kobrineli]

I'm okay with having only explicit variant.

Or is there something else?

No, nothing else, I just think it looks prettier with implicit variant, so it's not so important.