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

Add support for Alpine Linux /lib/apk/db/installed (Resolves #72) #107

Merged
merged 16 commits into from
Jan 9, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ A wide range of lockfiles are supported by utilizing this [lockfile package](htt
- `requirements.txt`[\*](https://github.com/google/osv-scanner/issues/34)
- `gradle.lockfile`
- `buildscript-gradle.lockfile`
- `/lib/apk/db/installed`

#### Example

Expand Down
1 change: 1 addition & 0 deletions pkg/lockfile/ecosystems.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,6 @@ func KnownEcosystems() []Ecosystem {
MavenEcosystem,
PipEcosystem,
PubEcosystem,
AlpineEcosystem,
}
}
Empty file.
123 changes: 123 additions & 0 deletions pkg/lockfile/fixtures/apk/installed_multiple
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
C:Q1/JgpM8J6DWI/541tUX+uHEzSjqo=
P:alpine-baselayout-data
V:3.4.0-r0
A:x86_64
S:11664
I:77824
T:Alpine base dir structure and init scripts
U:https://git.alpinelinux.org/cgit/aports/tree/main/alpine-baselayout
L:GPL-2.0-only
o:alpine-baselayout
m:redacted <[email protected]
t:1667573027
c:bd965a7ebf7fd8f07d7a0cc0d7375bf3e4eb9b24
r:alpine-baselayout
F:etc
R:fstab
Z:Q11Q7hNe8QpDS531guqCdrXBzoA/o=
R:group
Z:Q13K+olJg5ayzHSVNUkggZJXuB+9Y=
R:hostname
Z:Q16nVwYVXP/tChvUPdukVD2ifXOmc=
R:hosts
Z:Q1BD6zJKZTRWyqGnPi4tSfd3krsMU=
R:inittab
Z:Q1TsthbhW7QzWRe1E/NKwTOuD4pHc=
R:modules
Z:Q1toogjUipHGcMgECgPJX64SwUT1M=
R:mtab
a:0:0:777
Z:Q1kiljhXXH1LlQroHsEJIkPZg2eiw=
R:nsswitch.conf
Z:Q19DBsMnv0R2fajaTjoTv0C91NOqo=
R:passwd
Z:Q1TchuuLUfur0izvfZQZxgN/LJhB8=
R:profile
Z:Q1Ia5UTXvRkAH1lTZK8lm8qRBdRF4=
R:protocols
Z:Q11fllRTkIm5bxsZVoSNeDUn2m+0c=
R:services
Z:Q1oNeiKb8En3/hfoRFImI25AJFNdA=
R:shadow
a:0:42:640
Z:Q1ltrPIAW2zHeDiajsex2Bdmq3uqA=
R:shells
Z:Q1ojm2YdpCJ6B/apGDaZ/Sdb2xJkA=
R:sysctl.conf
Z:Q14upz3tfnNxZkIEsUhWn7Xoiw96g=

C:Q1Pk7x1woArbB1nzkMPJPq1TECwus=
P:musl
V:1.2.3-r4
A:x86_64
S:388955
I:634880
T:the musl c library (libc) implementation
U:https://musl.libc.org/
L:MIT
o:musl
m:redacted <[email protected]>
t:1668104640
c:f93af038c3de7146121c2ea8124ba5ce29b4b058
p:so:libc.musl-x86_64.so.1=1
F:lib
R:ld-musl-x86_64.so.1
a:0:0:755
Z:Q1tGxgx2FLrD+0Uk03NUBwbbEiRCU=
R:libc.musl-x86_64.so.1
a:0:0:777
Z:Q17yJ3JFNypA4mxhJJr0ou6CzsJVI=

C:Q1NN3sp0yr99btRysqty3nQUrWHaY=
P:busybox
V:1.35.0-r29
A:x86_64
S:509600
I:962560
T:Size optimized toolbox of many common UNIX utilities
U:https://busybox.net/
L:GPL-2.0-only
o:busybox
m:redacted <[email protected]>
t:1668852790
c:1dbf7a793afae640ea643a055b6dd4f430ac116b
D:so:libc.musl-x86_64.so.1
p:cmd:busybox=1.35.0-r29
r:busybox-initscripts
F:bin
R:busybox
a:0:0:755
Z:Q1yVNLeeB7VouhCO/kz+dbfL3dY4c=
F:etc
R:securetty
Z:Q1mB95Hq2NUTZ599RDiSsj9w5FrOU=
R:udhcpd.conf
Z:Q1EgLFjj67ou3eMqp4m3r2ZjnQ7QU=
F:etc/logrotate.d
R:acpid
Z:Q1TylyCINVmnS+A/Tead4vZhE7Bks=
F:etc/network
F:etc/network/if-down.d
F:etc/network/if-post-down.d
F:etc/network/if-post-up.d
F:etc/network/if-pre-down.d
F:etc/network/if-pre-up.d
F:etc/network/if-up.d
R:dad
a:0:0:775
Z:Q1ORf+lPRKuYgdkBBcKoevR1t60Q4=
F:sbin
F:tmp
M:0:0:1777
F:usr
F:usr/sbin
F:usr/share
F:usr/share/udhcpc
R:default.script
a:0:0:755
Z:Q1HWpG3eQD8Uoi4mks2E3SSvOAUhY=
F:var
F:var/cache
F:var/cache/misc
F:var/lib
F:var/lib/udhcpd
32 changes: 32 additions & 0 deletions pkg/lockfile/fixtures/apk/installed_single
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
C:Q1Ef3iwt+cMdGngEgaFr2URIJhKzQ=
P:apk-tools
V:2.12.10-r1
A:x86_64
S:120973
I:307200
T:Alpine Package Keeper - package manager for alpine
U:https://gitlab.alpinelinux.org/alpine/apk-tools
L:GPL-2.0-only
o:apk-tools
m:Natanael Copa <[email protected]>
t:1666552494
c:0188f510baadbae393472103427b9c1875117136
D:musl>=1.2 ca-certificates-bundle so:libc.musl-x86_64.so.1 so:libcrypto.so.3 so:libssl.so.3 so:libz.so.1
p:so:libapk.so.3.12.0=3.12.0 cmd:apk=2.12.10-r1
F:etc
F:etc/apk
F:etc/apk/keys
F:etc/apk/protected_paths.d
F:lib
R:libapk.so.3.12.0
a:0:0:755
Z:Q1opjpYqXgzmOVo7EbNe8l5Xol08g=
F:lib/apk
F:lib/apk/exec
F:sbin
R:apk
a:0:0:755
Z:Q1/4bmOPe/H1YhHRzlrj27oufThMw=
F:var
F:var/lib
F:var/lib/apk
56 changes: 56 additions & 0 deletions pkg/lockfile/parse-apk-installed.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package lockfile

import (
"bufio"
"fmt"
"os"
"strings"
)

const AlpineEcosystem Ecosystem = "Alpine"

func ParseApkInstalled(pathToLockfile string) ([]PackageDetails, error) {
var packages []PackageDetails

file, err := os.Open(pathToLockfile)
if err != nil {
return packages, fmt.Errorf("could not open %s: %w", pathToLockfile, err)
}
defer file.Close()

scanner := bufio.NewScanner(file)

var curPkg PackageDetails = PackageDetails{}

for scanner.Scan() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we should take a similar approach to what I've done in the yarn.lock parser, which was to do the parsing in two phases: 1. group the lines for each package, 2. parse each collection of lines in PackageDetails.

While technically it's doing more loops, it should make it easier to reduce the amount of conditional branches (especially the duplicate ones outside the loop) which in my experience tends to be worth it - what do you think?

(if you do agree, then I think you should be able to use pretty much the exact same logic/structure as the yarn.lock parser, then add an extra check to handle if the name couldn't be found, which could go in a couple of different places)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problems for me, I'll try and get back to you soon.
I think that additional loops are not an issue since lockfiles are generally small.
There will be more code, but aligned to other parsers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @G-Rath ,

I have a question:

it should make it easier to reduce the amount of conditional branches
(especially the duplicate ones outside the loop)

maybe I'm missing something but with your solution, aren't you performing the same pattern and number of conditional branches?
I'm referring to parse-yarn-lock.go: here and here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @G-Rath ,
committed changes. Tests ok.
Thank you again for your advice and support.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe I'm missing something but with your solution, aren't you performing the same pattern and number of conditional branches?

Correct, but the footprint is smaller since it's just a length check + an append; the idea of that function is effectively to try and get us out of that raw unstructured state as fast as possible, so that we can do the more critical "business logic" stuff against a good structure which usually lets us avoid more complex versions of that pattern (which is where bugs can easily get introduced).

I view it as a lesser evil type situation - I won't say it's impossible to avoid having a duplicated check like that somewhere in this code, but I would expect it to have a significant tradeoff in at least one area (performance, readability, size, etc) that would make it worse than duplicating these three lines (I'm happy to be proven wrong though, if anyone has some ideas).

line := scanner.Text()
if line == "" {
// First line is empty or multiple empty lines, no current package values
if (PackageDetails{}) == curPkg {
continue
}
// Empty line follows a package info block. Append package before going to next one
packages = append(packages, curPkg)
curPkg = PackageDetails{}
continue
}
// File SPECS: https://wiki.alpinelinux.org/wiki/Apk_spec
if strings.HasPrefix(line, "P:") {
cmaritan marked this conversation as resolved.
Show resolved Hide resolved
curPkg.Name = strings.Split(line, "P:")[1]
cmaritan marked this conversation as resolved.
Show resolved Hide resolved
} else if strings.HasPrefix(line, "V:") {
curPkg.Version = strings.Split(line, "V:")[1]
cmaritan marked this conversation as resolved.
Show resolved Hide resolved
curPkg.Ecosystem = AlpineEcosystem
curPkg.CompareAs = AlpineEcosystem
}

}
if (PackageDetails{}) != curPkg {
packages = append(packages, curPkg)
}

if err := scanner.Err(); err != nil {
return packages, fmt.Errorf("error while scanning %s: %w", pathToLockfile, err)
}

return packages, nil
}
68 changes: 68 additions & 0 deletions pkg/lockfile/parse-apk-installed_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package lockfile_test

import (
"github.com/google/osv-scanner/pkg/lockfile"
"testing"
)

func TestParseApkInstalled_Empty(t *testing.T) {
t.Parallel()

packages, err := lockfile.ParseApkInstalled("fixtures/apk/installed_empty")
cmaritan marked this conversation as resolved.
Show resolved Hide resolved

if err != nil {
t.Errorf("Got unexpected error: %v", err)
}

expectPackages(t, packages, []lockfile.PackageDetails{})
}

func TestParseApkInstalled_Single(t *testing.T) {
t.Parallel()

packages, err := lockfile.ParseApkInstalled("fixtures/apk/installed_single")

if err != nil {
t.Errorf("Got unexpected error: %v", err)
}

expectPackages(t, packages, []lockfile.PackageDetails{
{
Name: "apk-tools",
Version: "2.12.10-r1",
Ecosystem: lockfile.AlpineEcosystem,
CompareAs: lockfile.AlpineEcosystem,
},
})
}

func TestParseApkInstalled_Multiple(t *testing.T) {
t.Parallel()

packages, err := lockfile.ParseApkInstalled("fixtures/apk/installed_multiple")

if err != nil {
t.Errorf("Got unexpected error: %v", err)
}

expectPackages(t, packages, []lockfile.PackageDetails{
{
Name: "alpine-baselayout-data",
Version: "3.4.0-r0",
Ecosystem: lockfile.AlpineEcosystem,
CompareAs: lockfile.AlpineEcosystem,
},
{
Name: "musl",
Version: "1.2.3-r4",
Ecosystem: lockfile.AlpineEcosystem,
CompareAs: lockfile.AlpineEcosystem,
},
{
Name: "busybox",
Version: "1.35.0-r29",
Ecosystem: lockfile.AlpineEcosystem,
CompareAs: lockfile.AlpineEcosystem,
},
})
}
1 change: 1 addition & 0 deletions pkg/lockfile/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ var parsers = map[string]PackageDetailsParser{
"yarn.lock": ParseYarnLock,
"gradle.lockfile": ParseGradleLock,
"buildscript-gradle.lockfile": ParseGradleLock,
"installed": ParseApkInstalled,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably not worth the extra complexity to try and solve right now, but I'm interested in how others feel about this - while I know installed is the name of the file, it feels a little too generic; I wouldn't be surprised if there's another tool out there that creates a file with the same name that we might one day want to support.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it's too generic, it is not unlikely someone might just have a installed file in their repository completely unrelated to alpine, which will spit out potentially confusing warnings.

A quick fix could be to check whether installed is in /lib/apk/db/installed and only scan that, though that is quite limited, since you might be copying it out, or mounting a docker image ...etc.

Copy link
Collaborator

@G-Rath G-Rath Jan 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this should be handled similar to git, sboms and csvs in osv-detector (which are a parser located in lockfile, but are never assumed when scanning directories - you have to explicitly say "this is a CSV I want you to check") - because /lib/apk/db/installed is very specific, so you're only ever going to get it checked if you run the scanner from anywhere along that path with -r anyway

So what if instead it had a dedicated flag, like --check-apk-installed?

#94 could then be used as a way of telling the scanner to treat arbitrary files as installed, but the scanner would never automatically assume a file called installed should be parsed as-such

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea, I made an issue here #124 to track it since as you said it's beyond the scope of this PR. We probably should solve this before the next release to avoid changing behavior.

}

func ListParsers() []string {
Expand Down