Shielded AccessControl #190

emnul · 2025-07-23T23:12:01Z

Types of changes

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation Update (if none of the other choices apply)

Closes #88

This pull request introduces a Shielded AccessControl module. Roles are stored as MerkleTree commitments to avoid disclosing information regarding the roles an account may have. Role commitments are created with using the following hashing scheme SHA256 ( SHA256(PublicKey | roleIdentifier | nonce) | index ) where index is the index of the role commitment in the MerkleTree. We track this using a mapping from a role commitment to an index in the MerkleTree. A Counter tracks the next index available in the MerkleTree. The index also enforces uniqueness for all role commitments in the MerkleTree. A Bytes<32> salt value is included in the ledger state that would allow devs to use a public pseudorandom salt value instantiated using the Initializable module to strengthen the underlying HKDF function. A salt value equal to length of the output of the chosen hashing function (SHA256 in our case) is recommended.

The nonce is created locally using HKDF-SHA256(SK, info="role-nonce" || roleId || PK || ContractAddress, length=32) where HKDF refers to HMAC-based Extract-and-Expand Key Derivation Function. Length is set to 32 to allow us to hash the nonce in-circuit with the PublicKey and role identifier (all values must be of type Bytes<32>). Additionally, it's recommended that length of HKDF be set It's important to note that since HKDF is not verifiable on-chain we cannot enforce its use. Users may generate nonce however they want, but we should strongly recommend using the method described above as well as provide an implementation to ensure an attacker cannot break privacy.

PR Checklist

I have read the Contributing Guide
I have added tests that prove my fix is effective or that my feature works
I have added documentation of new methods and any new behavior or changes to existing behavior
CI Workflows Are Passing

Notes on Contract Design

Why use assertions instead of if statements in `_checkMerkleTree`?

We're able to reduce circuit size by about 280 rows for each circuit using assertions instead of if statements

Why use SHA256( SHA256(PublicKey | roleIdentifier | nonce) | index) as the hashing scheme for role commitments

We're able to significantly reduce circuit size by about 4000 rows for each circuit using this scheme.

Why not use an Anonymous Role-Based Access Control implementation?

One of the goals of this Shielded Access Control implementation is to keep the API consistent with the Unshielded Access Control module. An Anonymous Role-Based Access Control implementation would require an entirely different design. Please let us know if you'd like to see an Anonymous Role-Based Access Control implementation in the Github Discussions

Why does compilation take so long?

Unfortunately, Compact does not have a ZK friendly hashing function that can be used to derive state at this time, so the SHA256 hash function must be used. The usage of SHA256 incurs a large circuit size penalty on the contract which in turn slows compilation times.

Why include contract address in nonce creation scheme?

Adding the contract address improves domain separation

contracts/src/access/witnesses/ShieldedAccessControlWitnesses.ts

andrew-fleming · 2025-09-15T01:38:22Z

contracts/src/access/ShieldedAccessControl.compact

+   * - A nullifier for the role commitment produced by SHA256(roleId | account | nonce)
+   * must not exist in the `_roleCommitmentNullifiers` set.


Sooo we need to improve the nullifier construction. The purpose of the nullifier set is to track which commitments have been used or spent without revealing which specific commitment was used

https://docs.midnight.network/develop/how-midnight-works/keeping-data-private#the-commitmentnullifier-pattern

andrew-fleming · 2025-09-15T03:27:24Z

contracts/src/access/ShieldedAccessControl.compact

+  export circuit hasRole(roleId: Bytes<32>, account: Either<ZswapCoinPublicKey, ContractAddress>): Role {
+    assert(!Utils_isContractAddress(account), "ShieldedAccessControl: contract address roles are not yet supported");
+
+    const nonce = wit_secretNonce(roleId);
+    const index = wit_getRoleIndex(roleId, account);


I'm not sure I understand the purpose of this circuit. The name hasRole implies that anyone can check if account has the roleId role, but the caller would need to have the secret nonce associated with account to calculate the commitment. Only the role grantee would be able to call this on themself then, right?

If the idea is that we're checking if the caller has the role, I'd change the circuit name to callerHasRole and remove account and just use ownPublicKey. If this is meant to check any account has any role, I don't see how this works with the secret nonce witness. Let me know if I'm missing something

I definitely think a callerHasRole circuit is a great addition, but hasRole, in my conceptualization of this system, would be useful for admin users and I think still deserves a place in the contract. The name could probably be improved to make it clear the caller has privileged access to user info.

andrew-fleming · 2025-09-15T03:59:40Z

contracts/src/access/test/ShieldedAccessControl.test.ts

+    type FailingCircuits = [
+      method: keyof ShieldedAccessControlSimulator,
+      isValidNonce: boolean,
+      isValidIndex: boolean,
+      isValidPath: boolean,
+      args: unknown[],
+    ];
+    const checkedCircuits: FailingCircuits[] = [
+      ['assertOnlyRole', false, true, true, [DEFAULT_ADMIN_ROLE]],
+      ['assertOnlyRole', true, false, true, [DEFAULT_ADMIN_ROLE]],
+      ['assertOnlyRole', true, true, false, [DEFAULT_ADMIN_ROLE]],
+      ['assertOnlyRole', false, false, true, [DEFAULT_ADMIN_ROLE]],
+      ['assertOnlyRole', true, false, false, [DEFAULT_ADMIN_ROLE]],
+      ['assertOnlyRole', false, true, false, [DEFAULT_ADMIN_ROLE]],
+      ['assertOnlyRole', false, false, false, [DEFAULT_ADMIN_ROLE]],
+      ['grantRole', false, true, true, [DEFAULT_ADMIN_ROLE, Z_ADMIN]],
+      ['grantRole', true, false, true, [DEFAULT_ADMIN_ROLE, Z_ADMIN]],
+      ['grantRole', true, true, false, [DEFAULT_ADMIN_ROLE, Z_ADMIN]],
+      ['grantRole', false, false, true, [DEFAULT_ADMIN_ROLE, Z_ADMIN]],
+      ['grantRole', true, false, false, [DEFAULT_ADMIN_ROLE, Z_ADMIN]],
+      ['grantRole', false, true, false, [DEFAULT_ADMIN_ROLE, Z_ADMIN]],
+      ['grantRole', false, false, false, [DEFAULT_ADMIN_ROLE, Z_ADMIN]],
+      ['revokeRole', false, true, true, [DEFAULT_ADMIN_ROLE, Z_ADMIN]],
+      ['revokeRole', true, false, true, [DEFAULT_ADMIN_ROLE, Z_ADMIN]],
+      ['revokeRole', true, true, false, [DEFAULT_ADMIN_ROLE, Z_ADMIN]],
+      ['revokeRole', false, false, true, [DEFAULT_ADMIN_ROLE, Z_ADMIN]],
+      ['revokeRole', true, false, false, [DEFAULT_ADMIN_ROLE, Z_ADMIN]],
+      ['revokeRole', false, true, false, [DEFAULT_ADMIN_ROLE, Z_ADMIN]],
+      ['revokeRole', false, false, false, [DEFAULT_ADMIN_ROLE, Z_ADMIN]],
+    ];


This is doing too much IMO. It might be worth keeping describe blocks focused on one circuit at a time. It's a preference, but IMO it's more organized and easier to follow...or maybe have helper assertion functions if there's a lot of repetition

This def works though if you want to keep it. Let's get everything working first and then we can laser in on the testing deets

contracts/src/access/test/ShieldedAccessControl.test.ts

Co-authored-by: Andrew Fleming <[email protected]> Signed-off-by: ⟣ €₥ℵ∪ℓ ⟢ <[email protected]>

…elin/midnight-contracts into shielded-access-control

0xisk

Thank you @emnul, that’s a partial review — I left some comments.

(From my experince in the client-side) Midnight’s key management has two main keys: ZswapCoinPublicKey and EncPublicKey. Since inside Compact we only deal with ZswapCoinPublicKey, I think it’s best to explicitly call it zcpk or zpk instead of just pk. This should help prevent any confusion for client-side developers who might otherwise mistake it for EncPublicKey.
If that makes sense, then to avoid further confusion, I’d also suggest unifying the terminology where OwnerId and AccountId are currently used interchangeably. We could instead consistently use zcpk for the raw key and shieldedZcpk for the derived identifier (SHA256(zcpk, nonce)).

0xisk · 2025-09-23T11:44:49Z

contracts/src/access/ShieldedAccessControl.compact

+ * disclosing information about role holder. Role commitments are created with the following
+ * hashing scheme SHA256(roleId | account | nonce | merkleTreeIndex).
+ *
+ * @notice Using the SHA256 hashing function comes at a significant performace cost. In the future, we


Suggested change

* @notice Using the SHA256 hashing function comes at a significant performace cost. In the future, we

* @notice Using the SHA256 hashing function comes at a significant performance cost. In the future, we

0xisk · 2025-09-23T11:46:28Z

contracts/src/access/ShieldedAccessControl.compact

+ * in the top-level contract and be unique. One way to achieve this is by
+ * using `export sealed ledger` hash digests that are initialized in the top-level contract:
+ *
+ * ```typescript


Suggested change

* ```typescript

* ```compact

it won't highlight here anyways.

0xisk · 2025-09-23T11:46:59Z

contracts/src/access/ShieldedAccessControl.compact

+ *
+ * To restrict access to a circuit, use {assertOnlyRole}:
+ *
+ * ```typescript


Suggested change

* ```typescript

* ```compact

0xisk · 2025-09-23T11:51:40Z

contracts/src/access/ShieldedAccessControl.compact

+   * @type {MerkleTree<10, roleCommitment>}
+   * @type {MerkleTree<10, Bytes<32>>} _operatorRoles
+   */
+  export ledger _operatorRoles: MerkleTree<10, Bytes<32>>;


Why do we have 10 as a fixed depth for the tree?

It was an arbitrarily large number I thought would be sufficient for most use cases. It's not possible to dynamically determine the tree size nor would that be desirable considering the additional initialization overhead every circuit would need.

0xisk · 2025-09-23T11:53:15Z

contracts/src/access/ShieldedAccessControl.compact

+  export ledger _currentMerkleTreeIndex: Counter;
+
+  export ledger DEFAULT_ADMIN_ROLE: Bytes<32>;


We need to include docs here.

0xisk · 2025-09-23T12:19:49Z

contracts/src/access/ShieldedAccessControl.compact

+   * The `id` is expected to be computed off-chain as:
+   * `id = SHA256(pk, nonce)`
+   *
+   * - `pk`: The owner's public key.


Suggested change

* - `pk`: The owner's public key.

* - `zcpk`: The owner's ZswapCoinPublickey.

0xisk · 2025-09-23T12:33:49Z

contracts/src/access/ShieldedAccessControl.compact

+   * - The type data of `account` - a ZswapCoinPublicKey or ContractAddress.
+   *
+   * @param {Bytes<32>} roleId - The role identifier.
+   * @param {Either<ZswapCoinPublicKey, ContractAddress>} account - A ZswapCoinPublicKey or ContractAddress.


Suggested change

* @param {Either<ZswapCoinPublicKey, ContractAddress>} account - A ZswapCoinPublicKey or ContractAddress.

* @param {Either<ZswapCoinPublicKey, ContractAddress>} accountId - A ZswapCoinPublicKey or ContractAddress.

0xisk · 2025-09-23T12:34:09Z

contracts/src/access/ShieldedAccessControl.compact

+   *
+   * @param {Bytes<32>} roleId - The role identifier.
+   * @param {Either<ZswapCoinPublicKey, ContractAddress>} account - A ZswapCoinPublicKey or ContractAddress.
+   * @param {Bytes<32>} nonce - A nonce created using SHA256(SK | "role-nonce" | role | PK)


Suggested change

* @param {Bytes<32>} nonce - A nonce created using SHA256(SK | "role-nonce" | role | PK)

0xisk · 2025-09-23T12:36:42Z

contracts/src/access/ShieldedAccessControl.compact

+  }
+
+  /**
+   * @description Attempts to grant `roleId` to `account` and returns a boolean indicating if `roleId` was granted.


Suggested change

* @description Attempts to grant `roleId` to `account` and returns a boolean indicating if `roleId` was granted.

* @description Attempts to grant `roleId` to `accountId` and returns a boolean indicating if `roleId` was granted.

0xisk · 2025-09-23T13:21:52Z

contracts/src/access/ShieldedAccessControl.compact

+  export circuit _revokeRole(roleId: Bytes<32>, accountId: Bytes<32>): Boolean {
+    const role = getRole(roleId, accountId);
+    if (!role.isApproved) {
+      return false;
+    }
+
+    _roleCommitmentNullifiers.insert(disclose(role.commitmentNullifier));
+    return true;
+  }


I'm curcious are we talking care of cleaning the _operatorRoles MT after this revoke to be storage efficient?

I had thought about doing this initially, but I could not come up with a Merkle tree management solution that wasn't overly complex or did not add significant ledger storage / circuit size overhead.

andrew-fleming added 30 commits April 15, 2025 21:17

bump midnight-js

bccf098

remove unused deps

780132e

remove eslint and prettier, add biome

3419a25

run fmt

45a2563

run lint

fec71f5

organize imports

86565b3

remove dev deps from contracts/

24c2e78

remove unused deps

82e64e5

update turbo and scripts

ebb2728

add clean script

cd757c0

add clean script

a5fb60b

clean up test config

b4fb0df

remove jest reporters

3462103

bump turbo to 2.5

de6ab59

bump turbo

7c211d7

fix package names

5cfcf80

fix conflicts

f49e2d3

set up compact compiler and builder

f23d40b

update scripts

69e86c1

update yarn.lock

065f48b

update readme

dfef7ec

fix fmt

d4678fd

fix conflicts

8cdd29e

fix fmt

6135d9c

remove lingering file

8bbfce3

update biome, fix fmt

7634d77

add requirements in dev section

18ab0f3

add devdeps to contracts packages

bedff4f

simplify workspace

dc7ae8f

remove unnecessary button rule

ff967e5

emnul added 15 commits September 8, 2025 16:17

Merge branch 'main' into shielded-access-control

1fa7122

Update Role field name

26d3576

Add tests

da274c4

Remove _unsafeGrantRole

7ae6407

Improve tests

5a1b0b4

Add helper method

923d779

Change privateState fn signatures

07dd40b

Add tests

fdcf13c

Merge branch 'main' into shielded-access-control

b08a600

Should not throw if commitment in nullifer set

7e6a6a0

Export nullifiers for testing

8e5c26f

Rename var and change return behavior

dd0cd82

Add _checkRole, grantRole tests

f24eeb0

fmt files

2cb227e

WIP

061ac4a

andrew-fleming reviewed Sep 13, 2025

View reviewed changes

contracts/src/access/witnesses/ShieldedAccessControlWitnesses.ts Outdated Show resolved Hide resolved

andrew-fleming reviewed Sep 15, 2025

View reviewed changes

emnul and others added 11 commits September 15, 2025 13:50

Update contracts/src/access/witnesses/ShieldedAccessControlWitnesses.ts

a98144e

Co-authored-by: Andrew Fleming <[email protected]> Signed-off-by: ⟣ €₥ℵ∪ℓ ⟢ <[email protected]>

Optimize loop

9d9c256

Merge branch 'shielded-access-control' of https://github.com/OpenZepp…

52936ef

…elin/midnight-contracts into shielded-access-control

Refactor Shielded Design

5d2da06

Move resetProxy call to end of fn

713d3d4

Fixes incorrect indexing bug

8a5fbba

WIP refactor tests

2fce76c

Merge branch 'main' into shielded-access-control

306ec1f

Fix compiler errors, refactor mock and witnesses

2fb86e7

Refactor test suite

a2e644d

Refactor simulator for new design

16928c3

0xisk requested changes Sep 23, 2025

View reviewed changes

WIP

740bb83

		* - A nullifier for the role commitment produced by SHA256(roleId \| account \| nonce)
		* must not exist in the `_roleCommitmentNullifiers` set.

	* @notice Using the SHA256 hashing function comes at a significant performace cost. In the future, we
	* @notice Using the SHA256 hashing function comes at a significant performance cost. In the future, we

		export ledger _currentMerkleTreeIndex: Counter;

		export ledger DEFAULT_ADMIN_ROLE: Bytes<32>;

	* - `pk`: The owner's public key.
	* - `zcpk`: The owner's ZswapCoinPublickey.

	* @param {Either<ZswapCoinPublicKey, ContractAddress>} account - A ZswapCoinPublicKey or ContractAddress.
	* @param {Either<ZswapCoinPublicKey, ContractAddress>} accountId - A ZswapCoinPublicKey or ContractAddress.

	* @description Attempts to grant `roleId` to `account` and returns a boolean indicating if `roleId` was granted.
	* @description Attempts to grant `roleId` to `accountId` and returns a boolean indicating if `roleId` was granted.

Shielded AccessControl #190

Are you sure you want to change the base?

Shielded AccessControl #190

Uh oh!

Conversation

emnul commented Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Types of changes

PR Checklist

Notes on Contract Design

Why use assertions instead of if statements in _checkMerkleTree?

Why use SHA256( SHA256(PublicKey | roleIdentifier | nonce) | index) as the hashing scheme for role commitments

Why not use an Anonymous Role-Based Access Control implementation?

Why does compilation take so long?

Why include contract address in nonce creation scheme?

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

0xisk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

emnul commented Jul 23, 2025 •

edited

Loading

Why use assertions instead of if statements in `_checkMerkleTree`?

0xisk left a comment •

edited

Loading