Adding business logic for reachability enricher - leveraging atom. #340
Conversation
andream16
force-pushed
the
feature/reachable
branch
from
b8da70b to
c9ef139
Compare
andream16
changed the title
andream16
force-pushed
the
feature/reachable
branch
from
c9ef139 to
6d4bef5
Compare
|
Need to add task. |
|
Need to add README.md |
andream16
force-pushed
the
feature/reachable
branch
from
6d4bef5 to
cd7e7d8
Compare
| return nil | ||
| }) | ||
|
|
||
| if err := g.Wait(); err != nil && !isCtxErr(err) { |
There was a problem hiding this comment.
Gracefully exit when done and handle correctly context cancellations.
| } | ||
| ) | ||
|
|
||
| func TestEnricher(t *testing.T) { |
There was a problem hiding this comment.
This is the main test. Executes everything end to end.
| @@ -0,0 +1,8 @@ | |||
| package conf | |||
There was a problem hiding this comment.
_test files are ignored by go build but they can still be used to access unexported bits in tests. Useful for not polluting the available API but still be able to keep tests consistent.
|
|
||
| // Enrich looks for untagged inputs and processes them outputting if any of them is reachable. | ||
| // The reachability checks leverage atom - https://github.com/AppThreat/atom. | ||
| func (r *enricher) Enrich(ctx context.Context) error { |
There was a problem hiding this comment.
This is the main business logic layer.
| @@ -0,0 +1,92 @@ | |||
| package main | |||
There was a problem hiding this comment.
If you are not familiar with the concept, code in internal cannot be imported by other go modules. The build tool would report an error if this happens - leading to a failing build.
This makes sure to leak to potential user only the API that we actually want to expose. This is a nice Go built in feature
| } | ||
|
|
||
| func NewParser() (*Parser, error) { | ||
| purlPkg, err := regexp.Compile(`(?P<p1>[^/:]+/(?P<p2>[^/]+))(?:(?:.|/)v\d+)?@`) |
There was a problem hiding this comment.
why are we using regexp to parse this and not https://github.com/package-url/packageurl-go?
There was a problem hiding this comment.
Will look into it!
| @@ -0,0 +1,59 @@ | |||
| package conf | |||
There was a problem hiding this comment.
perhaps this should be part of the base enricher?
There was a problem hiding this comment.
What do you mean by that?
| @@ -0,0 +1,97 @@ | |||
| package fs | |||
There was a problem hiding this comment.
perhaps this should also be part of the base enricher. not for this PR but maybe good to be in a ticket for the OSS refactoring project
| return nil, errors.New("invalid empty reachable purls") | ||
| } | ||
|
|
||
| matcherFileLine, err := regexp.Compile(`(?P<file>[^/]+):(?P<line>[\d-]+)`) |
There was a problem hiding this comment.
does atom only have lines?
i thought it had ranges, happy to be wrong if your tests show otherwise :)
There was a problem hiding this comment.
From my tests and my understanding it looks ok but please feel free to test too!
|
|
||
| lineStart, lineEnd, err := s.searchLineRange(fileContent) | ||
| if err != nil { | ||
| return false, fmt.Errorf("failed to parse line range: %w", err) |
There was a problem hiding this comment.
we use go-errors Errorf by convention
|
Closed in favour of #360 (review) |
Code changes to create a new enricher which enriches existing vulnerability reports with atom reachability reports.
This is a refactor from #271.
I tried to decompose the code by responsibility and added godoc. Let me know if some bits are not clear :).
I opted for passing around concrete types instead of abstractions as we are not leveraging mocks and we're not expecting to swap implementations. This can be done later, if needed. Right now this choice helps with making things clearer and easier to find.
Still working on the Task. Will add it later!