feat(wrlinux): Add Wind River Linux vulnerability data (#177) #178

ssajal-wrl · 2022-11-09T21:24:13Z

Signed-off-by: Sakib Sajal [email protected]

CLAassistant · 2022-11-09T21:24:19Z

All committers have signed the CLA.

ssajal-wrl · 2022-11-18T21:20:25Z

Hi, do I need to do anything else for this PR?

DmitriyLewen

Hello @ssajal-wrl
Thanks for your work!
I added some comments.

DmitriyLewen · 2022-11-21T06:59:27Z

wrlinux/wrlinux_test.go

+ {
+ name: "perfect data",
+ args: args{
+ filePath: "./testdata/golden",


Looks like it is not golden file.
It is source file.
Better to use names/filepaths from source.
e.g. testdata/foo/bar/CVE-1234-12345

DmitriyLewen · 2022-11-21T07:00:05Z

wrlinux/wrlinux_test.go

+ "Status": "ignored",
+ "Note": ""
+ },
+ 10.19.45.1": {


Tests cases are broken.
Fix them or better use golden files.

DmitriyLewen · 2022-11-21T07:04:08Z

wrlinux/wrlinux.go

+ }
+ lines := strings.Split(string(all), "\n")
+
+ for i := 0; i < len(lines); i++ {


We try to use same logic, if it is possible.
Can you use bufio here:

vuln-list-update/debian/tracker/debian.go

Lines 177 to 179 in 728ebae

s := bufio.NewScanner(f)

for s.Scan() {

line := s.Text()

I can use bufio to read the file, but I still have to read in the whole file and iterate over it the same way since I have inner loops which modify iterator "i".

oh... now i understand what you meant.

DmitriyLewen · 2022-11-21T07:17:37Z

wrlinux/wrlinux.go

+ status := strings.TrimSpace(s[1])
+
+ // Some advisories have status with "Patches_" prefix and it should be skipped
+ // e.g. Patches_qtwebkit-opensource-src: needs-triage


can you create tests case for this?

DmitriyLewen · 2022-11-21T07:19:34Z

wrlinux/wrlinux.go

+ if isPatch(status) && !strings.HasPrefix(s[0], "Patches_") {
+ pkgRel := strings.SplitN(s[0], "_", 2)
+ release := Release(pkgRel[0])
+ pkgName := Package(strings.Trim(pkgRel[1], ":"))


looks like we don't need trim :, because we did this here:

vuln-list-update/wrlinux/wrlinux.go

Line 230 in 0b161fc

s := strings.SplitN(line, ":", 2)

DmitriyLewen · 2022-11-21T07:20:45Z

wrlinux/wrlinux.go

+ status := Status{
+ Status: fields[0],
+ }
+ if len(fields) > 1 {


can you add comment with example, please?

DmitriyLewen · 2022-11-21T07:25:46Z

wrlinux/wrlinux.go

+ }
+
+ // Parse References
+ if strings.HasPrefix(line, "References:") {


it would be better if the test case contains multiple References values.

DmitriyLewen · 2022-11-21T07:26:10Z

wrlinux/wrlinux.go

+ }
+
+ // Parse Notes
+ if strings.HasPrefix(line, "Notes:") {


ssajal-wrl · 2023-02-22T04:36:30Z

@DmitriyLewen I have addressed all your comments/concerns and pushed the changes.
Please review it when you have time :)

ssajal-wrl · 2023-03-08T16:37:06Z

Hi, any updates?

DmitriyLewen

sorry for waiting!
Looks good.
added some small notes.

DmitriyLewen · 2023-03-09T04:07:12Z

wrlinux/wrlinux.go

+ "io"
+ "bufio"
+ "log"
+ "os"
+ "path/filepath"
+ "strings"
+ "time"
+
+ "github.com/aquasecurity/vuln-list-update/git"
+ "github.com/araddon/dateparse"
+ "golang.org/x/xerrors"
+ "github.com/aquasecurity/vuln-list-update/utils"


format import with gofmt please.

DmitriyLewen · 2023-03-09T04:07:44Z

wrlinux/wrlinux.go

+ vuln = &Vulnerability{}
+ vuln.Patches = map[Package]Statuses{}
+
+ lines := []string{}


Suggested change

lines := []string{}

var lines []string

DmitriyLewen · 2023-03-09T04:08:33Z

wrlinux/wrlinux.go

+ }
+ lines := strings.Split(string(all), "\n")
+
+ for i := 0; i < len(lines); i++ {


oh... now i understand what you meant.

DmitriyLewen · 2023-03-09T04:11:43Z

wrlinux/wrlinux_test.go

+ type args struct {
+ filePath string
+ }


I thinks we can move filePath to testCases struct.
Looks liike we don't need args struct.

DmitriyLewen · 2023-03-09T04:13:00Z

wrlinux/wrlinux_test.go

+ Candidate: "CVE-2020-24241",
+ PublicDate: time.Date(2020, 8, 25, 0, 0, 0, 0, time.UTC),
+ Description: "In Netwide Assembler (NASM) 2.15rc10, there is heap use-after-free in saa_wbytes in nasmlib/saa.c.",
+ Priority: "medium",
+ Bugs: []string{
+ "LINCD-2974",
+ "LIN1019-5289",
+ "LIN1018-6614",
+ "LIN10-7689",
+ },


gofmt is also needed here

DmitriyLewen · 2023-03-09T04:22:12Z

wrlinux/wrlinux.go

+ gc := git.Config{}
+ dir := filepath.Join(utils.CacheDir(), cveTrackerDir)
+ for _, url := range repoURLs {
+ _, err = gc.CloneOrPull(url, dir, "master", false)


i think we can remove this folder after adding CVEs.
wdyt?

absolutely!

ssajal-wrl · 2023-03-15T19:45:23Z

@DmitriyLewen addressed all your comments above. Please take a look when you can!

DmitriyLewen

Looks good.
1 small comment

DmitriyLewen · 2023-03-16T07:07:55Z

wrlinux/wrlinux.go

+ log.Printf("failed to clone or pull: %s: %v", url, err)
+ log.Printf("removing %s directory", cveTrackerDir)
+ if err := os.RemoveAll(dir); err != nil {
+ return xerrors.Errorf("failed to remove %s directory: %w", cveTrackerDir, err)
+ }


this is extra code

vuln-list-update/wrlinux/wrlinux.go

Line 86 in 365b5e6

defer os.RemoveAll(dir)

this is enough

correct and addressed.

DmitriyLewen

good job! @ssajal-wrl
Thanks for your work.
Can you sign CLA(#178 (comment))?

@knqyf263 i approved this PR. Can you take a look and merge this PR, if you don't see any notes.

ssajal-wrl · 2023-03-20T14:09:53Z

Hi, I have signed the CLA for vuln-list-update but it still shows as pending, I am not sure how to resolve this. Any suggestions?

DmitriyLewen · 2023-03-21T02:30:06Z

@ssajal-wrl message have small text:)

You have signed the CLA already but the status is still pending? Let us recheck it.

did you try it?

sajal-wr · 2023-03-21T15:42:45Z

I had to sign CLA with the account that authored the commit, sorry for the delay.

ssajal-wrl · 2023-04-03T15:48:19Z

Looks like this PR is good to be merged, anything else that I need to address?

knqyf263 · 2023-04-04T11:04:50Z

We're sorry for the late response. We're not sure if we can keep maintaining this feature. Could you suspend your work until further notice? We want to see how many people need this one.

) Signed-off-by: Sakib Sajal <[email protected]>

FuzzyGophers · 2024-01-10T15:33:28Z

We're sorry for the late response. We're not sure if we can keep maintaining this feature. Could you suspend your work until further notice? We want to see how many people need this one.

Hi, @knqyf263, what are the concerns with maintaining this feature? Would you please provide more details about your concerns?

knqyf263 · 2024-01-15T06:58:51Z

@FuzzyGophers More features bring more bugs.

FuzzyGophers · 2024-01-16T16:38:59Z

@FuzzyGophers More features bring more bugs.

Thank you for the follow up, @knqyf263.

The addition of Wind River Linux is a "standard" approach.

The only concern I can see is whether Wind River Linux continues to host the git repo with the required vulnerability data.

I will be happy to maintain this if it can be merged.

Would this address your concerns?

knqyf263 · 2024-01-17T06:23:32Z

I will be happy to maintain this if it can be merged.

It's hard for outside contributors to keep maintaining OSS for the long term. It is likely that we, the maintainers, will eventually maintain it because this is our job. Unfortunately, our resources are not plentiful.

ssajal-wrl mentioned this pull request Nov 9, 2022

(feat) Wrlinux plugin aquasecurity/trivy#3155

Closed

6 tasks

knqyf263 requested a review from DmitriyLewen November 20, 2022 07:26

DmitriyLewen requested changes Nov 21, 2022

View reviewed changes

ssajal-wrl force-pushed the wrlinux_plugin branch from 0b161fc to 7224d5f Compare February 3, 2023 20:19

ssajal-wrl force-pushed the wrlinux_plugin branch from 7224d5f to 88d9f77 Compare February 22, 2023 04:34

DmitriyLewen reviewed Mar 9, 2023

View reviewed changes

ssajal-wrl force-pushed the wrlinux_plugin branch from 88d9f77 to 365b5e6 Compare March 15, 2023 19:43

DmitriyLewen reviewed Mar 16, 2023

View reviewed changes

ssajal-wrl force-pushed the wrlinux_plugin branch from 365b5e6 to 3a009fd Compare March 16, 2023 14:11

DmitriyLewen approved these changes Mar 17, 2023

View reviewed changes

ssajal-wrl force-pushed the wrlinux_plugin branch from 3a009fd to d314775 Compare March 30, 2023 18:45

feat(wrlinux): Add Wind River Linux vulnerability data (aquasecurity#177

5c04b4c

) Signed-off-by: Sakib Sajal <[email protected]>

ssajal-wrl force-pushed the wrlinux_plugin branch from d314775 to 5c04b4c Compare May 5, 2023 19:23

feat(wrlinux): Add Wind River Linux vulnerability data (#177) #178

Are you sure you want to change the base?

feat(wrlinux): Add Wind River Linux vulnerability data (#177) #178

Conversation

ssajal-wrl commented Nov 9, 2022

CLAassistant commented Nov 9, 2022 • edited Loading

ssajal-wrl commented Nov 18, 2022

DmitriyLewen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssajal-wrl commented Feb 22, 2023

ssajal-wrl commented Mar 8, 2023

DmitriyLewen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssajal-wrl commented Mar 15, 2023

DmitriyLewen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DmitriyLewen left a comment • edited Loading

Choose a reason for hiding this comment

ssajal-wrl commented Mar 20, 2023

DmitriyLewen commented Mar 21, 2023

sajal-wr commented Mar 21, 2023

ssajal-wrl commented Apr 3, 2023

knqyf263 commented Apr 4, 2023

FuzzyGophers commented Jan 10, 2024

knqyf263 commented Jan 15, 2024

FuzzyGophers commented Jan 16, 2024 • edited Loading

knqyf263 commented Jan 17, 2024

CLAassistant commented Nov 9, 2022 •

edited

Loading

DmitriyLewen left a comment •

edited

Loading

FuzzyGophers commented Jan 16, 2024 •

edited

Loading