IAM: Urlencode IAM policies in responses to match AWS

dfangl commented 1 month ago

Motivation

Currently, moto does not urlencode (or quote) IAM policy strings in its return values.

While this seemingly matches the AWS behavior, as described here: https://docs.aws.amazon.com/IAM/latest/APIReference/API_GetRolePolicy.html , more investigation shows AWS actually quoting the IAM policies, as shown in responses (printed by boto3 in debug mode):

<GetUserPolicyResponse xmlns="https://iam.amazonaws.com/doc/2010-05-08/">\n  <GetUserPolicyResult>\n    <PolicyDocument>%7B%22Version%22%3A%20%222012-10-17%22%2C%20%22Statement%22%3A%20%5B%7B%22Effect%22%3A%20%22Allow%22%2C%20%22Action%22%3A%20%5B%22apigatway%3APUT%22%5D%2C%20%22Resource%22%3A%20%5B%22arn%3Aaws%3Aapigateway%3Aeu-central-1%3A%3A%2Ftags%2Farn%253Aaws%253Aapigateway%253Aeu-central-1%253A%253A%252Frestapis%252Faaeeieije%22%5D%7D%5D%7D</PolicyDocument>\n    <PolicyName>test-policy-e6e63d20</PolicyName>\n    <UserName>test-user-f7e28e2a</UserName>\n  </GetUserPolicyResult>\n  <ResponseMetadata>\n    <RequestId>f3802db8-a14c-464e-a771-ff7fe32b6258</RequestId>\n  </ResponseMetadata>\n</GetUserPolicyResponse>\n

<GetRolePolicyResponse xmlns="https://iam.amazonaws.com/doc/2010-05-08/">\n  <GetRolePolicyResult>\n    <PolicyDocument>%7B%22Version%22%3A%20%222012-10-17%22%2C%20%22Statement%22%3A%20%5B%7B%22Effect%22%3A%20%22Allow%22%2C%20%22Action%22%3A%20%5B%22apigatway%3APUT%22%5D%2C%20%22Resource%22%3A%20%5B%22arn%3Aaws%3Aapigateway%3Aeu-central-1%3A%3A%2Ftags%2Farn%253Aaws%253Aapigateway%253Aeu-central-1%253A%253A%252Frestapis%252Faaeeieije%22%5D%7D%5D%7D</PolicyDocument>\n    <PolicyName>test-policy-deb509f4</PolicyName>\n    <RoleName>test-role-c91b9d39</RoleName>\n  </GetRolePolicyResult>\n  <ResponseMetadata>\n    <RequestId>39d72c56-b704-4f5b-b877-faed38d66107</RequestId>\n  </ResponseMetadata>\n</GetRolePolicyResponse>\n

<GetGroupPolicyResponse xmlns="https://iam.amazonaws.com/doc/2010-05-08/">\n  <GetGroupPolicyResult>\n    <PolicyDocument>%7B%22Version%22%3A%20%222012-10-17%22%2C%20%22Statement%22%3A%20%5B%7B%22Effect%22%3A%20%22Allow%22%2C%20%22Action%22%3A%20%5B%22apigatway%3APUT%22%5D%2C%20%22Resource%22%3A%20%5B%22arn%3Aaws%3Aapigateway%3Aeu-central-1%3A%3A%2Ftags%2Farn%253Aaws%253Aapigateway%253Aeu-central-1%253A%253A%252Frestapis%252Faaeeieije%22%5D%7D%5D%7D</PolicyDocument>\n    <GroupName>test-group-aef07898</GroupName>\n    <PolicyName>test-policy-ce391a0a</PolicyName>\n  </GetGroupPolicyResult>\n  <ResponseMetadata>\n    <RequestId>43faabdb-8ec0-4268-b0c1-8fc4f8d5c9ce</RequestId>\n  </ResponseMetadata>\n</GetGroupPolicyResponse>\n

<GetRoleResponse xmlns="https://iam.amazonaws.com/doc/2010-05-08/">\n  <GetRoleResult>\n    <Role>\n      <Path>/</Path>\n      <AssumeRolePolicyDocument>%7B%22Version%22%3A%222012-10-17%22%2C%22Statement%22%3A%5B%7B%22Effect%22%3A%22Allow%22%2C%22Principal%22%3A%7B%22Service%22%3A%22lambda.amazonaws.com%22%7D%2C%22Action%22%3A%22sts%3AAssumeRole%22%2C%22Condition%22%3A%7B%22StringEquals%22%3A%7B%22aws%3ASourceArn%22%3A%22arn%253Aaws%253Aapigateway%253Aeu-central-1%253A%253A%252Frestapis%252Faaeeieije%22%7D%7D%7D%5D%7D</AssumeRolePolicyDocument>\n      <MaxSessionDuration>3600</MaxSessionDuration>\n      <RoleId>AROA4U6S5KWRHJDLN2CH2</RoleId>\n      <RoleLastUsed/>\n      <RoleName>test-role-607dbc4b</RoleName>\n      <Arn>arn:aws:iam::869636330914:role/test-role-607dbc4b</Arn>\n      <CreateDate>2024-09-25T15:10:45Z</CreateDate>\n    </Role>\n  </GetRoleResult>\n  <ResponseMetadata>\n    <RequestId>48170cd2-4fe2-456b-8132-abd1d739e77b</RequestId>\n  </ResponseMetadata>\n</GetRoleResponse>\n

While this does not impact many users, the disparity shows when you try to set a field in the IAM policies to a urlencoded value, like necessary for matching the resource for the apigateway.TagResource operation: https://docs.aws.amazon.com/service-authorization/latest/reference/list_amazonapigatewaymanagement.html#amazonapigatewaymanagement-Tags

Since boto3 will unquote the string, it does not matches the input anymore.

Changes

Urlencode returned policy documents, so a single decode will still keep the already-urlencoded parts of the policy correct.
Add tests testing this behavior by adding policy documents with urlencoded parts, and asserting that the response matches the input.

dfangl commented 1 month ago

There are some repercussions in 3 other tests, I will verify them and the correct behavior tomorrow.

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.54%. Comparing base (d0affa8) to head (05a82a9). Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
moto/iam/models.py	87.50%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #8157 +/- ## ========================================== - Coverage 94.54% 94.54% -0.01% ========================================== Files 1158 1158 Lines 100093 100100 +7 ========================================== + Hits 94633 94639 +6 - Misses 5460 5461 +1 ``` | [Flag](https://app.codecov.io/gh/getmoto/moto/pull/8157/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getmoto) | Coverage Δ | | |---|---|---| | [servertests](https://app.codecov.io/gh/getmoto/moto/pull/8157/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getmoto) | `28.88% <25.00%> (-0.01%)` | :arrow_down: | | [unittests](https://app.codecov.io/gh/getmoto/moto/pull/8157/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getmoto) | `94.51% <87.50%> (-0.01%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getmoto#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dfangl commented 1 month ago

So, everything seems to be in order, the TypeError which is not covered is required per specification of the default method, but should never be reached in our case.

getmoto / moto