From caa6254d526a2fd4a0fd19fb450eb5cba6f30e5e Mon Sep 17 00:00:00 2001 From: craman Date: Fri, 30 Aug 2024 13:12:19 -0700 Subject: [PATCH 1/2] enforce resource ownership for delete assertion operation Signed-off-by: craman --- .../java/com/yahoo/athenz/zms/ZMSImpl.java | 22 ++++++++++ .../athenz/zms/utils/ResourceOwnership.java | 13 ++++++ .../athenz/zms/ResourceOwnershipTest.java | 44 ++++++++++++++++++- .../zms/utils/ResourceOwnershipTest.java | 23 ++++++++++ 4 files changed, 100 insertions(+), 2 deletions(-) diff --git a/servers/zms/src/main/java/com/yahoo/athenz/zms/ZMSImpl.java b/servers/zms/src/main/java/com/yahoo/athenz/zms/ZMSImpl.java index 47c5f00ae62..d2ef2e935fa 100644 --- a/servers/zms/src/main/java/com/yahoo/athenz/zms/ZMSImpl.java +++ b/servers/zms/src/main/java/com/yahoo/athenz/zms/ZMSImpl.java @@ -5844,6 +5844,16 @@ public void deleteAssertion(ResourceContext ctx, String domainName, String polic verifyAuthorizedServiceOperation(((RsrcCtxWrapper) ctx).principal().getAuthorizedService(), caller); + // extract our policy object to get its attributes + + AthenzDomain domain = getAthenzDomain(domainName, false); + Policy policy = getPolicyFromDomain(policyName, domain); + + if (policy == null) { + throw ZMSUtils.notFoundError("Invalid policy name specified", caller); + } + + ResourceOwnership.verifyPolicyAssertionsDeleteResourceOwnership(policy, resourceOwner, caller); dbService.executeDeleteAssertion(ctx, domainName, policyName, null, assertionId, auditRef, caller); } @@ -10114,6 +10124,18 @@ Group getGroupFromDomain(final String groupName, AthenzDomain domain) { return null; } + Policy getPolicyFromDomain(final String policyName, AthenzDomain domain) { + if (domain != null && domain.getPolicies() != null) { + final String resourcePolicyName = ResourceUtils.policyResourceName(domain.getName(), policyName); + for (Policy policy : domain.getPolicies()) { + if (policy.getName().equalsIgnoreCase(resourcePolicyName)) { + return policy; + } + } + } + return null; + } + boolean isAllowedPutMembershipAccess(Principal principal, final AthenzDomain domain, final String resourceName) { // evaluate our domain's roles and policies to see if either update or update_members diff --git a/servers/zms/src/main/java/com/yahoo/athenz/zms/utils/ResourceOwnership.java b/servers/zms/src/main/java/com/yahoo/athenz/zms/utils/ResourceOwnership.java index 1eadf19694e..55ae2465daa 100644 --- a/servers/zms/src/main/java/com/yahoo/athenz/zms/utils/ResourceOwnership.java +++ b/servers/zms/src/main/java/com/yahoo/athenz/zms/utils/ResourceOwnership.java @@ -762,6 +762,19 @@ public static boolean ownershipCheckFailure(boolean bOwnerSpecified, final Strin (!bOwnerSpecified && !StringUtil.isEmpty(objectOwner))); } + public static void verifyPolicyAssertionsDeleteResourceOwnership(Policy policy, final String resourceOwner, + final String caller) { + + // if the object has no owner then we're good for the enforcement check + + ResourcePolicyOwnership resourceOwnership = policy.getResourceOwnership(); + if (resourceOwnership == null || resourceOwnership.getAssertionsOwner() == null) { + return; + } + + verifyDeleteResourceOwnership(resourceOwner, resourceOwnership.getAssertionsOwner(), caller); + } + public static void verifyDeleteResourceOwnership(final String resourceOwner, final String objectOwner, final String caller) { diff --git a/servers/zms/src/test/java/com/yahoo/athenz/zms/ResourceOwnershipTest.java b/servers/zms/src/test/java/com/yahoo/athenz/zms/ResourceOwnershipTest.java index 229f7df988e..797c1c72c0f 100644 --- a/servers/zms/src/test/java/com/yahoo/athenz/zms/ResourceOwnershipTest.java +++ b/servers/zms/src/test/java/com/yahoo/athenz/zms/ResourceOwnershipTest.java @@ -1937,8 +1937,7 @@ public void testResourcePolicyOwnershipAssertions() { // apply the assertion with the same ownership which should be ok - zmsImpl.putAssertion(ctx, domainName, policyName, auditRef, "TF1", assertion); - + Assertion assertion1 = zmsImpl.putAssertion(ctx, domainName, policyName, auditRef, "TF1", assertion); // apply the assertion with a different value and verify it's rejected try { @@ -1948,6 +1947,39 @@ public void testResourcePolicyOwnershipAssertions() { assertEquals(ResourceException.CONFLICT, ex.getCode()); } + // delete the assertion with different ownership and it should be rejected + + try { + zmsImpl.deleteAssertion(ctx, domainName, policyName, assertion1.getId(), auditRef, "TF2"); + fail(); + } catch (ResourceException ex) { + assertEquals(ResourceException.CONFLICT, ex.getCode()); + } + + // delete the assertion with null ownership and it should be rejected + + try { + zmsImpl.deleteAssertion(ctx, domainName, policyName, assertion1.getId(), auditRef, null); + fail(); + } catch (ResourceException ex) { + assertEquals(ResourceException.CONFLICT, ex.getCode()); + } + + // delete the assertion with same ownership and it should be ok + try { + zmsImpl.deleteAssertion(ctx, domainName, policyName, assertion1.getId(), auditRef, "TF1"); + } catch (ResourceException ex) { + fail(); + } + + // apply the assertion again with resource owner TF1 + assertion1 = zmsImpl.putAssertion(ctx, domainName, policyName, auditRef, "TF1", assertion); + policy = zmsImpl.getPolicy(ctx, domainName, policyName); + resourceOwnership = policy.getResourceOwnership(); + assertNotNull(resourceOwnership); + assertNull(resourceOwnership.getObjectOwner()); + assertEquals(resourceOwnership.getAssertionsOwner(), "TF1"); + // apply the assertion with a null value and verify it's rejected try { @@ -1967,6 +1999,14 @@ public void testResourcePolicyOwnershipAssertions() { assertNull(resourceOwnership.getObjectOwner()); assertEquals(resourceOwnership.getAssertionsOwner(), "TF1"); + // delete the assertion with the ignore ownership flag which should be ok + zmsImpl.deleteAssertion(ctx, domainName, policyName, assertion1.getId(), auditRef, "ignore"); + policy = zmsImpl.getPolicy(ctx, domainName, policyName); + resourceOwnership = policy.getResourceOwnership(); + assertNotNull(resourceOwnership); + assertNull(resourceOwnership.getObjectOwner()); + assertEquals(resourceOwnership.getAssertionsOwner(), "TF1"); + // create a new policy without any assertions final String policyName2 = "policy2"; diff --git a/servers/zms/src/test/java/com/yahoo/athenz/zms/utils/ResourceOwnershipTest.java b/servers/zms/src/test/java/com/yahoo/athenz/zms/utils/ResourceOwnershipTest.java index 55158a94a93..fa85b9dfb26 100644 --- a/servers/zms/src/test/java/com/yahoo/athenz/zms/utils/ResourceOwnershipTest.java +++ b/servers/zms/src/test/java/com/yahoo/athenz/zms/utils/ResourceOwnershipTest.java @@ -285,4 +285,27 @@ public void testSkipEnforceResourceOwnership() { assertTrue(ResourceOwnership.skipEnforceResourceOwnership("ignore")); ResourceOwnership.ENFORCE_RESOURCE_OWNERSHIP = saveConfig; } + + @Test + public void testVerifyPolicyAssertionsDeleteResourceOwnership() { + + ResourceOwnership.verifyPolicyAssertionsDeleteResourceOwnership(new Policy(), "resourceOwner", "unit-test"); + ResourceOwnership.verifyPolicyAssertionsDeleteResourceOwnership(new Policy() + .setResourceOwnership(new ResourcePolicyOwnership()), "resourceOwner", "unit-test"); + + Policy assertionsOwnerPolicy = new Policy().setResourceOwnership(new ResourcePolicyOwnership().setAssertionsOwner("assertions-owner")); + try { + ResourceOwnership.verifyPolicyAssertionsDeleteResourceOwnership(assertionsOwnerPolicy, "resourceOwner", "unit-test"); + fail(); + } catch (ResourceException ex) { + assertEquals(ex.getCode(), 409); + } + + Policy objectOwnerPolicy = new Policy().setResourceOwnership(new ResourcePolicyOwnership().setObjectOwner("object-owner")); + try { + ResourceOwnership.verifyPolicyAssertionsDeleteResourceOwnership(objectOwnerPolicy, "resourceOwner", "unit-test"); + } catch (ResourceException ex) { + fail(); + } + } } From be8f01c21d3a80db51e83ca889da2e290fff2254 Mon Sep 17 00:00:00 2001 From: Henry Avetisyan Date: Thu, 5 Sep 2024 13:03:09 -0700 Subject: [PATCH 2/2] Update ResourceOwnershipTest.java --- .../java/com/yahoo/athenz/zms/utils/ResourceOwnershipTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/servers/zms/src/test/java/com/yahoo/athenz/zms/utils/ResourceOwnershipTest.java b/servers/zms/src/test/java/com/yahoo/athenz/zms/utils/ResourceOwnershipTest.java index 4e059b0ce22..82f5aba37bc 100644 --- a/servers/zms/src/test/java/com/yahoo/athenz/zms/utils/ResourceOwnershipTest.java +++ b/servers/zms/src/test/java/com/yahoo/athenz/zms/utils/ResourceOwnershipTest.java @@ -286,7 +286,7 @@ public void testSkipEnforceResourceOwnership() { ResourceOwnership.ENFORCE_RESOURCE_OWNERSHIP = saveConfig; } - @Test + @Test public void testVerifyRoleMembersDeleteResourceOwnership() { ResourceOwnership.verifyRoleMembersDeleteResourceOwnership(new Role(), "resourceOwner", "unit-test");