Skip to content

Commit b0ac6b0

Browse files
alextwoodsalextwoods
authored
Allow unquoted account ids in IAM Policies (#6662)
* WIP - just adding a test * Allow integer AWS accountIDs and boolean values in IamPolicyReader. * Add changelog * Remove support for unquoted accountIds with leading zeros (IAM rejects these)
1 parent fd5a479 commit b0ac6b0

3 files changed

Lines changed: 134 additions & 11 deletions

File tree

‎.changes/next-release/bugfix-IAMPolicyBuilder-60dd1f5.json‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"type": "bugfix",
3+
"category": "IAM Policy Builder",
4+
"contributor": "",
5+
"description": "Allow integer AWS account IDs and boolean values when reading IAM policies from JSON with `IamPolicyReader`."
6+
}

‎services-custom/iam-policy-builder/src/main/java/software/amazon/awssdk/policybuilder/iam/internal/DefaultIamPolicyReader.java‎

Lines changed: 36 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -111,10 +111,10 @@ private IamStatement readStatement(Map<String, JsonNode> statementObject) {
111111
.effect(getString(statementObject, "Effect"))
112112
.principals(readPrincipals(statementObject, "Principal"))
113113
.notPrincipals(readPrincipals(statementObject, "NotPrincipal"))
114-
.actionIds(readStringOrArrayAsList(statementObject, "Action"))
115-
.notActionIds(readStringOrArrayAsList(statementObject, "NotAction"))
116-
.resourceIds(readStringOrArrayAsList(statementObject, "Resource"))
117-
.notResourceIds(readStringOrArrayAsList(statementObject, "NotResource"))
114+
.actionIds(readStringOrArrayAsList(statementObject, "Action", false))
115+
.notActionIds(readStringOrArrayAsList(statementObject, "NotAction", false))
116+
.resourceIds(readStringOrArrayAsList(statementObject, "Resource", false))
117+
.notResourceIds(readStringOrArrayAsList(statementObject, "NotResource", false))
118118
.conditions(readConditions(statementObject.get("Condition")))
119119
.build();
120120
}
@@ -134,7 +134,7 @@ private List<IamPrincipal> readPrincipals(Map<String, JsonNode> statementObject,
134134
List<IamPrincipal> result = new ArrayList<>();
135135
Map<String, JsonNode> principalsNodeObject = principalsNode.asObject();
136136
principalsNodeObject.keySet().forEach(
137-
k -> result.addAll(IamPrincipal.createAll(k, readStringOrArrayAsList(principalsNodeObject, k)))
137+
k -> result.addAll(IamPrincipal.createAll(k, readStringOrArrayAsList(principalsNodeObject, k, true)))
138138
);
139139
return result;
140140
}
@@ -154,15 +154,15 @@ private List<IamCondition> readConditions(JsonNode conditionNode) {
154154
conditionObject.forEach((operator, keyValueNode) -> {
155155
Map<String, JsonNode> keyValueObject = expectObject(keyValueNode, "Condition key");
156156
keyValueObject.forEach((key, value) -> {
157-
if (value.isString()) {
158-
result.add(IamCondition.create(operator, key, value.asString()));
159-
} else if (value.isArray()) {
157+
if (value.isArray()) {
160158
List<String> values =
161159
value.asArray()
162160
.stream()
163-
.map(valueNode -> expectString(valueNode, "Condition values entry"))
161+
.map(valueNode -> expectConditionValue(valueNode, "Condition values entry"))
164162
.collect(toList());
165163
result.addAll(IamCondition.createAll(operator, key, values));
164+
} else {
165+
result.add(IamCondition.create(operator, key, expectConditionValue(value, "Condition value entry")));
166166
}
167167
});
168168

@@ -171,7 +171,7 @@ private List<IamCondition> readConditions(JsonNode conditionNode) {
171171
return result;
172172
}
173173

174-
private List<String> readStringOrArrayAsList(Map<String, JsonNode> statementObject, String nodeKey) {
174+
private List<String> readStringOrArrayAsList(Map<String, JsonNode> statementObject, String nodeKey, boolean allowAccountIds) {
175175
JsonNode node = statementObject.get(nodeKey);
176176

177177
if (node == null) {
@@ -185,7 +185,12 @@ private List<String> readStringOrArrayAsList(Map<String, JsonNode> statementObje
185185
if (node.isArray()) {
186186
return node.asArray()
187187
.stream()
188-
.map(n -> expectString(n, nodeKey + " entry"))
188+
.map(n -> {
189+
if (allowAccountIds) {
190+
return expectStringOrAccountId(n, nodeKey + " entry");
191+
}
192+
return expectString(n, nodeKey + " entry");
193+
})
189194
.collect(toList());
190195
}
191196

@@ -206,6 +211,26 @@ private String expectString(JsonNode node, String name) {
206211
return node.asString();
207212
}
208213

214+
private String expectStringOrAccountId(JsonNode node, String name) {
215+
if (node.isNumber()) {
216+
return node.asNumber();
217+
}
218+
Validate.isTrue(node.isString(), "%s was not a string", name);
219+
return node.asString();
220+
}
221+
222+
// condition values are generally String, however in some cases they may be an AWS accountID or a boolean.
223+
private String expectConditionValue(JsonNode node, String name) {
224+
if (node.isNumber()) {
225+
return node.asNumber();
226+
}
227+
if (node.isBoolean()) {
228+
return Boolean.toString(node.asBoolean());
229+
}
230+
Validate.isTrue(node.isString(), "%s was not a string", name);
231+
return node.asString();
232+
}
233+
209234
private Map<String, JsonNode> expectObject(JsonNode node, String name) {
210235
Validate.isTrue(node.isObject(), "%s was not an object", name);
211236
return node.asObject();

‎services-custom/iam-policy-builder/src/test/java/software/amazon/awssdk/policybuilder/iam/IamPolicyReaderTest.java‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,11 @@
1818
import static java.util.Arrays.asList;
1919
import static java.util.Collections.singletonList;
2020
import static org.assertj.core.api.Assertions.assertThat;
21+
import static org.junit.jupiter.api.Assertions.assertEquals;
22+
import static org.junit.jupiter.api.Assertions.assertThrows;
2123
import static software.amazon.awssdk.policybuilder.iam.IamEffect.ALLOW;
2224

25+
import java.io.UncheckedIOException;
2326
import org.junit.jupiter.api.Test;
2427

2528
class IamPolicyReaderTest {
@@ -89,6 +92,34 @@ class IamPolicyReaderTest {
8992
.statements(singletonList(ONE_ELEMENT_LISTS_STATEMENT))
9093
.build();
9194

95+
private static final IamPolicy INTEGER_ACCOUNT_ID_POLICY =
96+
IamPolicy.builder()
97+
.version("Version")
98+
.statements(singletonList(
99+
IamStatement
100+
.builder()
101+
.effect("Allow")
102+
.principals(
103+
IamPrincipal.createAll("AWS", asList("123456789012", "555555555555")))
104+
.addCondition(
105+
IamCondition.create("StringNotEquals", "aws:PrincipalAccount", "123456789012"))
106+
.build()
107+
))
108+
.build();
109+
110+
private static final IamPolicy BOOLEAN_CONDITION_POLICY =
111+
IamPolicy.builder()
112+
.version("Version")
113+
.statements(singletonList(
114+
IamStatement
115+
.builder()
116+
.effect("Allow")
117+
.addCondition(
118+
IamCondition.create("Bool", "aws:SecureTransport", "true"))
119+
.build()
120+
))
121+
.build();
122+
92123
private static final IamStatement COMPOUND_PRINCIPAL_STATEMENT =
93124
IamStatement.builder()
94125
.effect(ALLOW)
@@ -174,6 +205,67 @@ public void readMinimalPolicyWorks() {
174205
.isEqualTo(MINIMAL_POLICY);
175206
}
176207

208+
@Test
209+
public void readPolicyWithIntegerAccountsWorks() {
210+
assertThat(READER.read("{\n"
211+
+ " \"Version\" : \"Version\",\n"
212+
+ " \"Statement\" : {\n"
213+
+ " \"Effect\" : \"Allow\",\n"
214+
+ " \"Principal\" : { \n"
215+
+ " \"AWS\": [ \n"
216+
+ " 123456789012,\n"
217+
+ " \"555555555555\" \n"
218+
+ " ]\n"
219+
+ " },\n"
220+
+ " \"Condition\" : {\n"
221+
+ " \"StringNotEquals\": {\n"
222+
+ " \"aws:PrincipalAccount\": [\n"
223+
+ " 123456789012\n"
224+
+ " ]\n"
225+
+ " }\n"
226+
+ " }\n"
227+
+ " }\n"
228+
+ "}")).isEqualTo(INTEGER_ACCOUNT_ID_POLICY);
229+
}
230+
231+
@Test
232+
public void readPolicyWithLeadingZeroIntegerFails() {
233+
// while accountIds may have leading zeros, IAM rejects policy documents with
234+
// unquoted accountIDs with leading zeros because these are invalid json.
235+
Exception e = assertThrows(Exception.class, () ->
236+
{
237+
READER.read("{\n"
238+
+ " \"Version\" : \"Version\",\n"
239+
+ " \"Statement\" : {\n"
240+
+ " \"Effect\" : \"Allow\",\n"
241+
+ " \"Condition\" : {\n"
242+
+ " \"StringNotEquals\": {\n"
243+
+ " \"aws:PrincipalAccount\": [\n"
244+
+ " 001234567890\n"
245+
+ " ]\n"
246+
+ " }\n"
247+
+ " }\n"
248+
+ " }\n"
249+
+ "}");
250+
});
251+
assertThat(e.getMessage()).contains("Invalid numeric value: Leading zeroes not allowed");
252+
}
253+
254+
@Test
255+
public void readPolicyWithBooleanConditionWorks() {
256+
assertThat(READER.read("{\n"
257+
+ " \"Version\" : \"Version\",\n"
258+
+ " \"Statement\" : {\n"
259+
+ " \"Effect\" : \"Allow\",\n"
260+
+ " \"Condition\" : {\n"
261+
+ " \"Bool\": {\n"
262+
+ " \"aws:SecureTransport\": true\n"
263+
+ " }\n"
264+
+ " }\n"
265+
+ " }\n"
266+
+ "}")).isEqualTo(BOOLEAN_CONDITION_POLICY);
267+
}
268+
177269
@Test
178270
public void readCompoundPrincipalsWorks() {
179271
assertThat(READER.read("{\n" +

0 commit comments

Comments
 (0)