From 9652202db028b64fcf441034d54b32a8e73ab5a1 Mon Sep 17 00:00:00 2001 From: Pierre Troger Date: Tue, 3 Sep 2024 16:09:47 +0200 Subject: [PATCH] debugging the false positive --- .../core/open-policy-agent.engine.ts | 2 + .../__test__/criteria/intent/amount_test.rego | 9 ++ .../rego/__test__/policies/spendings.rego | 14 ++ .../__test__/policies/spendings_test.rego | 48 ++++++ .../rego/criteria/spendingLimit.rego | 6 + docker-compose.yml | 5 +- .../4-transfer-a-to-b.ts | 2 +- .../tiered-eth-transfer-policy.spec.ts | 146 +++++++++++------- .../armory-sdk/src/lib/__test__/util/setup.ts | 1 + .../policy/set/tiered-eth-transfer.json | 47 ++++-- .../src/lib/schema/policy.schema.ts | 17 +- 11 files changed, 221 insertions(+), 76 deletions(-) diff --git a/apps/policy-engine/src/open-policy-agent/core/open-policy-agent.engine.ts b/apps/policy-engine/src/open-policy-agent/core/open-policy-agent.engine.ts index b4ded1901..dce4400e3 100644 --- a/apps/policy-engine/src/open-policy-agent/core/open-policy-agent.engine.ts +++ b/apps/policy-engine/src/open-policy-agent/core/open-policy-agent.engine.ts @@ -187,6 +187,8 @@ export class OpenPolicyAgentEngine implements Engine { // an array of results with an inner result. We perform a typecast here to // satisfy TypeScript compiler. Later, we parse the schema a few lines // below to ensure type-safety for data coming from external sources. + + console.log('###input ', JSON.stringify(input, null, 2)) const results = (await this.opa.evaluate(input, POLICY_ENTRYPOINT)) as { result: unknown }[] const parse = z.array(resultSchema).safeParse(results.map(({ result }) => result)) diff --git a/apps/policy-engine/src/resource/open-policy-agent/rego/__test__/criteria/intent/amount_test.rego b/apps/policy-engine/src/resource/open-policy-agent/rego/__test__/criteria/intent/amount_test.rego index ab259eca4..4e39679f8 100644 --- a/apps/policy-engine/src/resource/open-policy-agent/rego/__test__/criteria/intent/amount_test.rego +++ b/apps/policy-engine/src/resource/open-policy-agent/rego/__test__/criteria/intent/amount_test.rego @@ -25,3 +25,12 @@ test_checkIntentAmountValue { checkIntentAmount({"currency": "fiat:usd", "operator": operators.greaterThanOrEqual, "value": oneMaticValue}) with input as requestWithEip1559Transaction with data.entities as entities checkIntentAmount({"currency": "fiat:usd", "operator": operators.lessThanOrEqual, "value": oneMaticValue}) with input as requestWithEip1559Transaction with data.entities as entities } + +test_checkIntentAmount2 { + requ := object.union(requestWithEip1559Transaction, { + "intent": object.union(requestWithEip1559Transaction.intent, { + "amount": "100000000000000000000000000000000000000000000000000000000000000000000", + }), + }) + checkIntentAmount({"operator": operators.greaterThan, "value": "10" }) with input as requ with data.entities as entities +} \ No newline at end of file diff --git a/apps/policy-engine/src/resource/open-policy-agent/rego/__test__/policies/spendings.rego b/apps/policy-engine/src/resource/open-policy-agent/rego/__test__/policies/spendings.rego index 584c6a164..4f9aed9fd 100644 --- a/apps/policy-engine/src/resource/open-policy-agent/rego/__test__/policies/spendings.rego +++ b/apps/policy-engine/src/resource/open-policy-agent/rego/__test__/policies/spendings.rego @@ -219,4 +219,18 @@ permit[{"policyId": "spendingLimitWithRange"}] = reason { "approvalsSatisfied": [], "approvalsMissing": [], } +} + +pemrmit[{"policyId": "minimalSpendingLimit"}] = reason { + checkSpendingLimit({ + "limit": "1", + "operator": "lte", + }) + + reason = { + "type": "permit", + "policyId": "minimalSpendingLimit", + "approvalsSatisfied": [], + "approvalsMissing": [], + } } \ No newline at end of file diff --git a/apps/policy-engine/src/resource/open-policy-agent/rego/__test__/policies/spendings_test.rego b/apps/policy-engine/src/resource/open-policy-agent/rego/__test__/policies/spendings_test.rego index faf7b3c60..762fdfc4b 100644 --- a/apps/policy-engine/src/resource/open-policy-agent/rego/__test__/policies/spendings_test.rego +++ b/apps/policy-engine/src/resource/open-policy-agent/rego/__test__/policies/spendings_test.rego @@ -373,3 +373,51 @@ test_spendingLimitWithRange { "type": "permit", } } + +test_spendingLimitTooHighForRange { + spendingLimitTooHighForRangeReq = { + "action": "signTransaction", + "principal": {"userId": "test-alice-uid"}, + "resource": {"uid": "eip155:eoa:0xddcf208f219a6e6af072f2cfdc615b2c1805f98e"}, + "approvals": [ + { + "id": "0x6af10b6d5024963972ba832486ea1ae29f1b99cb1191abe444b52e98c69f7487", + "userId": "test-antoine-user-uid", + "key": { + "kty": "EC", + "alg": "ES256K", + "kid": "0x6af10b6d5024963972ba832486ea1ae29f1b99cb1191abe444b52e98c69f7487", + "crv": "secp256k1", + "x": "QwUuAC2s22VKwoS5uPTZgcTN_ztkwt9VWKRae3bikEQ", + "y": "lZgwfE7ZDz9af9_PZxq9B7pVwAarfIaFESATYp-Q7Uk" + } + } + ], + "intent": { + "to": "eip155:1:0x76d1b7f9b3f69c435eef76a98a415332084a856f", + "from": "eip155:1:0x0301e2724a40e934cce3345928b88956901aa127", + "type": "transferNative", + "amount": "1000000000000000000000", + "token": "eip155:1/slip44:60" + }, + "transactionRequest": { + "chainId": 1, + "from": "0x0301e2724a40e934cce3345928b88956901aa127", + "to": "0x76d1b7f9b3f69c435eef76a98a415332084a856f", + "value": "0x8AC7230489E80000" + }, + "feeds": [ + { + "source": "armory/price-feed", + "sig": "eyJhbGciOiJFSVAxOTEiLCJraWQiOiIweDBjNjIwZjRiYzhlOTMxMTBiZDljZDc5ZTVkNjM3YTI0MGQ1NWUwZjI3MzNmZDdlOTViNzM0N2QzYjA2MjMxZmMiLCJ0eXAiOiJKV1QifQ.eyJkYXRhIjoiMHg0NDEzNmZhMzU1YjM2NzhhMTE0NmFkMTZmN2U4NjQ5ZTk0ZmI0ZmMyMWZlNzdlODMxMGMwNjBmNjFjYWFmZjhhIiwiaWF0IjoxNzI1Mjg3MjEwLCJpc3MiOiJodHRwczovL2FybW9yeS5uYXJ2YWwueHl6Iiwic3ViIjoiMHg2OTY2MzEzNDAwMTZGY2FFMmJCYmEyREQ3QmYxZjFBMkY4ZTJBNTRmIn0.jgr7A-dB_tHX42IDG0Cx8fE7Mtu1xBb5g3oW3qdbRl4-j4XmOdZjdOS6m73yNQ-Dz-RbCxQrbSk3zwaQODrHJBw", + "data": {} + }, + { + "source": "armory/historical-transfer-feed", + "sig": "eyJhbGciOiJFSVAxOTEiLCJraWQiOiIweDY2YTY3YWI1ODI2OWY0NGFhYmE2NDUxNzZmNGI5M2Y1ZTY3MTU2N2I0NTQ0MjkwZTE5OGU5ODYxYzM0OTNkMmQiLCJ0eXAiOiJKV1QifQ.eyJkYXRhIjoiMHg0ZjUzY2RhMThjMmJhYTBjMDM1NGJiNWY5YTNlY2JlNWVkMTJhYjRkOGUxMWJhODczYzJmMTExNjEyMDJiOTQ1IiwiaWF0IjoxNzI1Mjg3MjEwLCJpc3MiOiJodHRwczovL2FybW9yeS5uYXJ2YWwueHl6Iiwic3ViIjoiMHhkOWYzYjNhMDY3ZmU0NmI2M0U0YjBkZUZlQjJBMGI3YWU2N2E4MjIxIn0.jzwVozBI4oOaBUQn3b1TdDlg1I9IN29xVwx2xy3MujN53TuZIweIgTcBqmRtnppRqAdZUo51R7k8Y6PDO8j-gBw", + "data": [] + } + ] +} + not permit[{"policyId": "minimalSpendingLimit"}] with input as spendingLimitTooHighForRangeReq with data.entities as {} +} \ No newline at end of file diff --git a/apps/policy-engine/src/resource/open-policy-agent/rego/criteria/spendingLimit.rego b/apps/policy-engine/src/resource/open-policy-agent/rego/criteria/spendingLimit.rego index 2aba870d5..7d31e0860 100644 --- a/apps/policy-engine/src/resource/open-policy-agent/rego/criteria/spendingLimit.rego +++ b/apps/policy-engine/src/resource/open-policy-agent/rego/criteria/spendingLimit.rego @@ -63,6 +63,10 @@ calculateCurrentSpendings(params) = result { conditions = object.union(spendingWildcardConditions, params) timeWindow = conditions.timeWindow filters = conditions.filters + print("filters: ", filters) + print("timeWindow: ", timeWindow) + print("\n\ntransferFeed", transferFeed) + print("\n\nintentTransferObjects", intentTransferObjects) transfers = array.concat(transferFeed, intentTransferObjects) result = sum([spending | @@ -107,7 +111,9 @@ calculateCurrentSpendings(params) = result { checkSpendingLimit(params) { conditions = object.union(spendingWildcardConditions, params) + print("conditions: ", conditions) spendings = calculateCurrentSpendings(conditions) + print("spendings: ", spendings) operator = conditions.operator limit = to_number(conditions.limit) diff --git a/docker-compose.yml b/docker-compose.yml index f5ff04191..bf3bf81f6 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -28,7 +28,10 @@ services: environment: ALLOW_EMPTY_PASSWORD: 'true' logging: - driver: none + driver: "json-file" + options: + max-size: "10m" + max-file: "3" volumes: - ./.data/redis:/data diff --git a/examples/approvals-by-spending-limit/4-transfer-a-to-b.ts b/examples/approvals-by-spending-limit/4-transfer-a-to-b.ts index 24940b65d..beb4590b3 100644 --- a/examples/approvals-by-spending-limit/4-transfer-a-to-b.ts +++ b/examples/approvals-by-spending-limit/4-transfer-a-to-b.ts @@ -16,7 +16,7 @@ const transactionRequest = { from: '0x0301e2724a40E934Cce3345928b88956901aA127', // Account A to: '0x76d1b7f9b3F69C435eeF76a98A415332084A856F', // Account B chainId: 1, - value: '0x429D069189E0000', // 0.3 ETH + value: '0x7FFFFFFFFFFFFDFF', // 0.3 ETH gas: 123n, maxFeePerGas: 789n, maxPriorityFeePerGas: 456n, diff --git a/packages/armory-sdk/src/lib/__test__/e2e/scenarii/tiered-eth-transfer-policy.spec.ts b/packages/armory-sdk/src/lib/__test__/e2e/scenarii/tiered-eth-transfer-policy.spec.ts index 099a4e1e7..5df0b14ee 100644 --- a/packages/armory-sdk/src/lib/__test__/e2e/scenarii/tiered-eth-transfer-policy.spec.ts +++ b/packages/armory-sdk/src/lib/__test__/e2e/scenarii/tiered-eth-transfer-policy.spec.ts @@ -1,5 +1,5 @@ /* eslint-disable jest/consistent-test-it */ -import { Action, Decision, entitiesSchema, FIXTURE, policySchema, Request } from '@narval/policy-engine-shared' +import { Action, Decision, entitiesSchema, FIXTURE, policySchema, Request, ValueOperators } from '@narval/policy-engine-shared' import { v4 } from 'uuid' import defaultEntities from '../../../../resource/entity/default.json' import tieredEthTransfer from '../../../../resource/policy/set/tiered-eth-transfer.json' @@ -36,10 +36,14 @@ describe('tiered approvals and spending limits', () => { // Generate a new client ID for each test run, otherwise historical data with persist between tests if using a long-lived db. const clientId = v4() + // 18446744073709551615 + // 9223372036854776000 beforeAll(async () => { const entities = entitiesSchema.parse(defaultEntities) - const policies = tieredEthTransfer.map((policy) => policySchema.parse(policy)) + const policies = tieredEthTransfer.map((policy) => { + return policySchema.parse(policy) + }) await createClient(systemManagerHexPk, { clientId, @@ -50,64 +54,57 @@ describe('tiered approvals and spending limits', () => { clientId, host: getAuthHost(), entities, - policies - }) - }) - - it('permits member to transfer less than or equal to 1 ETH', async () => { - const { authClient } = await buildAuthClient(antoinePrivateKey, { - host: getAuthHost(), - clientId + policies: [ { + "id": "tier1-low-value-transfers", + "description": "Permit members to transfer up to 0.00000000001 ETH per day without approval", + "when": [ + { + "criterion": "checkPrincipalRole", + "args": [ + "member" + ] + }, + { + "criterion": "checkIntentToken", + "args": [ + "eip155:1/slip44:60" + ] + }, + { + "criterion": "checkIntentAmount", + "args": { + "value": "1", + "operator": "lte" as ValueOperators + } + } + ], + "then": "permit" + }] }) - - const lowValueRequest = genNonce({ - ...request, - transactionRequest: { - ...request.transactionRequest, - value: '0xDE0B6B3A7640000' // 0.3 ETH - } - }) - - const response = await authClient.requestAccessToken(lowValueRequest) - expect(response).toMatchObject({ value: expect.any(String) }) }) - it('requires manager approval for transfers between 1 and 10 ETH', async () => { - expect.assertions(2) - - const { authClient: managerClient } = await buildAuthClient(carolPrivateKey, { - host: getAuthHost(), - clientId - }) - - const { authClient } = await buildAuthClient(antoinePrivateKey, { - host: getAuthHost(), - clientId - }) - - const mediumValueRequest = genNonce({ - ...request, - transactionRequest: { - ...request.transactionRequest, - value: '0x4563918244F40000' // 5 ETH - } - }) - - const res = await authClient.authorize(mediumValueRequest) - expect(res.decision).toEqual(Decision.CONFIRM) + // it('permits member to transfer less than or equal to 1 ETH', async () => { + // const { authClient } = await buildAuthClient(antoinePrivateKey, { + // host: getAuthHost(), + // clientId + // }) - if (res.decision === Decision.CONFIRM) { - await managerClient.approve(res.authId) + // const lowValueRequest = genNonce({ + // ...request, + // transactionRequest: { + // ...request.transactionRequest, + // value: '0xDE0B6B3A7640000' // 0.3 ETH + // } + // }) - const accessToken = await authClient.getAccessToken(res.authId) - expect(accessToken).toMatchObject({ value: expect.any(String) }) - } - }) + // const response = await authClient.requestAccessToken(lowValueRequest) + // expect(response).toMatchObject({ value: expect.any(String) }) + // }) - // it('requires admin approval for transfers between 10 and 100 ETH', async () => { + // it('requires manager approval for transfers between 1 and 10 ETH', async () => { // expect.assertions(2) - // const { authClient: adminClient } = await buildAuthClient(alicePrivateKey, { + // const { authClient: managerClient } = await buildAuthClient(carolPrivateKey, { // host: getAuthHost(), // clientId // }) @@ -117,25 +114,62 @@ describe('tiered approvals and spending limits', () => { // clientId // }) - // const highValueRequest = genNonce({ + // const mediumValueRequest = genNonce({ // ...request, // transactionRequest: { // ...request.transactionRequest, - // value: '0x8AC7230489E80000' // 10 ETH + // value: '0x4563918244F40000' // 5 ETH // } // }) - // const res = await authClient.authorize(highValueRequest) + // const res = await authClient.authorize(mediumValueRequest) // expect(res.decision).toEqual(Decision.CONFIRM) // if (res.decision === Decision.CONFIRM) { - // await adminClient.approve(res.authId) + // await managerClient.approve(res.authId) // const accessToken = await authClient.getAccessToken(res.authId) // expect(accessToken).toMatchObject({ value: expect.any(String) }) // } // }) + it('requires admin approval for transfers between 10 and 100 ETH', async () => { + expect.assertions(1) + + const { authClient: adminClient } = await buildAuthClient(alicePrivateKey, { + host: getAuthHost(), + clientId + }) + + const { authClient } = await buildAuthClient(antoinePrivateKey, { + host: getAuthHost(), + clientId + }) + + const highValueRequest = genNonce({ + action: Action.SIGN_TRANSACTION, + nonce: 'test-nonce-4', + transactionRequest: { + from: '0x0301e2724a40E934Cce3345928b88956901aA127', + to: '0x76d1b7f9b3F69C435eeF76a98A415332084A856F', + value: '0x7FFFFFFFFFFFFDFF', // 10 ETH + chainId: 1 + }, + resourceId: 'eip155:eoa:0x0301e2724a40e934cce3345928b88956901aa127' + }) + + const res = await authClient.authorize(highValueRequest) + expect(res.decision).toEqual(Decision.FORBID) + + if (res.decision === Decision.CONFIRM) { + await adminClient.approve(res.authId) + + const accessToken = await authClient.getAccessToken(res.authId) + expect(accessToken).toMatchObject({ value: expect.any(String) }) + } + }) + + // it('requires two admin approvals for transfers above 100 ETH', async () => { // expect.assertions(3) diff --git a/packages/armory-sdk/src/lib/__test__/util/setup.ts b/packages/armory-sdk/src/lib/__test__/util/setup.ts index 6ad031b06..ceb54c229 100644 --- a/packages/armory-sdk/src/lib/__test__/util/setup.ts +++ b/packages/armory-sdk/src/lib/__test__/util/setup.ts @@ -6,6 +6,7 @@ import { AuthAdminClient, AuthClient, AuthConfig } from '../../auth' import { createHttpDataStore, DataStoreConfig, EntityStoreClient, PolicyStoreClient } from '../../data-store' import { VaultAdminClient, VaultConfig } from '../../vault' + export const getAuthHost = () => 'http://localhost:3005' export const getAuthAdminApiKey = () => 'armory-admin-api-key' diff --git a/packages/armory-sdk/src/resource/policy/set/tiered-eth-transfer.json b/packages/armory-sdk/src/resource/policy/set/tiered-eth-transfer.json index d1a9d8de8..6c9e384de 100644 --- a/packages/armory-sdk/src/resource/policy/set/tiered-eth-transfer.json +++ b/packages/armory-sdk/src/resource/policy/set/tiered-eth-transfer.json @@ -5,11 +5,15 @@ "when": [ { "criterion": "checkPrincipalRole", - "args": ["member"] + "args": [ + "member" + ] }, { "criterion": "checkIntentToken", - "args": ["eip155:1/slip44:60"] + "args": [ + "eip155:1/slip44:60" + ] }, { "criterion": "checkSpendingLimit", @@ -22,7 +26,9 @@ }, "filters": { "perPrincipal": true, - "tokens": ["eip155:1/slip44:60"] + "tokens": [ + "eip155:1/slip44:60" + ] } } } @@ -33,14 +39,6 @@ "id": "tier2-medium-value-transfers", "description": "Require manager approval for transfers between 1-10 ETH per day", "when": [ - { - "criterion": "checkPrincipalRole", - "args": ["member"] - }, - { - "criterion": "checkIntentToken", - "args": ["eip155:1/slip44:60"] - }, { "criterion": "checkSpendingLimit", "args": { @@ -52,7 +50,26 @@ }, "filters": { "perPrincipal": true, - "tokens": ["eip155:1/slip44:60"] + "tokens": [ + "eip155:1/slip44:60" + ] + } + } + }, + { + "criterion": "checkSpendingLimit", + "args": { + "limit": "10000000000000000000", + "operator": "lte", + "timeWindow": { + "type": "rolling", + "value": 86400 + }, + "filters": { + "perPrincipal": true, + "tokens": [ + "eip155:1/slip44:60" + ] } } }, @@ -63,11 +80,13 @@ "approvalCount": 1, "countPrincipal": false, "approvalEntityType": "Narval::UserRole", - "entityIds": ["manager"] + "entityIds": [ + "manager" + ] } ] } ], "then": "permit" } -] +] \ No newline at end of file diff --git a/packages/policy-engine-shared/src/lib/schema/policy.schema.ts b/packages/policy-engine-shared/src/lib/schema/policy.schema.ts index 07de202d3..504edfde4 100644 --- a/packages/policy-engine-shared/src/lib/schema/policy.schema.ts +++ b/packages/policy-engine-shared/src/lib/schema/policy.schema.ts @@ -154,14 +154,23 @@ export const approvalConditionSchema = z.object({ entityIds: z.array(z.string().min(1)) }) -export const timeWindowSchema = z.object({ - type: timeWindowTypeSchema.optional(), - period: timeWindowPeriodSchema.optional(), - value: z.number().optional(), +const fixedTimeWindowSchema = z.object({ + type: z.literal('rolling'), + value: z.number().int(), startDate: z.number().int().optional(), endDate: z.number().int().optional() }) +const rollingTimeWindowSchema = z.object({ + type: z.literal('fixed'), + period: timeWindowPeriodSchema, + startDate: z.number().int().optional(), + endDate: z.number().int().optional() +}) + + +export const timeWindowSchema = z.discriminatedUnion('type', [rollingTimeWindowSchema, fixedTimeWindowSchema]) + export const transferFiltersSchema = z.object({ perPrincipal: z.boolean().optional(), tokens: z.array(z.string()).min(1).optional(),