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: Add Julia language analyzer support #5635

Merged
merged 6 commits into from
May 15, 2024

Conversation

Octogonapus
Copy link
Contributor

@Octogonapus Octogonapus commented Nov 22, 2023

Description

This PR adds Julia language support for generating SBOMs.

Related issues

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

@Octogonapus Octogonapus force-pushed the analyzer_language_julia branch 5 times, most recently from ccf889f to 1024114 Compare November 22, 2023 22:04
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 @Octogonapus
Thanks for your work!

I left few comments.

Can you also add tests for SPDX, CycloneDX to see how we use UUID for these formats?

docs/docs/scanner/vulnerability/language/index.md Outdated Show resolved Hide resolved
docs/docs/coverage/language/julia.md Outdated Show resolved Hide resolved
docs/docs/coverage/language/julia.md Show resolved Hide resolved
docs/docs/coverage/language/index.md Show resolved Hide resolved
pkg/fanal/analyzer/language/julia/pkg/pkg.go Outdated Show resolved Hide resolved
pkg/fanal/analyzer/language/julia/pkg/pkg.go Outdated Show resolved Hide resolved
pkg/fanal/analyzer/language/julia/pkg/pkg.go Outdated Show resolved Hide resolved
pkg/fanal/analyzer/language/julia/pkg/pkg.go Outdated Show resolved Hide resolved
pkg/fanal/analyzer/language/julia/pkg/pkg.go Outdated Show resolved Hide resolved
@Octogonapus
Copy link
Contributor Author

Can you also add tests for SPDX, CycloneDX to see how we use UUID for these formats?

Yes I would like to do this. Can you help me by pointing out where there is an example of how to do this? I looked around the code for a while but I can't figure out how to export an SBOM in code (except of course by invoking the trivy command).

@DmitriyLewen
Copy link
Contributor

DmitriyLewen commented Nov 28, 2023

@Octogonapus
Copy link
Contributor Author

I pushed an SPDX test with Julia, though it currently does not pass because the SPDX marshaller is doing some sort of deduplication based on the package name. i.e. if I changed one of the B packages to C, then the test could pass. Do you know what is causing that?

@DmitriyLewen
Copy link
Contributor

Do you mean that the packages are in random order?

@Octogonapus
Copy link
Contributor Author

No, I mean that in the SPDX marshal test I added, there are two packages named B, one of which (the last one) gets entirely removed during marshaling. The final SBOM contains 4 package entries instead of 5, and 4 relationships instead of 5.

@DmitriyLewen
Copy link
Contributor

Now i understand you.
This happens because we are overwriting hasher function for the SPDX test:

hasher := func(v interface{}, format hashstructure.Format, opts *hashstructure.HashOptions) (uint64, error) {
h.Reset()
var str string
switch v.(type) {
case ftypes.Package:
str = v.(ftypes.Package).Name + v.(ftypes.Package).FilePath
case string:
str = v.(string)
case *ftypes.OS:
str = v.(*ftypes.OS).Name
default:
require.Failf(t, "unknown type", "%T", v)
}
if _, err := h.Write([]byte(str)); err != nil {
return 0, err
}
return h.Sum64(), nil
}

Some packages (like go binary packages) don't have ID, so use that instead of Name here:

str = v.(ftypes.Package).Name + v.(ftypes.Package).FilePath

Perphaps we can just add Indirect field.
wdyt?

@Octogonapus
Copy link
Contributor Author

Thanks, good idea, that worked.

@Octogonapus
Copy link
Contributor Author

Octogonapus commented Dec 1, 2023

I pushed a CycloneDX test as well. There are a few problems with it that will require changing its marshaller:

  1. There is a problem with the CycloneDX dependencies format. Only package URLs are used, so in the case of a shadowed dep, you don't know which B package is being referenced since there are two pkg:julia/B@1.9.0. Are we required to use PURLs or can we use another format? SPDX uses an arbitrary hash here and that works well.
  2. I noticed this comment: https://vscode.dev/github/Octogonapus/trivy/blob/analyzer_language_julia/pkg/sbom/cyclonedx/core/cyclonedx.go#L87. Indeed this is the case as you can see in my (failing) CycloneDX test. Can this functionality be changed, or will that not conform to the CycloneDX specification? If we can't change it, Julia users should simply not use CycloneDX.
  3. (applicable to SPDX as well) I have a concern about the hasher used in production, given that we use a custom hasher in tests. Should we update the default hasher rather than providing a custom one in our tests? If not, how do we know a proper hasher implementation (i.e. one which does not discard shadowed packages) will be used in production?

I guess if we really needed to, we could also change the Julia PURL spec as package-url/purl-spec#237 has not been accepted yet. The PURLs could become pkg:UUID@version e.g. pkg:edca9bc6-334e-11e9-3554-9595dbb4349c@1.9.0.

@DmitriyLewen
Copy link
Contributor

DmitriyLewen commented Dec 4, 2023

Only package URLs are used, so in the case of a shadowed dep, you don't know which B package is being referenced since there are two pkg:julia/B@1.9.0

This is a strange case. Why does Julia create duplicate dependencies?
Why can't we use existing B for A?

About PURL and BomRef - i think we can use next logic:

  • create PURL as you offered
  • use pkgID (ID from julia file) for BomRef

Can this functionality be changed

What if we update BomRef logic? if we use pkgID, we will not get a conflict, because BomRef will be unique for each julia library.

The PURLs could become pkg:UUID@version e.g. pkg:edca9bc6-334e-11e9-3554-9595dbb4349c@1.9.0.

I don't think it's convenient. You need to read the lock file to find out the dependency name. But we can use tag_id for UUID (as for https://github.com/package-url/purl-spec/blob/master/PURL-TYPES.rst#swid)
wdyt?

Should we update the default hasher rather than providing a custom one in our tests

If I remember correctly, we added this because SPDX uses different hashes for each run(take a look this test -

{
name: "conda generating SPDX SBOM",
args: args{
command: "rootfs",
format: "spdx-json",
input: "testdata/fixtures/repo/conda",
},
golden: "testdata/conda-spdx.json.golden",
},
).

@Octogonapus
Copy link
Contributor Author

Octogonapus commented Dec 6, 2023

This is a strange case. Why does Julia create duplicate dependencies?
Why can't we use existing B for A?

Julia allows you to add two dependencies with the same name but different UUIDs. This is an edge case, but given that it is allowed, I think it is important to test.

Your suggestions for the PURL sounds good to me. After reviewing the spec, I can simply add a qualifier for the uuid. I will work on adding this to the spec and to https://github.com/package-url/packageurl-go.

@Octogonapus
Copy link
Contributor Author

I added two integration tests for Julia SBOMs (SPDX and CycloneDX). I can't manage to run the tests, though:

cd integration
go test
package github.com/aquasecurity/trivy/integration: build constraints exclude all Go files in /home/salmon/Documents/code/trivy/integration

or

mage test:integration
No .go files marked with the mage build tag in this directory.

So the tests might be wrong. If admin could trigger CI, that would be helpful so I can fix the tests.

@DmitriyLewen
Copy link
Contributor

DmitriyLewen commented Dec 7, 2023

You need to run mage test:integration from the root directory of the trivy repository:

➜ ls -dhl */
drwxr-xr-x  11 work  staff   352B Jul 31 09:25 brand/
drwxr-xr-x   4 work  staff   128B Aug 30 08:51 ci/
drwxr-xr-x   3 work  staff    96B Jul 31 09:25 cmd/
drwxr-xr-x  10 work  staff   320B Oct 31 12:24 contrib/
drwxr-xr-x  11 work  staff   352B Dec  7 10:47 docs/
drwxr-xr-x   4 work  staff   128B Jul 31 09:25 examples/
drwxr-xr-x   3 work  staff    96B Jul 31 09:25 helm/
drwxr-xr-x  15 work  staff   480B Dec  7 11:40 integration/
drwxr-xr-x   3 work  staff    96B Nov 30 10:08 internal/
drwxr-xr-x   5 work  staff   160B Dec  7 12:07 magefiles/
drwxr-xr-x   5 work  staff   160B Oct 31 12:24 misc/
drwxr-xr-x  43 work  staff   1.3K Nov 30 10:08 pkg/
drwxr-xr-x   5 work  staff   160B Jul 31 09:25 rpc/

➜ mage test:integration
?       github.com/aquasecurity/trivy/pkg/fanal/test/integration/docker [no test files]
...

I will work on adding this to the spec and to https://github.com/package-url/packageurl-go.

Let us know about this work. It may make sense to wait for these changes before merging this PR.

@Octogonapus
Copy link
Contributor Author

You need to run mage test:integration from the root directory of the trivy repository

That is what I did.

Let us know about this work. It may make sense to wait for these changes before merging this PR.

I've updated the spec PR. Luckily there are no changes required in packageurl-go.

@DmitriyLewen
Copy link
Contributor

That is what I did.

hm.. this is strange.
How did you install mage?

Also you can copy command from mage file (e.g.

trivy/magefiles/magefile.go

Lines 239 to 243 in 01edbda

// Integration runs integration tests
func (t Test) Integration() error {
mg.Deps(t.FixtureContainerImages)
return sh.RunWithV(ENV, "go", "test", "-timeout", "15m", "-v", "-tags=integration", "./integration/...", "./pkg/fanal/test/integration/...")
}
)
and use it in bash.

Anyway i ran tests in CI/CD.

@Octogonapus
Copy link
Contributor Author

I reinstalled mage from source and it is working now 🤷

I fixed the integration tests. Everything is looking good to me, now that the PURL includes the UUID.

pkg/sbom/cyclonedx/marshal.go Outdated Show resolved Hide resolved
pkg/purl/purl.go Outdated Show resolved Hide resolved
pkg/fanal/analyzer/language/julia/pkg/pkg.go Outdated Show resolved Hide resolved
// Removes dependencies not specified as direct dependencies in the Project.toml file.
// This is not strictly necessary, but given that test dependencies are in flux right now, this is future-proofing.
// https://pkgdocs.julialang.org/v1/creating-packages/#target-based-test-specific-dependencies
func (a juliaAnalyzer) removeExtraDependencies(fsys fs.FS, dir string, app *types.Application) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

We recently added Dev flag to Package (Dev dependencies are excluded from the report by default, but users can use --include-dev-deps to include them).

But I'm not sure that Julia's users need this feature.
What do you think? Do users need dev dependencies in reports?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I think this will be helpful! I have tried to implement this from looking at other code and I've updated the test with some dev deps. LMK if it looks correct.

pkg/fanal/analyzer/language/julia/pkg/pkg.go Outdated Show resolved Hide resolved
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.

LGTM.
Left small comments. Take a look, please

pkg/fanal/analyzer/language/julia/pkg/pkg.go Outdated Show resolved Hide resolved
pkg/fanal/analyzer/language/julia/pkg/pkg.go Outdated Show resolved Hide resolved
pkg/fanal/analyzer/language/julia/pkg/pkg_test.go Outdated Show resolved Hide resolved
pkg/fanal/analyzer/language/julia/pkg/pkg.go Outdated Show resolved Hide resolved
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.

LGTM now!
@Octogonapus Thanks for your work!

@knqyf263 i approved this PR. Take a look, when you have time, please.

@DmitriyLewen
Copy link
Contributor

@Octogonapus
Can you fix linter errors, please?

@Octogonapus
Copy link
Contributor Author

Will be fixed by aquasecurity/go-dep-parser#292

@Octogonapus Octogonapus force-pushed the analyzer_language_julia branch 3 times, most recently from 90e9f1b to 60df344 Compare February 26, 2024 19:36
@Octogonapus Octogonapus changed the title feat(julia): Add Julia language analyzer support feat: Add Julia language analyzer support Feb 28, 2024
Copy link

github-actions bot commented May 9, 2024

This PR is stale because it has been labeled with inactivity.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and will be auto-closed. label May 9, 2024
@Octogonapus Octogonapus force-pushed the analyzer_language_julia branch 2 times, most recently from 01e1987 to fbc517f Compare May 10, 2024 03:43
@Octogonapus
Copy link
Contributor Author

I'm still trying to keep this PR up to date... though it can be hard with how many refactors happen on main

@github-actions github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and will be auto-closed. label May 11, 2024
Copy link
Collaborator

@knqyf263 knqyf263 left a comment

Choose a reason for hiding this comment

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

Thanks for your patient. LGTM. I left a small comment.

Comment on lines 409 to 417
{
name: "julia generating CycloneDX SBOM",
args: args{
command: "rootfs",
format: "cyclonedx",
input: "testdata/fixtures/repo/julia",
},
golden: "testdata/julia-cyclonedx.json.golden",
},
Copy link
Collaborator

@knqyf263 knqyf263 May 14, 2024

Choose a reason for hiding this comment

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

We introduced the intermediate representation for SBOM. We don't need tests for SPDX and CycloneDX unless Julia needs any specific handling for SBOM. Either of them is fine.

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 specific handling is needed. I have removed the CycloneDX test.

@knqyf263 knqyf263 added this pull request to the merge queue May 15, 2024
Merged via the queue into aquasecurity:main with commit fecafb1 May 15, 2024
17 checks passed
@Octogonapus Octogonapus deleted the analyzer_language_julia branch May 15, 2024 13:30
@knqyf263 knqyf263 mentioned this pull request May 30, 2024
6 tasks
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 Julia language support for SBOM
4 participants