-
Notifications
You must be signed in to change notification settings - Fork 109
fix(plugins/gcpaudit): route gcp.policyDelta by IAM proto, not gcs_bucket allowlist #1355
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| /* | ||
| Copyright (C) 2026 The Falco Authors. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package gcpaudit | ||
|
|
||
| import ( | ||
| "reflect" | ||
| "testing" | ||
| ) | ||
|
|
||
| // TestPolicyDeltaPath guards against #1351: gcp.policyDelta was returning the | ||
| // BigQuery datasetChange path for every non-gcs_bucket resource type, leaving | ||
| // the field empty for project, folder, organization, service_account, and any | ||
| // other GCP resource that uses the generic IAM SetIamPolicy flow. | ||
| func TestPolicyDeltaPath(t *testing.T) { | ||
| serviceData := []string{"protoPayload", "serviceData", "policyDelta", "bindingDeltas"} | ||
| datasetChange := []string{"protoPayload", "metadata", "datasetChange", "bindingDeltas"} | ||
|
|
||
| cases := []struct { | ||
| resource string | ||
| want []string | ||
| }{ | ||
| {"gcs_bucket", serviceData}, | ||
| {"project", serviceData}, | ||
| {"folder", serviceData}, | ||
| {"organization", serviceData}, | ||
| {"service_account", serviceData}, | ||
| // Unknown / future resource types default to the generic IAM path, | ||
| // not the BigQuery-specific datasetChange path. | ||
| {"unknown_type", serviceData}, | ||
| {"", serviceData}, | ||
| {"bigquery_dataset", datasetChange}, | ||
| } | ||
|
|
||
| for _, tc := range cases { | ||
| t.Run(tc.resource, func(t *testing.T) { | ||
| got := policyDeltaPath(tc.resource) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test covers the path selection nicely, but it stops at May you add a fixture-based test that feeds a captured exported |
||
| if !reflect.DeepEqual(got, tc.want) { | ||
| t.Errorf("policyDeltaPath(%q) = %v, want %v", tc.resource, got, tc.want) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
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.
Nice improvement over the old logic 🙏 defaulting to
serviceData.policyDeltaand special-casing BigQuery is the right direction, and I don't see a breaking change here (gcs_bucketandbigquery_datasetkeep the same paths).A couple of caveats about this rationale though:
protoPayload.serviceDatais officially deprecated by Google (they recommendmetadatanow), and it's shared by several services, not just BigQuery. So "BigQuery is the outlier" is a bit of an oversimplification - not a blocker, but the comment may not hold as more services migrate tometadata.More importantly, this plugin ingests exported logs via the Pub/Sub sink, and
serviceDatais known to be stripped on exported entries even when it's populated in Logs Explorer 👉 https://permiso.io/blog/gcp-servicedata-officially-deprecated-actively-dangerousSo for the very resources #1351 is about (project/folder/org/service_account),
gcp.policyDeltamay still come back empty in practice after this change. Have you confirmed it's populated on a real exportedsetIamPolicyevent? 🤔 If not, we may needmetadataas a fallback.