feat: Update KubeArmor to use OCI hooks instead of depending on container runtime socket #1714
Conversation
AbdelrahmanElawady
force-pushed
the
hook
branch
3 times, most recently
from
9d510ed
to
86c5478
Compare
AbdelrahmanElawady
force-pushed
the
hook
branch
2 times, most recently
from
dab1324
to
4e65d92
Compare
|
Container created before KubeArmor now are handled with a simple detached process that waits on KubeArmor to start then sends all previous containers. |
|
Also, in order to try out this PR. You only need to build this code and run it on a CRI-O Kubernetes cluster and operator with snitch will take care of setting up the cluster. |
daemon1024
requested review from
achrefbensaad,
daemon1024 and
DelusionalOptimist
KubeArmor/core/hook_handler.go
Outdated
| listenPath := filepath.Join(kubearmorDir, "ka.sock") | ||
| _ = os.Remove(listenPath) // in case kubearmor crashed and the socket wasn't removed |
There was a problem hiding this comment.
| listenPath := filepath.Join(kubearmorDir, "ka.sock") | |
| _ = os.Remove(listenPath) // in case kubearmor crashed and the socket wasn't removed | |
| _ = os.Remove(listenPath) // in case kubearmor crashed and the socket wasn't removed | |
| listenPath := filepath.Join(kubearmorDir, "ka.sock") | |
There was a problem hiding this comment.
just reordering for better clarity
There was a problem hiding this comment.
hmm, listenPath is defined in the first line, so that wouldn't work?
KubeArmor/core/hook_handler.go
Outdated
| listenPath := filepath.Join(kubearmorDir, "ka.sock") | ||
| _ = os.Remove(listenPath) // in case kubearmor crashed and the socket wasn't removed |
There was a problem hiding this comment.
you need to handle the error case
| for { | ||
| conn, err := socket.Accept() | ||
| if err != nil { | ||
| log.Fatal(err) |
There was a problem hiding this comment.
Are we really sure we want to stop the entier execution if a single connection is not handled ?
There was a problem hiding this comment.
The reason for some of the fatal error handling is because if we just logged that there is an error and continued, some containers might go unnoticed for the duration of KubeArmor lifetime. Do you think it's better if we leave KubeArmor running and not monitoring some containers or restart KubeArmor and get all containers from the start (using some form of exiting)?
| return | ||
| } | ||
| if err != nil { | ||
| log.Fatal(err) |
There was a problem hiding this comment.
Please handle in other parts in the code as well
KubeArmor/core/hook_handler.go
Outdated
| data := struct { | ||
| Operation string `json:"operation"` | ||
| Detached bool `json:"detached"` | ||
| Container types.Container `json:"container"` | ||
| }{} |
There was a problem hiding this comment.
Define the struct outside function at the top of the file
KubeArmor/core/hook_handler.go
Outdated
| return | ||
| } | ||
|
|
||
| if data.Operation == "create" { |
KubeArmor/core/hook_handler.go
Outdated
| dm.ContainersLock.Lock() | ||
| if _, ok := dm.Containers[container.ContainerID]; !ok { | ||
| dm.Containers[container.ContainerID] = container | ||
| dm.ContainersLock.Unlock() |
There was a problem hiding this comment.
Better to unlock once outside the if-else block rather than unlocking at each branch
pkg/KubeArmorOperator/hook/crio.go
Outdated
| } | ||
|
|
||
| func (h *crioHandler) close() { | ||
| _ = h.conn.Close() |
pkg/KubeArmorOperator/hook/crio.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
are we sure we want to quit at the first error ?
pkg/KubeArmorOperator/hook/crio.go
Outdated
| type containerInfo struct { | ||
| SandboxID string `json:"sandboxID"` | ||
| Pid int `json:"pid"` | ||
| RuntimeSpec specs.Spec `json:"runtimeSpec"` | ||
| Privileged bool `json:"privileged"` | ||
| } |
There was a problem hiding this comment.
move this to the top of the file
AbdelrahmanElawady
force-pushed
the
hook
branch
from
83996bc
to
74160c2
Compare
Signed-off-by: AbdelrahmanElawady <[email protected]>
Signed-off-by: AbdelrahmanElawady <[email protected]>
Signed-off-by: AbdelrahmanElawady <[email protected]>
Signed-off-by: AbdelrahmanElawady <[email protected]>
Purpose of PR?:
This PR utilizes OCI hooks to get container details inside KubeArmor instead of using container runtime socket. It depends on some Kubernetes annotations to get container name, pod name, namespace name and AppArmor profile (it looks like AppArmor is the only one required out of those but further testing is required).
This PR also updates snitch to configure hooks on the host (currently only CRI-O is supported).
Fixes #1390
Does this PR introduce a breaking change?
No, the goal is to have the same functionality as mounting container runtime socket without the security concerns of doing that.
If the changes in this PR are manually verified, list down the scenarios covered::
Tested with Getting Started example for now but more testing will be done.
Additional information for reviewer? :
Checklist:
<type>(<scope>): <subject>