Proposal on better exception handling

[mstemm]

This proposes adding exceptions as a first class object to falco rules
files.

It adds a new key "exceptions" to rule objects that allows a rule
writer to define tuples of field names that comprise an exception, and a
new top level object "exception" that contains lists of tuples of field
values that define exceptions to rules.

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

If contributing rules or changes to rules, please make sure to also uncomment one of the following line:

/kind rule-update

/kind rule-create

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area build

/area engine

/area rules

/area tests

/area proposals

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

docs(proposals): Exceptions handling proposal

[Kaizhe]

Drop some comments.

[leodido]

My second round of review, after the one we had all together on Slack.

Just some nits.

In the meantime, I'm trying to better figure out all the use case combinations about the "backward compatibility" topic.

Anyways, thanks a lot for this Mark!

[leogr]

First of all, thank you for this proposal!

It seems good to me, just some very minor issues that @Kaizhe and @leodido already have highlighted (I put a 👍 on their comments).

Other than that, just a question: could this strategy be used to optimize the performance of rule matching at runtime?

[mstemm]

About performance, depending on the implementation, it could make rule evaluation faster. If we stored the set of field values separate from the condition, we could match against them in parallel. However, @ldegio suggested we actually fold the exception into the condition to keep the rule parsing centralized around condition expressions, so that's what I proposed here.

[mstemm]

Okay, I think I addressed all the feedback. Please take another look!

[Kaizhe]

Right now, the exception works in two parts:

1. Define exception handlers (e.g. filter names) for each rule
2. 1st class Exception object with:
   • Exception name matches the rule name
   • define values that match the filters (e.g. container.image.repository=xxx, fd.name=yyy) for each exception handler.

My main concern is on the usability:

1. If the exception handlers doesn't meet user's use case (e.g. missing some filters), he need to either overwrite the rule with new exception handlers or use the placeholder macros.

My proposal is similar to SCC in openshift, which is the 1st class object as SecurityContext. Users have full flexibility of what filters can be used as exceptions.

[mstemm]

So @Kaizhe 's proposal would define Exceptions like this instead (please let me know if I misinterpreted it):

- rule: Write below binary dir
  desc: an attempt to write to any file below a set of binary directories
  condition: >
    bin_dir and evt.dir = < and open_write
    and not package_mgmt_procs
    and not exe_running_docker_save
    and not python_running_get_pip
    and not python_running_ms_oms
    and not user_known_write_below_binary_dir_activities
...
exception: Write below binary dir
  fields:
   - proc_writer: [proc.name, fd.directory]
   - container_writer: [container.image.repository, fd.directory]
  items:
  - container_writer:
    - [docker.io/golang-alpine, /usr/libexec/go]
  append: true

The big difference is that the definition of the fields that make up an exception move into the exception object and not the rule, with the rule being unchanged.

I don't think it's a really big difference but I do prefer the version in the original proposal, with exception fields embedded in the rule and exception values provided separately:

• I think it's a good practice to keep the exception fields close to the rule. A rule writer should know best what makes up a good exception to a rule, and can define tuples of fields that define an exception. This can discourage an exception based on something overly broad such as a process or image name in isolation.
• Although we want to encourage rule writers to define exceptions as a tuple of field names, in practice we hope that many rules will not require any exceptions. However, if field names are in a separate object, you always have to create a separate exception object for each new rule and jump between them to find the rule's conditions and valid exceptions.
• Appending/overriding exceptions is very unambiguous when the items are in a separate object. You're always simply appending the tuple values to each named list when append=true, or replacing the lists when append=false. If you have the fields and items together in a single object, what does append mean? Is it legal to add to the list of field name tuples with append=true, or should append only apply to the field values?

I'd like of course to see what other people think. I'm sure we can make it work one way or the other.

[Kaizhe]

@mstemm , I am thinking the exception in a different way:

exception: Write below binary dir
  excep_1:
    container.image.repository: docker.io/golang-alpine
    fd.name: /usr/libexec/go
  excep_2:
    proc.name: touch
    fd.name: /check
  append: true

[mstemm]

I see—the field names and values would be combined.

That would make it really hard to express lists of exceptions though, wouldn’t it? You’d have to repeat the field names over and over for each set of field values.

[Kaizhe]

I see—the field names and values would be combined.

I see—the field names and values would be combined.

That would make it really hard to express lists of exceptions though, wouldn’t it? You’d have to repeat the field names over and over for each set of field values.

You're right, or

exception: Write below binary dir
  excep_1:  # (container.image.repository in <list> and fd.name in <list>)
    container.image.repository: [docker.io/golang-alpine]
    fd.name: [/usr/libexec/go]
  excep_2: # (proc.name in <list> and fd.name in <list>)
    proc.name: [touch]
    fd.name: [/check]
  append: true

This is like privilege grant in the role definition in k8s.

[mstemm]

We discussed this during the Falco community call on 9/16 and I think there was a slight preference for having the exception fields embedded in the rule. I encouraged people during the call to add their preferences to the comments here.

It also sounds like there was consensus to add this to Falco, so hopefully we can merge this PR and I'll also jump in on the implementation!

[fntlnz]

I've been going trough this with @leodido so this comment is from a shared thought:

We analyzed the proposed language addition and we understood that to make a complete exception one would need to write something like:

- rule: Write below binary dir
  desc: an attempt to write to any file below a set of binary directories
  condition: >
    bin_dir and evt.dir = < and open_write
    and not package_mgmt_procs
    and not exe_running_docker_save
    and not python_running_get_pip
    and not python_running_ms_oms
    and not user_known_write_below_binary_dir_activities
  exceptions:
   - proc_writer: [proc.name, fd.directory]
   - container_writer: [container.image.repository, fd.directory]

- exception: Write below binary dir
  items:
  - proc_writer:
    - [apk, /usr/lib/alpine]
    - [npm, /usr/node/bin]
  - container_writer:
    - [docker.io/alpine, /usr/libexec/alpine]

We unrolled that and we think that if we want to make the exception a rules only construct, we'd prefer them to be 100% colocated to the rule. Something like this:

- rule: Write below binary dir
  desc: an attempt to write to any file below a set of binary directories
  condition: >
    bin_dir and evt.dir = < and open_write
    and not package_mgmt_procs
    and not exe_running_docker_save
    and not python_running_get_pip
    and not python_running_ms_oms
    and not user_known_write_below_binary_dir_activities
  exceptions:
      - proc.name: apk
        fd.directory: /usr/lib/alpine
      - proc.name: npm
        fd.directory: /usr/node/bin
      - container.image.repository: docker.io/alpine
        fd.directory: /usr/libexec/alpine

This completely remove the need of a first class citizen exception construct which could also lead to misunderstandings while using them because one might think that they are reusable given their top level nature.

Ideas? Do you envision any pros/cons of this other approach? We kind tried to reverse engineer your thought for the syntax :)

[fntlnz]

Thanks for adding this detail.

[poiana]

LGTM label has been added.

Git tree hash: 82f158129fc67a4116ebc1eac91a2b38d17b6078

[poiana]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fntlnz, leogr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [fntlnz,leogr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment