falcosecurity / driverkit

Kit for building Falco drivers: kernel modules or eBPF probes
Apache License 2.0
64 stars 53 forks source link

Usage and break of builder image listing #256

Closed Lowaiz closed 1 year ago

Lowaiz commented 1 year ago

Motivation The new feature of auto listing of builder image totally break the usage of Driverkit inside a deployed Kubernetes Pod.

The function ImageSearch seems to look for the local unix socket, but, as the DinD is an unsafe pratice, this kind of right (accessing Docker socket, so Privileged pods) is not an option I want to deploy. Resulting ImageSearch to fail, causing the build to fail.

Another problem with the current implementation is the authentication to private repositories that seems not handle. And it seems that the command docker search struggle to work with gitlab container registry,

In another topic, I find putting gcc version inside the builder image name not quite optimum. A solution could be to apply multiple tag to images containing multiple gcc version, i.e.: driverkit-builder-any-x86_64:gcc-8.0.0, driverkit-builder-any-x86_64:gcc-6.0.0, driverkit-builder-any-x86_64:gcc-5.0.0, ... could point the same image containing the multiple gcc versions avoiding to create ultra long name, creating a better flavoring.

Feature

Alternatives

I tried to deep dive the docker SDK, but I can't find a way to fix it. The usage of Docker search API function add a big complexity while a simple check of present of base image name, with a possible prefix, and with flavoured tag for gcc and with a possible flavoured tagname for target-specific build (other images than anyones) could erase most of that complexity.

FedeDP commented 1 year ago

Hi @Lowaiz! Sorry for breaking kubernetes :/ We left the issue and the PR opened for like 2 months because i knew something could go wrong. I also pinged you directly in the PR because i know you are the one using kubernetes; that's unfortunate!

The function ImageSearch seems to look for the local unix socket, but, as the DinD is an unsafe pratice, this kind of right (accessing Docker socket, so Privileged pods) is not an option I want to deploy. Resulting ImageSearch to fail, causing the build to fail.

Yep i agree, didn't think of this aspect since we just use the docker builder. I think we can easily fix this by providing a --builderindex option that overrides the search logic with a fixed list of images. It would make a lot of sense too IMHO.

Another problem with the current implementation is the authentication to private repositories that seems not handle. And it seems that the command docker search struggle to work with gitlab container registry,

About gitlab container registry, that's weird :/ About auth, yep i know; i left a comment about that too in the PR. Given we did not support docker login either to download images, i figured it could be implemented as a standalone feat in a follow up PR.

In another topic, I find putting gcc version inside the builder image name not quite optimum. A solution could be to apply multiple tag to images containing multiple gcc version

Well that would be better, but then you lose the ability to lookup a certain tag (like, eg: master); moreover, docker search would just return driverkit-builder-any-x86_64 as it does not differentiate per-tag.

@dwindsor wdyt? I'd like to also evaluate using falcoctl to list all images in a docker repo (ie: using some pkg from it). @alacuku might have more insights on this. In that case, we won't use docker search anymore and everyone will be happy :)

alacuku commented 1 year ago

@Lowaiz

In another topic, I find putting gcc version inside the builder image name not quite optimum. A solution could be to apply multiple tag to images containing multiple gcc version, i.e.: driverkit-builder-any-x86_64:gcc-8.0.0, driverkit-builder-any-x86_64:gcc-6.0.0, driverkit-builder-any-x86_64:gcc-5.0.0, ... could point the same image containing the multiple gcc versions avoiding to create ultra long name, creating a better flavoring

This one should be the way to go. A single repository for the driverkit-builder images. Tags should be used to differentiate the GCC versions. driverkit should be able to list the driverkit-builder's` tags and decide which one to use based on the required GCC version.

@FedeDP, @dwindsor, falcoctl exposes an OCI package that is used to interact with OCI repositories. The required logic to interact with an authenticated repository and list the tags is already in place. By using falcoctl packages we should be able to address the following issues:

Let me know what do you think, if it's ok I would like to work on this fix.

FedeDP commented 1 year ago

Aldo I'd love all of this to be in place! It would be a great improvement. Big +1 from me! I recalled you guys had done something similar in Falcoctl, but this feat was developed while Falcoctl was still in high pace development therefore we couldn't rely on it.

So, all in all, i think this is the way to go.

Lowaiz commented 1 year ago

Hi @FedeDP and @alacuku ,

First of all, thanks you for the reactivity and the feedbacks!

Sorry for breaking kubernetes :/ We left the issue and the PR opened for like 2 months because i knew something could go wrong. I also pinged you directly in the PR because i know you are the one using kubernetes; that's unfortunate!

I'm sorry, I didn't pay a lot of attention to my Github account lately. Anyway, I always rely on dev env to test updates, so it didn't break any production stuff.

Given we did not support docker login either to download images, i figured it could be implemented as a standalone feat in a follow up PR.

With the hardcoded version of image names and the kubernetes deployment, Driverkit wasn't interacting with the registry, giving the image name directly to the kube, that can pull the image. That's why I didn't need the auth before :D

I think we can easily fix this by providing a --builderindex option that overrides the search logic with a fixed list of images.

falcoctl exposes an OCI package that is used to interact with OCI repositories.

I think both feature could be useful depending on the use cases. The flag for non-complex logic and static image name, avoiding to plug a Container registry, and the falcoctl to be up to date as quickly as possible, when using a lot of automation.

Let me know if you do not have the time for the flag development, I think I can handle it.

FedeDP commented 1 year ago

First of all, thanks you for the reactivity and the feedbacks!

We don't love to break existing use cases, you know :)

I think Aldo and eventually me will handle the Falcoctl port of the image listing logic; the builderindex flag is safe and makes sense, you are right! (I should've thought about it in the first place!). If you have the bandwidth to work on it, it would be great!

I'm also thinking about how can we better test k8s runner, it seems like you are the only one using it and I want to avoid situations like this one in the future! Also, would you be interested in joining the driverkit maintainers team? It would be great to have you on board! cc @dwindsor :)

dwindsor commented 1 year ago

+1 from me! Thank you @Lowaiz for both your work and reviews! @EXONER4TED

FedeDP commented 1 year ago

In case you are interested, @Lowaiz , we will need to open a vote: poll-issue between repo maintainers, as stated in falcosecurity governance: https://github.com/falcosecurity/evolution/blob/main/GOVERNANCE.md#maintainership

EXONER4TED commented 1 year ago

Huge +1 from me as well if interested 👍