feat(rootio): add language package support for security patches#570
feat(rootio): add language package support for security patches#570chait-slim wants to merge 10 commits intoaquasecurity:mainfrom
Conversation
b9e0a50 to
9e7fdf5
Compare
DmitriyLewen
left a comment
There was a problem hiding this comment.
Hi @chait-slim,
When do you plan to integrate the new language files and the new format in your feed?
We need to update vuln-list-update before this PR.
pkg/vulnsrc/rootio/rootio.go
Outdated
| feedFile.Close() | ||
| return eb.With("file_path", feedFilePath).Wrapf(err, "json decode error") | ||
| } | ||
| feedFile.Close() |
There was a problem hiding this comment.
You can create a separate function to open and parse a language file.
Then you can use defer to close the language file.
pkg/vulnsrc/rootio/rootio.go
Outdated
| func (vs VulnSrc) updateLanguagePackages(dir string) error { | ||
| // Process each supported ecosystem | ||
| for _, ecosystem := range vs.supportedEcosystems { | ||
| feedFilePath := filepath.Join(dir, "vuln-list", rootioDir, string(ecosystem), "feed.json") |
There was a problem hiding this comment.
use const for file name
pkg/vulnsrc/rootio/rootio.go
Outdated
| func (vs VulnSrc) saveEcosystem(ecosystem types.Ecosystem, feeds map[string][]Feed) error { | ||
| vs.logger.Info("Saving Root.io Language DB", "ecosystem", ecosystem) | ||
| err := vs.dbc.BatchUpdate(func(tx *bolt.Tx) error { | ||
| for platform, platformFeeds := range feeds { | ||
| dataSource := types.DataSource{ | ||
| ID: source.ID, | ||
| Name: source.Name + fmt.Sprintf(" (%s)", ecosystem), | ||
| URL: source.URL, | ||
| } | ||
| if err := vs.dbc.PutDataSource(tx, platform, dataSource); err != nil { | ||
| return oops.Wrapf(err, "failed to put data source") | ||
| } | ||
| if err := vs.commit(tx, platform, platformFeeds); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| }) | ||
| if err != nil { | ||
| return oops.Wrapf(err, "batch update error") | ||
| } | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
it looks like you can use save function.
The idea is to use the same file. |
|
@DmitriyLewen addressed all comments |
|
@DmitriyLewen Based on the new implementation, the Root.io data source will now use a simplified feed structure: Feed Files:
Key Changes from Previous Implementation:
Benefits:
This structure aligns with the goal of efficiently delivering Root.io security patches while keeping the implementation |
|
@DmitriyLewen Would appreciate a re-review 🙏 |
|
Hello @chait-slim |
pkg/vulnsrc/rootio/rootio.go
Outdated
| osFeedFileName = "os_feed.json" // OS vulnerability data | ||
| appFeedFileName = "app_feed.json" // Language ecosystem vulnerability data | ||
| feedFileName = "cve_feed.json" // Legacy OS feed filename for backward compatibility |
There was a problem hiding this comment.
We updated these files in vuln-list-update PR
Can you update this PR?
- New file paths
- IIUC now both files use same struct.
pkg/vulnsrc/rootio/rootio.go
Outdated
| } | ||
|
|
||
| // Convert to internal format | ||
| platformName := fmt.Sprintf(ecosystemFormat, ecosystem) |
pkg/vulnsrc/rootio/rootio.go
Outdated
| } | ||
|
|
||
| // NewEcosystemVulnSrcGetter creates a getter for language ecosystem packages | ||
| func NewEcosystemVulnSrcGetter(ecosystem types.Ecosystem) VulnSrcGetter { |
There was a problem hiding this comment.
I think we can use NewVulnSrcGetter
pkg/vulnsrc/rootio/rootio.go
Outdated
| return allAdvsSlice, nil | ||
| } | ||
|
|
||
| func (vs VulnSrcGetter) getEcosystemAdvisories(params db.GetParams) ([]types.Advisory, error) { |
There was a problem hiding this comment.
As I wrote in aquasecurity/trivy#9427 (comment) - I think we need to merge advisories (from default sources + root.io) here
pkg/vulnsrc/rootio/rootio.go
Outdated
| // Get package data from either format | ||
| var pkgName string | ||
| var cvesMap map[string]RawCVEInfo | ||
|
|
||
| if pkg.Pkg != nil { | ||
| // Old format | ||
| pkgName = pkg.Pkg.Name | ||
| cvesMap = pkg.Pkg.CVEs | ||
| } else { | ||
| // New format | ||
| pkgName = pkg.Name | ||
| cvesMap = pkg.CVEs | ||
| } | ||
|
|
||
| for cveID, cveInfo := range cvesMap { |
There was a problem hiding this comment.
IIUC we no longer need to check for the old/new format.
|
@DmitriyLewen updated the code as far as I understand |
- add and use bucket.go file. Use same format for all buckets - check supported ecosystems in bucket package - remove duplicated code
pkg/vulnsrc/rootio/rootio.go
Outdated
| // App feed might not exist yet, skip if file not found | ||
| if os.IsNotExist(err) { | ||
| vs.logger.Debug("App feed not found") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
We will merge this PR only after the PR for vuln-list-update is merged.
So this code is not needed here.
However, if we forget to remove it, we might miss an error in case vuln-list does not contain application files.
pkg/vulnsrc/rootio/rootio.go
Outdated
| @@ -41,6 +44,16 @@ var ( | |||
| vulnerability.Debian, | |||
There was a problem hiding this comment.
we can use ecosystems here.
|
Hello @chait-slim Can you take a look + update tests? |
| @@ -186,6 +203,10 @@ func NewVulnSrcGetter(baseOS types.SourceID) VulnSrcGetter { | |||
| } | |||
|
|
|||
| func (vs VulnSrcGetter) Get(params db.GetParams) ([]types.Advisory, error) { | |||
There was a problem hiding this comment.
We need to think about this.
How we will get advisories for language packages.
There was a problem hiding this comment.
WDYT about the implementation chosen? generating a bucket based on the ecosystem and having only a single attribute being used
There was a problem hiding this comment.
We can’t use this approach.
If we use the default bucket naming (::), Trivy will display Root.io fixed versions for all users.
That would be incorrect — root.io advisories should only be shown for root.io images.
Therefore, the only option is to use the provider logic for language packages and merge advisories in the Get function.
flowchart TD
A[Inspect packages in result] --> B{Does version include “root.io” suffix?}
B -- Yes --> C[Create provider]
C --> D[Use Get function. We should merge standard advisories + root.io advisories in Get function]
B -- No --> F[Use standard detector]
There was a problem hiding this comment.
I think I've solved it now
| }, | ||
| Value: types.Advisory{ | ||
| VulnerableVersions: []string{"<4.0.2"}, | ||
| PatchedVersions: []string{"4.0.2+root.io.1"}, |
There was a problem hiding this comment.
Hello @chait-slim,
I’m working on updating this and the related Trivy PR.
I found that versions with the +root.io suffix are local versions in Python —
see PEP 440.
However, such versions shouldn’t be used in repositories.
Did you plan to change this?
There was a problem hiding this comment.
Hi @DmitriyLewen, according to PEP440:
Local version identifiers SHOULD NOT be used when publishing upstream projects to a public index server, but MAY be used to identify private builds created directly from the project source. Local version identifiers SHOULD be used by downstream projects when releasing a version that is API compatible with the version of the upstream project identified by the public version identifier, but contains additional changes (such as bug fixes)
We are the downstream project in this case and we are pushing to a private index server, rather than a public one. So we do abide to this
There was a problem hiding this comment.
Hello @chait-slim
One more question:
I looked more closely at PEP 440. It states:
“Except where specifically noted below, local version identifiers MUST NOT be permitted in version specifiers, and local version labels MUST be ignored entirely when checking if candidate versions match a given version specifier.”
So we can’t use local versions in comparison specifiers (e.g., < 4.0.3+root.io.1).
(Our https://github.com/aquasecurity/go-pep440-version returns an error in such cases.)
"", "==", and "!=" are allowed for local versions.
For vulnerableVersions, we can do the following instead:
• <= 4.0.3 instead of < 4.0.3+root.io.1
• <= 4.0.3 || 4.0.3+root.io.1 instead of < 4.0.3+root.io.2
Would this approach work for you,
or are you planning to use local versions together with "<", ">", etc.? (i ask, because i saw these cases in tests in PRs)
There was a problem hiding this comment.
Actually, 100% yes. For application patches we deal with exact versions. I will update the test cases
There was a problem hiding this comment.
I will update the test cases
No need to do this, please.
I’m working on updating this and the Trivy PR, and I already have some local changes :)
I’ll update the tests myself.
There was a problem hiding this comment.
@DmitriyLewen Sorry for the late response. Following this comment we made some changes on our side. For now, I've reduced the checks for python and java
There was a problem hiding this comment.
For now, I've reduced the checks for python and java
Just to make sure I understood you correctly — you mean that your feed will include advisories only for Python and Java at the initial stage of support, right?
python and java
We've already discussed Python and found a solution.
I'll check the Maven comparator and get back to you.
There was a problem hiding this comment.
Hello @chait-slim
one small question:
Are the versions in the tests correct?
Do you intend to use the +root.io.x suffix for Maven?
This version is valid and compares correctly.
➜ java -jar ~/.m2/repository/org/apache/maven/maven-artifact/3.9.11/maven-artifact-3.9.11.jar "6.0.0" "6.0.0+root.io.1"
Display parameters as parsed by Maven (in canonical form and as a list of tokens) and comparison result:
1. 6.0.0 -> 6; tokens: [6]
6.0.0 < 6.0.0+root.io.1
2. 6.0.0+root.io.1 -> 6-+root.io.1; tokens: [6, [+root, io, 1]]However, in Maven the + character is treated as a common character inside the qualifier, not as a separator (unlike -, _, or .).(https://maven.apache.org/pom.html#version-order-specification)
That’s why I want to double-check whether you intentionally want to use + here, instead of a more typical separator such as -.
There was a problem hiding this comment.
Hello @chait-slim
So we can’t use local versions in comparison specifiers (e.g., < 4.0.3+root.io.1).
(Our aquasecurity/go-pep440-version returns an error in such cases.)
"", "==", and "!=" are allowed for local versions.
For vulnerableVersions, we can do the following instead:
• <= 4.0.3 instead of < 4.0.3+root.io.1
• <= 4.0.3 || 4.0.3+root.io.1 instead of < 4.0.3+root.io.2
Would this approach work for you,
or are you planning to use local versions together with "<", ">", etc.? (i ask, because i saw these cases in tests in PRs)
Actually, 100% yes. For application patches we deal with exact versions. I will update the test cases
I have news about this one.
After repeated feedback about using local versions, we decided to offer an alternative.
We prepared a PR for our library to add the possibility to use local versions in Python specifiers.
aquasecurity/go-pep440-version#10
Enabling this option will require changes in Trivy's code, so let me know if you decide to use it before updating your feed.
- get language advisories for base ecosystem - use ecosystem for VulnSrcGetter
Extends Root.io vulnerability source to support language ecosystem packages
(pip, npm, rubygems, maven, go, nuget, cargo) in addition to OS packages.