Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GAP-24] Agreement Permissions Management #56

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Original file line number Diff line number Diff line change
@@ -0,0 +1,196 @@
---
gap: 24
title: Agreement Permissions Management
description: Add ability to manage and delegate rights to Agreement and all related entities.
author: stranger80 (@stranger80)
status: Draft
type: Feature
---

## Abstract
The fundamental model in Golem assumes that visibility and rights to manage an Agreement, and all related domain entities (Activity, Invoice, DebitNote, Payment) are limited to the Requestor and Provider nodes which signed this Agreement. For more complex Golem application scenarios, however, a mechanism is required whereby the permissions can be delegated to a different node/identity. This GAP includes a summary of proposed permission model, enhancements to relevant APIs and some considerations on possible implementations.

## Motivation
The main objective of this GAP is to enable "offline requestor" scenarios, where the original Requestor delegates the Agreement permissions to another node (single or a consortium of nodes) so that it can disconnect from the network. This feature is a "building block" required by "Golem Supervisor" concept, where "supervisor swarms" will need to delegate Agreement Permissions between themselves to exercise control over a "self-sustaining" Golem application. Note that the "supervisor swarms" shall require other, higher-level permissions management and consensus protocols, which are out of scope of this GAP, and will be described in a dedicated GAP.

A coherent permissions management model may also enable previously unfeasible scenarios where eg. a "broker" Requestor node negotiates Agreements with Providers and then sells them to intrested customers, who then execute the Agreement by launching Activities and accepting Invoices/DebitNotes.

## Specification
The technical specification should describe the syntax and semantics of any new feature.

### Scope

Following entities are in scope of Agreement's permissions management:
- Agreement
- Activity
- Invoice
- DebitNote
- Payment
- ExecBatch

All other Golem entities (as exposed by Golem low-level APIs) are not covered by Agreement Permissions mechanism.

### Roles
From the point of view of Agreement permissions, following actor roles are relevant:

- *Owner* - Requestor and Provider which signed the Agreement as a result of negotiations are always the Owners of the Agreement (and all related entities).
- *Grantee* - any other (non-Owner) node/ID, which has been granted permissions and is included in Agreement's Access Control List.

### Permissions catalogue

A **permission** indicates an ability to perform a specific action which refers to an Agreement or any related entity. For the purposes of rights management, the permissions are indicated via literals, which include permission's category and specific action - where actions map to respective low-level Golem REST API operations, as implemented. Following table summarizes permissions which are covered by this GAP:

|Permission|Comments|
|-|-|
| req:getAgreement | |
| req:getAgreementPermissions | |
| req:collectAgreementEvents | |
| req:getActivityState | |
| req:getActivityUsage | |
| req:getRunningCommand | |
| req:getExecBatchResults | |
| req:getDebitNotes | |
| req:getDebitNote | |
| req:getPaymentsForDebitNote | |
| req:getDebitNoteEvents | |
| req:getInvoices | |
| req:getInvoice | |
| req:getPaymentsForInvoice | |
| req:getInvoiceEvents | |
| req:getPayments | |
| req:terminateAgreement | |
| req:setAgreementPermissions | See: [Transitive permissions](#transitive-permissions) below |
| req:createActivity | |
| req:exec | |
| req:callEncrypted | |
| req:destroyActivity | |
| req:acceptDebitNote | See: [Accept Invoice and Debit Note logic](#accept-invoice-and-debit-note-logic) below |
| req:rejectDebitNote | |
| req:acceptInvoice | See: [Accept Invoice and Debit Note logic](#accept-invoice-and-debit-note-logic) below |
| req:rejectInvoice | |
stranger80 marked this conversation as resolved.
Show resolved Hide resolved

If a permission to a specific REST API action on a given Agreement is granted to a Grantee, this Grantee is allowed to make respective REST API call referring to the Agreement (or related entity).

**Note:** this GAP deliberately leaves Provider-side action permissions out of scope.

### Access Control List

An Agreement shall have an *Access Control List* (ACL) associated with it, to indicate all rights granted to any non-Owner node/ID.
nieznanysprawiciel marked this conversation as resolved.
Show resolved Hide resolved

An indicative example of an Agreement's ACL is shown below:

```
agreementACL = {
version: 123,
aclEntries: [
{
"grantee": "0x7735df0c0bbb3ca65c55d61eadded653573e0152",
"permissions" : [
"req:createActivity",
"req:exec",
"req:destroyActivity"
]
},
...
{
"grantee": "0x0053b2c00006d1d8b75bb7dfa068990a19e4c32e",
"permissions" : [
"req:terminateAgreement",
"req:acceptInvoice",
"req:rejectInvoice"
]

}
]
}
```

**Note:** the ACL includes a `version` attribute - this attribute is required to prevent accidental ACL overwrite "races" - see **SetAgreementPermissions** method below.

### MarketAPI: SetAgreementPermissions

A new method shall be added in Market API to enable setting of an Agreement's ACL.

**Who:**
- Requestor

**Input:**
- agreementId
- new ACL content

**Output:**
- OK
- Error

#### "Race condition" prevention

Note that the ACL artifact includes a `version` attribute, which shall meet following conditions:
- If `setAgreementPermissions` is called with a `version` attribute value matching the latest recorded ACL version - the ACL update is accepted, and the recorded `versin` is incremented.
- If `setAgreementPermissions` is called with a `version` attribute value lower than the latest recorded ACL version - the method shall return Error with detailed message similar to 'newer version exists'.


### MarketAPI: GetAgreementPermissions

A new method shall be added in Market API to enable retrieving an Agreement's ACL.

**Who:**
- Requestor

**Input:**
- agreementId

**Output:**
- OK + Agreement's current ACL
- Error

### Accept Invoice and Debit Note logic

**Note** that one of the parameters required for `AcceptDebitNote`/`AcceptInvoice` Payment API calls is an `allocationId`. The ACL allows for delegation of rights to an Agreement and associated entities, but there is no equivalent rights management for funds managed/owned by Agreement Owner.

Therefore a rights Grantee, who is willing to Accept an incoming Debit Note or Invoice must pass `allocationId` of their own Allocation.
Copy link
Contributor

@shadeofblue shadeofblue Jan 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, in this simple model, it's really unclear how the payment process would work in case there is more than one grantee ... who pays? anyone? do we have a "race" to pay? what incentive would grantees have to be the ones to pay? what if multiple grantees sent their AcceptDebitNote or AcceptInvoice messages independently and agree to pay?

Copy link
Contributor Author

@stranger80 stranger80 Feb 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • we abstract from incentives and intents to pay - we only allow for different requestors to pay.

  • should we raise a "bug" to core team to ensure AcceptDebitNote/AcceptInvoice (as well as Reject) is race-resistant, ie. only a single accept on Invoice/DebitNote should end with 'OK', subsequent attempts should return info "invoice/debitnote already accepted". Must also ensure for this to be exercised on provider-side, as there must be a single-source-of truth and atomicity achieved for Accept operations.

    • we ignore scenarios where a malicious provider may want to change the sequence of accept/reject messages (or hold/ignore incoming messages)
  • how to solve the need to propagate Agreement/Activity related events over the "entitled" Requestors. We need to discuss this with the core team [ACTION - setup workshop with core team].

  • "Rogue grantee" problem. What options do we have?
    0. Do not consider "rogue grantee" problem at all - assume that 'grantees' are reliable

    1. Have a consensus mechanism over setAgreementPermissions
      a. any 'setAgreementPermissions' permission amendment must be controlled by a forum of grantees
      b. any permission amendment must be individually controlled by a forum
      c. only change in set of grantees must be controlled by a forum
    2. Implement a multi-grantee consensus protocol on an upper layer (eg. Supervisor protocol),
    3. Do not allow propagation of rights by 'grantees' (initially)
      • first implement ACLs without ability to grant rights by grantees (this will prevent fully autonomous dapps for time being)
      • subsequently solve the 'Rogue grantee' problem and allow for rights propagation (grantees grant rights)
    4. Go with 3. and specify requirements for a generic "credentials" mechanism which would attest consensus between multiple parties, etc. This calls for a dedicated GAP [ACTION: create placeholder] to specify requirements and suggest potential implementations which could be applied to any API call, or any Golem Entity, etc.

MB: I like 4 :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Rogue grantee" problem

  • Option II doesn't solve a problem. We can have consensus on upper layer, but yagna must be consensus aware, otherwise one grantee can still Agreement despite consensus being against him.
    • But in this case grantees keeping consensus correctly could drop the Agreement and recreate it with new Node without giving permissions to malicious actor

Other options:

  • (Initially) split upgrading permissions from downgrading. I would say that giving permissions to new Node is less harmful than stealing Agreement

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mtg w/Witek on 14.02.:

  1. On API "races" - this must simply be specified as a requirement, and included in test scenarios. NOTE: we should explictly list relevant Test Cases in the test suite. Also include "conflict races", ie. one requestor Accepts another Rejects...
  2. On entity and event propagation:
    a. currently yagna (on network level) does not have QoS features, ie. it does not support "retry" on messages sent between sides. This means that if Requestor is offline, the Provider is unable to reliably send TerminateAgreement message (currently one side is unable to Terminate Agreement while the other side is offline). This seems to be a gap from the point of view of "Offline Requestor model"!!!
    b. it seems there is a gap in yagna core which currently makes 'attach/detach' impossible
    c. agreementId/nodeId propagation:
    1. either the network must notify the new grantee with agreementIds
    2. or we add REST API call to "force synchronization" with a specified node

Copy link
Contributor

@approxit approxit Feb 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small collection of my afterthoughts, which I use in our next session:

Assuming that:

  • Requestor should connect to provider as a client-server pattern
  • Provider is responsible for final validation and enforcing ACL (aka backend validation)
  • Requestor should locally complain if some action is done regardless of no permission (aka frontend validation)
  • Provider will receive ACL with collection of permissions and grantees node ids

Finally... Requestor should receive similar set of ACL with similar data (but from grantee perspective) - permissions and provider id.
In result, requestor should at least have its own copy of ACL (with provider id), to continue working after kinda-disaster/kinda-offline recovery.

Having only activity id is not enough to revive provider id, without any help of access to supervising entity.

Copy link
Contributor Author

@stranger80 stranger80 Feb 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update on 24.02.:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7.03.2023:

  • Ad ACL permissions, grantees, delegation - amend ACL to include various "credential sets" required (optional) where node identity is one of credential types, but also other credential types may be required (eg. a multi-sig artifact).
  • Kamil to propose an illustrative example of ACL
  • Ad. Agreement information propagation across network (required to forward the AgreementId to a new grantee)
    • Step 1: (suboptimal) each node knowing Agreement's ACL should periodically distribute it to affected parties (all grantees as listed in ACL) - can we extract all the affected parties straight.
    • Step 2: (optimal) A dedicated, generic GAP describing the "new node welcome notification" mechanism, as well as node "synchronization with network"
      • Ask Witek to draft brief input into the GAP - summarize the concept as discussed on the meeting

Copy link
Contributor

@approxit approxit Mar 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Below is example for defining structure with separate collection of credential providing methods and separate collection of different permission sets.
Because those entries are interconnected and they need to refer from one to each other, using custom names help significantly.

Specific key names and values are provisional and are subject of change.

Relation between authTypes and permissionGroups could be included in permissionGroups (as shown below) or could be moved to analogical structure in authTypes.

{
	"version": 123,
	"authTypes": {
		"basic": {
			"type": "basic",
			"password": "..."
		},
		"token": {
			"type": "bearer",
			"token": "..."
		},
		"cert": {
			"type": "pem",
			"privateKey": "...",
		},
		"nodesOwned": {
			"type": "nodeId",
			"nodes": [
				"node_id_kj24",
				"node_id_9fd8",
				"node_id_gq68",
			]
		},
		"nodesNotOwned": {
			"type": "nodeId",
			"nodes": [
				"node_id_kj43",
				"node_id_ml23",
				"node_id_8s0d"
			]
		}
	},
	"permissionGroups": {
		"group1": {
			"authTypes": [
				"basic",
				"nodeOwned"
			],
			"permissions": [
				"req:createActivity",
				"req:exec",
				"req:destroyActivity"
			]
		},
		"group2": {
			"authTypes": [
				"token",
				"nodeNotOwned",
				"cert"
			],
			"permissions": [
				"req:terminateAgreement",
				"req:acceptInvoice",
				"req:rejectInvoice"
			]
		}
	}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorporated the proposed ACL improvements in the GAP draft, with some tweaks (eg. not "authTypes", but rather "attestationTypes", etc.)
We need to discuss the potentially useful attestation types, as we must not include any secrets in the ACL itself!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notes after 14.03.:

  • Continue on "generic attestations" - need to align on what do the "content-dependent" attestations include? The attestation artifact (eg. ACL list) must be:
    • Canonicalized to allow reliable verification
    • Specified element of the request - how is this specified?
      • API method does specify which element of it can be covered by attestation (eg. ACL content in SetAgreementPermissions)
      • ...any other option?


### Transitive permissions

**Note** granting access to `req:setAgreementPermissions` effectively allows grantee to delegates access rights further. This implies transitivity of permissions.

### Implementation considerations

To implement the Agreement Permissions Management it is necessary to introduce permission control in yagna REST API. However there is a number of possible ways to manage and 'guard' the Agreement's ACL.

1. ACL managed and exerted on Provider node

In this model, the ACL is persisted and managed on the Provider node indicated by the Agreement. Therefore all actions resulting from REST API calls must be routed to Provider node, and the permissionsneed to be validated there. Any attempt to violate the permissions as indicated by the ACL must result with "permissions violation" error returned to the calling node (and respective REST API caller).

The benefit of this model is the fact that ACL is managed and verified in one place, which limits the possibility of race conditions, therefore increasing the robustness of implementation.

The drawback is - all actions must be routed to the Provider for validation, and that increases delay before a REST API call result is returned to the caller.

2. ACL distributed and exerted on Requestor and Grantee nodes

In this model, the ACL is managed by the Provider node indicated by the Agreement, however each change in the ACL is distributed to all Grantee nodes. This allows for the permission verification to happen on Grantee nodes.

The benefit is - immediate verification of permissions, as each Grantee node can verify the permissions against their copy of the ACL.

The drawback is the risk of local ACL copies going out of sync versus theProvider's master-copy. This introduces a risk of race conditions, and generally reduces the security level of the mechanism.

**Recommendation:** Implementation 1.

## Rationale

Permission management enhancements are to be applied on Market API only, as only Agreement ACLs are to be managed. An alternative is a broad, generic Permissions API, but it seems overkill.


## Backwards Compatibility
The ACL mechanism does not impact the actions allowed for Agreement Owners. This implies that all Requestor Agent modules designed for a model where owning Requestor is the only identity calling Requestor-side APIs will continue to work successfully.

Question: do we need some mechanism for compatibility of Requestor Agent apps vs Requestor yagna daemons? Daemon capability list (there was a GAP for that)?


## Test Cases
Indicative test scenario suite is available [here](gap-24_test_cases.md).

## Security Considerations
This GAP is all about security and permissions. It is recommended to obtain an external security review/pen test for the implementation.

## Copyright
Copyright and related rights waived via [CC0](https://creativecommons.org/publicdomain/zero/1.0/).
76 changes: 76 additions & 0 deletions gaps/gap-24_agreement_permissions_management/gap-24_test_cases.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
# Agreement Permissions Management - Test Scenarios

## 1. API permissions
For each APIs, the effectiveness of permissions grant/revoke must be verified - following "template" test scenarios should applied to each API.

### 1.1. Permissions grant - one Grantee positive scenario
Grant individual permission to an API to a specific Grantee node A, and run Requestor Agent process on node A to confirm the granted actions are **possible**. Run Requestor Agent process on node B to confirm the granted actions are **not possible**.

### 1.2. Permissions grant - one Grantee negative scenario
Revoke individual permission to an API to a specific Grantee node A, and run Requestor Agent process on node A to confirm the granted actions are **not possible**.


## 2. API "race conditions"
For all state-changing API calls, it must be ensured that the actual state change can only be performed by an entitled Grantee, and any subsequent state-changing call from a different Grantee will be handled gracefully (according to the ACL and the allowed state transitions).

### 2.1. Accept/Reject Invoice
Given: two Grantee nodes (A and B) with permission to call Accept/Reject Invoice

### 2.1.1. Double "AcceptInvoice" call
- An `Invoice` is issued and can be downloaded by both Grantees A and B
- Grantee A issues a `AcceptInvoice` call - with success (Invoice gets paid by Grantee A)
- Grantee B issues a `AcceptInvoice` call - with error stating that the Invoice has already been paid

### 2.1.2. "AcceptInvoice" vs "RejectInvoice" call
- An `Invoice` is issued and can be downloaded by both Grantees A and B
- Grantee A issues a `AcceptInvoice` call - with success (Invoice gets paid by Grantee A)
- Grantee B issues a `RejectInvoice` call - with error stating that the Invoice has already been paid

### 2.1.3. "RejectInvoice" vs "AcceptInvoice" call
- An `Invoice` is issued and can be downloaded by both Grantees A and B
- Grantee A issues a `RejectInvoice` call - with success (Invoice gets rejected, no payment is issued)
- Grantee B issues a `AcceptInvoice` call - with success (Invoice gets paid by Grantee B)

### 2.2. Accept/Reject DebitNote
Given: two Grantee nodes (A and B) with permission to call Accept/Reject DebitNote
### 2.2.1. Double "AcceptDebitNote" call
As per similar Invoice scenario.
### 2.2.2. "AcceptDebitNote" vs "RejectDebitNote" call
As per similar Invoice scenario.
### 2.2.2. "RejectDebitNote" vs "AcceptDebitNote" call
As per similar Invoice scenario.

### 2.3. CreateActivity
TBD

### 2.4. DestroyActivity
Given: two Grantee nodes (A and B) with permission to call DestroyActivity
### 2.4.1. Double "DestroyActivity" call
- An `Activity` is created and is visible to both Grantees A and B
- Grantee A issues a `DestroyActivity` call - with success (Activity gets terminated)
- Grantee B issues a `DestroyActivity` call - with warning saying that the Actvity has already been terminated

### 2.5. TerminateAgreement
Given: two Grantee nodes (A and B) with permission to call TerminateAgreement
### 2.5.1. Double "TerminateAgreement" call
- An `Agreement` is created and is visible to both Grantees A and B
- Grantee A issues a `TerminateAgreement` call - with success (Agreement gets terminated)
- Grantee B issues a `TerminateAgreement` call - with warning saying that the Agreement has already been terminated


## 3. Event propagation
It must be ensured that relevant API Events get propagated to all relevant permission Grantees.

### 3.1. CollectAgreementEvents - AgreementTerminatedEvent
- An `Agreement` is created and is visible to both Grantees A and B
- Grantee B begins long-polling on a `CollectAgreementEvents` call
- Grantee A issues a `TerminateAgreement` call - with success (Agreement gets terminated)
- Grantee B receives a `AgreementTerminatedEvent`

### 3.2. GetInvoiceEvents - InvoiceReceivedEvent
- An `Agreement` is created and is visible to both Grantees A and B
- An `Activity` is launched
- Grantee B begins long-polling on a `GetInvoiceEvents` call
- Grantee A issues a `DestroyActivity` call - with success (Activity gets terminated, Invoice gets issued by Provider)
- Grantee B receives a `InvoicReceivedEvent`