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.

Sign up for GitHub

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

Jump to bottom

feat: Update KubeArmor to use OCI hooks instead of depending on container runtime socket #1714

Open

AbdelrahmanElawady wants to merge 4 commits into kubearmor:main from AbdelrahmanElawady:hook

Contributor

AbdelrahmanElawady commented Apr 8, 2024

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? :

Containers created before KubeArmor was deployed is still not handled.

Checklist:

Bug fix. Fixes #
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update
PR Title follows the convention of <type>(<scope>): <subject>
Commit has unit tests
Commit has integration tests

AbdelrahmanElawady force-pushed the hook branch 3 times, most recently from 9d510ed to 86c5478 Compare

April 8, 2024 20:41

AbdelrahmanElawady force-pushed the hook branch 2 times, most recently from dab1324 to 4e65d92 Compare

April 24, 2024 21:06

Contributor Author

AbdelrahmanElawady commented Apr 24, 2024

Container created before KubeArmor now are handled with a simple detached process that waits on KubeArmor to start then sends all previous containers.

Contributor Author

AbdelrahmanElawady commented Apr 24, 2024

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.

AbdelrahmanElawady marked this pull request as ready for review

April 24, 2024 21:40

daemon1024 requested review from achrefbensaad, daemon1024 and DelusionalOptimist

April 25, 2024 06:52

achrefbensaad reviewed

View reviewed changes

Member

achrefbensaad left a comment

1/2

KubeArmor/core/hook_handler.go Outdated

Comment on lines 29 to 30

		listenPath := filepath.Join(kubearmorDir, "ka.sock")
		_ = os.Remove(listenPath) // in case kubearmor crashed and the socket wasn't removed

Member

achrefbensaad Apr 25, 2024

Suggested change

      
            	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")

Member

achrefbensaad Apr 25, 2024

just reordering for better clarity

Contributor Author

AbdelrahmanElawady Apr 25, 2024

hmm, listenPath is defined in the first line, so that wouldn't work?

KubeArmor/core/hook_handler.go Outdated

Comment on lines 29 to 30

		listenPath := filepath.Join(kubearmorDir, "ka.sock")
		_ = os.Remove(listenPath) // in case kubearmor crashed and the socket wasn't removed

Member

achrefbensaad Apr 25, 2024

you need to handle the error case

KubeArmor/core/hook_handler.go

+              	for {
+              		conn, err := socket.Accept()
+              		if err != nil {
+              			log.Fatal(err)

Member

achrefbensaad Apr 25, 2024

Are we really sure we want to stop the entier execution if a single connection is not handled ?

Contributor Author

AbdelrahmanElawady Apr 25, 2024 •

edited

Loading

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)?

KubeArmor/core/hook_handler.go

+              			return
+              		}
+              		if err != nil {
+              			log.Fatal(err)

Member

achrefbensaad Apr 25, 2024

same comment as above

Member

achrefbensaad Apr 25, 2024

Please handle in other parts in the code as well

KubeArmor/core/hook_handler.go Outdated

Comment on lines 69 to 73

+              		data := struct {
+              			Operation string          `json:"operation"`
+              			Detached  bool            `json:"detached"`
+              			Container types.Container `json:"container"`
+              		}{}

Member

achrefbensaad Apr 25, 2024

Define the struct outside function at the top of the file

KubeArmor/core/hook_handler.go Outdated

+              			return
+              		}
+              		if data.Operation == "create" {

Member

achrefbensaad Apr 25, 2024

Better to use enums

KubeArmor/core/hook_handler.go Outdated

+              	dm.ContainersLock.Lock()
+              	if _, ok := dm.Containers[container.ContainerID]; !ok {
+              		dm.Containers[container.ContainerID] = container
+              		dm.ContainersLock.Unlock()

Member

achrefbensaad Apr 25, 2024

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()

Member

achrefbensaad Apr 25, 2024

handle error

pkg/KubeArmorOperator/hook/crio.go Outdated

Comment on lines 47 to 59

+              		if err != nil {
+              			return nil, err
+              		}

Member

achrefbensaad Apr 25, 2024

are we sure we want to quit at the first error ?

pkg/KubeArmorOperator/hook/crio.go Outdated

Comment on lines 101 to 106

+              type containerInfo struct {
+              	SandboxID   string     `json:"sandboxID"`
+              	Pid         int        `json:"pid"`
+              	RuntimeSpec specs.Spec `json:"runtimeSpec"`
+              	Privileged  bool       `json:"privileged"`
+              }

Member

achrefbensaad Apr 25, 2024

move this to the top of the file

AbdelrahmanElawady force-pushed the hook branch from 83996bc to 74160c2 Compare

April 25, 2024 08:03

AbdelrahmanElawady force-pushed the hook branch 2 times, most recently from 8eb8ead to 145a7c3 Compare

May 22, 2024 21:59

AbdelrahmanElawady added 4 commits

September 18, 2024 22:58


          Listen to hook messages and update snitch to setup hooks on host

118d286

Signed-off-by: AbdelrahmanElawady <[email protected]>


          Detach a process to pass all containers to KubeArmor

0a3a8b0

Signed-off-by: AbdelrahmanElawady <[email protected]>


          Declare types and enums and add better error handling

71c213c

Signed-off-by: AbdelrahmanElawady <[email protected]>


          Remove unused flag and continue on error

255e9c2

Signed-off-by: AbdelrahmanElawady <[email protected]>

AbdelrahmanElawady force-pushed the hook branch from 145a7c3 to 255e9c2 Compare

September 18, 2024 20:06

AbdelrahmanElawady mentioned this pull request

Importing bindings package in an executable used as OCI Hook always fails in rootless containers/podman#24489

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet