TACo Encryption Allow Logic Interfaces #89

KPrasch · 2023-06-21T14:45:50Z

Implements the contracts from the specification.

Co Authored By @theref @cygnusv @jMyles @derekpierre

Based over #86

cygnusv

Looking good! Some preliminar comments.

cygnusv · 2023-06-21T15:04:09Z

contracts/contracts/coordination/AllowList.sol

+        bytes memory evidence,
+        bytes memory ciphertextHash
+    ) public view override returns(bool) {
+        address enricoAddress = address(uint160(bytes20(evidence)));


An address can't be the evidence, otherwise anyone can put any address there. The evidence was defined as a signature, so what we want here is to do something like:
address enricoAddress = ecrecover(ciphertextHash, v, r, s)
where v, r, s are somehow extracted from the signature evidence, and ciphertextHash should be somehow casted to bytes32 (what ecrecover expects). See https://docs.soliditylang.org/en/latest/cheatsheet.html#mathematical-and-cryptographic-functions

@theref and I considered recovering the address on-chain via ECDSA, however in the case of the AllowList for example, the only way an address can be "authorized" is by Ritual.authority setting it in a permissioned way. In other words, only the ritual's authority can add new enricos. Are we missing something?

Let's say the authority authorizes 0x1234. I don't own that address, but I just write 0x1234 as evidence when creating the conditions and access control policy. Ursulas would accept this evidence and that's not what we want. We need to provide evidence that I own that address.

If enricos are submitting a signature of the ciphertext and policy, doesn't that mitigate the issue you indicate (as enrico cannot simply "write 0x1234 as evidence")?

Ursula can recover the address from the signature provided in AccessControlPolicy.authorization and check that it's in the allow list.

If enricos are submitting a signature of the ciphertext and policy, doesn't that mitigate the issue you indicate (as enrico cannot simply "write 0x1234 as evidence")?

Yes, that's what I suggested initially.

Ursula can recover the address from the signature provided in AccessControlPolicy.authorization and check that it's in the allow list.

Right, what I'm saying is that what you called AccessControlPolicy.authorization is what must be submitted as evidence by Ursula. That's what I originally meant by the evidence is attached to the ciphertext.

Hmmm, not sure I'm following - why "must" that be the case?

Is there a problem with trusting ursula to perform a valid signature recovery from AccessControlPolicy.authorization?

Is that different than trusting ursula to perform a valid condition evaluation?

Another way of framing it: What is the advantage of doing an authorization signature recovery onchian vs. in Ursula's runtime?

The main advantage I see is that this allows for modular and pluggable evidence evaluation mechanisms (via smart contracts), without having to change Ursula's code

cygnusv · 2023-06-21T15:05:58Z

contracts/contracts/coordination/AllowList.sol

+import "./Coordinator.sol";
+
+
+contract AllowList is AccessControlDefaultAdminRules, IAccessController {


If you don't plan to use role management functions, AccessControl may be enough.

contracts/contracts/coordination/AllowList.sol

contracts/contracts/coordination/Coordinator.sol

contracts/contracts/coordination/IAccessControler.sol

theref · 2023-07-09T20:04:36Z

@KPrasch I think you've overwritten some work here, I'll push to another branch so that we can compare

contracts/contracts/coordination/GlobalAllowList.sol

vzotova · 2023-07-17T23:45:45Z

contracts/contracts/coordination/GlobalAllowList.sol

+            "Only ritual authority is permitted");
+        require(coordinator.isRitualFinalized(ritualID),
+            "Only active rituals can add authorizations");
+        for (uint i=0; i<addresses.length; i++) {


Suggested change

for (uint i=0; i<addresses.length; i++) {

for (uint256 i=0; i < addresses.length; i++) {

vzotova · 2023-07-17T23:45:58Z

contracts/contracts/coordination/GlobalAllowList.sol

+            "Only ritual authority is permitted");
+        require(coordinator.isRitualFinalized(ritualID),
+            "Only active rituals can add authorizations");
+        for (uint i=0; i<addresses.length; i++) {


Suggested change

for (uint i=0; i<addresses.length; i++) {

for (uint256 i=0; i < addresses.length; i++) {

derekpierre · 2023-07-18T13:27:47Z

contracts/contracts/coordination/Coordinator.sol

@@ -184,11 +198,15 @@ contract Coordinator is AccessControlDefaultAdminRules {
        return ritual.participant;
    }

-    function initiateRitual(
+    function _initiateRitual(


Should we walk back this change i.e. change _initiateRitual -> initiatRitual and remove the function on L254, since the params are now the same?

derekpierre · 2023-07-18T13:28:52Z

contracts/contracts/coordination/Coordinator.sol

@@ -359,13 +391,13 @@ contract Coordinator is AccessControlDefaultAdminRules {
    }

    function getParticipantFromProvider(
-        uint256 ritualID,
+        uint32 ritualID,


Should we be consistent with our camel-case (other spots as well - L400 and L412)?

Suggested change

uint32 ritualID,

uint32 ritualId,

contracts/contracts/coordination/GlobalAllowList.sol

derekpierre · 2023-07-18T13:37:00Z

contracts/contracts/coordination/GlobalAllowList.sol

+        require(coordinator.getAuthority(ritualID) == msg.sender,
+            "Only ritual authority is permitted");
+        require(coordinator.isRitualFinalized(ritualID),
+            "Only active rituals can add authorizations");


Does it make sense to turn these requires into one of those solidity modifiers like onlyOwner etc., in this case onlyAuthorityForActiveRitual or something like that...

It can be used for authorizing and deauthorizing.

derekpierre · 2023-07-18T13:41:26Z

tests/test_coordinator.py

@@ -93,49 +93,93 @@ def coordinator(project, deployer, stake_info, flat_rate_fee_model, initiator):
    return contract


+@pytest.fixture()
+def global_allow_list(project, deployer, coordinator):
+    contract = project.GlobalAllowList.deploy(


Should we add tests (maybe not in this file) for the actual GlobalAllowList contract?

Yes, certainly. I intend to push new tests before merging, but still want to get a review in the meantime 👍🏻

derekpierre · 2023-07-18T13:42:19Z

contracts/contracts/coordination/IRitualAuthorizer.sol

@@ -0,0 +1,9 @@
+pragma solidity ^0.8.0;
+
+interface IRitualAuthorizer {


piotr-roslaniec · 2023-08-14T11:06:15Z

contracts/contracts/coordination/GlobalAllowList.sol

+        coordinator = _coordinator;
+    }
+
+    function isAuthorized(


Is the DKG client supposed to call this method at some point? Fore example, when picking nodes for the DKG cohort?

Not when picking nodes for the cohort but rather when decryption is requested from the nodes. Each node that receives a decryption request will check to make sure that this encryptor is allowed to use the corresponding ritual/encrypting key and therefore worthy to be decrypted. This is showcased in WIP PR 3194 - see https://github.com/nucypher/nucypher/pull/3194/files#diff-8df83dacf71b2ff2ca3636cd470cba70eb6b141e3c8843fadfab72514aafa3f6R199 and https://github.com/nucypher/nucypher/pull/3194/files#diff-911a370c18d0d0e3ea62099fc5ff6daa16915da19cf5399f24e404b7d8d6d168R790

cygnusv

Great work! My main comment is regarding how hash within the authorization check is performed and the lack of tests for that.

cygnusv · 2023-08-18T07:39:21Z

contracts/contracts/coordination/Coordinator.sol

@@ -9,6 +9,8 @@ import "./IFeeModel.sol";
 import "./IReimbursementPool.sol";
 import "../lib/BLS12381.sol";
 import "../../threshold/IAccessControlApplication.sol";
+import "../../../../nucypher/tests/acceptance/contracts/.cache/openzeppelin/v4.8.1/access/IAccessControl.sol";


How many headaches you gave us in Barcelona...

cygnusv · 2023-08-18T07:51:25Z

contracts/contracts/coordination/Coordinator.sol

        // TODO: restrict to ritualID < rituals.length?
        return getRitualState(rituals[ritualId]);
    }

+    function isRitualFinalized(uint32 ritualId) external view returns (bool){
+        return getRitualState(rituals[ritualId]) == RitualState.FINALIZED;
+    }


Not a comment for this PR, but something to consider I just realized: we may need to discern if a covenant is currently active or not, depending whether it's within its arranged duration or not. The fact that the ritual is FINALIZED is a prerequisite for that to happen, of course. I can address that in #107

cygnusv · 2023-08-18T08:00:09Z

contracts/contracts/coordination/GlobalAllowList.sol

+import "./IRitualAuthorizer.sol";
+import "./Coordinator.sol";
+
+contract GlobalAllowList is AccessControlDefaultAdminRules, IRitualAuthorizer {


After re-reading this PR, I think a more descriptive name for the interface is IEncryptionAuthorizer, and subsequently the contract could be GlobalEncryptionAllowList (although not as necessary if the interface is renamed).

cygnusv · 2023-08-18T08:08:00Z

contracts/contracts/coordination/GlobalAllowList.sol

+
+    function setCoordinator(Coordinator _coordinator) public {
+        require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "Only admin can set coordinator");
+        coordinator = _coordinator;


What's the use case for setting the Coordinator after deployment, if it was already set on construction time?

In the event the coordinator is redeployed to support a security patch or new feature.

cygnusv · 2023-08-18T08:18:56Z

contracts/contracts/coordination/GlobalAllowList.sol

+        bytes memory evidence,
+        bytes memory digest
+    ) public view override returns(bool) {
+        address recovered_address = keccak256(digest)


If digest is already a digest, there should be no need to call keccak256. Related to this is the lack of tests for this method. Maybe we can just put a pin on this for the moment and revisit in a separate PR, but something needs to be done here.

…tuals use a default authorization contract deployemnt (AllowList). Co-authored-by: James Campbell <[email protected]> Co-authored-by: Kieran Prasch <[email protected]>

…ization status.

…han int.

…acters.

…ual initialization

KPrasch · 2023-08-18T13:33:24Z

Thanks for all of the reviews and suggestions on this PR. I introduced some new tests and covered every RFC. Let me know if there's anything else before merging. If you've already reviewed, perhaps just skim the new commits. 🤠

derekpierre

🎸

KPrasch requested review from cygnusv and theref June 21, 2023 14:45

KPrasch changed the title ~~[WIP] CBD Auth/Deauth~~ [WIP] CBD Access Control Interfaces Jun 21, 2023

cygnusv reviewed Jun 21, 2023

View reviewed changes

KPrasch added a commit to KPrasch/nucypher-contracts that referenced this pull request Jun 21, 2023

Respond to RFCs in PR nucypher#89

5f5da4b

KPrasch mentioned this pull request Jun 22, 2023

Beefing up the Coordinator: Restrictions to initiator, ritual duration, fee model, automatic TX reimbursements, etc #86

Merged

cygnusv changed the base branch from main to development July 3, 2023 10:48

theref pushed a commit to KPrasch/nucypher-contracts that referenced this pull request Jul 4, 2023

Respond to RFCs in PR nucypher#89

9fccf05

theref force-pushed the auth branch from 5f5da4b to 9fccf05 Compare July 4, 2023 09:48

theref pushed a commit to KPrasch/nucypher-contracts that referenced this pull request Jul 8, 2023

Respond to RFCs in PR nucypher#89

f9d5dc4

theref force-pushed the auth branch 2 times, most recently from 0be3cf3 to e5e8a05 Compare July 8, 2023 17:12

KPrasch added a commit to KPrasch/nucypher-contracts that referenced this pull request Jul 9, 2023

Respond to RFCs in PR nucypher#89

db11b8f

KPrasch force-pushed the auth branch from a7bc077 to db11b8f Compare July 9, 2023 17:49

KPrasch changed the title ~~[WIP] CBD Access Control Interfaces~~ [WIP] TACo Encryption Allow Logic Interfaces Jul 10, 2023

KPrasch force-pushed the development branch from 3eebc89 to a4de3c9 Compare July 12, 2023 13:09

KPrasch added a commit to KPrasch/nucypher-contracts that referenced this pull request Jul 12, 2023

Respond to RFCs in PR nucypher#89

5e7ebb0

KPrasch force-pushed the auth branch from f286509 to b22142d Compare July 12, 2023 13:16

KPrasch added a commit to KPrasch/nucypher-contracts that referenced this pull request Jul 12, 2023

Respond to RFCs in PR nucypher#89

8b0d629

KPrasch force-pushed the auth branch from b22142d to 259b626 Compare July 12, 2023 13:57

KPrasch marked this pull request as ready for review July 17, 2023 12:38

KPrasch changed the title ~~[WIP] TACo Encryption Allow Logic Interfaces~~ TACo Encryption Allow Logic Interfaces Jul 17, 2023

KPrasch requested review from cygnusv, derekpierre, vzotova and piotr-roslaniec July 17, 2023 12:39

vzotova reviewed Jul 17, 2023

View reviewed changes

derekpierre reviewed Jul 18, 2023

View reviewed changes

cygnusv mentioned this pull request Jul 27, 2023

DKG Authorization Models nucypher/nucypher#3050

Open

piotr-roslaniec approved these changes Aug 14, 2023

View reviewed changes

cygnusv approved these changes Aug 18, 2023

View reviewed changes

KPrasch and others added 18 commits August 18, 2023 12:28

IAccessController contract

8d3dbcd

first version of AllowList contract

546a8b9

Implements per-ritual customization of authorization contract, new ri…

bfb63fa

…tuals use a default authorization contract deployemnt (AllowList). Co-authored-by: James Campbell <[email protected]> Co-authored-by: Kieran Prasch <[email protected]>

Respond to RFCs in PR nucypher#89

ae6dda5

AllowList is now for everybody.

4a9f72f

What... I mean... how...? Resolved: to blame AI.

29b3f36

Fixing mangled rebase.

8918d49

Removed defaultAccessController from Ritual init.

5499348

ritualID is uint32. It used to be, but it is now, too.

95ccb8b

New functions for getting authority from ritual ID and checking final…

7e8b9df

…ization status.

Guardrail for state that we don't think is reachable anyhow.

441d7ad

Invoking internal function by passing ritual storage pointer rather t…

908c593

…han int.

We decided to implement the operator in the correct order of its char…

83b406c

…acters.

AllowLogic enforcement via ECDSA 'evidence' check.

5a3bf88

This mapping now tracks authorizations by ritual.

7bbae03

updates coordinator tests to include allow logic contracts during rit…

d15dfe8

…ual initialization

fixes typing bug for ECDSA usage in GlobalAllowList.

d4a1c40

tests for GlobalAllowList auth functions.

ef675bf

KPrasch force-pushed the auth branch from eabc12f to ef675bf Compare August 18, 2023 13:16

Apply RFCs for PR nucypher#89

0610332

vzotova approved these changes Aug 18, 2023

View reviewed changes

derekpierre approved these changes Aug 18, 2023

View reviewed changes

KPrasch merged commit 653b455 into nucypher:development Aug 18, 2023
2 checks passed

derekpierre mentioned this pull request Aug 21, 2023

Onchain enrico allow list nucypher/nucypher#3147

Closed

derekpierre mentioned this pull request Sep 20, 2023

v7.0.0 Major feature freeze nucypher/nucypher#3200

Closed

cygnusv mentioned this pull request Oct 27, 2023

Does Enrico bear a secret in order to demonstrate his standing to adhere conditions? nucypher/taco-web#155

Closed

		import "./Coordinator.sol";


		contract AllowList is AccessControlDefaultAdminRules, IAccessController {

	for (uint i=0; i<addresses.length; i++) {
	for (uint256 i=0; i < addresses.length; i++) {

		@@ -0,0 +1,9 @@
		pragma solidity ^0.8.0;

		interface IRitualAuthorizer {

TACo Encryption Allow Logic Interfaces #89

TACo Encryption Allow Logic Interfaces #89

Conversation

KPrasch commented Jun 21, 2023 • edited Loading

cygnusv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KPrasch Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KPrasch Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KPrasch Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theref commented Jul 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekpierre Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

cygnusv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KPrasch commented Aug 18, 2023

derekpierre left a comment

Choose a reason for hiding this comment

KPrasch commented Jun 21, 2023 •

edited

Loading

KPrasch Jun 21, 2023 •

edited

Loading

KPrasch Jun 21, 2023 •

edited

Loading

KPrasch Jun 21, 2023 •

edited

Loading

derekpierre Aug 15, 2023 •

edited

Loading