Refactor alert_processor proc to be based on the ECS correlation logic

[sfc-gh-nlele]

This PR rewrites the alert processor to have similar correlation logic to the ECS-based alert processor.

Testing:
An alert in results.alerts before correlation:

Alert processor's result on the above alert:

Resulting correlated alert:

---

Similar alert which should receive the same correlation ID as the above alert, before correlation:

Resulting correlated alert:

--

The changes have been retested.

[sfc-gh-pkommini]

@sfc-gh-nlele Is there an "after correlation" alert screenshot? Like the "before correlation" alert shot?

[sfc-gh-nlele]

@sfc-gh-pkommini Was just adding it, added it to the description.

[sfc-gh-afedorov]

looks mostly good, but let me sleep on it and give it another go tomorrow

[sfc-gh-afedorov]

did you mean action?

(caught by GPT-4!)

[sfc-gh-nlele]

Sorry yes.

(caught by GPT-4!)

Wow 😮

[sfc-gh-afedorov]

remove else if the first if always returns so that it's more clearly an early bail condition on the rest of the function

[sfc-gh-nlele]

Ah right makes sense 👍

[sfc-gh-afedorov]

I'm not sure if I follow when this would throw an exception -- could you make it more clear, or remove the exception and just do match = exec(...)[0] which would be either the first row object or undefined?

[sfc-gh-nlele]

This was to try and catch any unexpected errors here, in order to match the ECS version in terms of exception handling. Will just get the first row 👍

[sfc-gh-afedorov]

sorry for delay on this, I think this should be the last of it but give me one more chance to re-review after changes

[sfc-gh-afedorov]

no need to repeat the key in JS instead of {x: x} you can just do {x}

[sfc-gh-afedorov]

did you determine whether this return value is something that can be recorded? if not, let's remove this as it's misleading. if so, what value do you expect to be returned here?

[sfc-gh-nlele]

It doesn't seem like we can store the results, without an insert statement running in the proc, to record these values.

However, the return value here is something like below right now, we could maybe pair these with alert_id for more context.

But I think since the individual call statement results will still be available for 24 hours in the query history, it might not hurt to return these?

{
  "alert_correlation_result": [
     {
        "number of multi-joined rows updated": 0,
        "number of rows updated": 1
     },
     {
        "number of multi-joined rows updated": 0,
        "number of rows updated": 1
    },
    {
        "number of multi-joined rows updated": 0,
        "number of rows updated": 1
   }
  ]
}

[sfc-gh-afedorov]

another common pattern here is to use the boolean || and && as short circuit operators, i.e.

match = exec(GET_CORRELATED_ALERT, [actor, object, action, time])[0] || {}
return match['CORRELATION_ID'] || generate_uuid()

[sfc-gh-afedorov]

why select * if all you use is the CORRELATED_ID?

[sfc-gh-nlele]

Had kept the SQL to be same as the original, but makes sense, will just select the correlation_id.

[sfc-gh-afedorov]

since this queries the db, let's maybe call it find_related_correlation_id or something else that hints the database is going to be selected?

[sfc-gh-afedorov]

this naming is a little confusing since in other places they are constant strings. since it's only used once, could you move this into the for loop directly instead of naming it?

[sfc-gh-afedorov]

use row instead of x

[sfc-gh-afedorov]

would it be possible to call UUID_STRING() here, instead, in something like COLLATE(?, UUID_STRING())

[sfc-gh-nlele]

Did you mean COALESCE instead? Would we want it to serve as a fail-safe, and not remove generate_uuid() from find_related_correlation_id?

[sfc-gh-afedorov]

lgtm

[sfc-gh-afedorov]

I think you can drop this separate call or replace it with null now that you have the piece with COALESCE below?

[sfc-gh-nlele]

Makes sense, kept a null here, since if the return is undefined because of match['CORRELATION_ID'], it can't be bound with a statement.

[sfc-gh-afedorov]

I think you can return null now that you have the piece with COALESCE below?

[sfc-gh-nlele]

Retested that the changes work.