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

fix(query): support GCP IAM policy members as lists #6548

Merged

Conversation

Tohar-orca
Copy link
Contributor

@Tohar-orca Tohar-orca commented Jul 26, 2023

Proposed Changes

  • The service_account_with_improper_privileges rule should take into account policies with multiple bindings

I submit this contribution under the Apache-2.0 license.

Copy link
Collaborator

@cxMiguelSilva cxMiguelSilva left a comment

Choose a reason for hiding this comment

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

Hi @Tohar-orca, thank you so much for this amazing contribution.
The proposed changes LGTM 🚀

@cxMiguelSilva
Copy link
Collaborator

cxMiguelSilva commented Jul 26, 2023

Hi @asofsilva, kindly do the QA checking for these changes.

@cxMiguelSilva cxMiguelSilva added terraform Terraform query gcp PR related with GCP Cloud QA Pending QA Validation labels Jul 26, 2023
"issueType": "IncorrectValue",
"keyExpectedValue": sprintf("google_iam_policy[%s].binding[%s].role should not have admin, editor, owner, or write privileges for service account member", [name, format_int(x, 10)]),
"keyActualValue": sprintf("google_iam_policy[%s].binding[%s].role has admin, editor, owner, or write privilege for service account member", [name, format_int(x, 10)]),
"searchLine": common_lib.build_search_line(["resource", "google_iam_policy", name, "binding", x, "role"], []),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"searchLine": common_lib.build_search_line(["resource", "google_iam_policy", name, "binding", x, "role"], []),
"searchLine": common_lib.build_search_line(["data", "google_iam_policy", name, "binding", x, "role"], []),

Hi @Tohar-orca, @asofsilva noticed that the line is not correctly detected during the QA checks. Would you mind changing the search lines for the Policies that check the data from resource to data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course! Done :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks

cxMiguelSilva
cxMiguelSilva previously approved these changes Aug 1, 2023
@github-actions github-actions bot added community Community contribution and removed terraform Terraform query labels Aug 24, 2023
@gabriel-cx gabriel-cx changed the title Support GCP IAM policy members as lists fix(query): Support GCP IAM policy members as lists Aug 24, 2023
@github-actions github-actions bot added the query New query feature label Aug 24, 2023
@gabriel-cx gabriel-cx changed the title fix(query): Support GCP IAM policy members as lists fix(query): support GCP IAM policy members as lists Aug 24, 2023
@asofsilva
Copy link
Contributor

Hi @Tohar-orca,
I would like to suggest you to add a new positive test file (ex: positive4.tf) where all the serviceAccounts have improper roles, in order to have a test result with more than one vulnerability. For example, a file like:

data "google_iam_policy" "admin" {
  binding {
    role = "roles/admin"
    members = [
      "serviceAccount:your-custom-sa@your-project.iam.gserviceaccount.com",
    ]
  }
  binding {
    role = "roles/editor"
    members = [
      "serviceAccount:alice@gmail.com",
    ]
  }
}

What do you think?

Thank you :)

@Tohar-orca
Copy link
Contributor Author

Hi @Tohar-orca, I would like to suggest you to add a new positive test file (ex: positive4.tf) where all the serviceAccounts have improper roles, in order to have a test result with more than one vulnerability. For example, a file like:

data "google_iam_policy" "admin" {
  binding {
    role = "roles/admin"
    members = [
      "serviceAccount:your-custom-sa@your-project.iam.gserviceaccount.com",
    ]
  }
  binding {
    role = "roles/editor"
    members = [
      "serviceAccount:alice@gmail.com",
    ]
  }
}

What do you think?

Thank you :)

Hey @asofsilva good idea, I've added a test as you suggested :)

@gabriel-cx gabriel-cx merged commit b363199 into Checkmarx:master Aug 29, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution gcp PR related with GCP Cloud QA Pending QA Validation query New query feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants