Skip to content

Commit

Permalink
v1.1.3 (#87)
Browse files Browse the repository at this point in the history
bugfixes: 

* NotPrincipal in resource policies, '*' for matching principals
* Codebuild tag handling fixed
* Querying with cached resource policies fixed for IAM Role Trust Policies
* SQS messaging on missing queue policy fixed
* Orgs messaging on improper args fixed
* Orgs without SCPs fixed
* fixed handling for condition contexts, case-insensitive keys
* fix for #86 - Did not include Edges for existing Lambda functions

additions:

* updated output of (arg)query, changed edge descriptions to use node searchable-names
* add endgame support for Secrets Manager

Co-authored-by: Erik Steringer <[email protected]>
  • Loading branch information
ncc-erik-steringer and Erik Steringer authored Jul 13, 2021
1 parent 6139b13 commit 722efec
Show file tree
Hide file tree
Showing 19 changed files with 271 additions and 79 deletions.
2 changes: 1 addition & 1 deletion principalmapper/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,4 @@
# You should have received a copy of the GNU Affero General Public License
# along with Principal Mapper. If not, see <https://www.gnu.org/licenses/>.

__version__ = '1.1.2'
__version__ = '1.1.3'
4 changes: 2 additions & 2 deletions principalmapper/common/edges.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,9 @@ def __init__(self, source, destination, reason: str, short_reason: str):
def describe_edge(self) -> str:
"""Returns a human-readable string explaining the edge"""
return "{} {} {}".format(
arns.get_resource(self.source.arn),
self.source.searchable_name(),
self.reason,
arns.get_resource(self.destination.arn)
self.destination.searchable_name()
)

def to_dictionary(self) -> dict:
Expand Down
2 changes: 1 addition & 1 deletion principalmapper/graphing/codebuild_edges.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ def _gen_resource_tag_conditions(tag_list: List[dict]):
}
for tag in tag_list:
condition_result.update({
'aws:ResourceTag/{}'.format(tag['Key']): tag['Value']
'aws:ResourceTag/{}'.format(tag['key']): tag['value']
})
# TODO: make sure we're handling RequestTag and TagKeys correctly
# condition_result.update({
Expand Down
2 changes: 1 addition & 1 deletion principalmapper/graphing/gathering.py
Original file line number Diff line number Diff line change
Expand Up @@ -402,7 +402,7 @@ def get_sqs_queue_policies(session: botocore.session.Session, account_id: str, r
"Version": "2012-10-17"
}
))
logger.info('Queue {} does not have a bucket policy, adding a "stub" policy instead.'.format(queue_name))
logger.info('Queue {} does not have a queue policy, adding a "stub" policy instead.'.format(queue_name))
except botocore.exceptions.ClientError as ex:
logger.info('Unable to search SQS in region {} for queues. The region may be disabled, or the current principal may not be authorized to access the service. Continuing.'.format(sqs_region))
logger.debug('Exception was: {}'.format(ex))
Expand Down
5 changes: 4 additions & 1 deletion principalmapper/graphing/lambda_edges.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ def return_edges(self, nodes: List[Node], region_allow_list: Optional[List[str]]
logger.debug('Exception details: {}'.format(ex))

logger.info('Generating Edges based on Lambda data.')
logger.debug('Identified {} Lambda functions for processing'.format(len(function_list)))
result = generate_edges_locally(nodes, function_list, scps)

for edge in result:
Expand Down Expand Up @@ -87,7 +88,7 @@ def generate_edges_locally(nodes: List[Node], function_list: List[dict], scps: O
node_destination.trust_policy,
'sts:AssumeRole',
node_destination.arn,
{},
{}
)

if sim_result != ResourcePolicyEvalResult.SERVICE_MATCH:
Expand Down Expand Up @@ -173,6 +174,7 @@ def generate_edges_locally(nodes: List[Node], function_list: List[dict], scps: O
reason,
'Lambda'
)
result.append(new_edge)
break

# check that source can modify a Lambda function and pass it another execution role
Expand All @@ -192,6 +194,7 @@ def generate_edges_locally(nodes: List[Node], function_list: List[dict], scps: O
reason,
'Lambda'
)
result.append(new_edge)
break

return result
4 changes: 2 additions & 2 deletions principalmapper/graphing/orgs_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,10 @@ def process_arguments(parsed_args: Namespace):

# filter the args first
if parsed_args.account is not None:
print('Cannot specify offline-mode param `--account` when calling `pmapper graph org_create`. If you have '
print('Cannot specify offline-mode param `--account` when calling `pmapper graph orgs create`. If you have '
'credentials for a specific account to graph, you can use those credentials similar to how the '
'AWS CLI works (environment variables, profiles, EC2 instance metadata). In the case of using '
'a profile, use the `--profile [PROFILE]` argument before specifying the `graph` subcommand.')
'a profile, use the `--profile [PROFILE]` argument before specifying the `orgs` subcommand.')
return 64

# get the botocore session and go to work creating the OrganizationTree obj
Expand Down
2 changes: 1 addition & 1 deletion principalmapper/querying/argquery_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ def process_arguments(parsed_args: Namespace):

if parsed_args.with_resource_policy:
resource_policy = query_utils.pull_cached_resource_policy_by_arn(
graph.policies,
graph,
parsed_args.resource
)
elif parsed_args.resource_policy_text:
Expand Down
67 changes: 42 additions & 25 deletions principalmapper/querying/local_policy_simulation.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,13 @@

from principalmapper.common import Node, Policy
from principalmapper.util import arns
from principalmapper.util.case_insensitive_dict import CaseInsensitiveDict

logger = logging.getLogger(__name__)


def has_matching_statement(principal: Node, effect_value: str, action_to_check: str, resource_to_check: str,
condition_keys_to_check: dict) -> bool:
condition_keys_to_check: CaseInsensitiveDict) -> bool:
"""Locally determine if a node's attached policies (and group's policies if applicable) has at least one matching
statement with the given effect. This is the meat of the local policy evaluation.
"""
Expand All @@ -55,7 +56,7 @@ def has_matching_statement(principal: Node, effect_value: str, action_to_check:

def policy_has_matching_statement(policy: Union[Policy, dict], effect_value: str, action_to_check: str,
resource_to_check: str,
condition_keys_to_check: dict) -> bool:
condition_keys_to_check: CaseInsensitiveDict) -> bool:
"""Searches a specific Policy/dict object for a statement with a matching Effect/Action/Resource/Condition"""

if isinstance(policy, Policy):
Expand Down Expand Up @@ -114,7 +115,7 @@ def policy_has_matching_statement(policy: Union[Policy, dict], effect_value: str
return False


def _get_condition_match(condition: Dict[str, Dict[str, Union[str, List]]], context: Dict) -> bool:
def _get_condition_match(condition: Dict[str, Dict[str, Union[str, List]]], context: CaseInsensitiveDict) -> bool:
"""
Internal method. It digs through Null, Bool, DateX, NumericX, StringX conditions and returns false if any of
them don't match what the context has.
Expand All @@ -137,7 +138,7 @@ def _get_condition_match(condition: Dict[str, Dict[str, Union[str, List]]], cont
block,
policy_key,
condition[block][policy_key],
{policy_key: context_value}):
CaseInsensitiveDict({policy_key: context_value})):
return False
elif block.startswith('ForAnyValue:'):
# fail to match unless at least one of the provided context values match
Expand Down Expand Up @@ -342,19 +343,28 @@ def _get_condition_match(condition: Dict[str, Dict[str, Union[str, List]]], cont
return True


def _get_str_match(block: str, policy_key: str, policy_value: Union[str, List[str]], context: dict) -> bool:
def _get_str_match(block: str, policy_key: str, policy_value: Union[str, List[str]], context: CaseInsensitiveDict) -> bool:
"""Helper method for dealing with String* conditions, including: StringEquals, StringNotEquals,
StringEqualsIgnoreCase, StringNotEqualsIgnoreCase, StringLike, StringNotLike
Observed policy simulator behavior for *IgnoreCase: if I compare the following, it returns denied:
Observed policy simulator behavior for Unicode: if I compare the following, it returns denied:
* ê <- 'LATIN SMALL LETTER E WITH CIRCUMFLEX'
* ê <- 'LATIN SMALL LETTER E' + 'COMBINING CIRCUMFLEX ACCENT'
So even though they're the "same" they end up not matching. Just using casefold() on the strings is enough to match
the policy simulator behavior without having to dip into the insanity of unicode.
So even though they're the "same" they end up not matching.
Many thanks to https://stackoverflow.com/a/29247821 for helping this code on this journey.
Observed policy simulator behavior for capitalization: if I compare the following, it returns denied:
* ß
* ss
So even though they're the "same" after casefolding (str casefold method) they end up not matching.
Just using lower() on the strings is enough to match the policy simulator behavior without having to dip into the
insanity of unicode.
Many thanks to https://stackoverflow.com/a/45745761 for the extra information.
"""

if_exists_op = 'IfExists' in block
Expand All @@ -365,7 +375,7 @@ def _get_str_match(block: str, policy_key: str, policy_value: Union[str, List[st
for value in _listify_string(policy_value):
for context_value in _listify_string(context[policy_key]):
if 'IgnoreCase' in block:
if value.casefold() == context_value.casefold():
if value.lower() == context_value.lower():
return True
else:
if value == context_value:
Expand All @@ -385,7 +395,7 @@ def _get_str_match(block: str, policy_key: str, policy_value: Union[str, List[st
for value in _listify_string(policy_value):
for context_value in _listify_string(context[policy_key]):
if 'IgnoreCase' in block:
if value.casefold() == context_value.casefold():
if value.lower() == context_value.lower():
return False
else:
if value == context_value:
Expand Down Expand Up @@ -418,7 +428,7 @@ def _expand_str_and_compare(pattern: str, input_value: str) -> bool:
return re.match(pattern_string, input_value, flags=re.UNICODE) is not None


def _get_num_match(block: str, policy_key: str, policy_value: Union[str, List[str]], context: dict) -> bool:
def _get_num_match(block: str, policy_key: str, policy_value: Union[str, List[str]], context: CaseInsensitiveDict) -> bool:
"""Helper method for dealing with Numeric* conditions, including: NumericEquals, NumericNotEquals,
NumericLessThan, NumericLessThanEquals, NumericGreaterThan, NumericGreaterThanEquals
Expand Down Expand Up @@ -469,7 +479,7 @@ def _get_num_match(block: str, policy_key: str, policy_value: Union[str, List[st
return False


def _get_bool_match(block: str, policy_key: str, policy_value: Union[str, List[str]], context: dict) -> bool:
def _get_bool_match(block: str, policy_key: str, policy_value: Union[str, List[str]], context: CaseInsensitiveDict) -> bool:
"""Helper method for dealing with Bool. For 'true' policy values, returns True if context has 'true' as a value. For
'false' policy values, returns True if context has value that's not 'true'. Returns False if no context value.
"""
Expand All @@ -489,7 +499,7 @@ def _get_bool_match(block: str, policy_key: str, policy_value: Union[str, List[s
return False


def _get_straight_str_match(block: str, policy_key: str, policy_value: Union[str, List[str]], context: dict) -> bool:
def _get_straight_str_match(block: str, policy_key: str, policy_value: Union[str, List[str]], context: CaseInsensitiveDict) -> bool:
"""Helper method for dealing with BinaryEquals
Does a straight string comparison to search for a match.
Expand All @@ -507,7 +517,7 @@ def _get_straight_str_match(block: str, policy_key: str, policy_value: Union[str
return False


def _get_ipaddress_match(block: str, policy_key: str, policy_value: Union[str, List[str]], context: dict) -> bool:
def _get_ipaddress_match(block: str, policy_key: str, policy_value: Union[str, List[str]], context: CaseInsensitiveDict) -> bool:
"""Helper method for dealing with *IpAddress conditions: IpAddress, NotIpAddress
Parses the policy value as an IPvXNetwork, then the context value as an IPvXAddress, then uses
Expand Down Expand Up @@ -540,7 +550,7 @@ def _get_ipaddress_match(block: str, policy_key: str, policy_value: Union[str, L
return True


def _get_date_match(block: str, policy_key: str, policy_value: Union[str, List[str]], context: dict) -> bool:
def _get_date_match(block: str, policy_key: str, policy_value: Union[str, List[str]], context: CaseInsensitiveDict) -> bool:
"""Helper method for dealing with Date* conditions: DateEquals, DateNotEquals, DateGreaterThan,
DateGreaterThanEquals, DateLessThan, DateLessThanEquals.
Expand Down Expand Up @@ -606,7 +616,7 @@ def _convert_timestamp_to_datetime_obj(timestamp: str):
return dt.datetime.fromtimestamp(float(timestamp), dt.timezone.utc) # TODO: concern around float imprecision


def _get_arn_match(block: str, policy_key: str, policy_value: Union[str, List[str]], context: dict) -> bool:
def _get_arn_match(block: str, policy_key: str, policy_value: Union[str, List[str]], context: CaseInsensitiveDict) -> bool:
"""Helper method for dealing with Arn* conditions: ArnEquals, ArnLike, ArnNotEquals, ArnNotLike"""

if_exists_op = 'IfExists' in block
Expand Down Expand Up @@ -636,7 +646,7 @@ def _get_arn_match(block: str, policy_key: str, policy_value: Union[str, List[st
return False


def _get_null_match(policy_key: str, policy_value: Union[str, List[str]], context: Dict) -> bool:
def _get_null_match(policy_key: str, policy_value: Union[str, List[str]], context: CaseInsensitiveDict) -> bool:
"""Helper method for dealing with Null conditions"""
for value in _listify_string(policy_value):
if value == 'true': # key is expected not to be in context, or empty
Expand All @@ -650,7 +660,7 @@ def _get_null_match(policy_key: str, policy_value: Union[str, List[str]], contex

def resource_policy_matching_statements(node_or_service: Union[Node, str], resource_policy: dict,
action_to_check: str, resource_to_check: str,
condition_keys_to_check: dict) -> list:
condition_keys_to_check: CaseInsensitiveDict) -> list:
"""Returns if a resource policy has a matching statement for a given service (ec2.amazonaws.com for example)."""

results = []
Expand All @@ -672,7 +682,7 @@ def resource_policy_matching_statements(node_or_service: Union[Node, str], resou
matches_principal = True
if isinstance(node_or_service, Node):
if 'AWS' in statement['NotPrincipal']:
if _principal_matches_in_statement(node_or_service, _listify_string(statement['Principal']['AWS'])):
if _principal_matches_in_statement(node_or_service, _listify_string(statement['NotPrincipal']['AWS'])):
matches_principal = False
else:
if 'Service' in statement['NotPrincipal']:
Expand Down Expand Up @@ -724,11 +734,16 @@ class ResourcePolicyEvalResult(Enum):

def resource_policy_authorization(node_or_service: Union[Node, str], resource_owner: str, resource_policy: dict,
action_to_check: str, resource_to_check: str,
condition_keys_to_check: dict) -> ResourcePolicyEvalResult:
condition_keys_to_check: Union[dict, CaseInsensitiveDict]) -> ResourcePolicyEvalResult:
"""Returns a ResourcePolicyEvalResult for a given request, based on the resource policy."""

if isinstance(condition_keys_to_check, dict):
prepped_condition_keys = CaseInsensitiveDict(condition_keys_to_check)
else:
prepped_condition_keys = condition_keys_to_check

matching_statements = resource_policy_matching_statements(node_or_service, resource_policy, action_to_check,
resource_to_check, condition_keys_to_check)
resource_to_check, prepped_condition_keys)
if len(matching_statements) == 0:
return ResourcePolicyEvalResult.NO_MATCH

Expand Down Expand Up @@ -776,6 +791,8 @@ def _principal_matches_in_statement(principal: Node, aws_principal_field: list):
return True
elif arns.get_account_id(principal.arn) == value:
return True
elif value == '*':
return True
else:
principal_root_str = 'arn:{}:iam::{}:root'.format(arns.get_partition(principal.arn),
arns.get_account_id(principal.arn))
Expand Down Expand Up @@ -805,7 +822,7 @@ def policies_include_matching_allow_action(principal: Node, action_to_check: str
return False


def _statement_matches_action(statement: dict, action: str, condition_keys: Optional[dict] = None,
def _statement_matches_action(statement: dict, action: str, condition_keys: Optional[CaseInsensitiveDict] = None,
is_resource_policy_check: bool = False) -> bool:
"""Helper function, returns True if the given action is in the given policy statement"""
exempted_services = ('sns', 'sqs')
Expand Down Expand Up @@ -853,7 +870,7 @@ def _statement_matches_action(statement: dict, action: str, condition_keys: Opti
return True


def _statement_matches_resource(statement: dict, resource: str, condition_keys: Optional[dict] = None) -> bool:
def _statement_matches_resource(statement: dict, resource: str, condition_keys: Optional[CaseInsensitiveDict] = None) -> bool:
"""Helper function, returns True if the given resource is in the given policy statement"""
if 'Resource' in statement:
for item in _listify_string(statement['Resource']):
Expand Down Expand Up @@ -889,7 +906,7 @@ def _compose_pattern(string_to_transform) -> Pattern:


def _matches_after_expansion(string_to_check: str, string_to_check_against: str,
condition_keys: Optional[dict] = None) -> bool:
condition_keys: Optional[CaseInsensitiveDict] = None) -> bool:
"""Helper function that checks the string_to_check against string_to_check_against.
Handles matching with respect to wildcards, variables.
Expand Down
1 change: 1 addition & 0 deletions principalmapper/querying/presets/connected.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ def print_connected_results(graph: Graph, source_nodes: List[Node], dest_nodes:
print('{} is able to access {}:'.format(snode.searchable_name(), dnode.searchable_name()))
for edge in path:
print(' {}'.format(edge.describe_edge()))
print()


def write_connected_results(graph: Graph, source_nodes: List[Node], dest_nodes: List[Node], skip_admins: bool = False,
Expand Down
9 changes: 7 additions & 2 deletions principalmapper/querying/presets/endgame.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

from principalmapper.common import Graph, Policy, Node
from principalmapper.querying import query_interface
from principalmapper.util.case_insensitive_dict import CaseInsensitiveDict

_service_resource_exposure_map = {
's3': {
Expand All @@ -37,6 +38,10 @@
'kms': {
'pattern': re.compile(r"^arn:aws:kms:[a-z0-9-]+:[0-9]+:key/.*"),
'actions': ['kms:PutKeyPolicy']
},
'secretsmanager': {
'pattern': re.compile(r"^arn:aws:secretsmanager:[a-z0-9-]+:[0-9]+:.*"),
'actions': ['secretsmanager:PutResourcePolicy']
}
}

Expand All @@ -50,7 +55,7 @@ def handle_preset_query(graph: Graph, tokens: List[str], skip_admins: bool = Fal
* "preset"
* "endgame"
* <service> : (*|s3|sns|sqs|kms)
* <service> : (*|s3|sns|sqs|kms|secretsmanager)
"""
endgame_map = compose_endgame_map(graph, tokens[2], skip_admins)
for policy, nodes in endgame_map.items():
Expand All @@ -74,7 +79,7 @@ def compose_endgame_map(graph: Graph, service_to_include: str = '*', skip_admins
node_confirmed = False

if 'conditions' not in node.cache:
node.cache['conditions'] = query_interface._infer_condition_keys(node, {})
node.cache['conditions'] = query_interface._infer_condition_keys(node, CaseInsensitiveDict())

if (not skip_admins) or (not node.is_admin):
for action in definition['actions']:
Expand Down
Loading

0 comments on commit 722efec

Please sign in to comment.