From 7a620ac8b37c68a5c1372949f5d87647411c1c58 Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Fri, 23 Jun 2023 09:55:29 +0200 Subject: [PATCH] Refactor ARN validation code (#1619) Refactor ARN validation code SUMMARY Adds resource_id and resource_type to parse_aws_arn() return value. Adds validate_aws_arn() to handle common pattern matching for ARNs. ISSUE TYPE Feature Pull Request COMPONENT NAME ec2_instance iam_user ADDITIONAL INFORMATION Related to ansible-collections/community.aws#1846 - We've been doing things like assuming the aws partition. Reviewed-by: Alina Buzachis (cherry picked from commit 344dbd1a0f6cc6c2e6cfd582ba989f070001444a) --- changelogs/fragments/1846-arn-validation.yml | 5 + plugins/module_utils/arn.py | 38 +++ plugins/modules/ec2_instance.py | 5 +- plugins/modules/iam_user.py | 7 +- .../module_utils/arn/test_parse_aws_arn.py | 186 +++++++++++++-- .../module_utils/arn/test_validate_aws_arn.py | 217 ++++++++++++++++++ .../ec2_instance/test_determine_iam_role.py | 4 +- 7 files changed, 437 insertions(+), 25 deletions(-) create mode 100644 changelogs/fragments/1846-arn-validation.yml create mode 100644 tests/unit/module_utils/arn/test_validate_aws_arn.py diff --git a/changelogs/fragments/1846-arn-validation.yml b/changelogs/fragments/1846-arn-validation.yml new file mode 100644 index 0000000000..56be4417e2 --- /dev/null +++ b/changelogs/fragments/1846-arn-validation.yml @@ -0,0 +1,5 @@ +minor_changes: +- ec2_instance - refactored ARN validation handling (https://github.com/ansible-collections/amazon.aws/pull/1619). +- iam_user - refactored ARN validation handling (https://github.com/ansible-collections/amazon.aws/pull/1619). +- module_utils.arn - added ``validate_aws_arn`` function to handle common pattern matching for ARNs (https://github.com/ansible-collections/amazon.aws/pull/1619). +- module_utils.arn - add ``resource_id`` and ``resource_type`` to ``parse_aws_arn`` return values (https://github.com/ansible-collections/amazon.aws/pull/1619). diff --git a/plugins/module_utils/arn.py b/plugins/module_utils/arn.py index 22d91b3f36..d62b4c4d80 100644 --- a/plugins/module_utils/arn.py +++ b/plugins/module_utils/arn.py @@ -6,14 +6,46 @@ import re +def validate_aws_arn( + arn, partition=None, service=None, region=None, account_id=None, resource=None, resource_type=None, resource_id=None +): + details = parse_aws_arn(arn) + + if not details: + return False + + if partition and details.get("partition") != partition: + return False + if service and details.get("service") != service: + return False + if region and details.get("region") != region: + return False + if account_id and details.get("account_id") != account_id: + return False + if resource and details.get("resource") != resource: + return False + if resource_type and details.get("resource_type") != resource_type: + return False + if resource_id and details.get("resource_id") != resource_id: + return False + + return True + + def parse_aws_arn(arn): """ + Based on https://docs.aws.amazon.com/IAM/latest/UserGuide/reference-arns.html + The following are the general formats for ARNs. arn:partition:service:region:account-id:resource-id arn:partition:service:region:account-id:resource-type/resource-id arn:partition:service:region:account-id:resource-type:resource-id The specific formats depend on the resource. The ARNs for some resources omit the Region, the account ID, or both the Region and the account ID. + + Note: resource_type handling is very naive, for complex cases it may be necessary to use + "resource" directly instead of resource_type, this will include the resource type and full ID, + including all paths. """ m = re.search(r"arn:(aws(-([a-z\-]+))?):([\w-]+):([a-z0-9\-]*):(\d*|aws|aws-managed):(.*)", arn) if m is None: @@ -25,6 +57,12 @@ def parse_aws_arn(arn): result.update(dict(account_id=m.group(6))) result.update(dict(resource=m.group(7))) + m2 = re.search(r"^(.*?)[:/](.+)$", m.group(7)) + if m2 is None: + result.update(dict(resource_type=None, resource_id=m.group(7))) + else: + result.update(dict(resource_type=m2.group(1), resource_id=m2.group(2))) + return result diff --git a/plugins/modules/ec2_instance.py b/plugins/modules/ec2_instance.py index a70b6bf15d..6c5a7e9f71 100644 --- a/plugins/modules/ec2_instance.py +++ b/plugins/modules/ec2_instance.py @@ -975,7 +975,7 @@ from ansible.module_utils.common.dict_transformations import snake_dict_to_camel_dict from ansible.module_utils.six import string_types -from ansible_collections.amazon.aws.plugins.module_utils.arn import parse_aws_arn +from ansible_collections.amazon.aws.plugins.module_utils.arn import validate_aws_arn from ansible_collections.amazon.aws.plugins.module_utils.botocore import is_boto3_error_code from ansible_collections.amazon.aws.plugins.module_utils.botocore import is_boto3_error_message from ansible_collections.amazon.aws.plugins.module_utils.ec2 import ensure_ec2_tags @@ -1791,8 +1791,7 @@ def pretty_instance(i): def determine_iam_role(name_or_arn): - result = parse_aws_arn(name_or_arn) - if result and result["service"] == "iam" and result["resource"].startswith("instance-profile/"): + if validate_aws_arn(name_or_arn, service="iam", resource_type="instance-profile"): return name_or_arn iam = module.client("iam", retry_decorator=AWSRetry.jittered_backoff()) try: diff --git a/plugins/modules/iam_user.py b/plugins/modules/iam_user.py index 75d412a6bc..f4f1483cdd 100644 --- a/plugins/modules/iam_user.py +++ b/plugins/modules/iam_user.py @@ -182,8 +182,9 @@ from ansible.module_utils.common.dict_transformations import camel_dict_to_snake_dict -from ansible_collections.amazon.aws.plugins.module_utils.modules import AnsibleAWSModule +from ansible_collections.amazon.aws.plugins.module_utils.arn import validate_aws_arn from ansible_collections.amazon.aws.plugins.module_utils.botocore import is_boto3_error_code +from ansible_collections.amazon.aws.plugins.module_utils.modules import AnsibleAWSModule from ansible_collections.amazon.aws.plugins.module_utils.tagging import ansible_dict_to_boto3_tag_list from ansible_collections.amazon.aws.plugins.module_utils.tagging import boto3_tag_list_to_ansible_dict from ansible_collections.amazon.aws.plugins.module_utils.tagging import compare_aws_tags @@ -208,7 +209,7 @@ def convert_friendly_names_to_arns(connection, module, policy_names): # List comprehension that looks for any policy in the 'policy_names' list # that does not begin with 'arn'. If there aren't any, short circuit. # If there are, translate friendly name to the full arn - if not any(not policy.startswith("arn:") for policy in policy_names if policy is not None): + if all(validate_aws_arn(policy, service="iam") for policy in policy_names if policy is not None): return policy_names allpolicies = {} paginator = connection.get_paginator("list_policies") @@ -218,7 +219,7 @@ def convert_friendly_names_to_arns(connection, module, policy_names): allpolicies[policy["PolicyName"]] = policy["Arn"] allpolicies[policy["Arn"]] = policy["Arn"] try: - return [allpolicies[policy] for policy in policy_names] + return [allpolicies[policy] for policy in policy_names if policy is not None] except KeyError as e: module.fail_json(msg="Couldn't find policy: " + str(e)) diff --git a/tests/unit/module_utils/arn/test_parse_aws_arn.py b/tests/unit/module_utils/arn/test_parse_aws_arn.py index 49375a855d..cc4b405765 100644 --- a/tests/unit/module_utils/arn/test_parse_aws_arn.py +++ b/tests/unit/module_utils/arn/test_parse_aws_arn.py @@ -24,6 +24,8 @@ region="us-east-1", account_id="123456789012", resource="outpost/op-1234567890abcdef0", + resource_type="outpost", + resource_id="op-1234567890abcdef0", ), dict( partition="aws-gov", @@ -31,6 +33,8 @@ region="us-gov-east-1", account_id="123456789012", resource="outpost/op-1234567890abcdef0", + resource_type="outpost", + resource_id="op-1234567890abcdef0", ), dict( partition="aws-cn", @@ -38,6 +42,8 @@ region="us-east-1", account_id="123456789012", resource="outpost/op-1234567890abcdef0", + resource_type="outpost", + resource_id="op-1234567890abcdef0", ), # Start the account ID with 0s, it's a 12 digit *string*, if someone treats # it as an integer the leading 0s can disappear. @@ -47,21 +53,93 @@ region="us-east-1", account_id="000123000123", resource="outpost/op-1234567890abcdef0", + resource_type="outpost", + resource_id="op-1234567890abcdef0", ), # S3 doesn't "need" region/account_id as bucket names are globally unique - dict(partition="aws", service="s3", region="", account_id="", resource="bucket/object"), + dict( + partition="aws", + service="s3", + region="", + account_id="", + resource="bucket/object", + resource_type="bucket", + resource_id="object", + ), # IAM is a 'global' service, so the ARNs don't have regions - dict(partition="aws", service="iam", region="", account_id="123456789012", resource="policy/foo/bar/PolicyName"), dict( - partition="aws", service="iam", region="", account_id="123456789012", resource="instance-profile/ExampleProfile" + partition="aws", + service="iam", + region="", + account_id="123456789012", + resource="policy/foo/bar/PolicyName", + resource_type="policy", + resource_id="foo/bar/PolicyName", + ), + dict( + partition="aws", + service="iam", + region="", + account_id="123456789012", + resource="instance-profile/ExampleProfile", + resource_type="instance-profile", + resource_id="ExampleProfile", + ), + dict( + partition="aws", + service="iam", + region="", + account_id="123456789012", + resource="root", + resource_type=None, + resource_id="root", ), - dict(partition="aws", service="iam", region="", account_id="123456789012", resource="root"), # Some examples with different regions - dict(partition="aws", service="sqs", region="eu-west-3", account_id="123456789012", resource="example-queue"), - dict(partition="aws", service="sqs", region="us-gov-east-1", account_id="123456789012", resource="example-queue"), - dict(partition="aws", service="sqs", region="sa-east-1", account_id="123456789012", resource="example-queue"), - dict(partition="aws", service="sqs", region="ap-northeast-2", account_id="123456789012", resource="example-queue"), - dict(partition="aws", service="sqs", region="ca-central-1", account_id="123456789012", resource="example-queue"), + dict( + partition="aws", + service="sqs", + region="eu-west-3", + account_id="123456789012", + resource="example-queue", + resource_type=None, + resource_id="example-queue", + ), + dict( + partition="aws", + service="sqs", + region="us-gov-east-1", + account_id="123456789012", + resource="example-queue", + resource_type=None, + resource_id="example-queue", + ), + dict( + partition="aws", + service="sqs", + region="sa-east-1", + account_id="123456789012", + resource="example-queue", + resource_type=None, + resource_id="example-queue", + ), + dict( + partition="aws", + service="sqs", + region="ap-northeast-2", + account_id="123456789012", + resource="example-queue", + resource_type=None, + resource_id="example-queue", + ), + dict( + partition="aws", + service="sqs", + region="ca-central-1", + account_id="123456789012", + resource="example-queue", + resource_type=None, + resource_id="example-queue", + ), # Some more unusual service names dict( partition="aws", @@ -69,6 +147,8 @@ region="us-east-1", account_id="123456789012", resource="stateful-rulegroup/ExampleDomainList", + resource_type="stateful-rulegroup", + resource_id="ExampleDomainList", ), dict( partition="aws", @@ -76,6 +156,8 @@ region="us-east-1", account_id="123456789012", resource="group/group-name", + resource_type="group", + resource_id="group-name", ), # A special case for resources AWS curate dict( @@ -84,22 +166,90 @@ region="us-east-1", account_id="aws-managed", resource="stateful-rulegroup/BotNetCommandAndControlDomainsActionOrder", + resource_type="stateful-rulegroup", + resource_id="BotNetCommandAndControlDomainsActionOrder", + ), + dict( + partition="aws", + service="iam", + region="", + account_id="aws", + resource="policy/AWSDirectConnectReadOnlyAccess", + resource_type="policy", + resource_id="AWSDirectConnectReadOnlyAccess", ), - dict(partition="aws", service="iam", region="", account_id="aws", resource="policy/AWSDirectConnectReadOnlyAccess"), # Examples merged in from test_arn.py - dict(partition="aws-us-gov", service="iam", region="", account_id="0123456789", resource="role/foo-role"), - dict(partition="aws", service="iam", region="", account_id="123456789012", resource="user/dev/*"), - dict(partition="aws", service="iam", region="", account_id="123456789012", resource="user:test"), - dict(partition="aws-cn", service="iam", region="", account_id="123456789012", resource="user:test"), - dict(partition="aws", service="iam", region="", account_id="123456789012", resource="user"), - dict(partition="aws", service="s3", region="", account_id="", resource="my_corporate_bucket/*"), - dict(partition="aws", service="s3", region="", account_id="", resource="my_corporate_bucket/Development/*"), + dict( + partition="aws-us-gov", + service="iam", + region="", + account_id="0123456789", + resource="role/foo-role", + resource_type="role", + resource_id="foo-role", + ), + dict( + partition="aws", + service="iam", + region="", + account_id="123456789012", + resource="user/dev/*", + resource_type="user", + resource_id="dev/*", + ), + dict( + partition="aws", + service="iam", + region="", + account_id="123456789012", + resource="user:test", + resource_type="user", + resource_id="test", + ), + dict( + partition="aws-cn", + service="iam", + region="", + account_id="123456789012", + resource="user:test", + resource_type="user", + resource_id="test", + ), + dict( + partition="aws", + service="iam", + region="", + account_id="123456789012", + resource="user", + resource_type=None, + resource_id="user", + ), + dict( + partition="aws", + service="s3", + region="", + account_id="", + resource="my_corporate_bucket/*", + resource_type="my_corporate_bucket", + resource_id="*", + ), + dict( + partition="aws", + service="s3", + region="", + account_id="", + resource="my_corporate_bucket/Development/*", + resource_type="my_corporate_bucket", + resource_id="Development/*", + ), dict( partition="aws", service="rds", region="es-east-1", account_id="000000000000", resource="snapshot:rds:my-db-snapshot", + resource_type="snapshot", + resource_id="rds:my-db-snapshot", ), dict( partition="aws", @@ -107,6 +257,8 @@ region="us-east-1", account_id="012345678901", resource="changeSet/Ansible-StackName-c6884247ede41eb0", + resource_type="changeSet", + resource_id="Ansible-StackName-c6884247ede41eb0", ), ] diff --git a/tests/unit/module_utils/arn/test_validate_aws_arn.py b/tests/unit/module_utils/arn/test_validate_aws_arn.py new file mode 100644 index 0000000000..d730ee6372 --- /dev/null +++ b/tests/unit/module_utils/arn/test_validate_aws_arn.py @@ -0,0 +1,217 @@ +# (c) 2022 Red Hat Inc. +# +# This file is part of Ansible +# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) + +import pytest + +from ansible_collections.amazon.aws.plugins.module_utils.arn import validate_aws_arn + +arn_test_inputs = [ + # Just test it's a valid ARN + ("arn:aws:outposts:us-east-1:123456789012:outpost/op-1234567890abcdef0", True, None), + # Bad ARN + ("arn:was:outposts:us-east-1:123456789012:outpost/op-1234567890abcdef0", False, None), + # Individual options + ( + "arn:aws:outposts:us-east-1:123456789012:outpost/op-1234567890abcdef0", + True, + {"partition": "aws"}, + ), + ( + "arn:aws:outposts:us-east-1:123456789012:outpost/op-1234567890abcdef0", + False, + {"partition": "aws-cn"}, + ), + ( + "arn:aws:outposts:us-east-1:123456789012:outpost/op-1234567890abcdef0", + True, + {"service": "outposts"}, + ), + ( + "arn:aws:outposts:us-east-1:123456789012:outpost/op-1234567890abcdef0", + False, + {"service": "iam"}, + ), + ( + "arn:aws:outposts:us-east-1:123456789012:outpost/op-1234567890abcdef0", + True, + {"region": "us-east-1"}, + ), + ( + "arn:aws:outposts:us-east-1:123456789012:outpost/op-1234567890abcdef0", + False, + {"region": "us-east-2"}, + ), + ( + "arn:aws:outposts:us-east-1:123456789012:outpost/op-1234567890abcdef0", + True, + {"account_id": "123456789012"}, + ), + ( + "arn:aws:outposts:us-east-1:123456789012:outpost/op-1234567890abcdef0", + False, + {"account_id": "111111111111"}, + ), + ( + "arn:aws:outposts:us-east-1:123456789012:outpost/op-1234567890abcdef0", + True, + {"resource": "outpost/op-1234567890abcdef0"}, + ), + ( + "arn:aws:outposts:us-east-1:123456789012:outpost/op-1234567890abcdef0", + False, + {"resource": "outpost/op-11111111111111111"}, + ), + ( + "arn:aws:outposts:us-east-1:123456789012:outpost/op-1234567890abcdef0", + True, + {"resource_type": "outpost"}, + ), + ( + "arn:aws:outposts:us-east-1:123456789012:outpost/op-1234567890abcdef0", + False, + {"resource_type": "notpost"}, + ), + ( + "arn:aws:outposts:us-east-1:123456789012:outpost/op-1234567890abcdef0", + True, + {"resource_id": "op-1234567890abcdef0"}, + ), + ( + "arn:aws:outposts:us-east-1:123456789012:outpost/op-1234567890abcdef0", + False, + {"resource_id": "op-11111111111111111"}, + ), + ( + "arn:aws:states:us-west-2:123456789012:stateMachine:HelloWorldStateMachine", + True, + {"resource_type": "stateMachine"}, + ), + ( + "arn:aws:states:us-west-2:123456789012:stateMachine:HelloWorldStateMachine", + False, + {"resource_type": "nopeMachine"}, + ), + ( + "arn:aws:states:us-west-2:123456789012:stateMachine:HelloWorldStateMachine", + True, + {"resource_id": "HelloWorldStateMachine"}, + ), + ( + "arn:aws:states:us-west-2:123456789012:stateMachine:HelloWorldStateMachine", + False, + {"resource_id": "CruelWorldStateMachine"}, + ), + # All options + ( + "arn:aws:outposts:us-east-1:123456789012:outpost/op-1234567890abcdef0", + True, + { + "partition": "aws", + "service": "outposts", + "region": "us-east-1", + "account_id": "123456789012", + "resource": "outpost/op-1234567890abcdef0", + "resource_type": "outpost", + "resource_id": "op-1234567890abcdef0", + }, + ), + ( + "arn:aws:outposts:us-east-1:123456789012:outpost/op-1234567890abcdef0", + False, + { + "partition": "aws-cn", + "service": "outposts", + "region": "us-east-1", + "account_id": "123456789012", + "resource": "outpost/op-1234567890abcdef0", + "resource_type": "outpost", + "resource_id": "op-1234567890abcdef0", + }, + ), + ( + "arn:aws:outposts:us-east-1:123456789012:outpost/op-1234567890abcdef0", + False, + { + "partition": "aws", + "service": "iam", + "region": "us-east-1", + "account_id": "123456789012", + "resource": "outpost/op-1234567890abcdef0", + "resource_type": "outpost", + "resource_id": "op-1234567890abcdef0", + }, + ), + ( + "arn:aws:outposts:us-east-1:123456789012:outpost/op-1234567890abcdef0", + False, + { + "partition": "aws", + "service": "outposts", + "region": "us-east-2", + "account_id": "123456789012", + "resource": "outpost/op-1234567890abcdef0", + "resource_type": "outpost", + "resource_id": "op-1234567890abcdef0", + }, + ), + ( + "arn:aws:outposts:us-east-1:123456789012:outpost/op-1234567890abcdef0", + False, + { + "partition": "aws", + "service": "outposts", + "region": "us-east-1", + "account_id": "111111111111", + "resource": "outpost/op-1234567890abcdef0", + "resource_type": "outpost", + "resource_id": "op-1234567890abcdef0", + }, + ), + ( + "arn:aws:outposts:us-east-1:123456789012:outpost/op-1234567890abcdef0", + False, + { + "partition": "aws", + "service": "outposts", + "region": "us-east-1", + "account_id": "123456789012", + "resource": "outpost/op-11111111111111111", + "resource_type": "outpost", + "resource_id": "op-1234567890abcdef0", + }, + ), + ( + "arn:aws:outposts:us-east-1:123456789012:outpost/op-1234567890abcdef0", + False, + { + "partition": "aws", + "service": "outposts", + "region": "us-east-1", + "account_id": "123456789012", + "resource": "outpost/op-1234567890abcdef0", + "resource_type": "notpost", + "resource_id": "op-1234567890abcdef0", + }, + ), + ( + "arn:aws:outposts:us-east-1:123456789012:outpost/op-1234567890abcdef0", + False, + { + "partition": "aws", + "service": "outposts", + "region": "us-east-1", + "account_id": "123456789012", + "resource": "outpost/op-1234567890abcdef0", + "resource_type": "outpost", + "resource_id": "op-11111111111111111", + }, + ), +] + + +@pytest.mark.parametrize("arn, result, kwargs", arn_test_inputs) +def test_validate_aws_arn(arn, result, kwargs): + kwargs = kwargs or {} + assert validate_aws_arn(arn, **kwargs) == result diff --git a/tests/unit/plugins/modules/ec2_instance/test_determine_iam_role.py b/tests/unit/plugins/modules/ec2_instance/test_determine_iam_role.py index 66bf66acec..0bd9a284e8 100644 --- a/tests/unit/plugins/modules/ec2_instance/test_determine_iam_role.py +++ b/tests/unit/plugins/modules/ec2_instance/test_determine_iam_role.py @@ -51,7 +51,7 @@ def __init__(self): @pytest.fixture def ec2_instance(monkeypatch): - monkeypatch.setattr(ec2_instance_module, "parse_aws_arn", lambda arn: None) + monkeypatch.setattr(ec2_instance_module, "validate_aws_arn", lambda arn, service, resource_type: None) monkeypatch.setattr(ec2_instance_module, "module", MagicMock()) ec2_instance_module.module.fail_json.side_effect = FailJsonException() ec2_instance_module.module.fail_json_aws.side_effect = FailJsonException() @@ -60,7 +60,7 @@ def ec2_instance(monkeypatch): def test_determine_iam_role_arn(params_object, ec2_instance, monkeypatch): # Revert the default monkey patch to make it simple to try passing a valid ARNs - monkeypatch.setattr(ec2_instance, "parse_aws_arn", utils_arn.parse_aws_arn) + monkeypatch.setattr(ec2_instance, "validate_aws_arn", utils_arn.validate_aws_arn) # Simplest example, someone passes a valid instance profile ARN arn = ec2_instance.determine_iam_role("arn:aws:iam::123456789012:instance-profile/myprofile")