falcosecurity / deploy-kubernetes

Kubernetes deployment resources for Falco
Apache License 2.0
11 stars 16 forks source link

Update Kubernetes configs - will fix a few things #95

Closed incertum closed 3 months ago

incertum commented 6 months ago
  1. Missing HOST_ROOT env in initContainers falco-driver-loader
env:
- name: HOST_ROOT
  value: /host

2.

For completeness suggest explicitly adding HOST_ROOT env to the falco container, plus add FALCO_HOSTNAME env as we now use it in Falco's metrics option and expose it as filter in evt.hostname, so it would be a great example template.

env:
- name: FALCO_HOSTNAME
  valueFrom:
    fieldRef:
      fieldPath: spec.nodeName
- name: HOST_ROOT
  value: /host
  1. DISREGARD

I notice we mount the entire /usr which is not needed, suggest changing to the following in order to adopt best production practices (least access):


volumes:
 - name: usr-src-kernels-fs
  hostPath:
    path: /usr/src/kernels/
...

volumeMounts:
- mountPath: /host/usr/src/kernels/
  name: usr-src-kernels-fs
  readOnly: true
...

4.

The docs here are not sufficient, happy to update them!

incertum commented 6 months ago

@leogr @FedeDP @Issif

leogr commented 6 months ago

The first 3 points should be addressed in the Helm Chart eventually, I guess. cc @alacuku

Anyway:

incertum commented 6 months ago

Sounds good, I wold still add HOST_ROOT=/host just so we serve up a better and clearer template. ACK re double-checking /usr

maxgio92 commented 6 months ago

Thanks @incertum! I agree with all points except for /usr mount as stated by @leogr if it's really needed, otherwise good point for me to selectively mount only /usr/src/kernels one.

incertum commented 6 months ago

We clarified that we indeed need entire /usr/src because of ubuntu distros etc, I updated the original post indicating to disregard that comment. Thanks.

incertum commented 6 months ago

Quick question since it came up in a different issue. Aren't we missing the host /etc mount for the falco container in order to resolve user uids to their names and such?

https://github.com/falcosecurity/deploy-kubernetes/blob/main/kubernetes/falco/templates/daemonset.yaml#L80

Add the following:

- mountPath: /host/etc
  name: etc-fs
leogr commented 6 months ago

Quick question since it came up in a different issue. Aren't we missing the host /etc mount for the falco container in order to resolve user uids to their names and such?

https://github.com/falcosecurity/deploy-kubernetes/blob/main/kubernetes/falco/templates/daemonset.yaml#L80

Add the following:

- mountPath: /host/etc
  name: etc-fs

I can't recall if we looked up from /etc for that purpose :thinking: @FedeDP ?

leogr commented 6 months ago

/assign

FedeDP commented 6 months ago

Yes, we actually need it to resolve user and group related info: https://github.com/falcosecurity/libs/blob/master/userspace/libsinsp/user.cpp#L60

I think we completely forgot to update our charts back when we implemented the feat. Great catch @incertum !

incertum commented 6 months ago

Hello, one more thing :upside_down_face:

See https://github.com/falcosecurity/cncf-green-review-testing/pull/9 we also totally forgot to include /run/k3s/containerd/containerd.sock as container runtime socket and k3s appears to be more frequently used now.

alacuku commented 5 months ago

Yes, we actually need it to resolve user and group related info: https://github.com/falcosecurity/libs/blob/master/userspace/libsinsp/user.cpp#L60

I think we completely forgot to update our charts back when we implemented the feat. Great catch @incertum !

Hi @incertum, @FedeDP I fixed it here: https://github.com/falcosecurity/charts/pull/601/commits/3cf8bdf753c436a0a41d3f34ade6c35921de1b01. Will be released once falco 0.37.0 is out. Is it ok, or do we need the fix before that?

FedeDP commented 5 months ago

Thanks @alacuku ! Great job, as always :) For me, it's ok to release the fix with the Falco 0.37.0 chart.