[Security Solution][Endpoint Exceptions] Fixes bug where behavior alerts do not show nested code signatures with subject name and trusted field

[parkiino]

Summary

When navigating to the endpoint exceptions form from an alert, we pre-populate certain exceptions fields based on the type of alert. There was a bug for behavior alerts where we did not use the proper nested code_signature field for windows and mac endpoints. Instead of showing the nested code_signature field that has the subject_name and trusted sub-fields, we only showed non-nested `code_signature subject field. This PR also refactors the code to account for the following behaviors that we want:

• If field.Ext.code_signature is present, we want to use the nested code_signature subject field with the subject_name and trusted sub-fields for
• If field.Ext.code_signature is not present, we will default to the non-nested field.code_signature.subject_name and field.code_signature.trusted field pair.
• We will only show non-empty pre-populated values and also only code signature values with the trusted field set to true
• Pre-populated code signature fields are only present in windows and mac OSes.
• Behavior, ransomware and default alerts had the code_signature adjustments
• Previously the code duplicated a set of the pre-populated fields PER code signature. Now, each pre-populated field is only shown once, followed by all valid code_signatures.
• Does not allow duplicate code signatures

SCREENSHOTS

Behavior alert w/ nested process.Ext.code_signature and non-nested dll.code_signature fields

Malware alert w/ nested file.Ext.code_signature

[elasticmachine]

Pinging @elastic/security-defend-workflows (Team:Defend Workflows)

[rylnd]

I gave a quick review on behalf of Detection Engine; I can't really speak to the logic involved in the helpers here (either before or after this change), but the tests are comprehensive and do a good job of documentation.

I did notice some type issues (which it looks like CI caught), so just request another review when this is green 👍 .

P.S. should we maybe create a separate file for endpoint-specific exceptions helpers? That way Detection Engine wouldn't need to co-own this large helpers file.

[rylnd]

Is this return type correct? The explicit object being returned below doesn't seem to conform to this type.

[tomsonpl]

Hey, sorry for coming late with the review, but I was waiting until all cases mentioned by @sukhwindersingh-qasource are fixed. Maybe I should have done more thorough review earlier, sorry!

I have a few issues I've found in this PR that I would suggest you take a look at:

1. Code Duplication

There's significant duplication across the following functions:

getFileCodeSignature
getProcessCodeSignature
getDllCodeSignature

Each function follows the same pattern:

• Check if the entity exists
• Check if it has an Ext property with code_signature
• If yes, call getCodeSignatureValue with the code signature
• Otherwise, check if it has a direct code_signature property with trusted === true
• If yes, return a specific array structure

2. Complex Conditional Checks

Multiple nested checks like dll && dll.Ext && dll.Ext.code_signature or if ('entries' in entry && entry.entries !== undefined) { make the code harder to read and maintain.

3. entriesToAdd

• Nested Conditionals: The function contains multiple nested if-else statements, making it hard to follow the logic.
• Repetitive Code: The pattern of calling addIdToEntries(filterEmptyExceptionEntries(...)) is repeated in each branch.
• Implicit Dependencies: The function relies on variables from the outer scope (isLinux, processCodeSignature, dllCodeSignature, commonFields).

Conclusion

I've put some recommendations closer to the code, however I haven't really run these changes so please take a look, verify and apply if you feel they make sense.
Although there are some places to consider improving, in general I think you did a great job with covering all these crazy edge cases 👍

[tomsonpl]

I believe all the 3 similar functions can be replaced with a single generic function, similar to this one: please verify your all edge cases.

/**
 * Generic function to get code signature entries from any entity
 */
export const getEntityCodeSignature = <T extends { Ext?: { code_signature?: CodeSignature[] | CodeSignature }, code_signature?: CodeSignature }>(
  entity: T | undefined,
  fieldPrefix: string
): EntriesArrayOrUndefined => {
  if (!entity) return undefined;
  
  // Check Ext.code_signature first
  if (entity.Ext?.code_signature) {
    return getCodeSignatureValue(entity.Ext.code_signature, `${fieldPrefix}.Ext.code_signature`);
  }
  
  // Then check direct code_signature
  if (entity.code_signature?.trusted === true) {
    return [
      {
        field: `${fieldPrefix}.code_signature.subject_name`,
        operator: 'included' as const,
        type: 'match' as const,
        value: entity.code_signature?.subject_name ?? '',
      },
      {
        field: `${fieldPrefix}.code_signature.trusted`,
        operator: 'included' as const,
        type: 'match' as const,
        value: entity.code_signature.trusted.toString(),
      },
    ];
  }
  
  return undefined;
};

[tomsonpl]

Then use it like this:

export const getFileCodeSignature = (alertData: Flattened<Ecs>): EntriesArrayOrUndefined => 
  getEntityCodeSignature(alertData.file, 'file');

[tomsonpl]

Use the generic function:

export const getProcessCodeSignature = (alertData: Flattened<Ecs>): EntriesArrayOrUndefined => 
  getEntityCodeSignature(alertData.process, 'process');

[tomsonpl]

Use the generic function:

export const getDllCodeSignature = (alertData: Flattened<Ecs>): EntriesArrayOrUndefined => 
  getEntityCodeSignature(alertData.dll, 'dll');

[tomsonpl]

Try something like this:

const codeSignature = alertData.dll?.Ext?.code_signature;

[tomsonpl]

The current implementation has complex logic for handling both array and single object cases. Consider simplifying:

export const getCodeSignatureValue = (
  codeSignature: Flattened<CodeSignature> | FlattenedCodeSignature[] | undefined,
  field: string
): EntryNested[] | undefined => {
  // Convert to array if it's a single object
  const signatures = Array.isArray(codeSignature) ? codeSignature : codeSignature ? [codeSignature] : [];
  
  // Filter and map in one operation
  const entries = signatures
    .filter(signature => signature?.trusted === true)
    .map(signature => ({
      field,
      type: 'nested' as const,
      entries: [
        {
          field: 'subject_name',
          operator: 'included' as const,
          type: 'match' as const,
          value: signature?.subject_name ?? '',
        },
        {
          field: 'trusted',
          operator: 'included' as const,
          type: 'match' as const,
          value: signature.trusted.toString(),
        },
      ],
    }));
  
  return entries.length > 0 ? entries : undefined;
};

Not sure if I covered all the edge cases again, so please verify. But I think this might improve the maintainability of the code.

[tomsonpl]

I assume these 'entries' in entry and 'value' in entry checks are only here acting as Type Guards? If so, maybe it would be clearer to move them outside:

function hasEntries(entry: any): entry is { entries: any[] } {
  return 'entries' in entry && entry.entries !== undefined;
}

function hasValue(entry: any): entry is { value: string } {
  return 'value' in entry && entry.value !== undefined && entry.value.length > 0;
}

[tomsonpl]

And even further, we can use the optional chaining operator ?. to simplify things eg.:

el?.value?.length > 0

[tomsonpl]

This is a tricky one, what do you think about something like this? :

function prepareEntries(...fieldArrays: EntriesArray[]): EntriesArray {
  // Combine all field arrays
  const combinedFields = fieldArrays.flat();
  
  // Filter empty entries and add IDs
  return addIdToEntries(filterEmptyExceptionEntries(combinedFields));
}

const entriesToAdd = () => {
  if (!isLinux) {
    const signatures = [];
    if (processCodeSignature) signatures.push(processCodeSignature);
    if (dllCodeSignature) signatures.push(dllCodeSignature);
    
    if (signatures.length > 0) {
      return prepareEntries(commonFields, ...signatures);
    }
  }
  return prepareEntries(commonFields);
};

This could be used in both: getPrepopulatedBehaviorException and getPrepopulatedRansomwareException

[tomsonpl]

The reason for doing it is to limit the number of if/else statements, not sure again if I covered all edge cases though.

[kapral18]

LGTM

[parkiino]

@elasticmachine merge upstream

[parkiino]

Similar to my last review: The changes to these helpers look logically sound, and the tests comprehensively demonstrate those changes. Since CI is now green, this looks good to me.

More broadly, and similarly to my comment in the previous review: I don't have a lot of context for these endpoint-specific bugs and their history, so it feels inadequate for me (or detection engine) to have equal code ownership/approval here. I would highly recommend perhaps splitting this (already very large) utils file into e.g. endpoint_utils, with more specific ownership for those more specialized helpers.

Hi Ryland - Thanks, yeah we are going to try to move all endpoint related code out at some point, but it will probably have to be for another PR

[elasticmachine]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: 22d9a9f
• Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-212325-22d9a9fc4160
• Security Deployment

Failed CI Steps

• FTR Configs #14
• FTR Configs #106

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
@kbn/securitysolution-ecs	349	351	+2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
securitySolution	9.1MB	9.1MB	+327.0B

Unknown metric groups

API count

id	before	after	diff
@kbn/securitysolution-ecs	353	355	+2

History

• 💔 Build #299603 failed 2fe4bbb
• 💔 Build #299268 failed 472aa7a
• 💚 Build #294965 succeeded 7421c40
• 💔 Build #294734 failed 7bc8bb0

[kibanamachine]

Starting backport for target branches: 8.19

https://github.com/elastic/kibana/actions/runs/14912315842

[kibanamachine]

💚 All backports created successfully

Status	Branch	Result
✅	8.19

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

[kibanamachine]

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.
cc: @parkiino

[kibanamachine]

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.
cc: @parkiino

[kibanamachine]

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.
cc: @parkiino

[kibanamachine]

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.
cc: @parkiino

[kibanamachine]

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.
cc: @parkiino