-
Notifications
You must be signed in to change notification settings - Fork 469
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
[GCP][CDR] Add actor.entity.id and target.entity.id fields to audit logs #11983
Conversation
co-authored by: @efd6
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
@romulets The issue stated |
This is the kind of question that emphasises why we want to be working from accepted specifications. Decisions should already have been made and be publicly visible so that we know that we are all on the same page and so that our users know what behaviour they can expect. |
🚀 Benchmarks reportTo see the full report comment with |
💚 Build Succeeded
History
cc @kubasobon |
Quality Gate passedIssues Measures |
@elastic/obs-service-integrations & @elastic/security-service-integrations I would appreciate someone taking a look here :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had some Nits and those are addressed. Thanks.
The changes mainly pertain to GCP audit logs owned by Security Service Integrations. I am providing code owner approval from obs-infraobs side.
Please get the code reviewed from security team before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor question and comment.
Already fine.
boolean isKubernetes = false; | ||
if (ctx.json?.resource?.type != null) { | ||
String typ = ctx.json.resource.type; | ||
isKubernetes = (typ == "k8s_cluster" || typ == "gke_cluster" || typ == "kubernetes"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine as is, but here's an alternative:
boolean isKubernetes = false; | |
if (ctx.json?.resource?.type != null) { | |
String typ = ctx.json.resource.type; | |
isKubernetes = (typ == "k8s_cluster" || typ == "gke_cluster" || typ == "kubernetes"); | |
} | |
boolean isKubernetes = ["k8s_cluster", "gke_cluster", "kubernetes"].contains(ctx.json?.resource?.type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a nice, concise way of setting the flag. I'll keep it in mind for the future.
String serviceName = ctx.json?.protoPayload?.serviceName ?: ''; | ||
if (serviceName == "compute.googleapis.com") { | ||
if (ctx.json?.protoPayload?.request?.networkInterfaces instanceof List) { | ||
for (def e: ctx.json.protoPayload.request.networkInterfaces) { | ||
addValue(entities, e.network); | ||
} | ||
} | ||
if (ctx.json?.protoPayload?.request?.serviceAccounts instanceof List) { | ||
for (def e: ctx.json.protoPayload.request.serviceAccounts) { | ||
addValue(entities, e.email); | ||
} | ||
} | ||
if (ctx.json?.protoPayload?.request?.disks instanceof List) { | ||
for (def e: ctx.json.protoPayload.request.disks) { | ||
addValue(entities, e.source); | ||
} | ||
} | ||
} else if (serviceName == "cloudresourcemanager.googleapis.com") { | ||
if (ctx.json?.protoPayload?.request?.policy?.bindings instanceof List) { | ||
for (def e: ctx.json.protoPayload.request.policy.bindings) { | ||
addValue(entities, e.role); | ||
for (def m: e.members) { | ||
addValue(entities, m); | ||
} | ||
} | ||
} | ||
if (ctx.json?.protoPayload?.response?.bindings instanceof List) { | ||
for (def e: ctx.json.protoPayload.response.bindings) { | ||
addValue(entities, e.role); | ||
for (def m: e.members) { | ||
addValue(entities, m); | ||
} | ||
} | ||
} | ||
} else if (serviceName == "iamcredentials.googleapis.com") { | ||
if (ctx.json?.protoPayload?.metadata?.identityDelegationChain instanceof List) { | ||
for (def e: ctx.json.protoPayload.metadata.identityDelegationChain) { | ||
addValue(entities, e); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the outer if
s necessary here? is it okay to just collect entities from any of these locations if they exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were using streams for similar PRs, but we got pointed toward this approach in #11762.
I agree that nested if
clauses are an overkill and we could just try to grab everything. However this way we get a nice readability bump, I think. It tells a story of where each value originates from, and lays solid groundwork for any future PRs.
Package gcp - 2.40.0 containing this change is available at https://epr.elastic.co/package/gcp/2.40.0/ |
Proposed commit message
Add
actor.entity.id
andtarget.entity.id
fields to properly identify events' origins and targets. It is a requirement for https://github.com/elastic/security-team/issues/9352.Warning
To be merged after #11762. Please remember to change the base tomain
.Merged. The base is now
main
.Checklist
changelog.yml
file.Related issues