standards: add core authorization primitives by HDauven · Pull Request #36 · dusk-network/contracts

HDauven · 2026-05-28T14:21:31Z

Part 2 of the Dusk standards stack. Replaces closed #28 after #27 was squash-merged.

Adds the core Dusk-native authorization model:

Principal for Moonlight, Phoenix, and contract identities
runtime call context handling
nonce and replay guards
signed authorization envelopes bound to chain, contract, domain, action, payload, nonce, and expiry
reentrancy guard security primitive
client-side signed authorization construction example

Review focus:

Phoenix vs Moonlight caller assumptions
action envelope binding
nonce/replay behavior
exposed low-level unbound authorization helpers

Validation:

make standards-ci

HDauven · 2026-05-28T14:25:08Z

This replaces closed #28. GitHub closed #28 when #27's head branch was deleted by the old auto-delete-head-branches repo setting; #36 is the same next stack step rebased on the merged #27/main.

moCello · 2026-05-28T15:34:30Z

+pub mod auth;
+pub mod core;
+pub mod security;


BLOCKER: Cryptographic authorization, nonce, replay, and reentrancy modules ship with no test coverage.

Context: auth/mod.rs (~760 lines), core/principal.rs (~220 lines), core/nonce.rs (~140 lines), core/replay.rs (~80 lines), security/reentrancy_guard.rs (~70 lines) contain no #[cfg(test)]. Only core/context.rs has tests (5 cases for the runtime-parts parser). The PR description names "Phoenix vs Moonlight caller assumptions, action envelope binding, nonce/replay behavior, exposed low-level unbound authorization helpers" as review focus — none of those invariants are pinned by a test. proptest, serde_json, and dusk-data-driver dev-deps are declared but only consumed by use foo as _; placeholders.

Impact: Regressions in any of these primitives (e.g. an envelope-mismatch path that silently passes, a replay key that double-consumes, a nonce that wraps, an expiry check that flips its inequality, a reentrancy lock that fails to unlock on panic) are undetectable by CI. Every downstream standard depends on these guarantees.

Fix: Add at least: nonce stream consume / use_next / invalidate_until / overflow / import_entries merge; replay consume duplicate panic and is_used; principal to_bytes round-trip across all three variants + ordering across kinds + serde round-trip; reentrancy enter panic on reentry + lock RAII unlock on panic via catch_unwind; AuthorizedAction::message_bytes byte layout + is_expired boundary at expires_at == 0; verify_moonlight / verify_phoenix happy path + wrong-signer + envelope mismatch + expired + nonce-out-of-order + Phoenix replay_key double-spend; authorize_principal[_action] context-match vs signed-fallback paths and rejection when both fail.

moCello · 2026-05-28T15:34:32Z

+fn assert_live(action: &AuthorizedAction, now: u64) {
+    if action.is_expired(now) {
+        panic!("Authorization: expired");
+    }
+}


WARNING: Hardcoded panic string breaks the error::* constant convention used everywhere else in this PR.

Context: Every other panic in auth/mod.rs, core/nonce.rs, and core/replay.rs panics with a error::* constant (e.g. error::UNAUTHORIZED, error::INVALID_NONCE, error::REPLAY), all prefixed DuskStandards: …. This single site uses a one-off literal "Authorization: expired" with a different prefix. Two sources of truth for the error vocabulary.

Fix: Add pub const EXPIRED: &str = "DuskStandards: expired"; in core/error.rs and panic with it.

moCello · 2026-05-28T15:34:34Z

+pub const ALREADY_INITIALIZED: &str = "DuskStandards: already initialized";
+pub const NOT_INITIALIZED: &str = "DuskStandards: not initialized";
+pub const UNAUTHORIZED: &str = "DuskStandards: unauthorized";
+pub const INVALID_OWNER: &str = "DuskStandards: invalid owner";
+pub const INVALID_PRINCIPAL: &str = "DuskStandards: invalid principal";
+pub const ZERO_PRINCIPAL: &str = "DuskStandards: zero principal";
+pub const REPLAY: &str = "DuskStandards: replay";
+pub const INVALID_NONCE: &str = "DuskStandards: invalid nonce";
+pub const DELAY_NOT_ELAPSED: &str = "DuskStandards: delay not elapsed";
+pub const OPERATION_DONE: &str = "DuskStandards: operation already done";
+pub const OPERATION_UNKNOWN: &str = "DuskStandards: operation unknown";
+pub const INVALID_OPERATION: &str = "DuskStandards: invalid operation";
+pub const OVERFLOW: &str = "DuskStandards: arithmetic overflow";
+pub const UNDERFLOW: &str = "DuskStandards: arithmetic underflow";


WARNING: Most of the error vocabulary is unused by this PR — speculative generality reserved for future stack PRs.

Context: Only UNAUTHORIZED, REPLAY, INVALID_NONCE, and OVERFLOW are referenced in the diff. ALREADY_INITIALIZED, NOT_INITIALIZED, INVALID_OWNER, INVALID_PRINCIPAL, ZERO_PRINCIPAL, DELAY_NOT_ELAPSED, OPERATION_DONE, OPERATION_UNKNOWN, INVALID_OPERATION, UNDERFLOW are forward declarations for stack parts not yet landed. Project rules say "Don't add features, refactor, or introduce abstractions beyond what the task requires."

Fix: Land each error string in the PR that first consumes it.

moCello · 2026-05-28T15:34:35Z

+    pub fn enter(&mut self) {
+        if self.entered {
+            panic!("ReentrancyGuard: reentrant call");
+        }
+        self.entered = true;
+    }
+
+    /// Exits the guarded section.
+    pub fn exit(&mut self) {
+        self.entered = false;
+    }
+
+    /// Enters the guarded section and returns an RAII lock.
+    pub fn lock(&mut self) -> ReentrancyLock<'_> {
+        self.enter();
+        ReentrancyLock {
+            entered: &mut self.entered,
+        }
+    }


WARNING: Public enter/exit lets the guard be left in an inconsistent state — mis-sequenceable public surface.

Context: A caller using enter() without a paired exit() leaves entered = true and locks all future calls out forever; exit() is callable without enter(), silently masking misuse; panic! inside an enter()/exit() pair never resets the flag (only the RAII Drop does). Also, the panic string "ReentrancyGuard: reentrant call" does not use an error::* constant or the DuskStandards: prefix — same drift as assert_live.

Fix: Narrow enter/exit to pub(crate) (or private) and force callers through lock() / run(). Switch the panic to a new error::REENTRANCY constant.

moCello · 2026-05-28T15:34:37Z

+        match expected.kind() {
+            PrincipalKind::Moonlight | PrincipalKind::Contract => {
+                if context.principal == Some(expected) {
+                    return expected;
+                }
+            }
+            PrincipalKind::Phoenix => {}
+        }
+
+        let Some(authorization) = authorization else {
+            panic!("{}", error::UNAUTHORIZED);
+        };
+        let principal = self.verify_signed(authorization, now);
+        if principal != expected {
+            panic!("{}", error::UNAUTHORIZED);
+        }
+        self.consume_verified(authorization);
+        principal
+    }


WARNING: When the runtime context matches expected, a provided SignedAuthorization is silently dropped — its nonce/replay state never consumed.

Context: Same shape repeats in authorize_principal_action at lines 461-479. A contract author calling authorize_principal(owner, ctx, Some(permit), now) expecting the permit's nonce to burn will find that — when the owner was also the runtime caller — the permit is never verified or consumed. The permit can then be replayed in a follow-up call where the context no longer matches.

Fix: Either document the "auth is ignored when context matches" behavior in the doc comment, or always consume the auth when present (verify the principal matches, then consume), or split into authorize_principal_context_only vs authorize_principal_with_permit.

moCello · 2026-05-28T15:34:39Z

+/// Phoenix Schnorr authorization.
+///
+/// This proves control of the Phoenix Schnorr public key represented by
+/// `action.principal`. It does not prove ownership or spending of a specific
+/// Phoenix note.
+#[derive(Archive, Serialize, Deserialize, Clone, Debug, PartialEq)]
+#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
+#[archive_attr(derive(CheckBytes))]
+pub struct PhoenixSignatureAuthorization {
+    /// Authorized action.
+    pub action: AuthorizedAction,
+    /// Phoenix Schnorr public key.
+    pub public_key: SchnorrPublicKey,
+    /// Signature over `action.message_hash()`.
+    pub signature: SchnorrSignature,
+    /// Optional extra replay key consumed with the signature.
+    pub replay_key: Option<ReplayKey>,
+}


NOTE: replay_key is Phoenix-only and its purpose is undocumented.

Context: MoonlightAuthorization (lines 187-194) has no replay_key. The doc on the field reads "Optional extra replay key consumed with the signature" without saying when a contract should require one, what it protects against beyond the nonce stream, or who supplies the key. The PR description's review-focus section calls this out but the doc does not.

Fix: Expand the field doc to explain the threat model (e.g. when two Phoenix authorizations share a nonce stream and the contract needs an extra dedup token) and add a usage example or guidance on whether contracts should accept None.

moCello · 2026-05-28T15:34:41Z

+    pub fn is_zero(&self) -> bool {
+        match self {
+            Self::Moonlight(bytes) => bytes.iter().all(|byte| *byte == 0),
+            Self::Phoenix(bytes) => bytes.iter().all(|byte| *byte == 0),
+            Self::Contract(id) => id.to_bytes().iter().all(|byte| *byte == 0),
+        }
+    }


NOTE: is_zero has no callers in this PR — dead public API, paired with the unused ZERO_PRINCIPAL error constant.

Context: The intent (reject zero principals at construction or in verify_*) is not wired up anywhere. Same speculative-generality smell as the unused error strings.

Fix: Either consume is_zero in verify_signed / Principal::moonlight / Principal::phoenix_public_key to reject the zero value with error::ZERO_PRINCIPAL, or drop both is_zero and ZERO_PRINCIPAL until a caller needs them.

moCello · 2026-05-28T15:34:42Z

+    /// Invalidates all nonces below `new_nonce`.
+    pub fn invalidate_until(
+        &mut self,
+        principal: Principal,
+        domain: NonceDomain,
+        new_nonce: u64,
+    ) {
+        if new_nonce < self.current(principal, domain) {
+            panic!("{}", error::INVALID_NONCE);
+        }
+        self.nonces.insert((principal, domain), new_nonce);
+    }


NOTE: Doc phrasing is slightly off — the function advances the next-expected nonce; "below" suggests per-nonce tracking.

Fix: "Advances the next-expected nonce to new_nonce, invalidating any unused nonce in [current, new_nonce). Panics if new_nonce would lower the stream."

moCello · 2026-05-28T15:34:43Z

+    /// Imports entries, rejecting conflicting duplicates through the set.
+    pub fn import_entries(
+        &mut self,
+        entries: impl IntoIterator<Item = ReplayEntry>,
+    ) {
+        for entry in entries {
+            self.used.insert((entry.principal, entry.key));
+        }
+    }


NOTE: "Rejecting conflicting duplicates" is misleading — duplicates are silently merged by the set, not rejected.

Fix: "Imports entries; duplicates are deduplicated by the set, not rejected."

moCello · 2026-05-28T15:34:45Z

+#[cfg(test)]
+use dusk_data_driver as _;
+#[cfg(test)]
+use dusk_vm as _;
+#[cfg(test)]
+use proptest as _;
+#[cfg(test)]
+use rand as _;
+#[cfg(test)]
+use serde_json as _;
+#[cfg(feature = "serde")]
+use serde_with as _;
+#[cfg(feature = "serde")]
+use time as _;


NOTE: use foo as _; shims declare dev-deps and serde adapters that are not used yet — same speculative-generality shape as the unused error constants.

Context: proptest, serde_json, dusk-data-driver, serde_with, time are all declared via the suppression shim with no actual consumer in the diff. The shim is the project's accepted alternative to #[allow(unused_crate_dependencies)], but here it suppresses a lint that is signalling something true (the dep is unused). Justified if the next PR in the stack lands the consumers; otherwise the deps should be removed.

Fix: Either land the proptest / data-driver / serde tests in this PR, or drop the deps until the consuming PR.

moCello

REQUEST CHANGES — cryptographic authorization, nonce, replay, and reentrancy primitives land with zero test coverage despite the PR's own review-focus list naming them; please add tests for the envelope-binding, nonce stream, replay key, expiry, and reentrancy invariants before merge. Warnings on the assert_live panic-string drift, unused error vocabulary, mis-sequenceable ReentrancyGuard::enter/exit, and silently-dropped permits in authorize_principal[_action] should also be addressed.

standards: add core authorization primitives

875eafd

HDauven marked this pull request as ready for review May 28, 2026 14:25

HDauven requested a review from moCello May 28, 2026 14:25

moCello reviewed May 28, 2026

View reviewed changes

moCello requested changes May 28, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

standards: add core authorization primitives#36

standards: add core authorization primitives#36
HDauven wants to merge 1 commit into
mainfrom
standards/02-core-auth-security

HDauven commented May 28, 2026

Uh oh!

HDauven commented May 28, 2026

Uh oh!

moCello May 28, 2026

Uh oh!

moCello May 28, 2026

Uh oh!

moCello May 28, 2026

Uh oh!

moCello May 28, 2026

Uh oh!

moCello May 28, 2026

Uh oh!

moCello May 28, 2026

Uh oh!

moCello May 28, 2026

Uh oh!

moCello May 28, 2026

Uh oh!

moCello May 28, 2026

Uh oh!

moCello May 28, 2026

Uh oh!

moCello left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

HDauven commented May 28, 2026

Uh oh!

HDauven commented May 28, 2026

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!

moCello left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants