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

feat: handling of same fix version with different elsa ids and include elsa ids for aqua storage,remove them before saving into oss db #484

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

santhosh1729
Copy link
Contributor

  • Modify Oracle tracker to include logic for saving advisory IDs (ELSA IDs) in the database.

- Modify Oracle tracker to include logic for saving advisory IDs (ELSA IDs) in the database.
Copy link
Contributor

@DmitriyLewen DmitriyLewen left a comment

Choose a reason for hiding this comment

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

Hello @santhosh1729
Thanks for your work!

We are worried about size of trivy-db.
Can you check and write actual and new sizes for DBs?

Also i left 1 comment, take a look, please.

for k, v := range latestVersions {
versionToArches[v] = append(versionToArches[v], k.Arch)
adv := versionToArches[v.FixedVersion]
adv.VendorIDs = v.VendorIDs
Copy link
Contributor

Choose a reason for hiding this comment

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

It can be wrong for case when 2 advisories with same fixedVersion got from 2 different ELSA-IDs
e.g.

  • ELSA-xxxx-0001 - fixedVersion == 0.0.1, arch == amd64
  • ELSA-xxxx-0002 - fixedVersion == 0.0.1, arch == aarch64
    for this case entry will include only one ELSA-xxxx-0002 (or 0001)

Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps we need to create 2 different entries for this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DmitriyLewen Thanks for the review.

As trivy doesn't need this vendor ids data in db, removed them before storing in db so there is no impact on db size.

Addressed same fix version with different elsa-ids and architectures case.

Can you please review this? Thank you!!

Copy link
Contributor

@DmitriyLewen DmitriyLewen Jan 30, 2025

Choose a reason for hiding this comment

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

Hello @santhosh1729
I'm busy with Trivy release right now.

I'll check your PR after release.

…e elsa ids for aqua storage,remove them before saving into oss db
@santhosh1729 santhosh1729 changed the title feat: update Oracle tracker to store advisory ID (ELSA ID) in database feat: handling of same fix version with different elsa ids and include elsa ids for aqua storage,remove them before saving into oss db Jan 23, 2025
@DmitriyLewen
Copy link
Contributor

@santhosh1729

I see a little differently how we can work with vendorIDs.
Can you take a look these changes? (these changes still require refactoring (update comments, etc.). but you will see the main logic)

@santhosh1729
Copy link
Contributor Author

@santhosh1729

I see a little differently how we can work with vendorIDs. Can you take a look these changes? (these changes still require refactoring (update comments, etc.). but you will see the main logic)

@DmitriyLewen
Copy link
Contributor

Hi @santhosh1729
Did you close the PR by mistake?

@santhosh1729 santhosh1729 reopened this Feb 10, 2025
@santhosh1729
Copy link
Contributor Author

yes, I reopened it.

Your changes work for the Aqua use case, but there is a chance the wrong fix version when a CVE is addressed in multiple advisories.

For example, CVE-2024-25742 affecting kernel-tools has different fix versions for advisories:

0:5.14.0-427.16.1.el9_4 – ELSA-2024-2758
0:4.18.0-553.el8_10 – ELSA-2024-3138

https://github.com/aquasecurity/vuln-list/blob/main/oval/oracle/2024/ELSA-2024-2758.json
https://github.com/aquasecurity/vuln-list/blob/main/oval/oracle/2024/ELSA-2024-3138.json

@DmitriyLewen
Copy link
Contributor

But these advisories are for different versions of Oracle Linux.
ELSA-2024-2758 for OL 9
ELSA-2024-3138 for OL 8.

So these advisories will be saved in different containers.
OSVer from Package should handle your case.

Or am I missing something?

@santhosh1729
Copy link
Contributor Author

Sorry, I overlooked the os version.

@DmitriyLewen
Copy link
Contributor

No problem. Can you update your PR based on these changes?

@santhosh1729
Copy link
Contributor Author

Sure, I will take care of that

The latest Trivy-DB includes log changes, and Aqua consumes these changes. we encounter an issue because the logger is a private variable.

Can I update the logger to be public everywhere?

388c617#diff-4e5beea79657fce401854eb1e0e4558528a32279f61876579816724a01b38d2cR51

@DmitriyLewen
Copy link
Contributor

hm...
Do you use something like this?

type WrappedVulnSrc struct {
	newField string
	oracleoval.VulnSrc
}

func (w WrappedVulnSrc) testFunc() {
	// you need to get logger here
}

Or do you have another case?

Anyway i think it is better to create another PR for these changes (we can review new PR the first)

@santhosh1729
Copy link
Contributor Author

We use OSS tracker by creating the object for struct

trivyLog "github.com/aquasecurity/trivy-db/pkg/log"

func NewVulnSrc() *alma.VulnSrc {
	dbc := aquaDB.AquaConfig{}
	return &alma.VulnSrc{
		DB: &AquaAlma{
			Operation: dbc,
		},
		Logger: trivyLog.WithPrefix("alma"),
	}
}

@DmitriyLewen
Copy link
Contributor

Got it!
Maybe we can use Functional Options Pattern (WithLogger, etc. functions).
But we are using DB field as exported field, so this looks like redundancy for trivy-db, and we can just make logger exported field.
@knqyf263 wdyt?

@knqyf263
Copy link
Collaborator

I'm on vacation now. I'll take a look tomorrow.

Copy link
Contributor

@DmitriyLewen DmitriyLewen left a comment

Choose a reason for hiding this comment

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

@santhosh1729 thanks for your work!
LGTM

@knqyf263 tell me if you will watch this PR. If not - I will merge it.

@knqyf263
Copy link
Collaborator

I'm reviewing this PR now.

@knqyf263
Copy link
Collaborator

knqyf263 commented Feb 12, 2025

@santhosh1729 Doesn't this work?

func NewVulnSrc() *alma.VulnSrc {
	vc := alma.NewVulnSrc()
	vc.DB = &AquaAlma{
		Operation: aquaDB.AquaConfig{},
	}
	return vc
}

@santhosh1729
Copy link
Contributor Author

type VulnSrc struct {
	DB // Those who want to customize Trivy DB can override put/get methods.
	DB     // Those who want to customize Trivy DB can override put/get methods.
	logger *log.Logger
}

https://github.com/aquasecurity/trivy-db/pull/486/files#diff-4e5beea79657fce401854eb1e0e4558528a32279f61876579816724a01b38d2cR51

The logger needs to be initialized; otherwise, an error will occur at vs.logger.Warn("Invalid path", log.FilePath(path))
https://github.com/aquasecurity/trivy-db/pull/486/files#diff-4e5beea79657fce401854eb1e0e4558528a32279f61876579816724a01b38d2cR94

@knqyf263
Copy link
Collaborator

knqyf263 commented Feb 12, 2025

It's initialized here through NewVulnSrc.

logger: log.WithPrefix("alma"),

@santhosh1729
Copy link
Contributor Author

Aqua uses this way by overriding the Put function on Aqua's end


type AquaAlma struct {
	aquaDB.Operation
}

func NewVulnSrc() *alma.VulnSrc {
	dbc := aquaDB.AquaConfig{}
	return &alma.VulnSrc{
		DB: &AquaAlma{
			Operation: dbc,
		},
		Logger: trivyLog.WithPrefix("alma"),
	}
}

@knqyf263
Copy link
Collaborator

knqyf263 commented Feb 12, 2025

Hmm. Why doesn't #484 (comment) work? The logger is initialized and you can override DB.

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.

3 participants