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

cisco_umbrella: handle user identities with full email address values #6119

Merged
merged 1 commit into from
May 23, 2023

Conversation

efd6
Copy link
Contributor

@efd6 efd6 commented May 8, 2023

What does this PR do?

The Umbrella identities list is a comma-separated list of values mapped to type by reference to the identity_types list (also comma-separated). When an identity is the "AD User" type the identity value may be an RFC822 email address. This value may legally include a comma. Unfortunately, this breaks the current approach to resolving the mapping between the identities and identity_types. Umbrella does provide what appears to be a primary identity in the identity field. This appears to match the "AD User" identity value when present, and in cases that are available, the "AD User" identity appears first in the identities list. This allows us to treat the identity specially by removing it from identities if it contains confounding commas, treat the remainder as previously and then reconstruct the array. If the "AD User identity does not appear in the first position, or does not match the value in the identity field, work would degenerate to searching for an RFC822 email address while maintaining the invariant that the length of the identities list matches the length of the identity_types list. This is not feasible.

None of this is documented.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Screenshots

@elasticmachine
Copy link

elasticmachine commented May 8, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-05-22T21:42:04.274+0000

  • Duration: 14 min 47 sec

Test stats 🧪

Test Results
Failed 0
Passed 10
Skipped 0
Total 10

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine
Copy link

elasticmachine commented May 8, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (1/1) 💚
Files 100.0% (1/1) 💚
Classes 100.0% (1/1) 💚
Methods 95.238% (20/21) 👎 -1.57
Lines 94.982% (530/558) 👍 4.542
Conditionals 100.0% (0/0) 💚

@efd6 efd6 marked this pull request as ready for review May 9, 2023 02:09
@efd6 efd6 requested a review from a team as a code owner May 9, 2023 02:09
@elasticmachine
Copy link

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

@efd6 efd6 force-pushed the cisco_umbrella_rfc822 branch 3 times, most recently from d11bafd to c75de24 Compare May 11, 2023 02:00
Comment on lines 237 to 240
if (!ctx.cisco.umbrella.identity.contains(',')) {
// No comma, so we are done.
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could add this into processor if condition itself?

Copy link
Contributor

@kcreddy kcreddy left a comment

Choose a reason for hiding this comment

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

Pipeline tests seem to be failing

@efd6
Copy link
Contributor Author

efd6 commented May 22, 2023

The tests are failing because of an update to the stack version and the break in registered domain processor behaviour. I'll fix that tomorrow and update the condition.

The Umbrella identities list is a comma-separated list of values mapped
to type by reference to the identity_types list (also comma-separated).
When an identity is the "AD User" type the identity value may be an
RFC822 email address. This value may legally include a comma.
Unfortunately, this breaks the current approach to resolving the mapping
between the identities and identity_types. Umbrella does provide what
appears to be a primary identity in the identity field. This appears to
match the "AD User" identity value when present, and in cases that are available,
the "AD User" identity appears first in the identities list. This allows
us to treat the identity specially by removing it from identities if it
contains confounding commas, treat the remainder as previously and then
reconstruct the array. If the "AD User identity does not appear in the
first position, or does not match the value in the identity field, work
would degenerate to searching for an RFC822 email address while
maintaining the invariant that the length of the identities list matches
the length of the identity_types list. This is not feasible.

None of this is documented.
@efd6 efd6 requested a review from kcreddy May 22, 2023 23:04
Copy link
Contributor

@kcreddy kcreddy left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼

@efd6 efd6 merged commit d71c213 into elastic:main May 23, 2023
1 check passed
@elasticmachine
Copy link

Package cisco_umbrella - 1.11.1 containing this change is available at https://epr.elastic.co/search?package=cisco_umbrella

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Integration:Cisco Umbrella
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants