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

K8s env, best practice, container.info over container.id #1384

Closed
wants to merge 1 commit into from

Conversation

epcim
Copy link

@epcim epcim commented Sep 8, 2020

What type of PR is this?

/kind rule-update

/area rules

What this PR does / why we need it:

In my case, I log to ES, I want all informations about running pods available within the alert/audit record. Output field of rule, frequently miss important informations (ie, namespace). container.info macro instead can provide bunch of usefull details if k8s is configured (otherwise it has fallback).

It is then better, to log all details and have them either available on alerts.

(Here I miss some clarification, havent found it documented, but it seems to me, that only details about an event, that are part of falcosidekick Outputfields (https://github.com/falcosecurity/falcosidekick/blob/master/outputs/alertmanager.go#L25) are the ones key=value between "()" of the output field of rule. Sorry I am lazy to find it again in src code - it might be bit different, but the truth is that alert does not have all event details.

Which issue(s) this PR fixes:
n/a

Special notes for your reviewer:
racionale here is as I cant get more details in alert, I do have need use flexible %container.info macro to give me all important (namespace, podname, ...)

Does this PR introduce a user-facing change?:

rule(Disallowed SSH Connection): use container.info in output fields
rule(Unexpected outbound connection destination): use container.info in output fields
rule(Unexpected inbound connection source): use container.info in output fields
rule(Modify Shell Configuration File): use container.info in output fields
rule(Read Shell Configuration File): use container.info in output fields
rule(Schedule Cron Jobs): use container.info in output fields
rule(Update Package Repository): use container.info in output fields
rule(Read ssh information): use container.info in output fields
rule(Write below etc): use container.info in output fields
rule(Read sensitive file trusted after startup): use container.info in output fields
rule(Write below rpm database): use container.info in output fields
rule(Modify binary dirs): use container.info in output fields
rule(Mkdir binary dirs): use container.info in output fields
rule(System user interactive): use container.info in output fields
rule(Terminal shell in container): use container.info in output fields
rule(System procs network activity): use container.info in output fields
rule(Program run with disallowed http proxy env): use container.info in output fields
rule(Interpreted procs inbound network activity): use container.info in output fields
rule(Interpreted procs outbound network activity): use container.info in output fields
rule(Unexpected UDP Traffic): use container.info in output fields
rule(User mgmt binaries): use container.info in output fields
rule(Create files below dev): use container.info in output fields
rule(Unexpected K8s NodePort Connection): use container.info in output fields
rule(Netcat Remote Code Execution in Container): use container.info in output fields
rule(Launch Suspicious Network Tool in Container): use container.info in output fields
rule(Launch Suspicious Network Tool on Host): use container.info in output fields
rule(Search Private Keys or Passwords): use container.info in output fields
rule(Clear Log Activities): use container.info in output fields
rule(Remove Bulk Data from Disk): use container.info in output fields
rule(Create Hidden Files or Directories): use container.info in output fields
rule(Launch Remote File Copy Tools in Container): use container.info in output fields
rule(Create Symlink Over Sensitive Files): use container.info in output fields
rule(Detect outbound connections to common miner pool ports): use container.info in output fields
rule(Detect crypto miners using the Stratum protocol): use container.info in output fields
rule(Packet socket created in container): use container.info in output fields
rule(Network Connection outside Local Subnet): use container.info in output fields
rule(Redirect STDOUT/STDIN to Network Connection in Container): use container.info in output fields

@poiana
Copy link

poiana commented Sep 8, 2020

Welcome @epcim! It looks like this is your first PR to falcosecurity/falco 🎉

@epcim
Copy link
Author

epcim commented Sep 8, 2020

To change the rules, I have used these few SED cmds..https://github.com/epcim/falco/blob/container-info-dockerfile/docker/falco/Dockerfile#L113 (the targed branch is example, I do have different build pipeline)

Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

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

LGTM

leogr
leogr previously approved these changes Sep 14, 2020
@krisnova krisnova added this to the 0.27.0 milestone Sep 24, 2020
Copy link
Member

@leodido leodido left a comment

Choose a reason for hiding this comment

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

Thansk @epcim for sending this.

Could you please adjust the release-note block as per contributing guidelines (it should theoretically list all the rules when you made changes) and rebase this on top of the master?

Anyways, I tend to agree with your changes but there's at least a point that we need to take into consideration.

When container.info is not available the output will contain its fallback. Is such fallback something like what we have now (eg., container_id=<ID>)?
Could you provide a fallback example, please?

@leodido leodido changed the title K8s env, best practice, container.info.over container.id K8s env, best practice, container.info over container.id Oct 13, 2020
@epcim
Copy link
Author

epcim commented Oct 13, 2020

Thansk @epcim for sending this.

Could you please adjust the release-note block as per contributing guidelines (it should theoretically list all the rules when you made changes) and rebase this on top of the master?

Anyways, I tend to agree with your changes but there's at least a point that we need to take into consideration.

When container.info is not available the output will contain its fallback. Is such fallback something like what we have now (eg., container_id=<ID>)?
Could you provide a fallback example, please?

  • ok will rebase
  • technically it possibly touch many rules, so release-note this time needs to be "generic statement" but I will list few If catch them.
  • as far as I remember, container.info is makro has container_id=<ID> as fallback

Modified rules:
rule(Disallowed SSH Connection)
rule(Unexpected outbound connection destination)
rule(Unexpected inbound connection source)
rule(Modify Shell Configuration File)
rule(Read Shell Configuration File)
rule(Schedule Cron Jobs)
rule(Update Package Repository)
rule(Read ssh information)
rule(Write below etc)
rule(Read sensitive file trusted after startup)
rule(Write below rpm database)
rule(Modify binary dirs)
rule(Mkdir binary dirs)
rule(System user interactive)
rule(Terminal shell in container)
rule(System procs network activity)
rule(Program run with disallowed http proxy env)
rule(Interpreted procs inbound network activity)
rule(Interpreted procs outbound network activity)
rule(Unexpected UDP Traffic)
rule(User mgmt binaries)
rule(Create files below dev)
rule(Unexpected K8s NodePort Connection)
rule(Netcat Remote Code Execution in Container)
rule(Launch Suspicious Network Tool in Container)
rule(Launch Suspicious Network Tool on Host)
rule(Search Private Keys or Passwords)
rule(Clear Log Activities)
rule(Remove Bulk Data from Disk)
rule(Create Hidden Files or Directories)
rule(Launch Remote File Copy Tools in Container)
rule(Create Symlink Over Sensitive Files)
rule(Detect outbound connections to common miner pool ports)
rule(Detect crypto miners using the Stratum protocol)
rule(Packet socket created in container)
rule(Network Connection outside Local Subnet)
rule(Redirect STDOUT/STDIN to Network Connection in Container)

Signed-off-by: Petr Michalec <epcim@apeliave.net>
@epcim
Copy link
Author

epcim commented Oct 19, 2020

@leodido so I have rebased + updated commit message.

Each rule change must be on its own commit...
For obvious reason I want to avoid split this into series of 37 commits.

Modified rules are:

  • Disallowed SSH Connection
  • Unexpected outbound connection destination
  • Unexpected inbound connection source
  • Modify Shell Configuration File
  • Read Shell Configuration File
  • Schedule Cron Jobs
  • Update Package Repository
  • Read ssh information
  • Write below etc
  • Read sensitive file trusted after startup
  • Write below rpm database
  • Modify binary dirs
  • Mkdir binary dirs
  • System user interactive
  • Terminal shell in container
  • System procs network activity
  • Program run with disallowed http proxy env
  • Interpreted procs inbound network activity
  • Interpreted procs outbound network activity
  • Unexpected UDP Traffic
  • User mgmt binaries
  • Create files below dev
  • Unexpected K8s NodePort Connection
  • Netcat Remote Code Execution in Container
  • Launch Suspicious Network Tool in Container
  • Launch Suspicious Network Tool on Host
  • Search Private Keys or Passwords
  • Clear Log Activities
  • Remove Bulk Data from Disk
  • Create Hidden Files or Directories
  • Launch Remote File Copy Tools in Container
  • Create Symlink Over Sensitive Files
  • Detect outbound connections to common miner pool ports
  • Detect crypto miners using the Stratum protocol
  • Packet socket created in container
  • Network Connection outside Local Subnet
  • Redirect STDOUT/STDIN to Network Connection in Container

@epcim
Copy link
Author

epcim commented Oct 26, 2020

@leodido all ok?

@Kaizhe
Copy link
Contributor

Kaizhe commented Oct 26, 2020

I think image=%container.image.repository will be redundant if we have container.info and
I have some concerns over replacing an output field, as it may breaks the consumer code who may parse by container.id

@leodido
Copy link
Member

leodido commented Oct 28, 2020

Two questions from @ldegio (during our Falco Community Calls) today:

  • why we can't just retain container_id=%container.id field and just append %container.info after it?
  • what is the typical format of a %container.info? does it contains a JSON object, tuples, what?

Because maybe, disregarding eventual container ID duplication, this could be the way to go to also take care of the @Kaizhe's legit concerns.

@Kaizhe
Copy link
Contributor

Kaizhe commented Oct 28, 2020

Two questions from @ldegio (during our Falco Community Calls) today:

  • why we can't just retain container_id=%container.id field and just append %container.info after it?
  • what is the typical format of a %container.info? does it contains a JSON object, tuples, what?

Because maybe, disregarding eventual container ID duplication, this could be the way to go to also take care of the @Kaizhe's legit concerns.

Appending container.info in the output looks good to me

Copy link
Contributor

@Kaizhe Kaizhe left a comment

Choose a reason for hiding this comment

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

Please append container.info instead of replacing container.id

@poiana poiana removed the lgtm label Oct 28, 2020
@epcim
Copy link
Author

epcim commented Nov 2, 2020

container.info

vs container.id vs. id=...

So do we need any other default than?

if string.find(v['output'], "%container.info", nil, true) ~= nil then

I have some concerns over replacing an output field, as it may breaks the consumer code who may parse by container.id

As long as container.info has fallback to container.id and as long as it containse container.id, thats not an issue:

@Kaizhe
Copy link
Contributor

Kaizhe commented Nov 2, 2020

container.info

vs container.id vs. id=...

So do we need any other default than?

if string.find(v['output'], "%container.info", nil, true) ~= nil then

I have some concerns over replacing an output field, as it may breaks the consumer code who may parse by container.id

As long as container.info has fallback to container.id and as long as it containse container.id, thats not an issue:

Can you confirm I can use the same json pointer(I don't need to change anything, e.g. pointer name, level) to get the container ID if you use container.info?

@epcim
Copy link
Author

epcim commented Nov 2, 2020

container.info

vs container.id vs. id=...
So do we need any other default than?

if string.find(v['output'], "%container.info", nil, true) ~= nil then

I have some concerns over replacing an output field, as it may breaks the consumer code who may parse by container.id

As long as container.info has fallback to container.id and as long as it contains container.id, thats not an issue:

Can you confirm I can use the same json pointer(I don't need to change anything, e.g. pointer name, level) to get the container ID if you use container.info?

Seems like, but somebody else better to confirm. I am not Falco developer, just contributor. Macro container.info need some ownership and as you suggested it req. to be backward compatible, future support and notify existing users early if any change in behaviour.

Also image=%container.image.repository will be redundant is still not fixed. Having some filter that would uniq. filter keys of extra_ would be handy.

@fntlnz fntlnz modified the milestones: 0.27.0, 0.28.0 Jan 15, 2021
@epcim
Copy link
Author

epcim commented Jan 20, 2021 via email

@leodido
Copy link
Member

leodido commented Apr 9, 2021

/milestone 0.28.1

@poiana poiana modified the milestones: 0.28.0, 0.28.1 Apr 9, 2021
@leogr leogr modified the milestones: 0.28.1, 0.29.0 Apr 26, 2021
@leogr leogr modified the milestones: 0.29.0, 0.30.0 Jun 17, 2021
@poiana
Copy link

poiana commented Sep 15, 2021

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

@leogr leogr modified the milestones: 0.30.0, 0.31.0 Sep 28, 2021
@poiana
Copy link

poiana commented Oct 28, 2021

Stale issues rot after 30d of inactivity.

Mark the issue as fresh with /remove-lifecycle rotten.

Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle rotten

@poiana
Copy link

poiana commented Nov 28, 2021

Rotten issues close after 30d of inactivity.

Reopen the issue with /reopen.

Mark the issue as fresh with /remove-lifecycle rotten.

Provide feedback via https://github.com/falcosecurity/community.
/close

@poiana
Copy link

poiana commented Nov 28, 2021

@poiana: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue with /reopen.

Mark the issue as fresh with /remove-lifecycle rotten.

Provide feedback via https://github.com/falcosecurity/community.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@poiana poiana closed this Nov 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants