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

Addressing conditional IAM role bindings #66

Merged

Conversation

szamfirov
Copy link
Contributor

Discovered when InSpec is checking conditional role bindings. In
case they don't have any members (it happens after the expiration of
the condition), the control simply fails because of the nil value.

The issue is not well surfaced. In the beginning we were getting errors
like this one:

...
     ✔  [gsk-gsk-direct-e7d9] Admin roles members is expected not to include /@[a-z][a-z0-9|-]{4,28}[a-z].iam.gserviceaccount.com/
     ✔  [gsk-gsk-direct-e7d9] Admin roles members is expected not to include /@[a-z][a-z0-9|-]{4,28}[a-z].iam.gserviceaccount.com/
     ×  [gsk-gsk-direct-e7d9] Admin roles members is expected not to include /@[a-z][a-z0-9|-]{4,28}[a-z].iam.gserviceaccount.com/
     expected nil not to include /@[a-z][a-z0-9|-]{4,28}[a-z].iam.gserviceaccount.com/, but it does not respond to `include?`
     ✔  [gsk-gsk-direct-e7d9] Admin roles members is expected not to include /@[a-z][a-z0-9|-]{4,28}[a-z].iam.gserviceaccount.com/
     ✔  [gsk-gsk-direct-e7d9] Admin roles members is expected not to include /@[a-z][a-z0-9|-]{4,28}[a-z].iam.gserviceaccount.com/
...

We then proceeded to add some debugging by adding the role as an
addition to the output:

...
     ✔  [gsk-gsk-direct-e7d9] Admin role [roles/ml.admin] members is expected not to include /@[a-z][a-z0-9|-]{4,28}[a-z].iam.gserviceaccount.com/
     ✔  [gsk-gsk-direct-e7d9] Admin role [roles/monitoring.admin] members is expected not to include /@[a-z][a-z0-9|-]{4,28}[a-z].iam.gserviceaccount.com/
     ×  [gsk-gsk-direct-e7d9] Admin role [roles/monitoring.admin_withcond_a3db6685e6af0a7c34cd] members is expected not to include /@[a-z][a-z0-9|-]{4,28}[a-z].iam.gserviceaccount.com/
     expected nil not to include /@[a-z][a-z0-9|-]{4,28}[a-z].iam.gserviceaccount.com/, but it does not respond to `include?`
     ✔  [gsk-gsk-direct-e7d9] Admin role [roles/pubsub.admin] members is expected not to include /@[a-z][a-z0-9|-]{4,28}[a-z].iam.gserviceaccount.com/
     ✔  [gsk-gsk-direct-e7d9] Admin role [roles/redis.admin] members is expected not to include /@[a-z][a-z0-9|-]{4,28}[a-z].iam.gserviceaccount.com/
...

You can see that the error above is regarding one of the conditional
role bindings related to the monitoring.admin role which has no
members. In that case the members array is empty (or nil) which
does not work well with include?.

With the proposed fix:

...
     ✔  [gsk-gsk-direct-e7d9] Admin role [roles/ml.admin] members is expected not to include /@[a-z][a-z0-9|-]{4,28}[a-z].iam.gserviceaccount.com/
     ✔  [gsk-gsk-direct-e7d9] Admin role [roles/monitoring.admin] members is expected not to include /@[a-z][a-z0-9|-]{4,28}[a-z].iam.gserviceaccount.com/
     ↺  [gsk-gsk-direct-e7d9] role bindings for role [roles/monitoring.admin_withcond_a3db6685e6af0a7c34cd] do not contain any members.
     ✔  [gsk-gsk-direct-e7d9] Admin role [roles/pubsub.admin] members is expected not to include /@[a-z][a-z0-9|-]{4,28}[a-z].iam.gserviceaccount.com/
     ✔  [gsk-gsk-direct-e7d9] Admin role [roles/redis.admin] members is expected not to include /@[a-z][a-z0-9|-]{4,28}[a-z].iam.gserviceaccount.com/
...

Discovered when InSpec is checking conditional role bindings. In
case they don't have any members (it happens after the expiration of
the condition), the control simply fails.
@binamov
Copy link
Member

binamov commented Oct 19, 2020

/gcbrun

Copy link
Member

@binamov binamov left a comment

Choose a reason for hiding this comment

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

:shipit: 🍹

Copy link
Collaborator

@KonradSchieban KonradSchieban left a comment

Choose a reason for hiding this comment

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

LGTM!

@KonradSchieban
Copy link
Collaborator

Thanks @szamfirov for your contribution! :shipit: 🍸

@KonradSchieban KonradSchieban merged commit 60702e0 into GoogleCloudPlatform:master Oct 19, 2020
@szamfirov szamfirov deleted the conditional-role-bindings branch October 20, 2020 07:54
KonradSchieban pushed a commit to KonradSchieban/inspec-gcp-cis-benchmark that referenced this pull request Jul 4, 2021
Discovered when InSpec is checking conditional role bindings. In
case they don't have any members (it happens after the expiration of
the condition), the control simply fails.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants