Fix/use go mod for go bom generation #42

sullivtr · 2021-03-02T18:33:03Z

Description

Golang BOM generation should use go.mod for dependency analysis instead of using go.sum

Changes

Refactor createGoBom to parse dependencies from go.mod, and use go.sum to gather checksum data for each dependency listed in go.mod
Generated BOM no longer includes all components gathered from go.sum, but only includes components gathered from go.mod

Checklist

Unit tests
Documentation

References

https://github.com/golang/go/wiki/Modules#is-gosum-a-lock-file-why-does-gosum-include-information-for-module-versions-i-am-no-longer-using

No, go.sum is not a lock file. The go.mod files in a build provide enough information for 100% reproducible builds.

…m, but gathers checksum data from go.sum

sullivtr · 2021-03-02T19:34:47Z

I just found one more edge case to handle when parsing the go.mod, so Ill close this for now and reopen once I have this fixed

prabhu · 2021-03-02T20:36:49Z

Hi @sullivtr Thank you for this significant contribution! If I understand correctly go.mod would have direct dependencies for a reproducible build where as go.sum would have all packages including direct and indirect dependencies right?

From a bill of materials perspective we can include all dependencies both direct and indirect but set the scope attribute to required for direct dependencies and optional for indirect ones. This way we would not break any existing go users who are looking for all the packages in the BoM file.

Would you be able to implement this change?

sullivtr · 2021-03-02T22:03:37Z

Hi @sullivtr Thank you for this significant contribution! If I understand correctly go.mod would have direct dependencies for a reproducible build where as go.sum would have all packages including direct and indirect dependencies right?

From a bill of materials perspective we can include all dependencies both direct and indirect but set the scope attribute to required for direct dependencies and optional for indirect ones. This way we would not break any existing go users who are looking for all the packages in the BoM file.

Would you be able to implement this change?

Hi @prabhu, thank you for the quick response on this.

There are a couple of reasons the go.sum should not be the primary source of truth when building the bill of materials.
(The reference material I linked above provides more detail). Ultimately, it is possible, and very common, for a go.sum file to contain checksums for modules that are no longer in use.

In part because go.sum is not a lock file, it retains cryptographic checksums for module versions even after you stop using a module or particular module version. This allows validation of the checksums if you later resume using something, which provides additional safety.

go.mod does in fact contain indirect dependencies which are typically added automatically during a go mod tidy or at build time. With that said, the BOM generation should use go.mod as its source of truth for dependency analysis, as go.sum does is not an accurate representation of transitive dependencies of a go project.

prabhu · 2021-03-03T09:12:35Z

Thank you for the detailed clarification @sullivtr

What we need is a way to retain both the old and new modes for go since we are more likely to return less results for existing users. Also noticed that this PR is changing the package lock file format. Let me take this PR to a new branch and add some flags to support both the invocation modes along with a clear warning that the old mode might be returning legacy package versions that are not in use.

Once this is done I will share the PR with you for comments.

sullivtr · 2021-03-03T16:09:21Z

@prabhu this should be fine. I think as long as we have the ability to generate with go.mod as the source of truth, it certainly does not hurt to have an option to generate based on go.sum if one so desires. I am happy to help in the effort to make this happen.
In the meantime, I'll take a quick stab at adding a --gosum flag to generate golang BOMs using the current behavior.

…urce of truth for dependencies.

sullivtr · 2021-03-03T17:21:44Z

@prabhu So I just went and added the flag (see my latest commit). The output looks like this when passing the --gosum flag:

Let me know if this works for you 😄

prabhu · 2021-03-03T17:55:45Z

oh nice, love the warning! Instead of cli argument can we use env variable USE_GOSUM. The reason is that while invoking this tool via slscan it is a bit easier to pass env variables without having to change the default commands.

Last change is to revert package-lock.json change and submit it as a separate PR.

…ion mode.

sullivtr · 2021-03-03T18:34:00Z

@prabhu done on a bun. I now added the USE_GOSUM env variable. I also reverted the changes on package lock json

prabhu · 2021-03-03T18:51:24Z

utils.js

+        group = name;
+      }
+      const version = tmpA[1];
+      await getGoPkgComponent(gosumData, group, name, version)


This seems to be an expensive operation since it is iterating all the lines in the text and constructing the string look to determine the hash. Is it possible to build a dict of key and hash as the value once and refer it here directly?

prabhu · 2021-03-03T18:51:44Z

utils.js

+        group = name;
+      }
+      const version = tmpA[3]
+      await getGoPkgComponent(gosumData, group, name, version)


Same comment as above

prabhu · 2021-03-03T18:53:13Z

index.js

+      if (DEBUG_MODE) {
+        console.log(`Parsing ${f}`);
+      }
+      goSumReader.push(fs.readFileSync(f, { encoding: "utf-8" }));


Instead of holding the raw text perhaps here we build up an object with key being the full name and value containing the hash. This can be passed to parseGoModData directly.

prabhu · 2021-03-03T18:54:08Z

I was about to click merge and then noticed few more issues. Any thoughts on the refactoring suggested?

sullivtr · 2021-03-03T18:56:24Z

I was about to click merge and then noticed few more issues. Any thoughts on the refactoring suggested?

I was on the fence about optimizing this since its not something typically ran in a performance dependent state (as far as I know). However, I can refactor for the optimizations.

…er lookup, instead of n * x iteration

sullivtr · 2021-03-03T20:10:35Z

@prabhu Just pushed some changes to use a dict for faster comparison with go.sum for hash. I did not benchmark the changes or anything, but theoretically, it should be much faster on larger datasets.

prabhu · 2021-03-03T20:28:47Z

This is looking a lot better and is quite fast too! I just started testing with some of the sample go apps and got the below error for one of them. Happy to still merge this but make a release after a bit more testing actually. Or can wait if you can take a look at what is going on.

bin/cdxgen ~/go/opa -o /tmp/bom.json
Parsing /Users/prabhu/go/opa/go.sum
(node:71610) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'replace' of undefined
    at createGoBom (/Volumes/Work/cdxgen/index.js:1063:33)
    at createXBom (/Volumes/Work/cdxgen/index.js:1503:18)

Problematic file

go.sum.txt
go.mod.txt

sullivtr · 2021-03-03T20:44:04Z

No worries. Ill look into this today, and see if I can repro

sullivtr · 2021-03-03T20:45:26Z

@prabhu were you running this with USE_GOSUM enabled? Or do you also have a go.mod file I can use alongside the go.sum you attached?

EDIT: NVM I was able to repo. Looking into this now

sullivtr · 2021-03-03T20:51:07Z

Okay, so I found the issue (whitespace/empty line at the end of the sum file). I just need to ensure there is a value before I try to process it. I will have this fix in before the end of the day 😄

sullivtr · 2021-03-03T20:54:40Z

Can you try now, @prabhu?

prabhu · 2021-03-03T21:05:15Z

index.js

+      checkSums = data.split("\n")
+      for (let d in checkSums) {
+        const l = checkSums[d];
+        if (l.length > 0) {


Nice one! Not a biggie that this might not be effective if the line is empty but with a space character.

Yeah, in this case the go.sum should always be generated, and the /linendings are likely dependent on the machine the go.sum was generated on. In theory a go.sum shouldn't ever contain an empty line with added whitespace

prabhu · 2021-03-03T21:06:08Z

Thanks a ton @sullivtr ! Will do a bit more testing by setting FETCH_LICENSE=true and make a release tonight.

sullivtr added 4 commits March 2, 2021 09:34

fix: go bom generation uses go.mod for dep analysis, instead of go.su…

e3aba38

…m, but gathers checksum data from go.sum

chore: code cleanup, and commenting

b7feabb

chore: linting fixes

0665a3c

docs: update docs to reflect changes:

3563db6

sullivtr closed this Mar 2, 2021

fix: handle parse of inline replace statements in go.mod

177ff97

sullivtr reopened this Mar 2, 2021

sullivtr added 2 commits March 3, 2021 08:21

fix: update createXBom to look for go.mod

9a536f2

feat: add --gosum flag to allow generation of BOMs using go.sum as so…

b004c92

…urce of truth for dependencies.

sullivtr added 6 commits March 3, 2021 09:56

chore: recomment long running test

9b5259c

fix: use env variable instead of flag for changing golang BOM generat…

d9502e1

…ion mode.

fix: remove unused legacyGo option

91aaad0

chore: re-add comma to fix diff

3407aea

chore: revert package.lock.json

cb31e29

chore: revert package.lock.json in test proj

e23edd8

prabhu reviewed Mar 3, 2021

View reviewed changes

refactor: optimize the parseGoModData. Use map for gosumData for fast…

3a56cf5

…er lookup, instead of n * x iteration

fix: ignore empty lines in sum file

6aeba1d

prabhu reviewed Mar 3, 2021

View reviewed changes

prabhu approved these changes Mar 3, 2021

View reviewed changes

prabhu merged commit 47e738f into CycloneDX:master Mar 3, 2021

sullivtr deleted the fix/use-go-mod-for-go-bom-generation branch March 3, 2021 22:06

dlorenc mentioned this pull request Jun 7, 2021

Golang parsing incorrect #62

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/use go mod for go bom generation #42

Fix/use go mod for go bom generation #42

sullivtr commented Mar 2, 2021

sullivtr commented Mar 2, 2021

prabhu commented Mar 2, 2021

sullivtr commented Mar 2, 2021 •

edited

Loading

prabhu commented Mar 3, 2021

sullivtr commented Mar 3, 2021 •

edited

Loading

sullivtr commented Mar 3, 2021 •

edited

Loading

prabhu commented Mar 3, 2021

sullivtr commented Mar 3, 2021 •

edited

Loading

prabhu Mar 3, 2021

prabhu Mar 3, 2021

prabhu Mar 3, 2021

prabhu commented Mar 3, 2021

sullivtr commented Mar 3, 2021

sullivtr commented Mar 3, 2021

prabhu commented Mar 3, 2021 •

edited

Loading

sullivtr commented Mar 3, 2021

sullivtr commented Mar 3, 2021 •

edited

Loading

sullivtr commented Mar 3, 2021

sullivtr commented Mar 3, 2021

prabhu Mar 3, 2021

sullivtr Mar 3, 2021 •

edited

Loading

prabhu commented Mar 3, 2021

Fix/use go mod for go bom generation #42

Fix/use go mod for go bom generation #42

Conversation

sullivtr commented Mar 2, 2021

Description

Changes

Checklist

References

sullivtr commented Mar 2, 2021

prabhu commented Mar 2, 2021

sullivtr commented Mar 2, 2021 • edited Loading

prabhu commented Mar 3, 2021

sullivtr commented Mar 3, 2021 • edited Loading

sullivtr commented Mar 3, 2021 • edited Loading

prabhu commented Mar 3, 2021

sullivtr commented Mar 3, 2021 • edited Loading

prabhu Mar 3, 2021

Choose a reason for hiding this comment

prabhu Mar 3, 2021

Choose a reason for hiding this comment

prabhu Mar 3, 2021

Choose a reason for hiding this comment

prabhu commented Mar 3, 2021

sullivtr commented Mar 3, 2021

sullivtr commented Mar 3, 2021

prabhu commented Mar 3, 2021 • edited Loading

sullivtr commented Mar 3, 2021

sullivtr commented Mar 3, 2021 • edited Loading

sullivtr commented Mar 3, 2021

sullivtr commented Mar 3, 2021

prabhu Mar 3, 2021

Choose a reason for hiding this comment

sullivtr Mar 3, 2021 • edited Loading

Choose a reason for hiding this comment

prabhu commented Mar 3, 2021

sullivtr commented Mar 2, 2021 •

edited

Loading

sullivtr commented Mar 3, 2021 •

edited

Loading

sullivtr commented Mar 3, 2021 •

edited

Loading

sullivtr commented Mar 3, 2021 •

edited

Loading

prabhu commented Mar 3, 2021 •

edited

Loading

sullivtr commented Mar 3, 2021 •

edited

Loading

sullivtr Mar 3, 2021 •

edited

Loading