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

Return non failing exit code if unsupported os found #470

Closed
wants to merge 2 commits into from

Conversation

simar7
Copy link
Member

@simar7 simar7 commented Apr 21, 2020

Fixes: #8

Signed-off-by: Simarpreet Singh [email protected]

@simar7 simar7 requested a review from knqyf263 April 21, 2020 01:14
@simar7 simar7 self-assigned this Apr 21, 2020
@knqyf263
Copy link
Collaborator

Did you try scanning a scratch image? It doesn't work in my environment.

@simar7
Copy link
Member Author

simar7 commented Apr 21, 2020

$ cat Dockerfile 
FROM scratch
ADD hello /
CMD ["/hello"]

$ docker build --tag hello .
Sending build context to Docker daemon   2.56kB
Step 1/3 : FROM scratch
 ---> 
Step 2/3 : ADD hello /
 ---> Using cache
 ---> 28a7d5aea667
Step 3/3 : CMD ["/hello"]
 ---> Using cache
 ---> a0f3d6b92afc
Successfully built a0f3d6b92afc
Successfully tagged hello:latest

$ ./trivy  hello:latest 
2020-04-21T09:21:59.066-0700    WARN    You should avoid using the :latest tag as it is cached. You need to specify '--clear-cache' option when :latest image is changed
2020-04-21T09:21:59.099-0700    WARN    Unsupported OS

$ echo $?                                               
0

@knqyf263
Copy link
Collaborator

OK. let me summarize the problems. First of all, this implementation ignores all errors from scanner.Scan. So, all errors say Unsupported OS and exit with 0.

$ cat Dockerfile
FROM alpine:3.11
RUN rm -rf /lib/apk/db/installed
$ docker build -t no-pkgs .
$ ./trivy no-pkgs
2020-04-21T19:55:28.057+0300    WARN    Unsupported OS

It says the following in master branch

2020-04-21T19:47:31.659+0300    FATAL   error in image scan: scan failed: failed to apply layers: no packages detected

If the database is corrupted, it says Unsupported OS.

image

$ ./trivy alpine:3.11
2020-04-21T19:57:36.223+0300    INFO    Detecting Alpine vulnerabilities...
2020-04-21T19:57:36.223+0300    WARN    Unsupported OS

There are a lot of cases ignoring important errors.

Also, I think they want to scan application dependencies even if the base OS is not supported by Trivy.

$ cat Dockerfile
FROM hello-world

ADD https://raw.githubusercontent.com/aquasecurity/trivy-ci-test/master/Cargo.lock .
$ docker build -t scratch-cargo
$ ./trivy scratch-cargo

@simar7 simar7 force-pushed the dontfail-for-unknown-os branch from 6fb1735 to 676d244 Compare April 21, 2020 22:08
@simar7
Copy link
Member Author

simar7 commented Apr 21, 2020

OK. let me summarize the problems. First of all, this implementation ignores all errors from scanner.Scan. So, all errors say Unsupported OS and exit with 0.

$ cat Dockerfile
FROM alpine:3.11
RUN rm -rf /lib/apk/db/installed
$ docker build -t no-pkgs .
$ ./trivy no-pkgs
2020-04-21T19:55:28.057+0300    WARN    Unsupported OS

It says the following in master branch

2020-04-21T19:47:31.659+0300    FATAL   error in image scan: scan failed: failed to apply layers: no packages detected

If the database is corrupted, it says Unsupported OS.

image

$ ./trivy alpine:3.11
2020-04-21T19:57:36.223+0300    INFO    Detecting Alpine vulnerabilities...
2020-04-21T19:57:36.223+0300    WARN    Unsupported OS

There are a lot of cases ignoring important errors.

Good points. Thanks. I have addressed them.

Also, I think they want to scan application dependencies even if the base OS is not supported by Trivy.

$ cat Dockerfile
FROM hello-world

ADD https://raw.githubusercontent.com/aquasecurity/trivy-ci-test/master/Cargo.lock .
$ docker build -t scratch-cargo
$ ./trivy scratch-cargo

What should be the acceptance criteria / desired behaviour here?

@knqyf263
Copy link
Collaborator

A vulnerability of libraries used by programming language should be detected. In the above case, we should detect vulnerabilities in Cargo.lock and ignore OS packages.

If we do it, we may be able to support DockerSlim as well.
#355

@knqyf263
Copy link
Collaborator

We should test busybox image including a lock file as well.
#397

@simar7
Copy link
Member Author

simar7 commented Apr 23, 2020

A vulnerability of libraries used by programming language should be detected. In the above case, we should detect vulnerabilities in Cargo.lock and ignore OS packages.

I added this aquasecurity/fanal#103

If we do it, we may be able to support DockerSlim as well.
#355

Supporting DockerSlim and Busybox will take some additional work that is outside the scope of this issue. I would vote in the favour of merging this PR and working on them as separate stories.

@knqyf263
Copy link
Collaborator

knqyf263 commented Apr 27, 2020

I felt it was easy to support Docker Slim and Busybox as much as this PR, so I implemented it in #476 to replace this PR.

The change is only a few lines. Let me know if I'm missing something important.

@simar7
Copy link
Member Author

simar7 commented Apr 28, 2020

I felt it was easy to support Docker Slim and Busybox as much as this PR, so I implemented it in #476 to replace this PR.

The change is only a few lines. Let me know if I'm missing something important.

Ah I see your point. I wanted to say that this PR was not going to add support for "scanning vulnerabilities" in busybox and dockerslim images. Maybe I was not clear. Either way, let's scrap this and move over to your PR.

@simar7 simar7 closed this Apr 28, 2020
@simar7 simar7 deleted the dontfail-for-unknown-os branch April 28, 2020 16:40
liamg pushed a commit that referenced this pull request Jun 7, 2022
liamg pushed a commit that referenced this pull request Jun 7, 2022
josedonizetti pushed a commit to josedonizetti/trivy that referenced this pull request Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to ignore Unknown OS
2 participants