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

Refactor vulndb events #41

Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open

Refactor vulndb events #41

wants to merge 29 commits into from

Conversation

ka3de
Copy link
Contributor

@ka3de ka3de commented Nov 15, 2022

This PR refactors the current "finding notifications" implementation.
Up until now only notifications for "detected findings" (open status) where sent. This PR introduces a new format for the finding notifications/events which will represent every finding state change:

  • New finding
  • New detection of an already open finding (might update finding details, resources, exposure values...)
  • Non detection of a currently OPEN finding (generates a FIX event)
    ...therefore, not only OPEN findings will be notified.

This PR is a step forward in deprecating the old notifications format and mechanisms in favor of a more complete one that will rely on Kafka as events bus.

Still, the former implementation has to be kept running concurrently by now, until the implementation of the components that rely in it can be refactored to comply with the new method and format. Taking this into account, this PR tries to isolate as much as possible the former implementation encapsulating its data structures and transformations necessary [from the new format] in the SNS "connector" implementation, so it is easy to deprecate it in the future. TODO comments have been left intentionally in the code explaining this situations and how to approach these parts in the future when deprecating it.

It's also important to consider that we are adding some overhead that might impact the service, particularly we should be very conscious about introducing new DB queries for a component that is already quite "DB intensive". In the case of this implementation we are required to add one new DB query in order to retrieve the details of every finding which state is altered after a check processing.
This query is already being used in the API and should add minimal overhead, see a graphical explanation of the query using Pev tool which shows that joins are executed based on indexed fields and aggregations cost is low as they are only applied to the resulting elements. The costliest part is the one that performs the matching with the finding_events and orders them, but that should be reasonable too.
Screenshot from 2022-11-30 16-36-31

@ka3de ka3de marked this pull request as ready for review November 30, 2022 13:28
@@ -0,0 +1,53 @@
package main
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add the Adevinta copyright notice at the beginning of this file.

)

var (
// ErrEmptyPayload is returned by the Push method of the [Client] when the
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know exactly why, but references in variables documentation, like [Client], are not being resolved by the godoc tool. That is, instead of rendering them as links, they are rendered straight with the brackets.

for _, fs := range findingsState {
fIDs = append(fIDs, fs.ID)
}
findings, err := p.store.GetFindingsExpanded(fIDs)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think here we have one race condition, concretely when we are gathering the expanded findings, the status of each finding could be different to the one that the finding has when the source that caused a modification in this finding was processed, it is true that, because how we are processing the checks, the new state can not be older that the one set after we processed this check, so the only effect could be to skip informing of an "old" status of a finding. For instance if the status changes of finding until a time is: Open, Fixed, Open, Fixed, we could potentially send to Kafka a history like this: Open, Fixed,Fixed. This could be acceptable but we need to be aware of it, so the consumers of the Kafka topic know the guarantees that the VulnerabilityDB provides in this async API.


// Notify finding state update
for _, f := range findings {
err = p.notifier.PushFinding(notify.FindingNotification{
Copy link
Contributor

Choose a reason for hiding this comment

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

Here I think we have another possible race condition, in this case it also happens when there is another concurrent goroutine processing other check execution that is updating the same finding. That's because we are not serializing the action of sending the data we just gathered from the database when we called the GetFindingExpanded method above in this function. This race condition could make us to send the history of the states of a given finding in there wrong order, for instance, instead of Open, Open, Fixed we could send Open, Fixed, Fixed which I think is not acceptable.

We could probably fix this problem, and the other race condition, by using advisory locks over a finding and only realize the advisory lock here after the finding state have been sent to Kafka.

@jesusfcr jesusfcr mentioned this pull request Apr 22, 2024
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.

2 participants