Skip to content
This repository has been archived by the owner on Jan 12, 2024. It is now read-only.

Commit

Permalink
* fix issue where removing an assumed-role's SDB permissions will mak… (
Browse files Browse the repository at this point in the history
#182)

* fix issue where removing an assumed-role's SDB permissions will make the assumed-role lose access to the SDB even if the base IAM role has permissions
* fix issue where an assumed-role doesn't inherit base IAM role's admin privileges
* fix STS auth API doc and API error message
  • Loading branch information
mayitbeegh authored Jan 9, 2019
1 parent baccaf3 commit f5c1ec9
Show file tree
Hide file tree
Showing 17 changed files with 324 additions and 47 deletions.
2 changes: 1 addition & 1 deletion API.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ as well as in the [integration tests](https://github.com/Nike-Inc/cerberus-integ

### Authenticate with Cerberus as an App [POST]

This endpoint takes a [signed AWS STS get-caller-identity POST request](https://docs.aws.amazon.com/general/latest/gr/signature-version-4.html) and returns a payload containing an auth token needed to access Cerberus. Region is limited to "us-east-1" only and host is limited to "host:sts.amazonaws.com" only during the signing process.
This endpoint takes a [signed AWS STS get-caller-identity POST request](https://docs.aws.amazon.com/general/latest/gr/signature-version-4.html) and returns a payload containing an auth token needed to access Cerberus. Host is limited to regional endpoints ("host:sts.{region}.amazonaws.com") during the signing process.

+ Request (application/json)

Expand Down
2 changes: 1 addition & 1 deletion gradle.properties
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,6 @@
# limitations under the License.
#

version=3.25.1
version=3.25.2
groupId=com.nike.cerberus
artifactId=cms
4 changes: 4 additions & 0 deletions src/main/java/com/nike/cerberus/dao/PermissionsDao.java
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ public Boolean doesIamPrincipalHaveRoleForSdb(String sdbId, String iamPrincipalA
return permissionsMapper.doesIamPrincipalHaveGivenRoleForSdb(sdbId, iamPrincipalArn, iamRootArn, rolesThatAllowPermission);
}

public Boolean doesAssumedRoleHaveRoleForSdb(String sdbId, String assumedRoleArn, String iamRoleArn, String iamRootArn, Set<String> rolesThatAllowPermission) {
return permissionsMapper.doesAssumedRoleHaveGivenRoleForSdb(sdbId, assumedRoleArn, iamRoleArn, iamRootArn, rolesThatAllowPermission);
}

public Boolean doesUserPrincipalHaveRoleForSdb(String sdbId, Set<String> rolesThatAllowPermission, Set<String> userGroupsThatPrincipalBelongsTo) {
return permissionsMapper.doesUserPrincipalHaveGivenRoleForSdb(sdbId, rolesThatAllowPermission, userGroupsThatPrincipalBelongsTo);
}
Expand Down
12 changes: 12 additions & 0 deletions src/main/java/com/nike/cerberus/dao/SafeDepositBoxDao.java
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ public List<SafeDepositBoxRoleRecord> getIamRoleAssociatedSafeDepositBoxRoles(fi
return safeDepositBoxMapper.getIamRoleAssociatedSafeDepositBoxRoles(awsIamRoleArn, iamRootArn);
}

public List<SafeDepositBoxRoleRecord> getIamAssumedRoleAssociatedSafeDepositBoxRoles(final String iamAssumedRoleArn,
final String awsIamRoleArn,
final String iamRootArn) {
return safeDepositBoxMapper.getIamAssumedRoleAssociatedSafeDepositBoxRoles(iamAssumedRoleArn, awsIamRoleArn, iamRootArn);
}

public List<SafeDepositBoxRecord> getUserAssociatedSafeDepositBoxes(final Set<String> userGroups) {
return safeDepositBoxMapper.getUserAssociatedSafeDepositBoxes(userGroups);
}
Expand All @@ -59,6 +65,12 @@ public List<SafeDepositBoxRecord> getIamPrincipalAssociatedSafeDepositBoxes(fina
return safeDepositBoxMapper.getIamPrincipalAssociatedSafeDepositBoxes(iamPrincipalArn, iamRootArn);
}

public List<SafeDepositBoxRecord> getAssumedRoleAssociatedSafeDepositBoxes(final String iamAssumedRoleArn,
final String iamRoleArn,
final String iamRootArn) {
return safeDepositBoxMapper.getIamAssumedRoleAssociatedSafeDepositBoxes(iamAssumedRoleArn, iamRoleArn, iamRootArn);
}

public List<SafeDepositBoxRecord> getSafeDepositBoxes(final int limit, final int offset) {
return safeDepositBoxMapper.getSafeDepositBoxes(limit, offset);
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/nike/cerberus/error/DefaultApiError.java
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ public enum DefaultApiError implements ApiError {
/**
* Signature does not match. Either the request is invalid or the request is signed with invalid region and/or wrong host.
*/
SIGNATURE_DOES_NOT_MATCH(99237, "Signature does not match. Make sure the request is signed with us-east-1 as region and sts.amazonaws.com as host.", SC_BAD_REQUEST),
SIGNATURE_DOES_NOT_MATCH(99237, "Signature does not match. Make sure the request is signed with sts.{region}.amazonaws.com as host.", SC_BAD_REQUEST),

/**
* AWS token expired.
Expand Down
6 changes: 6 additions & 0 deletions src/main/java/com/nike/cerberus/mapper/PermissionsMapper.java
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@ Boolean doesIamPrincipalHaveGivenRoleForSdb(@Param("sdbId") String sdbId,
@Param("iamRootArn") String iamRootArn,
@Param("rolesThatAllowPermission") Set<String> rolesThatAllowPermission);

Boolean doesAssumedRoleHaveGivenRoleForSdb(@Param("sdbId") String sdbId,
@Param("assumedRoleArn") String assumedRoleArn,
@Param("iamRoleArn") String iamRoleArn,
@Param("iamRootArn") String iamRootArn,
@Param("rolesThatAllowPermission") Set<String> rolesThatAllowPermission);

Boolean doesUserPrincipalHaveGivenRoleForSdb(@Param("sdbId") String sdbId,
@Param("rolesThatAllowPermission") Set<String> rolesThatAllowPermission,
@Param("userGroupsThatPrincipalBelongsTo") Set<String> userGroupsThatPrincipalBelongsTo);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,21 @@ public interface SafeDepositBoxMapper {
List<SafeDepositBoxRoleRecord> getIamRoleAssociatedSafeDepositBoxRoles(@Param("awsIamRoleArn") final String awsIamRoleArn,
@Param("iamRootArn") final String iamRootArn);

List<SafeDepositBoxRoleRecord> getIamAssumedRoleAssociatedSafeDepositBoxRoles(@Param("iamAssumedRoleArn") final String iamAssumedRoleArn,
@Param("awsIamRoleArn") final String awsIamRoleArn,
@Param("iamRootArn") final String iamRootArn);

List<SafeDepositBoxRecord> getUserAssociatedSafeDepositBoxes(@Param("userGroups") Set<String> userGroups);

List<SafeDepositBoxRecord> getUserAssociatedSafeDepositBoxesIgnoreCase(@Param("userGroups") Set<String> userGroups);

List<SafeDepositBoxRecord> getIamPrincipalAssociatedSafeDepositBoxes(@Param("iamPrincipalArn") final String iamPrincipalArn,
@Param("iamRootArn") final String iamRootArn);

List<SafeDepositBoxRecord> getIamAssumedRoleAssociatedSafeDepositBoxes(@Param("iamAssumedRoleArn") final String iamAssumedRoleArn,
@Param("iamRoleArn") final String iamRoleArn,
@Param("iamRootArn") final String iamRootArn);

SafeDepositBoxRecord getSafeDepositBox(@Param("id") String id);

int countByPath(@Param("path") String path);
Expand Down
28 changes: 17 additions & 11 deletions src/main/java/com/nike/cerberus/service/AuthenticationService.java
Original file line number Diff line number Diff line change
Expand Up @@ -484,14 +484,6 @@ protected Set<String> buildCompleteSetOfPolicies(final String iamPrincipalArn) {

final Set<String> allPolicies = buildPolicySet(iamPrincipalArn);

if (! awsIamRoleArnParser.isRoleArn(iamPrincipalArn)) {
logger.debug("Detected non-role ARN, attempting to collect policies for the principal's base role...");
final String iamPrincipalInRoleFormat = awsIamRoleArnParser.convertPrincipalArnToRoleArn(iamPrincipalArn);

final Set<String> additionalPolicies = buildPolicySet(iamPrincipalInRoleFormat);
allPolicies.addAll(additionalPolicies);
}

return allPolicies;
}

Expand All @@ -505,8 +497,18 @@ protected Set<String> buildCompleteSetOfPolicies(final String iamPrincipalArn) {
private Set<String> buildPolicySet(final String iamRoleArn) {
final String accountRootArn = awsIamRoleArnParser.convertPrincipalArnToRootArn(iamRoleArn);
final Set<String> policies = Sets.newHashSet(LOOKUP_SELF_POLICY);
final List<SafeDepositBoxRoleRecord> sdbRolesForIamPrincipal =
safeDepositBoxDao.getIamRoleAssociatedSafeDepositBoxRoles(iamRoleArn, accountRootArn);
final List<SafeDepositBoxRoleRecord> sdbRolesForIamPrincipal;
// This may cause issues for user/instance-profile if someone's relying on the old code that converts everything
// that's not a role ARN.
if (awsIamRoleArnParser.isAssumedRoleArn(iamRoleArn)) {
logger.debug("Detected assumed-role ARN, attempting to collect policies for the principal's base role...");
String baseIamRoleArn = awsIamRoleArnParser.convertPrincipalArnToRoleArn(iamRoleArn);
sdbRolesForIamPrincipal =
safeDepositBoxDao.getIamAssumedRoleAssociatedSafeDepositBoxRoles(iamRoleArn, baseIamRoleArn, accountRootArn);
} else {
sdbRolesForIamPrincipal =
safeDepositBoxDao.getIamRoleAssociatedSafeDepositBoxRoles(iamRoleArn, accountRootArn);
}

sdbRolesForIamPrincipal.forEach(i -> {
policies.add(buildPolicyName(i.getSafeDepositBoxName(), i.getRoleName()));
Expand Down Expand Up @@ -643,7 +645,11 @@ protected Map<String, String> generateCommonIamPrincipalAuthMetadata(final Strin
groups.add("registered-iam-principals");

// We will allow specific ARNs access to the user portions of the API
if (getAdminRoleArnSet().contains(iamPrincipalArn)) {
Set<String> adminRoleArnSet = getAdminRoleArnSet();

if (adminRoleArnSet.contains(iamPrincipalArn)
|| awsIamRoleArnParser.isAssumedRoleArn(iamPrincipalArn)
&& adminRoleArnSet.contains(awsIamRoleArnParser.convertPrincipalArnToRoleArn(iamPrincipalArn))) {
metadata.put(METADATA_KEY_IS_ADMIN, Boolean.toString(true));
groups.add("admin-iam-principals");
} else {
Expand Down
22 changes: 14 additions & 8 deletions src/main/java/com/nike/cerberus/service/PermissionsService.java
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,7 @@ public boolean doesPrincipalHaveOwnerPermissions(CerberusPrincipal principal, Sa
boolean principalHasOwnerPermissions = false;
switch (principal.getPrincipalType()) {
case IAM:
String iamPrincipalArn = principal.getName();
String iamRootArn = awsIamRoleArnParser.convertPrincipalArnToRootArn(iamPrincipalArn);
principalHasOwnerPermissions = permissionsDao.doesIamPrincipalHaveRoleForSdb(sdb.getId(), iamPrincipalArn, iamRootArn, Sets.newHashSet(ROLE_OWNER));
principalHasOwnerPermissions = doesIamPrincipalHavePermission(principal, sdb.getId(), Sets.newHashSet(ROLE_OWNER));
break;
case USER:
principalHasOwnerPermissions = userGroupsCaseSensitive ?
Expand Down Expand Up @@ -123,9 +121,7 @@ public boolean doesPrincipalHaveReadPermission(CerberusPrincipal principal, Stri
switch (principal.getPrincipalType()) {
case IAM:
// if the authenticated principal is an IAM Principal check to see that the iam principal is associated with the requested sdb
String iamPrincipalArn = principal.getName();
String iamRootArn = awsIamRoleArnParser.convertPrincipalArnToRootArn(iamPrincipalArn);
principalHasPermissionAssociationWithSdb = permissionsDao.doesIamPrincipalHaveRoleForSdb(sdbId, iamPrincipalArn, iamRootArn, Sets.newHashSet(ROLE_READ, ROLE_OWNER, ROLE_WRITE));
principalHasPermissionAssociationWithSdb = doesIamPrincipalHavePermission(principal, sdbId, Sets.newHashSet(ROLE_READ, ROLE_OWNER, ROLE_WRITE));
break;
case USER:
// if the the principal is a user principal ensure that one of the users groups is associated with the sdb
Expand All @@ -145,8 +141,7 @@ public boolean doesPrincipalHavePermission(CerberusPrincipal principal, String s
boolean hasPermission = false;
switch (principal.getPrincipalType()) {
case IAM:
String iamRootArn = awsIamRoleArnParser.convertPrincipalArnToRootArn(principal.getName());
hasPermission = permissionsDao.doesIamPrincipalHaveRoleForSdb(sdbId, principal.getName(), iamRootArn, action.getAllowedRoles());
hasPermission = doesIamPrincipalHavePermission(principal, sdbId, action.getAllowedRoles());
break;
case USER:
hasPermission = userGroupsCaseSensitive ?
Expand All @@ -168,6 +163,17 @@ public boolean doesPrincipalHavePermission(CerberusPrincipal principal, String s
return hasPermission;
}

protected boolean doesIamPrincipalHavePermission(CerberusPrincipal principal, String sdbId, Set<String> roles) {
String iamPrincipalArn = principal.getName();
String iamRootArn = awsIamRoleArnParser.convertPrincipalArnToRootArn(iamPrincipalArn);
if (awsIamRoleArnParser.isAssumedRoleArn(iamPrincipalArn)) {
String iamRoleArn = awsIamRoleArnParser.convertPrincipalArnToRoleArn(iamPrincipalArn);
return permissionsDao.doesAssumedRoleHaveRoleForSdb(sdbId, iamPrincipalArn, iamRoleArn, iamRootArn, roles);
} else {
return permissionsDao.doesIamPrincipalHaveRoleForSdb(sdbId, iamPrincipalArn, iamRootArn, roles);
}
}

/**
* Does a case-insensitive check to see if the collection contains the given String
* @param items List of strings from which to search
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,14 @@ public List<SafeDepositBoxSummary> getAssociatedSafeDepositBoxes(final CerberusP

switch (principal.getPrincipalType()) {
case IAM:
String rootArn = awsIamRoleArnParser.convertPrincipalArnToRootArn(principal.getName());
sdbRecords = safeDepositBoxDao.getIamPrincipalAssociatedSafeDepositBoxes(principal.getName(), rootArn);
String principalName = principal.getName();
String rootArn = awsIamRoleArnParser.convertPrincipalArnToRootArn(principalName);
if (awsIamRoleArnParser.isAssumedRoleArn(principalName)) {
String iamRoleArn = awsIamRoleArnParser.convertPrincipalArnToRoleArn(principalName);
sdbRecords = safeDepositBoxDao.getAssumedRoleAssociatedSafeDepositBoxes(principalName, iamRoleArn, rootArn);
} else {
sdbRecords = safeDepositBoxDao.getIamPrincipalAssociatedSafeDepositBoxes(principalName, rootArn);
}
break;
case USER:
sdbRecords = userGroupsCaseSensitive ?
Expand Down
12 changes: 12 additions & 0 deletions src/main/java/com/nike/cerberus/util/AwsIamRoleArnParser.java
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,18 @@ public boolean isRoleArn(final String arn) {
return iamRoleArnMatcher.find();
}

/**
* Returns true if the ARN is in format 'arn:aws:sts::000000000:assumed-role/example/role-session' and false if not
* @param arn - ARN to test
* @return - True if is 'role' ARN, False if not
*/
public boolean isAssumedRoleArn(final String arn) {

final Matcher iamAssumedRoleArnMatcher = IAM_ASSUMED_ROLE_ARN_PATTERN.matcher(arn);

return iamAssumedRoleArnMatcher.find();
}

/**
* Returns true if the ARN is in format 'arn:aws:iam::000000000:root' and false if not
* @param arn - ARN to test
Expand Down
24 changes: 24 additions & 0 deletions src/main/resources/com/nike/cerberus/mapper/PermissionsMapper.xml
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,30 @@
) as HAS_PERM;
</select>

<select id="doesAssumedRoleHaveGivenRoleForSdb" resultType="Boolean">
SELECT NOT 0 >= (
SELECT
COUNT(*)
FROM SAFE_DEPOSIT_BOX
LEFT JOIN AWS_IAM_ROLE_PERMISSIONS ON SAFE_DEPOSIT_BOX.ID = AWS_IAM_ROLE_PERMISSIONS.SDBOX_ID
LEFT JOIN ROLE ON AWS_IAM_ROLE_PERMISSIONS.ROLE_ID = ROLE.ID
LEFT JOIN AWS_IAM_ROLE ON AWS_IAM_ROLE_PERMISSIONS.AWS_IAM_ROLE_ID = AWS_IAM_ROLE.ID
WHERE
SAFE_DEPOSIT_BOX.ID = #{sdbId}
AND
(AWS_IAM_ROLE.AWS_IAM_ROLE_ARN = #{assumedRoleArn}
OR
AWS_IAM_ROLE.AWS_IAM_ROLE_ARN = #{iamRoleArn}
OR
AWS_IAM_ROLE.AWS_IAM_ROLE_ARN = #{iamRootArn})
AND
ROLE.NAME IN
<foreach item="role" collection="rolesThatAllowPermission" separator="," open="(" close=")">
#{role}
</foreach>
) as HAS_PERM;
</select>

<select id="doesUserPrincipalHaveGivenRoleForSdb" resultType="Boolean">
SELECT NOT 0 >= (
SELECT
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,26 @@
AIR.AWS_IAM_ROLE_ARN = #{iamRootArn})
</select>

<select id="getIamAssumedRoleAssociatedSafeDepositBoxRoles" resultType="SafeDepositBoxRoleRecord">
SELECT
SDB.NAME AS SAFE_DEPOSIT_BOX_NAME,
R.NAME AS ROLE_NAME
FROM
SAFE_DEPOSIT_BOX SDB
INNER JOIN
AWS_IAM_ROLE_PERMISSIONS AIRP ON SDB.ID = AIRP.SDBOX_ID
INNER JOIN
AWS_IAM_ROLE AIR ON AIRP.AWS_IAM_ROLE_ID = AIR.ID
INNER JOIN
ROLE R ON AIRP.ROLE_ID = R.ID
WHERE
(AIR.AWS_IAM_ROLE_ARN = #{iamAssumedRoleArn}
OR
AIR.AWS_IAM_ROLE_ARN = #{awsIamRoleArn}
OR
AIR.AWS_IAM_ROLE_ARN = #{iamRootArn})
</select>

<select id="getSafeDepositBoxes" resultType="SafeDepositBoxRecord">
SELECT
ID,
Expand Down Expand Up @@ -155,6 +175,31 @@
AWS_IAM_ROLE.AWS_IAM_ROLE_ARN = #{iamRootArn})
</select>

<select id="getIamAssumedRoleAssociatedSafeDepositBoxes" resultType="SafeDepositBoxRecord">
SELECT
DISTINCT SDB.ID,
SDB.CATEGORY_ID,
SDB.NAME,
SDB.DESCRIPTION,
SDB.PATH,
SDB.CREATED_BY,
SDB.LAST_UPDATED_BY,
SDB.CREATED_TS,
SDB.LAST_UPDATED_TS
FROM
SAFE_DEPOSIT_BOX SDB
INNER JOIN
AWS_IAM_ROLE_PERMISSIONS ON SDB.ID = AWS_IAM_ROLE_PERMISSIONS.SDBOX_ID
INNER JOIN
AWS_IAM_ROLE ON AWS_IAM_ROLE_PERMISSIONS.AWS_IAM_ROLE_ID = AWS_IAM_ROLE.ID
WHERE
(AWS_IAM_ROLE.AWS_IAM_ROLE_ARN = #{iamAssumedRoleArn}
OR
AWS_IAM_ROLE.AWS_IAM_ROLE_ARN = #{iamRoleArn}
OR
AWS_IAM_ROLE.AWS_IAM_ROLE_ARN = #{iamRootArn})
</select>

<select id="getSafeDepositBox" resultType="SafeDepositBoxRecord">
SELECT
ID,
Expand Down
12 changes: 12 additions & 0 deletions src/test/java/com/nike/cerberus/dao/SafeDepositBoxDaoTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,18 @@ public void getIamRoleAssociatedSafeDepositBoxRoles_returns_list_of_role_records
assertThat(actual).hasSameElementsAs(safeDepositBoxRoleRecordList);
}

@Test
public void getIamAssumedRoleAssociatedSafeDepositBoxRoles_returns_list_of_role_records() {
when(safeDepositBoxMapper.getIamAssumedRoleAssociatedSafeDepositBoxRoles("ASSUMED_ROLE_ARN", awsIamRoleArn, iamRootArn))
.thenReturn(safeDepositBoxRoleRecordList);

List<SafeDepositBoxRoleRecord> actual =
subject.getIamAssumedRoleAssociatedSafeDepositBoxRoles("ASSUMED_ROLE_ARN", awsIamRoleArn, iamRootArn);

assertThat(actual).isNotEmpty();
assertThat(actual).hasSameElementsAs(safeDepositBoxRoleRecordList);
}

@Test
public void getUserAssociatedSafeDepositBoxes_returns_list_of_role_records() {
when(safeDepositBoxMapper.getUserAssociatedSafeDepositBoxes(userGroupSet))
Expand Down
Loading

0 comments on commit f5c1ec9

Please sign in to comment.