nacljc review — 2026-09-26

Review of the repo at main (0.4.0-SNAPSHOT, commit 750ffc5), three days after it started, three releases in. Scope: src/nacljc/core.cljc, the test suite, bb tasks, CI/release workflows, and docs. Signet itself was explicitly out of scope; it is mentioned only where nacljc's own tasks or docs depend on it.

Verified today

Check	Result
bb test:bb	pass — 41 tests / 581 assertions
bb test:jvm	pass — 41 / 581
bb lint, bb fmt	clean, 0 warnings
nbb exit code on failing test	exits 1 (verified — bb test:nbb cannot silently pass)
bb test:signet-bb	fails against current ../signet main (see finding 1)
JVM read of a noaccess secret	InternalError (as documented)
JVM read of a destroyed secret	hard SIGSEGV → abort, exit 134, hs_err_pid*.log (contradicts docs — finding 2)

Verdict

The C-boundary discipline is the strong point and it holds up under reading: every fixed-size input is length-checked before allocation, every native buffer is wiped in a finally, every return code is mapped to a typed ex-info, the public API is pinned by a test, and each guard was shown to bite by removing it (feasibility.md's failure-count table is the right kind of evidence). The secret-memory design (guarded pages, noaccess between calls, use counter under a lock, canary-checked free, stack wipe afterwards) is sound in the single- and multi-threaded cases I traced, including the partial-failure rollbacks.

The findings below are mostly documentation drift and edge cases, plus one stale integration. Nothing found is a live memory-safety hole.

Findings

1. bb test:signet-* is broken against current signet — README overclaims

bb test:signet-bb fails at load:

Message: signet.impl.jvm has no destroy-material!
{:backend :jca} — signet/impl.clj:55

integration/signet-shim/signet/impl/jvm.clj implements "the same 16 functions" as signet's old backend contract; signet's signet.impl facade now requires 18 (destroy-material!, split-material were added for the vault/session work in signet 0.8–0.9). The marker mechanism still works (backend: :libsodium is detected), so the failure is loud, not silent — the suite fails rather than testing the wrong thing.

Consequences:

• README line "signet runs on it unchanged … passes signet's own, unmodified suite" is stale for signet main; it was true for the 0.7-era contract.
• test:signet-* are local-only tasks (CI doesn't run them, they need ../signet), so nothing is red except locally.
• test:jca is probably still fine: jca_crosscheck.clj uses signet.impl.jvm directly, bypassing the facade.

Options (not asked to change): pin the signet commit the shim targets, or grow destroy-material!/split-material in the shim (they map onto memzero!/secret-split).

2. Documented JVM fault behaviour is wrong for destroyed secrets

README and test/secrets/run.clj say the JVM "raises InternalError on macOS". Reproduced today: that is true only for reading a noaccess secret (page still mapped, PROT_NONE). Reading a destroyed secret — freed sodium_malloc memory — is a full VM crash:

SIGSEGV at StubRoutines::jint_disjoint_arraycopy
→ Abort trap: 6, exit 134, hs_err_pid*.log written

The two hs_err_pid*.log files in the repo root (2026-09-25) are exactly this, from test/secrets/child.cljc. The test passes either way (it only requires a non-zero exit and no READ), so the inaccuracy is invisible to CI.

Why it matters: a caller who retains a secret's .-ptr field (public on the deftype) and reads it after secret-destroy! does not get a catchable InternalError — the whole JVM dies. That is arguably the desired protection, but the docs should say so. CLAUDE.md's "bb test:secrets proves the fault" line inherits the same imprecision.

3. stackzero-bytes = 16 KiB is an unverified constant

The docstring says "libsodium's functions need a few KiB" — fine for Ed25519/HMAC/AEAD. The deepest call now wrapped by it is xwing-decapsulate, which runs ML-KEM-768 decapsulation internally; ML-KEM works on multi-KiB polynomial vectors on the stack. Whether the deepest frame chain stays under 16 KiB is inference, not measurement. Cheap mitigation: raise the constant, or check libsodium's ML-KEM stack footprint (the repo has ../libsodium cloned for exactly this kind of check).

4. Naming-convention drift inside core.cljc

The project's own rule is "! means the call writes state that outlives it; ! never means may-throw; throw-helpers are named throw-… / check-…".

• need-xwing! writes nothing and only throws — should be throw-… or lose the !. Private, so cosmetic, but it is the boundary file the rule was written for.
• Docstring wording is uneven: 0.2.0+ functions use the fixed Impure: … / Throws ::x when … phrasing; most 0.1.0 functions (random-bytes, memzero!, ed25519-*, x25519*, chacha20-*, sha-256, hmac-sha-256, constant-time-equal?) describe behaviour without the markers and without enumerating their ex-data :types.

5. finally paths can mask the original exception

• with-secret: if the body throws and secret-destroy! then throws ::secret-in-use (another thread holds the window), the destroy error replaces the body's — the more interesting error is lost.
• with-open-secrets: run! close-secret! stops at the first throwing close, leaving the remaining opened secrets readonly and skipping stackzero!. Both are unlikely (mprotect failures), but the unwind order is worth a second look in a file this carefully audited.

6. The version floor and ::unsupported-by-libsodium are untested in CI

Both CI jobs run libsodium ≥ 1.0.22 (Homebrew, or 1.0.22 built from source). Nothing exercises:

• ::libsodium-too-old at load (needs 1.0.18 — the common Ubuntu case that motivated the check),
• the ::unsupported-by-libsodium X-Wing path,
• the .so.23 fallback in default-locations (meant to reach the version check to say "too old").

test:loading covers missing/not-libsodium but not too-old. One CI job pointing NACLJC_LIBSODIUM at the distro's 1.0.18 would cover all three.

7. Docs disagree on the signet suite numbers

• README: "JVM: 105 tests / 500 assertions … babashka: 95 / 476"
• CLAUDE.md and feasibility.md: "102 / 436" and "93 / 415"

Presumably the README numbers are newer (signet grew). Both documents are in-repo and reachable from the README; pick one source of truth or mark feasibility.md as a frozen snapshot.

8. Windows fails cleanly but is undocumented

os returns :win32, default-locations has no Windows entry, and the load path throws ::library-not-found with the "install libsodium >= 1.0.19" hint — a good failure. But the README's Requirements table never says Windows is unsupported/untested; feasibility.md mentions it once. One README line would close the gap. Note: ffi/load-library may in fact load a DLL if NACLJC_LIBSODIUM points at one — worth a sentence either way.

9. Secret internals are reachable through public fields

.-ptr, .-n, .-lock, .-state on the Secret deftype are accessible to any caller (deftype fields can't be private). The security argument — "reads fault when noaccess" — holds, but:

• (.-ptr s) retained past secret-destroy! is a dangling pointer, and dereferencing it is the finding-2 crash.
• (.-state s) is an atom anyone can swap! — (swap! (.-state s) assoc :destroyed false) would un-destroy a freed secret and make the next use a use-after-free. Nothing prevents this today; a docstring note on the deftype ("fields are internal") is the cheap fix.

10. Smaller items

• release-check accepts ## 0.4.0 (unreleased) as a CHANGELOG section (substring match on "## 0.4.0") — a release could ship with the "(unreleased)" heading still in place.
• hs_err_pid*.log files accumulate in the repo root from every test:secrets JVM run (gitignored, but worth deleting or noting).
• test:jar runs only the core suite against the jar — loading and secrets checks always run against src/. Low value to change, but the jar path is the one users actually load.
• constant-time-equal? does not accept secrets, so comparing two secrets requires exporting both to the heap — the exact thing secrets exist to avoid. sodium_memcmp over two open windows would fix it; a reasonable 0.4.0 candidate.
• secret-import! wipes bs only on success — correct (failure keeps the caller's only copy), but the docstring reads as unconditional.
• secret-split overflow: (reduce + lengths) can overflow past Long/MAX_VALUE, but the wrapped sum is negative and every length is checked pos?, so rejection still happens — safe by accident, fine to leave.
• :ulong is used for the unsigned long long length args (crypto_sign_detached, crypto_hash_sha256, HMAC update). On LP64 (macOS/Linux) :ulong and :size_t are both 64-bit so it's correct today; on an LLP64 platform it would be a 32-bit truncation. Latent, matters only if Windows support is ever attempted.
• The HKDF shim in test/wasm/test/browser doesn't enforce the RFC 5869 255×32 output cap — already marked TODO in feasibility.md; test-only code, fine.
• test/browser/run.mjs's .. guard is dead code (normalize strips .. before the check; join confines to ROOT anyway). Harmless.

What's notably good

• The audit-hook design (*audit* recording alloc/wipe/protect/free) is what makes "rejected input allocates nothing" and "every buffer wiped" testable rather than aspirational — including the over-read case where C returns the right answer anyway.
• ed25519-sign taking the seed and deriving the 64-byte key inside native memory removes the double-public-key scalar-leak oracle by construction rather than by validation.
• check-not-released before test:jar prevents the local ~/.m2 shadowing footgun — clearly learned the hard way (comment says so).
• Release flow verifies the artifact that was actually published (test:clojars against an empty local repo) before cutting the GitHub release — the right order.
• Linux CI pins both the libsodium tarball and the bb binary by SHA-256 and asserts the bb binary is dynamically linked — the two failure modes it already hit are locked down.
• secret-split's sodium_add-onto-zeroed-memory copy is a genuinely neat substitute for the memcpy libsodium doesn't export, and it's tested against the multi-part and partial-failure cases.

---

Resolution (0.3.2, 2026-09-26)

Verified against the code (and libsodium's ML-KEM source for finding 3). New tests shown to fail against the pre-0.3.2 code (8 failures, 1 error).

#	Result
1	The shim is retired. bb test:signet runs signet's own suite in ../signet with nacljc overridden to this checkout: JVM 183/1000 + 54 parity checks, bb 173/974.
2	Fixed in README, secret-destroy!'s docstring and test:secrets: a read after destroy crashes the JVM (use after free). The JVM's crash logs go to target/.
3	Confirmed: indcpa_enc alone has about 12 KB of locals. X-Wing operations wipe 64 KiB; others keep 16 KiB. Also found: encapsulation (and decapsulation with a byte-array seed) opened no secret, so 0.3.1 did not wipe after them; now they always do.
4	need-xwing! is check-xwing. Every function, public and private, now states purity and errors in fixed wording.
5	Fixed: with-secret rethrows the body's exception (destroy error as suppressed); a failed close still closes the rest and wipes the stack.
6	Deferred to 0.4.0: a CI job against Ubuntu's libsodium 1.0.18.
7	README numbers current; feasibility.md marked as a frozen snapshot.
8	README: Windows not supported or tested.
9	A comment on the deftype: the fields are internal.
10	release-check requires a dated heading (tested both ways); secret-import!'s docstring says the wipe happens on success only. Deferred to 0.4.0: constant-time-equal? over secrets. No action: overflow, :ulong, test-only shims.