Support iam policy member of domain type in iam scanner's required mode, #1093

arbeit · 2018-02-14T12:24:32Z

Hello there!

This fixes issue #799. Please take a look and let me know what you think. Thanks!

codecov · 2018-02-14T12:30:15Z

Codecov Report

Merging #1093 into dev will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##              dev   #1093      +/-   ##
=========================================
+ Coverage   83.29%   83.3%   +0.01%     
=========================================
  Files         163     163              
  Lines        7829    7837       +8     
=========================================
+ Hits         6521    6529       +8     
  Misses       1308    1308

Impacted Files	Coverage Δ
...oogle/cloud/security/common/gcp_type/iam_policy.py	`85.39% <100%> (+1.44%)`	⬆️

blueandgold

Thanks for fixing this! Mostly style comment to improve readability by other people and in the future.

In addition, this fix will need to be ported to 2.0.

Finally, should we support this in whitelist mode and blacklist mode?

blueandgold · 2018-02-14T19:22:14Z

tests/scanner/audit/iam_rules_engine_test.py

+        rules_engine.rule_book.org_res_rel_dao = mock.MagicMock()
+        find_ancestor_mock = mock.MagicMock(
+            side_effect=[[self.org789]])
+        rules_engine.rule_book.org_res_rel_dao.find_ancestors = \


Backslashes should not be used for line continuation. So, do it like this:

rules_engine.rule_book.org_res_rel_dao.find_ancestors = ( find_ancestor_mock)

blueandgold · 2018-02-14T19:23:23Z

tests/scanner/audit/iam_rules_engine_test.py

+        actual_violations = set(rules_engine.find_policy_violations(
+            self.project1, policy))
+
+        # expected


We should drop this comment as the variable's name is already self-documenting.

blueandgold · 2018-02-14T19:26:37Z

tests/scanner/audit/iam_rules_engine_test.py

+        self.assertEqual(expected_violations, actual_violations)
+
+    def test_policy_all_projects_must_have_owners_from_wildcard_domain(self):
+        """Test a policy where the owner belongs to the required domain and the


This is a very good method doc, but the summary needs to be 1 line. So, let's break this up to something like this:

"""Test a policy where the owner belongs to wildcard domain. Test a policy where the owner belongs to the required domain and the domain is specified as a wildcard user ('members': ['user:*@xyz.edu']) """

blueandgold · 2018-02-14T19:27:07Z

tests/scanner/audit/iam_rules_engine_test.py

+        rules_engine.rule_book.org_res_rel_dao = mock.MagicMock()
+        find_ancestor_mock = mock.MagicMock(
+            side_effect=[[self.org789]])
+        rules_engine.rule_book.org_res_rel_dao.find_ancestors = \


ditto on backslash continution

blueandgold · 2018-02-14T19:29:28Z

tests/scanner/audit/iam_rules_engine_test.py

+        policy = {
+            'bindings': [{
+                'role': 'roles/owner',
+                'members': ['user:def@xyz.edu']


Can you please add a second test case for a non-xyz domain? user:def@abc.edu So, that we know for sure if there is a violation, it will be triggered?

blueandgold · 2018-02-14T19:38:14Z

google/cloud/security/common/gcp_type/iam_policy.py

@@ -288,6 +288,28 @@ def create_from(cls, member):
            member_name = identity_parts[1]
        return cls(identity_parts[0], member_name=member_name)

+    def _user_is_in_domain(self, other):


This naming should follow boolean naming convention, and might better as: is_matching_domain()

blueandgold · 2018-02-14T20:47:09Z

google/cloud/security/common/gcp_type/iam_policy.py

@@ -288,6 +288,28 @@ def create_from(cls, member):
            member_name = identity_parts[1]
        return cls(identity_parts[0], member_name=member_name)

+    def _user_is_in_domain(self, other):
+        """Determine whether user belongs to domain.


Technically, not a user. But more clear as IAM policy member. So:

"""Determine whether IAM policy member belongs to domain.

blueandgold · 2018-02-14T20:49:07Z

google/cloud/security/common/gcp_type/iam_policy.py

+        specification and the policy to check specifies users.
+
+        Args:
+            other (IamPolicyMember): The policy binding to check.


Please clarify this, by adding member like:

other (IamPolicyMember): The policy binding member to check.

blueandgold · 2018-02-14T20:51:02Z

tests/scanner/audit/iam_rules_engine_test.py

@@ -1405,6 +1403,87 @@ def test_project_required(self):
        ])
        self.assertItemsEqual(expected_violations, actual_violations)

+    def test_policy_all_projects_must_have_owners_from_domain(self):


Please clarify this method name, to show that it's using the domain member type. So, a good name would be:

test_policy_all_projects_must_have_owners_from_domain_type

blueandgold · 2018-02-14T20:52:19Z

tests/scanner/audit/iam_rules_engine_test.py

+
+        self.assertEqual(expected_violations, actual_violations)
+
+    def test_policy_all_projects_must_have_owners_from_wildcard_domain(self):


Please clarify this method name, to show that it's using the wildcard domain from user type. So, a good name would be:

test_policy_all_projects_must_have_owners_from_wildcard_domain_of_user_type

arbeit · 2018-02-14T21:47:52Z

@blueandgold -- addressed your review comments, please take a look. Thanks!

arbeit · 2018-02-14T21:49:52Z

@blueandgold not sure about whitelist / blacklist mode .. maybe we can open a separate issue for that?

arbeit · 2018-02-14T21:56:45Z

@blueandgold once this PR lands, I will port the changes to 2.0-dev

blueandgold

Awesome, this so much more understandable now!

* ported code to 2.0-dev based branch * fix tests

arbeit added 6 commits February 14, 2018 08:39

removed unused imports

a80f2d7

add failing test

02050e6

added (domain membership) test with wildcard user

9066b83

added domain membership check

d3cf428

handle rsplit() hiccups

2a8618b

better naming

cbfbefb

googlebot added the cla: yes label Feb 14, 2018

forseti-security-waffle-bot added the status: in-progress label Feb 14, 2018

less confusing example

ed7a63d

blueandgold self-requested a review February 14, 2018 19:06

blueandgold changed the title ~~Iam rule scan 799~~ Fix IAM rules scanner bug: requiring projects to have specific owners Feb 14, 2018

blueandgold suggested changes Feb 14, 2018

View reviewed changes

blueandgold changed the title ~~Fix IAM rules scanner bug: requiring projects to have specific owners~~ requiring projects to have specific owners Feb 14, 2018

blueandgold changed the title ~~requiring projects to have specific owners~~ Support iam policy member of domain type in iam scanner's required mode, Feb 14, 2018

arbeit added 2 commits February 14, 2018 22:20

addressing review comments, batch forseti-security#1

6143885

addressing review comments, batch forseti-security#2

c3283e1

arbeit self-assigned this Feb 14, 2018

blueandgold approved these changes Feb 16, 2018

View reviewed changes

Merge branch 'dev' into IAM-rule-scan-799

8d174ad

blueandgold merged commit 6a3bc81 into forseti-security:dev Feb 16, 2018

forseti-security-waffle-bot removed the status: in-progress label Feb 16, 2018

arbeit mentioned this pull request Feb 19, 2018

port of PR #1093 to the 2.0-dev branch #1123

Merged

blueandgold pushed a commit that referenced this pull request Feb 22, 2018

port of PR #1093 to the 2.0-dev branch (#1123)

60f9f10

* ported code to 2.0-dev based branch * fix tests

arbeit deleted the IAM-rule-scan-799 branch March 16, 2018 09:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support iam policy member of domain type in iam scanner's required mode, #1093

Support iam policy member of domain type in iam scanner's required mode, #1093

arbeit commented Feb 14, 2018

codecov bot commented Feb 14, 2018 •

edited

blueandgold left a comment

blueandgold Feb 14, 2018

blueandgold Feb 14, 2018

blueandgold Feb 14, 2018

blueandgold Feb 14, 2018

blueandgold Feb 14, 2018

blueandgold Feb 14, 2018

blueandgold Feb 14, 2018

blueandgold Feb 14, 2018

blueandgold Feb 14, 2018

blueandgold Feb 14, 2018

arbeit commented Feb 14, 2018

arbeit commented Feb 14, 2018

arbeit commented Feb 14, 2018

blueandgold left a comment


		self.assertEqual(expected_violations, actual_violations)

		def test_policy_all_projects_must_have_owners_from_wildcard_domain(self):

Support iam policy member of domain type in iam scanner's required mode, #1093

Support iam policy member of domain type in iam scanner's required mode, #1093

Conversation

arbeit commented Feb 14, 2018

codecov bot commented Feb 14, 2018 • edited

Codecov Report

blueandgold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arbeit commented Feb 14, 2018

arbeit commented Feb 14, 2018

arbeit commented Feb 14, 2018

blueandgold left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 14, 2018 •

edited