Adding Analyze command to guacone #1809

arorasoham9 · 2024-04-04T08:54:55Z

Description of the PR

Analyze command for guacone

Comparing largely similar SBOMs
guacone analyze diff --uri --sboms=https://anchore.com/syft/image/k8s.gcr.io/kube-apiserver-v1.24.1-583a02ce-8f7e-4794-91af-35f27ffeb73d,https://anchore.com/syft/image/k8s.gcr.io/kube-apiserver-v1.24.2-ee7e0a81-87de-4761-9689-4f7162d81e44

The diff is a 4 step process:

Create graph for SBOMs
Find all paths in both graphs
Find the paths that are present in one graph and not in the other, do this for both graphs.
Pair paths from one graph to the other that are closest, then find the difference between these pairs.

Each step has tests to ensure that it is stable. Meaning, if the steps are repeated for the same Input SBOM, the results are the same. The JSON you see are the test SBOMS I use to carry out the tests for each step of the diff. These JSON files are marshalled HasSBOM nodes for SBOMs I ingested into GUAC, pulled using the Node function.

PR Checklist

All commits have a Developer Certificate of Origin (DCO) -- they are generated using -s flag to git commit.
All new changes are covered by tests
If GraphQL schema is changed, make generate has been run
If OpenAPI spec is changed, make generate has been run
If collectsub protobuf has been changed, make proto has been run
All CI checks are passing (tests and formatting)
All dependent PRs have already been merged

arorasoham9 · 2024-04-04T18:16:00Z

@pxp928 ready for review.

pxp928 · 2024-04-04T18:19:45Z

Cool, @arorasoham9 can you add some screenshots to what the output will look like for the CLI?

pxp928 · 2024-04-04T18:21:58Z

pkg/analyzer/analyze.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package guacanalyze


one major task would be to add unit tests to this package. Some things like the graph, getting data from graphQL can be abstracted away but do you have some tests you can add here to test the functionality?

I don't have any at the moment. I can write a few. Could they come in a future PR?

Added unit tests as requested.

arorasoham9 · 2024-04-04T19:00:47Z

Cool, @arorasoham9 can you add some screenshots to what the output will look like for the CLI?

Added some screenshots. The Prettify function errored out at many instances so I improvised to print the same details that function does but taken from the graph DS itself, not graphQL.

pxp928 · 2024-04-05T19:37:26Z

pkg/analyzer/analyze.go

+    node *Node
+  )
+  if node, err = g.Vertex(ID); err !=  nil {
+    fmt.Println("Error setting node attribute", err)


need to return this as an error.

pxp928 · 2024-04-05T19:37:52Z

pkg/analyzer/analyze.go

+  )
+  if node, err = g.Vertex(ID); err !=  nil {
+    fmt.Println("Error setting node attribute", err)
+    os.Exit(1)


don't use os.Exit(1) should return error

pxp928 · 2024-04-05T19:38:36Z

pkg/analyzer/analyze.go

+
+func GetNodeAttribute(g graph.Graph[string, *Node],ID, key string) interface{} {
+  var (
+    err error


no need to instantiate this here, can just it on line 72?

pxp928 · 2024-04-05T19:39:10Z

pkg/analyzer/analyze.go

+  node.Attributes[key] = value
+}
+
+func GetNodeAttribute(g graph.Graph[string, *Node],ID, key string) interface{} {


why is this returning an interface{}

pxp928 · 2024-04-05T19:41:10Z

pkg/analyzer/analyze.go

+    foundHasSBOMPkg, err = model.HasSBOMs(ctx, gqlclient, model.HasSBOMSpec{Subject: &model.PackageOrArtifactSpec{Package: &model.PkgSpec{Id: &pkgResponse.Packages[0].Namespaces[0].Names[0].Versions[0].Id}},
+      })
+    if err != nil {
+      fmt.Printf("(purl)failed getting hasSBOM with error :%v", err)


dont use fmt.Printf, you should return an error via fmt.Errorf

pxp928 · 2024-04-05T19:41:32Z

pkg/analyzer/analyze.go

+func verifyAnalyzeFlags(slsas, sboms []string,  errSlsa, errSbom error, uri, purl, id bool) {
+
+  if (errSlsa != nil && errSbom != nil) || (len(slsas) ==0  && len(sboms) == 0 ){
+    fmt.Println("Must specify slsa or sboms ")


return a proper error and dont use os.Exit

pxp928 · 2024-04-05T19:43:01Z

pkg/analyzer/analyze.go

+      namespaceTwo, okTwo := GetNodeAttribute(analysisGraph,diffList.MissingAddedRemovedLinks[i][1], "Namespace[0]").(string)
+
+      if !okOne || !okTwo {
+        fmt.Println("Error getting node namespace attribute")


clean up and pass out proper error

pxp928 · 2024-04-05T19:44:39Z

pkg/analyzer/analyze.go

+  table.SetAlignment(tablewriter.ALIGN_LEFT)
+  table.Render()
+  if (!all && max > maxprint){
+    fmt.Println("Run with --all to see full list")


should user logger here: https://github.com/guacsec/guac/blob/main/pkg/handler/processor/process/process.go#L117

It should be able to be pulled from the context:

logger := logging.FromContext(ctx)

pxp928 · 2024-04-05T19:45:42Z

pkg/analyzer/analyze.go

+}
+
+func HasSBOMToGraph(cmd *cobra.Command, ctx context.Context, gqlclient graphql.Client) ( []graph.Graph[string, *Node]){
+  slsas, errSlsa := cmd.Flags().GetStringSlice("slsa")


pass in options or something similar and not the actual cmd. See: https://github.com/guacsec/guac/blob/main/cmd/guacone/cmd/s3.go#L80-L93

pxp928 · 2024-04-05T19:46:02Z

pkg/analyzer/analyze.go

+  if uri {
+    hasSBOMResponseOne, err = FindHasSBOMBy(model.HasSBOMSpec{} ,sboms[0],"", "", ctx, gqlclient)
+    if err != nil {
+      fmt.Println("(uri)failed to lookup sbom:", sboms[0], err)


pxp928 · 2024-04-05T19:49:23Z

pkg/analyzer/analyze_test.go

@@ -0,0 +1,66 @@
+package guacanalyze_test


missing header

jeffmendoza

Mostly code organization and naming comments.

jeffmendoza · 2024-04-05T19:37:18Z

cmd/guacone/cmd/analyze.go

+  "github.com/guacsec/guac/pkg/logging"
+  "github.com/spf13/cobra"
+  "github.com/spf13/viper"
+  analysis "github.com/guacsec/guac/pkg/analyzer"


Any reason for the import rename here?

jeffmendoza · 2024-04-05T19:38:11Z

cmd/guacone/cmd/analyze.go

+    if diff && intersect && union || diff && intersect || diff && union || intersect && union {
+      fmt.Println("Must specify only one of --diff, --intersect, --union")
+      return
+    }


This should be a positional argument if required, not a flag/option.

jeffmendoza · 2024-04-05T19:39:10Z

cmd/guacone/cmd/analyze.go

+    }
+
+    //create graphs
+    graphs := analysis.HasSBOMToGraph(cmd, ctx, gqlclient)


Don't pass cmd to a package under pkg/. Those shouldn't have a dependency on cli commands or cobra. Explicitly pass the parameters this needs.

jeffmendoza · 2024-04-05T19:40:52Z

cmd/guacone/cmd/analyze.go

+
+func init() {
+
+  rootCmd.PersistentFlags().Bool("intersect", false, "compute intesection of given sboms")


Don't add flags to rootCmd, this will show up on all guacone cmds if so. Use this analyzeCmd you are creating.
Also, cli options are normally under pkg/cli/store.go but this command has so many it might make sense to just leave them here.

jeffmendoza · 2024-04-05T19:41:19Z

cmd/guacone/cmd/analyze.go

+  rootCmd.PersistentFlags().Bool("purl", false, "input is a pURL")
+  rootCmd.PersistentFlags().Bool("id", false, "input is an Id")
+  rootCmd.PersistentFlags().Bool("metadata", false, "Compare SBOM metadata")
+  rootCmd.PersistentFlags().Bool("inclSoft", false, "Compare Included Softwares")


change to incl-soft and similar for others. All cli options use hyphen-case.

jeffmendoza · 2024-04-05T19:42:31Z

cmd/guacone/cmd/analyze.go

+  rootCmd.PersistentFlags().Int("maxprint", PRINT_MAX, "max number of similar sboms to print")
+  rootCmd.AddCommand(analyzeCmd)
+
+}


Add a newline to the end of the file

jeffmendoza · 2024-04-05T19:44:29Z

pkg/analyzer/analyze.go

+  )
+  if node, err = g.Vertex(ID); err !=  nil {
+    fmt.Println("Error setting node attribute", err)
+    os.Exit(1)


Shouldn't have any os.Exit in a package under pkg/, assume this is a library that can be called from anywhere. Propagate errors back to the caller.

jeffmendoza · 2024-04-05T19:45:36Z

pkg/analyzer/analyze.go

+  return foundHasSBOMPkg, nil
+}
+
+func verifyAnalyzeFlags(slsas, sboms []string,  errSlsa, errSbom error, uri, purl, id bool) {


All this should be under the file in cmd/, shouldn't have any flag validation here in pkg/.

jeffmendoza · 2024-04-05T19:46:15Z

pkg/analyzer/analyze.go

+  //print to stdout
+  printHighlightedAnalysis(dot, diffList, all, maxprint, action, analysisGraph )
+}
+func max(nums []int) int {


add an empty line between functions.

jeffmendoza · 2024-04-05T19:50:01Z

pkg/analyzer/analyze.go

+  //Create dot file
+  createGraphDotFile(dot, analysisGraph)
+  //print to stdout
+  printHighlightedAnalysis(dot, diffList, all, maxprint, action, analysisGraph )


Anything under pkg/ should not print anything to stdout. Any logic should result in a datastructure that returns back to cmd/ and is then printed. This helps for testability. Looks like much of this may need to be moved back to cmd/

pxp928 · 2024-04-18T12:06:56Z

@arorasoham9 did the node functionality get fixed?

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

arorasoham9 · 2024-05-21T03:32:36Z

@pxp928 Ready for review. Please let me know what changes you require to the output presentation.

nathannaveen

Hey, @arorasoham9, I have a suggestion. Could you move the test data to https://github.com/guacsec/guac-test?

There are over 1,000,000 lines of code in this PR, making it virtually impossible to review, and without adequate reviews, security bugs could be introduced into the code.

If the test files were to be moved into guac-test, then the majority of this code wouldn’t be in this PR so that we can properly review it.

If the test files were moved to guac-test, this code would not be built and intern would not affect the end user.

I am not entirely sure whether the JSON files are part of executable code. Even if they aren't right now I am not sure whether we will in the future.

While JSON files are typically safe, there's a potential risk that they could contain encoded scripts, which might inadvertently introduce security vulnerabilities.

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

arorasoham9 · 2024-05-24T10:21:21Z

@nathannaveen Done! I have moved the HasSBOM JSON test files for gaucanalyze to guac-test. I have also added some explanation for having these JSON files to the description of this PR.

I will require this PR merged before I can send a final commit to reflect the code changes to accept the JSON test from guac-test.

pxp928 · 2024-05-24T16:02:11Z

@arorasoham9 any luck in improving the output? It is very hard to understand. Like if you can show less information and just show only what is relevant, that would be better.

arorasoham9 · 2024-05-24T16:25:06Z

@pxp928 I am still thinking of ways to show only the relevant information. This is a better explanation for why you see so much even with just a small difference between two sboms. The diff works at the granularity of a path, not a node, so when it finds even one node different it classifies all paths with that participating node as different (and prints them). I believe the original aim was to find missing/different paths bw two sboms was to also offer a graph "structural" difference as well. I am trying to think how we can still offer the same differences in an easier to understand manner.

lumjjb

Thanks @arorasoham9 for working on this big feature!! definitely going to be very useful, would you be able to help break this up into a few separate PRs, that would be very helpful in getting this merged in faster!

maybe start with the analyzer package, and then add a table formatter pkg, and then the CLI options and the CLI itself?

lumjjb · 2024-05-24T20:37:20Z

cmd/guacone/cmd/analyze.go

+		uri, _ := cmd.Flags().GetBool("uri")
+		purl, _ := cmd.Flags().GetBool("purl")
+
+		metadata, _ := cmd.Flags().GetBool("metadata")


these flags should be initialized in https://github.com/guacsec/guac/blob/main/pkg/cli/store.go, and preferably prefixed in a way that shows its for guacanalyze

lumjjb · 2024-05-24T20:38:14Z

cmd/guacone/cmd/analyze.go

+
+		if test == "" {
+			if err = verifyAnalyzeFlags(slsas, sboms, errSlsa, errSbom, uri, purl, id); err != nil {
+				fmt.Fprintf(os.Stderr, "Error: %s", err)


nit: golang errors style guide is lowercase first characters.

lumjjb · 2024-05-24T20:39:02Z

cmd/guacone/cmd/analyze.go

+
+			//create graphs
+			graphs, err = hasSBOMToGraph(ctx, gqlclient, sboms, AnalyzeOpts{
+				Metadata: metadata, InclSoft: inclSoft, InclDeps: inclDeps, InclOccur: inclOccur, Namespaces: namespaces, URI: uri, PURL: purl, ID: id})


make it into different lines for readability.

lumjjb · 2024-05-24T20:39:51Z

cmd/guacone/cmd/analyze.go

+				os.Exit(1)
+			}
+
+			//create graphs


nit: the comment doesn't add much context to the code, either remove or expand.

lumjjb · 2024-05-24T20:40:33Z

cmd/guacone/cmd/analyze.go

+			}
+		}
+
+		if args[0] == "diff" {


would it make sense for these to be subcommands? otherwise this should be a flag maybe?

lumjjb · 2024-05-24T20:52:26Z

pkg/analyzer/analyzer.go

+	return n.ID
+}
+
+func SetNodeAttribute(g graph.Graph[string, *Node], ID, key string, value interface{}) bool {


can you provide additional context and documentation around the functions? it is not clear what should be in value, unless there's a good reason, i would highly recommend not having interface{} as the type of value. If anything it can be a hidden type that can be minted by the analyzer but must provide sufficient constructors to make sense of it.

lumjjb · 2024-05-24T20:52:45Z

pkg/analyzer/analyzer.go

+
+	switch action {
+	//0 is diff
+	case 0:


use an enum :)

lumjjb · 2024-05-24T20:55:13Z

pkg/analyzer/analyzer.go

+	return hex.EncodeToString(hash[:])
+}
+
+func AddGraphNode(g graph.Graph[string, *Node], _ID, color string) {


what is color used for? Is it the identity of an edge of just for analysis purpose? can it be passed it as an option on the node for extensibility?

_ID should be id to follow golang style?

lumjjb · 2024-05-24T20:56:11Z

cmd/guacone/cmd/analyze.go

+	},
+}
+
+func printDiffedPathTable(diffs []analyzer.DiffedPath) error {


would be helpful to move all printing stuff to either a different package or a different file

lumjjb · 2024-05-24T20:56:43Z

cmd/guacone/cmd/analyze.go

+	return nil
+}
+
+func readTwoSBOM(filename string) ([]graph.Graph[string, *analyzer.Node], error) {


why read two sboms from a file? i don't think that's a common pattern?

arorasoham9 requested a review from jeffmendoza as a code owner April 4, 2024 08:54

pull-request-size bot added the size/XL label Apr 4, 2024

arorasoham9 force-pushed the guacdiff branch from 383ff7d to 853c095 Compare April 4, 2024 17:59

pxp928 reviewed Apr 4, 2024

View reviewed changes

pxp928 reviewed Apr 5, 2024

View reviewed changes

pkg/analyzer/analyze_test.go Outdated

@@ -0,0 +1,66 @@

package guacanalyze_test

Copy link

Collaborator

pxp928 Apr 5, 2024

Choose a reason for hiding this comment

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

missing header

jeffmendoza reviewed Apr 5, 2024

View reviewed changes

pull-request-size bot added size/XXL size/XL and removed size/XL size/XXL labels Apr 10, 2024

arorasoham9 force-pushed the guacdiff branch from ccb7778 to 93255cc Compare April 29, 2024 14:28

pull-request-size bot added size/XXL and removed size/XL labels Apr 29, 2024

arorasoham9 force-pushed the guacdiff branch from 63a3593 to 3ac17a0 Compare May 17, 2024 05:21

arorasoham9 requested a review from mihaimaruseac as a code owner May 17, 2024 05:21

arorasoham9 added 10 commits May 17, 2024 11:02

adding files

70a37cd

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

fixing mod sum

0b56fe0

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

lint fixes

62f2fa5

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

lint fixes

a36daed

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

mod fixes

9689179

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

fix cli name

0bc691b

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

changes to work with test cases

d7f7503

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

adding tests and more fixes in analyze.go to support

728dfd7

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

fix for highlight analysis

096894c

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

changes requested

9a0c88e

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

arorasoham9 added 8 commits May 17, 2024 11:02

redundant check

16b7060

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

test

f527d22

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

major changes

3570aac

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

fixes to analyze command

f732755

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

fixes to analyze command

2a68d97

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

mod sum changes

dc55196

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

fixes to output

4624aa9

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

analyzer changes

0337623

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

arorasoham9 force-pushed the guacdiff branch from 3ac17a0 to 0337623 Compare May 17, 2024 05:32

arorasoham9 added 8 commits May 17, 2024 11:52

fixing test

384befb

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

removing unncessary structs and fixing filename

11f1c03

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

fix graph test case

f6e9980

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

fix graph paths test case

6d6d8cb

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

fix graph paths test case

c1fb873

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

fix graph paths test case

9a9d8cc

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

clubbed test cases

744dbe8

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

int overflow fix w / tilt

806a4bf

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

nathannaveen reviewed May 22, 2024

View reviewed changes

arorasoham9 and others added 4 commits May 23, 2024 19:02

Merge branch 'guacsec:main' into guacdiff

0675da5

removing unncessary files

a2bafee

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

removing unncessary files

947adc8

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

removing test JSON file

8949075

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

arorasoham9 mentioned this pull request May 24, 2024

uploading hasSBOMs for guacdiff tests in guacsec/guac guacsec/guac-test#1

Merged

Merge branch 'guacsec:main' into guacdiff

7682412

lumjjb requested changes May 24, 2024

View reviewed changes

Merge branch 'guacsec:main' into guacdiff

57c1b2c


		func init() {

		rootCmd.PersistentFlags().Bool("intersect", false, "compute intesection of given sboms")

Adding Analyze command to guacone #1809

Are you sure you want to change the base?

Adding Analyze command to guacone #1809

Conversation

arorasoham9 commented Apr 4, 2024 • edited

Description of the PR

PR Checklist

arorasoham9 commented Apr 4, 2024

pxp928 commented Apr 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arorasoham9 commented Apr 4, 2024 • edited

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

jeffmendoza 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

pxp928 commented Apr 18, 2024

arorasoham9 commented May 21, 2024

nathannaveen left a comment

Choose a reason for hiding this comment

arorasoham9 commented May 24, 2024

pxp928 commented May 24, 2024 • edited

arorasoham9 commented May 24, 2024 • edited

lumjjb 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

arorasoham9 commented Apr 4, 2024 •

edited

arorasoham9 commented Apr 4, 2024 •

edited

pxp928 commented May 24, 2024 •

edited

arorasoham9 commented May 24, 2024 •

edited