feat(access-prover): Implement ZK authorization circuit and CLI - #12
feat(access-prover): Implement ZK authorization circuit and CLI#12Flourishbar wants to merge 1 commit into
Conversation
|
Thanks for the implementation. The structure is solid, but I don't think we should merge this yet because the circuit currently ships a custom Poseidon implementation and custom-generated parameters. For a ZK authorization circuit, we should avoid rolling our own cryptographic primitives. Please replace the custom Poseidon implementation with the audited Arkworks Poseidon implementation from Also, please either justify or remove the extra session binding hash. Since Once those are addressed, this should be much safer to review and merge. |
Joaco2603
left a comment
There was a problem hiding this comment.
Review — PR #12
Thanks @Flourishbar for the implementation. The structure is solid and the end-to-end test is well thought out. However, I agree with Joaco2603 that there are cryptographic concerns that need addressing before merge.
Issue 1: Custom Poseidon implementation with self-generated parameters (CRITICAL)
The PR ships a full custom Poseidon implementation (circuit/mod.rs) that generates its own round constants, MDS matrix, and parameters using a hardcoded random seed (ChaCha8Rng::seed_from_u64(1337)):
let full_rounds = 8;
let partial_rounds = if width <= 3 { 56 } else { 60 };
let alpha = 5;This is problematic:
- The fixed seed
1337is not a standard or audited parameter generation method - The MDS matrix construction via rejection-sampled Cauchy matrix could produce weak instances
- The round counts (
R_f=8, R_p=56) do not match standard Poseidon parameter sets — for BN254 (∼256-bit security), standard Poseidon usesR_f=8, R_p=57for width 3 - Alpha=5 is correct for BN254 (the field modulus
q ≡ 1 mod 5), but the implementation must be audited
Replace this with ark-crypto-primitives Poseidon, which is already in the dependencies. It provides audited, standard parameters for BN254 and includes r1cs support:
use ark_crypto_primitives::crh::poseidon::constraints::CRH;Issue 2: Unused session binding constraint
The circuit computes:
let binding1 = poseidon_hash_circuit(&[user_secret_var, session_nonce_var], &config)?;
let _binding2 = poseidon_hash_circuit(&[binding1, ciphertext_hash_var], &config)?;_binding2 is assigned but never used. This creates unnecessary constraints that add no security — session_nonce and ciphertext_hash are already public inputs, so Groth16 binds the proof to them by default. Either remove this section or explain why it is needed (if the goal is to enforce a specific relationship between these values, the result must be constrained against something).
Issue 3: parse_fr parses arbitrary hex strings without domain separation
parse_fr accepts any hex string and left-pads with zeros to 32 bytes:
let start = 32 - bytes.len();
padded[start..].copy_from_slice(&bytes);
Ok(Fr::from_be_bytes_mod_order(&padded))This means two different hex strings can produce the same Fr value (e.g., "0x01" and "0x0001"). For a ZK protocol, input parsing should be canonical to avoid ambiguity in proof verification. Consider rejecting non-canonical encodings or using a domain-separated parse.
What I would keep
- The CLI architecture (setup/prove/verify subcommands)
- The proof serialization helpers
- The test structure (e2e happy + sad path with tampered inputs)
- The
MerklePathJson/SessionInputtypes
Veredicto: CHANGES_REQUESTED. The critical issue is the custom Poseidon — audited crypto primitives are non-negotiable for a ZK circuit that may handle production data. Happy to re-review once the Arkworks Poseidon is integrated and the unused constraint is addressed.
closes #1
Description
This PR introduces the
access-provercrate—a utility for generating and verifying zero-knowledge (ZK) anonymous access proofs. It allows a user to prove they are authorized (i.e. possess a secret whose hash belongs to a valid membership Merkle tree) without leaking their identity, wallet address, or the encrypted prompt itself.The proof is bound to a specific session nonce and ciphertext hash to prevent third-party replay or reuse.
Proposed Changes
Cargo.tomlworkspace members to includecrates/access-prover.crates/access-prover/Cargo.tomlwith Arkworks dependencies (ark-ff,ark-ec,ark-groth16,ark-relations,ark-r1cs-std, andark-crypto-primitives) using thebn254curve.crates/access-prover/src/circuit/mod.rsfeaturing a Poseidon hash membership proof circuit over a Merkle tree of depth 16.crates/access-prover/src/types.rsandcrates/access-prover/src/proof.rssupporting setup, proof generation, and verification, as well as exporting the verification key as a static byte array (vk_const.rs).crates/access-prover/cli/main.rswithsetup,prove, andverifysubcommands.generate_sessionto streamline developer testing.Technical Details
Public Inputs:
policy_root: The root hash of the authorized users Merkle tree.session_nonce: A unique session identifier.ciphertext_hash: The hash of the encrypted prompt.Private Inputs (Witnesses):
user_secret: The caller's private secret.authorization_note: The hash of the user secret (Poseidon(user_secret)).merkle_path: The siblings and indices representing the path from the note to the root.Verification & Testing
Automated Tests
Run the test suite locally with:
cargo test -p access-prover --target x86_64-pc-windows-gnu