Skip to content
This repository has been archived by the owner on Jun 5, 2023. It is now read-only.

Add location scanner #2075

Merged
merged 12 commits into from Oct 16, 2018
Merged

Add location scanner #2075

merged 12 commits into from Oct 16, 2018

Conversation

umairidris
Copy link
Collaborator

@umairidris umairidris commented Oct 4, 2018

Currently only supports buckets.

Part of #2053.

@codecov
Copy link

codecov bot commented Oct 4, 2018

Codecov Report

Merging #2075 into dev will decrease coverage by 0.08%.
The diff coverage is 81.69%.

@@            Coverage Diff             @@
##              dev    #2075      +/-   ##
==========================================
- Coverage   89.12%   89.03%   -0.09%     
==========================================
  Files         168      170       +2     
  Lines       12788    12941     +153     
==========================================
+ Hits        11397    11522     +125     
- Misses       1391     1419      +28
Impacted Files Coverage Δ
.../cloud/forseti/common/data_access/violation_map.py 100% <ø> (ø) ⬆️
.../cloud/forseti/scanner/scanner_requirements_map.py 100% <ø> (ø) ⬆️
google/cloud/forseti/common/gcp_type/bucket.py 100% <100%> (ø) ⬆️
google/cloud/forseti/common/gcp_type/resource.py 98.63% <100%> (+0.05%) ⬆️
...cloud/forseti/scanner/scanners/location_scanner.py 63.41% <63.41%> (ø)
...gle/cloud/forseti/common/gcp_type/resource_util.py 77.5% <71.42%> (-1.29%) ⬇️
...oud/forseti/scanner/audit/location_rules_engine.py 88.65% <88.65%> (ø)

@@ -47,6 +50,7 @@ def __init__(
name (str): The bucket's unique GCP name, with the
format "buckets/{id}".
display_name (str): The bucket's display name.
locations (List[str]): Locations this bucket resides in.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think extend the comment here to mention that a bucket will only have a single location.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@@ -229,6 +232,15 @@ def parent(self):
"""
return self._parent

@property
def locations(self):
"""Locations the resource resides in.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe mention that some resources support multiple locations (GKE), others will always have a single location (GCS/BQ/...), and others do not support location (will return None).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.



class LocationRulesEngine(base_rules_engine.BaseRulesEngine):
"""Rules engine for Liens."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and in other lines/files, a few instances of "Lien" still in comments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.


resource_type = raw_resource.get('type')

if resource_type not in {'project', 'folder', 'organization'}:
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this a class "constant" e.g.
SUPPORTED_RESOURCE_TYPES = frozenset(['project', 'folder', 'organization'])

Same for the supported applies_to types.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

resource (Resource): The GCP resource to check locations for.
This is where we start looking for rule violations and
we move up the resource hierarchy (if permitted by the
resource's "inherit_from_parents" property).
Copy link
Contributor

Choose a reason for hiding this comment

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

remove comment about 'inherit_from_parents'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

self.applies_to = applies_to

loc_re_str = '|'.join([
regular_exp.escape_and_globify(loc_wildcard)
Copy link
Contributor

Choose a reason for hiding this comment

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

loc_wildcard.lower() here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

"""
resources = []

resource_type_to_fn = {'bucket': bucket.Bucket.from_json}
Copy link
Contributor

Choose a reason for hiding this comment

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

Document what kind of function is expected here. Perhaps we could move this this out to somewhere the rule engine and scanner can both see, so there is a single source of truth for supported resources

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

mode='whitelist',
type='bucket',
locations=['eu*'],
)
Copy link
Contributor

Choose a reason for hiding this comment

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

One of the tests should cover exact matches (no wildcard), and multiple locations in a single rule

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@umairidris umairidris mentioned this pull request Oct 9, 2018
Copy link
Contributor

@joecheuk joecheuk left a comment

Choose a reason for hiding this comment

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

Thanks for adding the location scanner!

@joecheuk joecheuk merged commit 96dc0e7 into forseti-security:dev Oct 16, 2018
@umairidris umairidris deleted the loc branch October 17, 2018 18:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants