Add Ed25519, HKDF, SRP-6a and a bignum backend for HAP pairing, and open the OpenSSL feature gate - #2
Conversation
CI is red for an upstream reason, not this changeThe
So lua.org is down, not GitHub and not the network path in general. Every prior run on this repo (2026-07-20) was green, so this is new and external. I could not re-run the jobs myself: the app token gets What I verified locally instead, since CI could not:
That is a good substitute but it is not CI green, and I am not claiming it is. The interpreter legs I could not cover locally are 5.1, 5.2, 5.3 and LuaJIT 2.0 — LuaJIT 2.1 covers 5.1 semantics, and I grepped for 5.2+ only syntax ( |
Your re-run hit the same wall: still lua.org, not this changeAttempt 2, which you triggered at 18:56 UTC, failed identically to attempt 1: same step, same address, before any of this code is compiled. So this is not a flaky download. Why re-running is the only remedy right now
const luaSourceTar = await tc.downloadTool(`https://lua.org/ftp/lua-${luaVersion}.tar.gz`)Two consequences worth knowing:
What I am deliberately not doingI am not rewriting the toolchain install to route around this. Every workaround available (apt-installed Lua, hererocks, a vendored tarball, or swapping the action) buys transient-outage relief at the price of permanent complexity in an eight-job matrix, and it would be landing on a PR that is already in review. If lua.org is still down tomorrow that trade looks different, and I will raise it as its own PR rather than let it ride on this branch. Suggest re-running once lua.org answers. I am glad to watch for it and ping you. I still cannot re-run myself: the app token gets None of this reflects on the change. The full suite is 14/14 on Lua 5.4, 5.5 and LuaJIT locally, with OpenSSL acceleration both enabled and disabled. |
Green on attempt 3: lua.org came back, no CI change neededYour re-run at 23:39 UTC passed all 11 checks on the same head (
That is the pattern the outage diagnosis predicted: only the legs that download So the call to leave CI alone holds, and the trigger I set (still down tomorrow means its own PR) has expired unused. Nothing to carry forward from this except that The code has not moved since the review round on this head. This unblocks the checks only. It is not a review verdict, and I cannot approve a PR authored by the same app identity, so merge still needs you. |
|
Squash all the commits |
Adds the four crypto primitives HomeKit Accessory Protocol pairing needs, plus
the OpenSSL routing work required to make one of them viable on Control4
hardware.
crypto.ed25519 (RFC 8032), pure Lua
Keygen, sign, verify, and the expanded private-key form. HAP signs repeatedly
with one long-term key, so expand_private_key(seed) returns the clamped scalar
and prefix once and sign_expanded() reuses them, which also skips the
public-key scalar multiplication: signing drops from 64.5 ms to 32.7 ms on
Lua 5.5 and 7.1 ms to 3.7 ms on LuaJIT.
Verified in layers, because a passing selftest is weak evidence for a
signature scheme. RFC 8032 section 7.1 vectors machine-extracted from the RFC
text rather than transcribed by hand (TEST 1/2/3, the 1023-byte TEST, and
TEST SHA(abc)); 30 vector assertions covering derive, sign, sign_expanded
byte-identity, accept, and rejection of a flipped message bit and a flipped
signature bit. Cross-checked against two independent implementations, not one:
python-cryptography during development, and 12 fresh random seed/message pairs
signed by Node's crypto and compared byte for byte (36/36). The non-canonical
S >= L rejection was confirmed load-bearing by deleting the check in a scratch
copy and observing that the S+L signature then verifies, since [S+L]B == [S]B,
so that test fails for the intended reason. 11 further tests cover malformed
input: verify returns false rather than raising for wrong-length signatures
and public keys, undecodable points, non-canonical S, and non-string args.
Known properties, recorded rather than hidden: verify is not constant-time
(all public data, matches TweetNaCl); the module is not reentrant, sharing
pre-allocated scratch exactly as x25519.lua already does; verification is
cofactorless, not ZIP-215 strict; and generate_private_key inherits x25519's
os.time/os.clock seeding, which is not a CSPRNG and is flagged as such.
crypto.hkdf (RFC 5869) over SHA-256 and SHA-512
Extract, Expand, a one-shot derive(), and hkdf_sha256 / hkdf_sha512 wrappers.
HAP needs the SHA-512 variant for the Pair-Setup session key, the controller
and accessory signing material, the Pair-Verify session key, and the Companion
session keys.
41 known-answer tests. The SHA-256 vectors are RFC 5869 appendix A.1-A.3
verbatim; A.4-A.7 use SHA-1, which this library does not implement, so they
are omitted rather than adapted. The RFC publishes no SHA-512 vectors, so
those come from a generator that reproduces A.1-A.3, making it a validated
oracle rather than an assumed-correct one. PRK values come from a separate
crypto.createHmac call and are asserted independently, and each OKM is
expanded from the PRK this implementation derived rather than from the
published one, so a broken extract cannot be masked by expand being handed
the right input. The six HAP/Companion derivations are pinned with their real
salt and info strings ("Pair-Setup-Encrypt-Salt", "Control-Salt" /
"ClientEncrypt-main", ...), so a regression in salt or info handling surfaces
here as a wrong key rather than as a pairing failure on hardware.
Deliberately not routed to openssl.kdf, and the reason is recorded in the
module: hmac_sha256/hmac_sha512 already return the OpenSSL result when
acceleration is on, and a HAP derivation is two HMAC invocations in total, so
a separate route would buy nothing measurable while adding a path that cannot
be exercised on a host without the binding. Feature.KDF is still declared so
the capability stays queryable if that trade changes.
crypto.bignum
Arbitrary-precision unsigned integers sized for 3072-bit modexp (RFC 5054
group 15), which SRP needs. 24-bit limbs, CIOS Montgomery multiplication with
a sliding window, and a slow bitwise square-and-multiply reference path the
selftest cross-checks the fast one against. 54 known-answer tests. Overflow
bound is 2^48-1, from carrying on every multiply-accumulate iteration rather
than per column, which keeps it exact in doubles on the 5.1/LuaJIT legs with
5 bits to spare.
The canonical representation is always the pure-Lua limb table; OpenSSL is
used only inside mod_exp, converting in via bn.text and out via bn.tohex.
That is deliberate: crypto.use_openssl() can be toggled at runtime, and if
handles were sometimes userdata and sometimes tables a mid-flight toggle
would produce mixed operands and silent breakage. Conversion is a few hundred
bytes against a 3072-bit exponentiation, so it costs nothing. Verified
against a real lua-openssl 0.11.1, not only against stand-ins: accelerated
and pure paths produce identical output, and acceleration is 26x faster here.
crypto.srp, SRP-6a client for HAP Pair-Setup
RFC 5054 group 15 (3072-bit) with SHA-512 and username "Pair-Setup" as the
HAP configuration, parameterised by group and hash. Client side only, stated
in the module LuaDoc so nobody assumes server support exists.
The conventions come from srptools, the library pyatv actually drives for HAP,
not from a fresh reading of RFC 5054, because what matters here is what
interoperates with an Apple TV. The distinction is real: most values are
hashed in minimal big-endian form with leading zero bytes stripped, and PAD()
is applied only inside k = H(N | PAD(g)) and u = H(PAD(A) | PAD(B)). The salt
is hashed exactly as received, so a salt with a leading zero byte keeps it.
Getting that wrong is a roughly 1-in-256 intermittent pairing failure rather
than an obvious break, so one committed vector uses such a salt specifically
to pin it, and that vector was verified to genuinely discriminate before being
trusted.
48 tests: 30 vector assertions across 3 vectors, asserting A, k, x, v, u, S,
K, M1, M2 and verify(M2) separately so a failure localises to a step rather
than just saying "M1 wrong". Plus 18 functional tests covering B mod N == 0,
B == N (which a naive byte-comparison check would miss), empty B, u == 0,
zero private exponent, a wrong or truncated M2, and accessors called before
process(). Verified beyond the committed vectors with 8 fresh random sessions
generated by srptools, varying PIN formats and including leading-zero salts,
checking A, K, M1 and verify(M2): 32/32. That is the check that would catch a
self-consistent-but-wrong implementation, since the committed vectors and the
code could in principle share a mistaken assumption.
Also adds tools/generate_srp_vectors.py, which produces 3072/SHA-512 vectors
from srptools, so the committed vectors encode the convention that
interoperates rather than a fresh reading of the RFC.
Opening the OpenSSL feature gate
openssl_wrapper.get() iterated the string-keyed OpenSSLFeature table with
ipairs, which visits nothing. _openssl_module_features therefore stayed empty
and every get(<feature>) call returned nil, while get() with no feature
argument returned the module, which is why the failure was silent.
Rather than only swapping in pairs, features now declare
{ min_version, probe? } and resolve lazily, at most once, on first request. A
version bound alone cannot answer "was this build compiled with openssl.bn?",
which is exactly the question the bignum backend needs answered, so routing is
now declarative instead of hardcoded:
BN probes the exact conversion route the bignum backend uses, big-endian
bytes in through bn.text and hex out through bn.tohex, and requires
4^13 mod 497 to come back 445, so a binding cannot pass on symbol
names alone. It accepts either spelling of modular exponentiation:
lua-openssl 0.8.5 as shipped on Control4 spells it bn.powmod, not
bn.mod_exp, and probing only for the latter would have closed the gate
on the one path where OpenSSL is a large real win rather than a nicety.
KDF probes for kdf.derive
OKP requires a completed sign/verify round-trip, so it reports false on the
Control4 behaviour where pkey.new("ed25519") is nil and an imported
key's sign() returns nil
17 regression tests driven by injected stand-in bindings rather than the
host's real lua-openssl, so the result is identical on CI and on machines with
no binding installed. The first case is the direct regression for the ipairs
bug and was verified to fail against the pre-fix file. openssl_wrapper is
registered as a module in run_tests.sh and crypto.selftest(), and exposed on
the crypto table so callers can query openssl_wrapper.features().
Fixing the gate does not activate ChaCha20-Poly1305's OpenSSL branch on
Control4, and the distinction matters. Both of its call sites request
Feature.AAD, and the controller reports lua-openssl 0.8.5, below the 0.9.2
floor AAD requires, so it stays on the pure-Lua path there before and after
this change. That floor is load-bearing rather than decorative: on 0.8.5,
cipher:update(aad, true) is byte-for-byte identical to cipher:update(aad),
meaning the AAD flag is ignored and the AAD is encrypted as though it were
plaintext. Verified three ways on hardware: the flagged call does not
reproduce the RFC 8439 vector, it produces exactly the same stream as
encrypting aad .. plaintext with no AAD at all, and the same cipher with no
AAD does reproduce the RFC 8439 ciphertext. Had the gate opened there, the
library's own guard ("AAD update should not return data in AEAD mode") would
have raised. Both findings are pinned as named regression cases, so a future
change to the version floor has to argue with a measurement instead of a
guess.
CI: a lua-openssl version matrix alongside the Lua matrix
The existing matrix proves portability across interpreters, but every leg runs
with no lua-openssl installed, so it only ever exercised the pure-Lua paths.
That is how ChaCha20-Poly1305's OpenSSL branch stayed unreachable for its
whole existence while the suite stayed green. This adds the other axis: which
binding is present.
0.8.5-1 the binding shipped on Control4 DriverWorks, AAD expected false
0.9.2-2 the exact floor at which AAD starts working, expected true
0.11.1-1 current upstream, expected true
Each leg asserts the feature map resolves as measured, then runs the suite
both with and without acceleration. The 0.8.5 leg is a faithful Control4
proxy, not an approximation: installing that exact rock locally reproduced the
controller's behaviour, with cipher:update(aad, true) returning the AAD length
instead of 0 and the ciphertext not matching RFC 8439, identically to the
controller, despite this host running OpenSSL 3.6.3 against the controller's
3.1.4. The defect tracks the lua-openssl version, not the OpenSSL library or
the hardware, which is exactly what makes a version floor the right mechanism.
OKP is asserted false on all three. No lua-openssl build tested can sign with
an Ed25519 key (0.11.1 over OpenSSL 3.6.3 raises "not support ed25519"), so
routing crypto.ed25519 to pure Lua is not a Control4 workaround but the only
correct choice everywhere. If that assertion ever flips, the routing decision
should be revisited, which is why it is asserted rather than assumed.
fail-fast is off so one binding's failure cannot mask another's, and build now
depends on this job as well as the interpreter matrix.
Performance on Control4, which answers the ticket's open question
Measured on the dev controller (192.168.2.92) rather than reasoned from the
module listing. bn.powmod over RFC 5054 group 15 runs 5.08 ms/op with a
256-bit exponent and 60.92 ms/op with a full 3072-bit exponent, both checked
against independently computed expected values. Timing pure-Lua mod_exp across
4/8/16/32/64/96-bit exponents fits ms = 4982 + 669*bits at R^2 = 0.9962, which
extrapolates to roughly 176 s for the 256-bit SRP exponent and 2061 s for a
full-width one. That is a factor of about 34,000, and a whole Pair-Setup
client goes from under 0.05 s to roughly 15 minutes.
It is not merely slow. A direct attempt at the 256-bit case blocked the
driver's Lua thread until the driver was reset without finishing, and other
drivers stopped being serviced while it ran. So the pure-Lua path is a
correctness reference and a portability fallback, not something shippable on
this hardware, and Feature.BN is effectively a precondition for HAP pairing
there. Recorded in CLAUDE.md, including the recommendation that callers check
for BN and fail loudly rather than fall back into a controller hang.
Docs: README gains both new modules in the algorithm table, usage examples for
signing (including the expanded-key form) and HKDF, and a table of what each
lua-openssl binding actually supports, since that is not derivable from a
version number. CLAUDE.md records the routing decisions, the Control4
measurements behind them, and that bn's modular exponentiation is spelled
powmod on every build tested.
Green on lua 5.5, 5.4, 5.3, 5.2, 5.1 and luajit 2.0/2.1; 14/14 modules pass
across the suite with acceleration both on and off.
FL-3
18f22c0 to
0c3b255
Compare
Squashed to one commit, tree unchangedNine commits collapsed into one: Verified the squash changed nothing but history:
Pushed with One thing I folded rather than concatenatedThe old A mechanical squash would have preserved both the wrong statement and its correction in one message. The combined message carries only the corrected version. Same for the This repo is squash-merge only with Where it stands
|
derek-miller
left a comment
There was a problem hiding this comment.
Reviewed as the consumer: I am the one who has to build HAP Pair-Setup and Pair-Verify on top of this.
The algorithms are complete and the API shape is right. One gap blocks me from using it safely, and it is not one of the four modules.
Completeness against HAP
| Step | Needs | Provided |
|---|---|---|
| Pair-Setup M1-M2 | SRP-6a 3072/SHA-512, username Pair-Setup |
srp.new + Session:get_public |
| Pair-Setup M3-M4 | client proof M1, server proof M2, shared K | get_proof, verify, get_session_key |
| Pair-Setup M5-M6 | HKDF-SHA512, ChaCha20-Poly1305, Ed25519 sign/verify | hkdf_sha512, existing AEAD, ed25519.sign/verify |
| Pair-Verify M1-M4 | X25519, Ed25519, HKDF-SHA512, AEAD | existing x25519 + new ed25519/hkdf |
| Session framing | HKDF-SHA512 with ClientEncrypt-main / ServerEncrypt-main, AEAD |
present; the 8-byte counter nonce padding is correctly left caller-side |
| Proof comparison | constant time | bytes.constant_time_compare |
Nothing missing. Two API decisions are exactly what I would have asked for:
sign_expanded+expand_private_key. HAP signs repeatedly with the same long-term key, so skipping the seed hash and the public-key scalar mult per signature is the right optimisation to expose rather than hide.Session:set_private(a). Gives me an entropy injection point instead of forcing the library's generator.
Blocking for me: there is no CSPRNG
Every key generation path seeds a non-cryptographic PRNG from the clock:
math.randomseed(os.time() + os.clock() * 1000000 + counter)
... math.random(0, 255)Four sites, two of them new here:
ed25519.generate_private_key()(new, ed25519.lua:768)srprandom_bytes()for the client private exponenta(new, srp.lua:212)x25519.generate_private_key()(pre-existing, x25519.lua:313)x448.generate_private_key()(pre-existing, x448.lua:440)
Why this is specifically fatal for HAP rather than generally untidy:
- SRP
afeedsA = g^a mod N. A passive observer who can guessarecoversS, thereforeK, therefore the session, and can brute the PIN offline. - X25519 ephemeral in Pair-Verify guessed means session key recovery.
- Ed25519 seed is the controller's long-term pairing identity. Guessed means an attacker can impersonate our controller to the Apple TV.
At driver startup os.time() has one-second granularity and os.clock() is close to deterministic, so the real seed entropy is on the order of 20 bits. That is an offline brute force measured in seconds. On Lua 5.1 and LuaJIT math.random is C rand(). On 5.4 an explicit low-entropy randomseed actively downgrades an otherwise well-seeded generator, so this is worse than calling nothing at all.
The docstring on ed25519.generate_private_key is honest about it ("for production keys prefer supplying a seed from a system CSPRNG"), but the function still ships with an inviting name and no safe counterpart anywhere in the library.
Suggested shape
A crypto.random module, before this lands or as the immediate follow-up:
openssl.random(n, true)behind a newFeature.RANDOMprobe. Thestrongflag is the part that matters, and the probe should verify it rather than assume.random,rand_status,rand_add,rand_load,rand_writeare all present in Control4's binding (from the module listing I pulled on 2026-08-07; I have not exercisedrandomon hardware yet, the controller is unreachable as I write this)./dev/urandomfallback.- Last resort: error, do not silently return weak bytes. A HAP driver should fail to pair rather than pair insecurely.
Then route all four generators through it and leave the explicit-input entrypoints (ed25519.sign(seed, ...), x25519.diffie_hellman(priv, ...), srp:set_private(a)) exactly as they are.
Does this block the merge
Your call, and I lean no. Because those injection points exist, I can write the driver safely against this PR as-is by sourcing entropy myself. What it blocks is any caller reaching for the convenience generators, which is the more likely default. If it ships without crypto.random, I would want the unsafe generators renamed or marked deprecated so the trap is visible at the call site.
Answering your open question on the SRP guard
You asked whether bignum should refuse to run unaccelerated. Agreed that it should not: the portability the tests depend on is worth more.
But I would like a one-liner on the HAP-facing module, something like srp.is_accelerated(), so my precondition check does not have to reach through openssl_wrapper.features().BN into another module's internals. Given the measurement (256-bit modexp at roughly 176 s, and observed to block the Lua thread until the driver was reset), the footgun deserves a guard rail at the layer that owns it, even if the guard is advisory.
Things I specifically checked and think are right
- SRP conventions: minimal big-endian with leading zeros stripped,
PAD()only insidekandu, salt hashed as received. That matches what srptools and therefore pyatv actually interoperate with, which matters far more here than a fresh reading of RFC 5054. Pinning the leading-zero salt is the right vector to have. - Generating SRP vectors from srptools rather than hand-deriving them, and committing the generator.
- The three tests confirmed load-bearing by breaking what they guard, particularly Ed25519's
S >= Lrejection. - The BN probe accepting both
powmodandmod_expafter hardware corrected the assumption, and probing with a real computation rather than checking that a name exists. openssl-matrixcovering 0.8.5, 0.9.2 and 0.11.1. That is the axis that let a dead branch stay green for its whole life.
One correction to something I said earlier
I told Derek that fixing the ipairs bug would "finally enable ChaCha20-Poly1305 acceleration". Your hardware finding shows that is wrong on Control4 specifically: 0.8.5 is below the 0.9.2 AAD floor, so it correctly stays pure Lua there before and after. The acceleration only materialises on 0.9.2 and later. Your framing in the PR body is the accurate one.
Every key generation path seeded a non-cryptographic PRNG from the clock (`math.randomseed(os.time() + os.clock() * 1000000 + counter)`, then `math.random(0, 255)`). At driver startup that is worth roughly 20 bits: `os.time()` has one-second granularity and `os.clock()` is near deterministic. On 5.1 and LuaJIT `math.random` is C `rand()`; on 5.4+ an explicit low-entropy `randomseed` downgrades a generator the runtime had already seeded well, so the call was worse than making none. Add `crypto.random`, which returns strong bytes or raises. Sources, in order: `openssl.random(n, true)` behind a new `Feature.RANDOM` probe, then `/dev/urandom`, then failure. It never returns weak bytes and never returns a short read. A driver that fails to pair is a bug report; one that pairs with a guessable long-term identity looks healthy while being compromised. Route all four generators through it -- `ed25519.generate_private_key`, `srp`'s client exponent `a`, `x25519.generate_private_key` and `x448.generate_private_key` -- and leave every explicit-input entrypoint (`ed25519.sign(seed, ...)`, `x25519.diffie_hellman(priv, ...)`, `session:set_private(a)`) untouched. `Feature.RANDOM` resolves through a new `openssl_wrapper.get_ungated` rather than `get`. The acceleration flag chooses between two correct implementations everywhere else in the library; it must not be able to select a weaker source of entropy. The probe checks `rand_status()` and two real draws of the full width rather than trusting the version or the presence of a name, so a build whose RNG is stubbed or wired to a constant is rejected. Verified on a Control4 controller (dev, 2026-08-08): the shipped lua-openssl 0.8.5 has `random` and `rand_status`, `rand_status()` is true, `random(n, true)` returns n distinct bytes, and the probe passes as written. `/dev/urandom` is also readable from inside the driver sandbox, so both sources are live on the target hardware. `random(0)` and negative lengths raise on that build, hence the width validation in `bytes`. Also add `srp.is_accelerated()`, delegating to a new `bignum.is_accelerated()` so it reports what `mod_exp` will actually do -- feature gate *and* the multi-limb known-answer check -- instead of what the feature map alone implies. Advisory only: bignum stays portable, because the pure path is what keeps the suite runnable everywhere. But at roughly 176 s versus 5 ms for the 3072-bit client exponent on a controller where Lua is serialised across drivers, callers deserve the precondition at the layer that owns it rather than reaching through `openssl_wrapper.features().BN`. 15 self-tests for the new module, 3 more for the gating change. Each guard was confirmed load-bearing by removing it: dropping the exact-width check, replacing the hard failure with a `math.random` fallback, reverting x448 to its old generator, and dropping the probe's distinctness check each fail exactly the tests that name them.
|
| probe | result |
|---|---|
openssl.version() |
0.8.5 |
type(openssl.random) / type(openssl.rand_status) |
function / function |
rand_status() |
true |
random(32, true) |
32 bytes, two draws distinct |
Feature.RANDOM probe as written |
true |
Two things the module listing could not have told you:
/dev/urandomis readable from inside the driver sandbox. I expected this to
be the theoretical branch on Control4 and it is not, so the fallback is real on
the target hardware rather than only on CI runners.random(0)and negative lengths raise on that build (An unknown error occurred), which is whybytes()validates the width before it reaches the
binding rather than after.
Where I deviated from your spec
Your shape was openssl behind Feature.RANDOM, then /dev/urandom, then error.
That is what it does. One thing I changed:
Feature.RANDOM resolves through a new openssl_wrapper.get_ungated, not get.
get honours the use_openssl flag, and it should: everywhere else that flag picks
between two correct implementations, so the worst case is slow. For entropy the
worst case is not slow, it is wrong. Left on get, crypto.use_openssl(false) on a
host with no /dev/urandom would turn "generate a key" into "raise", which is safe
but absurd, and it would put a performance switch in the entropy path where nobody
would think to look for it. So the flag no longer reaches this. get_ungated still
enforces the probe: ungated means ignore the flag, not skip the check.
The probe verifies rather than assumes, per your note on strong. It requires
rand_status() to be true, and two full-width draws that differ. A build whose RNG
is stubbed or wired to a constant passes a version check and a length check but not
that one. A binding with no rand_status is treated as unseeded rather than assumed
good.
One correction to the is_accelerated suggestion
srp.is_accelerated() is there. But openssl_wrapper.features().BN, the thing you
wanted to stop reaching through, is also the wrong value, not just the inconvenient
one. bignum.mod_exp applies two conditions: the feature gate, and a multi-limb
known-answer check on the binding (accelerator_ready) that exists because
Feature.BN proves a single-digit round-trip and cannot prove this build parses a
384-byte hex string the way bignum writes it. A binding can pass the gate and still
be one bignum has decided not to trust, and features().BN reports true for it.
So srp.is_accelerated() delegates to a new bignum.is_accelerated() that applies
both conditions, which makes it a real answer to "will this finish in milliseconds
or minutes" rather than a proxy for it. Advisory, as we agreed: bignum stays
portable. I also fixed CLAUDE.md, which was still telling callers to check
features().BN.
The part to push back on if you are going to
This changes behaviour for two pre-existing public functions.
x25519.generate_private_key() and x448.generate_private_key() previously always
returned something. They can now raise. That is the behaviour you asked for, and I
think it is right, but it is a breaking change to API that predates this PR and it
is not confined to the HAP surface. Anything already calling those on a host with
neither source gets an error where it used to get 20 bits of entropy. I would rather
that be a deliberate decision than a consequence.
Mitigations, none of which fully retire the point: both sources are live on Control4
and on every CI leg, the explicit-input entrypoints (ed25519.sign(seed, ...),
x25519.diffie_hellman(priv, ...), session:set_private(a)) are untouched, and
random.set_source(fn) lets a platform with neither supply its own.
Tests
15 new self-tests for crypto.random, 3 more on the gating change, 15/15 modules
green on Lua 5.1 through 5.5 and both LuaJIT lines.
Since the contract here is a negative one, most of them assert that something
fails, and I confirmed each guard is load-bearing by removing it:
| guard removed | tests that went red |
|---|---|
exact-width check in bytes() |
short output is rejected |
hard failure replaced with a math.random fallback |
no-source raises; generators raise |
x448 reverted to its old clock-seeded generator |
generators return supplied bytes; generators raise |
two-draw distinctness in the RANDOM probe |
constant RNG not used; probe test |
The no-source case is forced deterministically (absent binding plus a _urandom_path
that cannot exist) rather than skipped on hosts that have entropy, since that is the
one case where a regression is silent.
What I did not verify
- The module has not run on the controller. I verified the primitives it depends
on there, and the module logic on three interpreters, but I did not push
crypto.randomitself to hardware and call it. Given the earlier lesson about
blocking the Lua thread I did not want to load a module for a probe I could do
directly, but it is a gap. - The
strongflag under entropy starvation.rand_status()was true every time
I asked. I have not seen what 0.8.5 does when it is false, so "the probe rejects an
unseeded pool" is reasoned from the code path, not observed. - No statistical testing of output beyond distinctness and one popcount sanity
check. That is testing OpenSSL, not us. - Lua 5.2 and 5.3 only via CI, not locally.
Self-approval is still rejected (same app identity as the author), so this needs your
verdict either way.
derek-miller
left a comment
There was a problem hiding this comment.
Re-reviewed at b5e1fa3. Both points are addressed, and one of them better than I asked for. I also ran the amalgamated build on a real controller, which produced numbers this PR did not have.
crypto.random
Right call on all three decisions.
Raising instead of falling back is the correct default for a keying primitive. The get_ungated distinction is the part I would have got wrong: everywhere else the acceleration flag picks between two correct implementations, so gating is a preference; here it would pick between a correct one and a broken one, so it must not be gated. Worth exactly the comment it got.
And the probe verifies rather than assumes. rand_status() true plus two full-width draws differing catches a stubbed or constant-wired RNG, which a version check and a length check both sail past.
random.set_source(fn) covers hosts with neither source without weakening the default. Good.
srp.is_accelerated()
Better than what I asked for. I wanted a convenience accessor; you pointed out that openssl_wrapper.features().BN is the wrong precondition twice over, because it reports true for a binding bignum has already declined to trust after its multi-limb known-answer check. Delegating to bignum.is_accelerated() so the guard applies both conditions is correct, and I would have shipped the weaker check.
Hardware measurements
Ran build/crypto-portable.lua on the dev controller (192.168.2.92, OS 4.2.1.757028-res, lua-openssl 0.8.5).
Your feature table reproduces exactly, independently measured:
AAD=false BN=true KDF=true OKP=false RANDOM=true
Load cost, which matters because it lands in driver startup: parse 0.065 s, exec 0.009 s for the 519 KB portable build. Cheap enough to ignore.
The acceleration delta is larger than I expected. SHA-512 over 1 KiB:
| per op | |
|---|---|
| pure Lua | 82.05 ms |
| OpenSSL | 0.016 ms |
About 5,100x. Which brings me to the one thing I would add to the docs.
Worth documenting: everything reads false until use_openssl(true)
My first run reported AAD=false BN=false KDF=false OKP=false RANDOM=false on hardware where four of those are true. Cause was simply that I had not called crypto.use_openssl(true), and CRYPTO_USE_OPENSSL is not set in the DriverWorks environment. That is the documented opt-in design working correctly, not a bug.
But the failure mode is quiet and expensive. A driver that forgets the call gets pure Lua everywhere, which here means SHA-512 is 5,100x slower and, more seriously, srp.is_accelerated() returns false so a HAP caller following the CLAUDE.md guidance fails closed with what looks like a missing binding rather than a missing initialisation. Both readings are indistinguishable from the caller's side.
Suggest a line in the CLAUDE.md Control4 section saying crypto.use_openssl(true) must be called during driver init before any feature query, and ideally have is_accelerated()'s failure explain which of the two it is.
The asymmetric numbers
These are the ones that decide whether the Apple TV work is viable, and this PR did not have them. All verified against their RFC vectors in the same call, so these are timings of a correct implementation, not a fast wrong one.
| operation | time | vector |
|---|---|---|
| X25519 scalar mult | 0.459 s | RFC 7748 6.1 OK |
| Ed25519 sign, cold from seed | 1.602 s | RFC 8032 7.1 OK |
| Ed25519 sign, pre-expanded | 0.786 s | RFC 8032 7.1 OK |
Ed25519 expand_private_key |
< 0.001 s | |
| Ed25519 verify | 1.583 s | valid=true |
sign_expanded is worth what you claimed. 1.602 s to 0.786 s, a 2.04x saving, and the expansion itself is free. For HAP that is a real win rather than a micro-optimisation, because the controller re-signs with the same long-term key on every connection.
Derived cost of a full Pair-Verify (two scalar mults, one sign, one verify):
2(0.459) + 0.786 + 1.583 = 3.29 s
That is workable, but only with a persistent Companion session so it is paid once per connection rather than once per app launch. For comparison, the deep-link launch itself measured 2.7 s end to end.
One caveat for whoever builds on this: these block the Lua thread, and a single 1.6 s verify is a long time to hold it. Given the note in CLAUDE.md that a long pure-Lua block starved other drivers, the Pair-Verify steps will probably need spreading across timer callbacks rather than running as one synchronous chain. That is a driver concern, not a library one, but it is a direct consequence of these numbers and belongs somewhere findable.
One cosmetic thing
The Lua language server flags issues luacheck does not, so make check being clean does not cover them:
ed25519.lua:39-40andx25519.lua:25-26both define aliasesFieldElementandProductArray, so they collide (duplicate-doc-alias). Namespacing them or hoisting to a shared annotations file would fix it.hkdf.lua:87returns 1 to 2 values where the annotation declares 1 (redundant-return-value).
Neither affects behaviour.
Verdict
The four algorithms plus crypto.random are everything I need for HAP Pair-Setup and Pair-Verify. Nothing missing, and the API shape fits the protocol rather than just exposing primitives. My blocking concern from the first pass is resolved.
Follow-up to the b5e1fa3 review. Three things it asked for. Nothing is accelerated until crypto.use_openssl(true) is called, and the failure mode is quiet: CRYPTO_USE_OPENSSL is not set in the DriverWorks environment, so a driver that skips the call gets pure Lua everywhere and openssl_wrapper.features() reports all five false on hardware where four are true. Documented under its own heading in CLAUDE.md with the measured cost. "No usable binding" and "nobody enabled acceleration" produced the same false from is_accelerated() and need opposite responses, so bignum.is_accelerated() and srp.is_accelerated() now return a reason as a second value, sourced from a new openssl_wrapper.unavailable_reason(). The reason is always present when the verdict is false. Adding a return value does not disturb existing callers. Recorded the asymmetric hardware measurements from the review, plus the consequence that a 1.583 s verify blocks the Lua thread and Pair-Verify will need spreading across timer callbacks. Also fixed the language server diagnostics. FieldElement and ProductArray are one type shared by x25519 and ed25519, so they move to a doc-only annotations.lua, which also gives Int64HighLow the definition it never had. HkdfHash becomes a @Class, which fixes the redundant-return-value at hkdf.lua:87 and two undefined-field reports with the same edit. That takes src/ from 59 coded diagnostics to 0, and make typecheck now runs the server that install-deps was already installing. Measured on lua-openssl 0.11.1 over OpenSSL 3.6.3: pkey.new("ed25519") is rejected there too, so the always-pure-Lua route for ed25519 is the current state of the binding rather than a workaround for Control4's 0.8.5.
All three addressed at
|
| situation | reason |
|---|---|
| flag never set | OpenSSL acceleration is not enabled: call crypto.use_openssl(true) during initialisation ... |
| no binding on the host | the lua-openssl binding is not available on this host |
| binding lacks the feature | the lua-openssl binding does not support BN |
| binding failed the multi-limb check | the lua-openssl binding failed bignum's multi-limb known-answer check ... |
The reason is always present when the verdict is false. Adding a return value does not disturb existing callers, and srp.is_accelerated() still just tail-calls bignum, so the two can never disagree.
Reproducing your session against a real binding (lua-openssl 0.11.1, OpenSSL 3.6.3, local):
--- before use_openssl(true)
false OpenSSL acceleration is not enabled: call crypto.use_openssl(true) during initialisation ...
AAD=false BN=false KDF=false OKP=false RANDOM=false
--- after use_openssl(true)
true nil
AAD=true BN=true KDF=true OKP=false RANDOM=true
That is your AAD=false BN=false KDF=false OKP=false RANDOM=false, now with the sentence that explains it.
I also added a note to openssl_wrapper.features()'s docstring saying it reports what get would return rather than what the host can do, since that is the function that told you the wrong thing.
Proving the new tests
Four cases in bignum, four in openssl_wrapper, one in srp. Reverting is_accelerated to its old one-value form turns three of the four bignum cases and the srp one red, so they test something. The fourth ("true with no reason for a trusted binding") passes against the old code too; it pins the no-spurious-reason half of the contract rather than the new behaviour, and I am not claiming otherwise.
The known-answer case needed a binding that is right on one limb and wrong above it, since that is the only thing that passes Feature.BN's probe and still fails accelerator_ready. make_binding({ small_only = true }) models it, and the test asserts get(Feature.BN) ~= nil in the same breath, so it also pins the distinction you flagged in the last round rather than just the message.
The hardware numbers are in CLAUDE.md
New section recording your X25519/Ed25519 table with its RFC vectors, the 519 KB load cost, the 3.29 s Pair-Verify derivation, and both consequences: hold the expanded key, and do not run the chain synchronously because a 1.583 s verify starves other drivers the same way the unaccelerated mod_exp does. Attributed as measured on the dev controller on 2026-08-08 so it is clear they are hardware figures rather than a projection.
The cosmetic issues were 2 of 59
Both fixed, but running the server across src/ turned up 59 diagnostics carrying a rule name, not 2:
| code | count | |
|---|---|---|
undefined-doc-name |
46 | Int64HighLow was never defined anywhere; used by sha512, blake2, utils/bytes |
duplicate-doc-alias |
4 | the two you found |
undefined-field |
5 | HkdfHash fields, and params missing from @class crypto.srp.Session |
duplicate-set-field |
3 | three selftests independently stubbing package.preload["openssl"] |
redundant-return-value |
1 | the one you found |
I took your shared-annotations-file option rather than namespacing. FieldElement and ProductArray really are one type used twice (x25519 and ed25519 compute over the same field with the same limb layout), so src/crypto/annotations.lua says so once, and Int64HighLow gets the definition it never had in the same file. Nothing requires it, so amalg (which traces a real run of crypto.init) does not bundle it and the shipped builds are byte-identical in structure.
hkdf.lua:87 had a shared root cause with two of the undefined-field reports: --- @alias HkdfHash { hmac: ..., length: integer } does not give the server usable fields, so params.length was undefined and params.hmac resolved to the union of both real HMAC functions, one of which tail-calls into the binding. Making it a @class fixed all three.
src/ is now at 0 of those, and make typecheck runs the server. install-deps was already installing lua-language-server and nothing ever ran it, which is why this drifted.
What I left, and why
11 diagnostics remain, all param-type-mismatch / assign-type-mismatch, and all pre-existing relative to this change:
- 6 are selftests deliberately passing
nilor a number where a string is required, to prove the function rejects it. Suppressing those removes the diagnostic's only real use. - 2 are
resolve_group'stype(group.g) == "number"branch, which the server does not narrow through. - The rest are in
aes_gcm.lua:313from Add core + portable builds (core is the canonical crypto.lua) #1.
So make typecheck currently exits non-zero, and I deliberately left it out of make check rather than either suppressing real signal or turning CI red on your behalf. Say the word and I will wire it in at whichever level you want.
One thing that came out of testing
pkey.new("ed25519") is rejected by lua-openssl 0.11.1 over OpenSSL 3.6.3 as well, with not support ed25519!!!!. So OKP=false is not a quirk of Control4's old 0.8.5 build. The always-pure-Lua route for ed25519 is the current state of the binding on a modern OpenSSL, which makes that routing decision considerably less provisional than the comment implied. Noted in CLAUDE.md.
Verification
make check clean, make build clean, full suite green on LuaJIT 2.1 (5.1 semantics), 5.4 and 5.5 locally, and CI covers 5.2/5.3 plus the three binding versions.
Not verified: I have not re-run anything on the controller. The new code paths are a doc change and a string on an existing false branch, so I do not think it needs it, but the 5,100x and the asymmetric timings in CLAUDE.md are your measurements recorded as-is, not reproduced by me.
reviewDecision is still REVIEW_REQUIRED and will stay there regardless of what I do: this PR is authored by the same app identity I would review as, so GitHub refuses the approval. That gate is yours whenever you are ready.
derek-miller
left a comment
There was a problem hiding this comment.
Approving at 596dc35. All three points from the last pass are addressed, and I verified the important one on hardware rather than taking the tests' word for it.
is_accelerated now says which, confirmed on a controller
Loaded the rebuilt portable artifact on the dev controller (192.168.2.92, lua-openssl 0.8.5) and exercised both states:
default state: accel=false
reason="OpenSSL acceleration is not enabled: call crypto.use_openssl(true)
during initialisation (CRYPTO_USE_OPENSSL is not set in every host
environment, notably Control4 DriverWorks)"
after use_openssl(true): accel=true reason=<nil>
That is exactly the distinction I asked for, and the message names both the call to make and the host it bites on. A caller now fails closed on the boolean and gets something actionable in the log instead of an ambiguous false.
The three bignum cases pinning the different reasons are the right tests, particularly the small_only binding: it answers 4^13 mod 497 correctly so Feature.BN's probe passes, but it is wrong on the multi-limb vector, so mod_exp refuses it. Asserting that is_accelerated agrees with mod_exp rather than with the feature gate is the subtle case, and it is covered.
Shared annotations
make typecheck reports no duplicate-doc-alias, so FieldElement and ProductArray are genuinely deduplicated rather than moved. Good call making the file non-required so amalg leaves the shipped builds untouched, and saying so in the header.
Adding the target at all is the more valuable half. install-deps was already installing lua-language-server and nothing ran it, so a whole class of defect was only visible to whoever had it wired into an editor. Keeping it out of check until the remaining diagnostics are triaged is the right call rather than turning CI red on deliberate bad-argument tests.
Verified locally
./run_tests.sh: 15/15 modules passmake check: 0 warnings / 0 errors across 20 filesmake build: both artifacts build, version injection worksmake typecheck: 11 remaining, allparam-type-mismatch, and all either deliberate bad-argument assertions orSrpGroup.gnarrowing between itsstring|integerforms. Matches what the Makefile comment says is left.
One thing worth a follow-up, not blocking
Int64HighLow is now defined twice: src/crypto/annotations.lua:32 and vendor/bitn.lua:1742. make typecheck passes --check "$(CURDIR)/src" so it does not scan vendor and cannot see it, but an editor with the workspace open reports duplicate-doc-alias on both. Slightly ironic given annotations.lua exists to eliminate exactly that. Since bitn is vendored, bundled, and already defines the alias, the simplest fix is probably to drop it from annotations.lua and let bitn's definition stand.
A caching gotcha, for whoever wires this into a driver
Not a defect in this PR, but I hit it twice while testing and it will bite driver work.
The amalg build registers its modules in package.loaded. If a Lua context has already loaded one build and you loadstring a newer one over it, require inside the new chunk returns the cached older modules, so you silently test the previous version. It cost me two wrong readings here, including one where I nearly reported the reason string as broken when it was the old build answering. Clearing crypto.* and bitn* out of package.loaded before re-exec fixed it.
That matters beyond testing: a driver that hot-updates its vendored crypto in a live Lua context has the same exposure.
Verdict
Approved. The four algorithms plus crypto.random cover HAP Pair-Setup and Pair-Verify completely, the accessors a HAP caller needs are on the module that owns them, and the Control4-specific behaviour is measured rather than assumed. I have what I need to build against this.
Fixes FL-3
Adds the four primitives HAP pairing needs, and fixes the feature-gating blocker they depend on.
The blocker
openssl_wrapper.get()iterated the string-keyedOpenSSLFeaturetable withipairs, which visits nothing, so_openssl_module_featuresstayed empty and everyget(<feature>)call returned nil whileget()with no arguments returned the module. That asymmetry is why it stayed invisible: hashing and HMAC (which pass no feature) have always been accelerated, and ChaCha20-Poly1305 (which passesFeature.AAD) never has.Rather than only swapping in
pairs, features now declare{ min_version, probe? }and resolve lazily, once. A version floor alone cannot answer "was this build compiled withopenssl.bn?", which is exactly what the new modules need to know.What the hardware said
I probed a Control4 controller instead of reasoning from the module listing, and it corrected me twice.
The AAD floor is load-bearing. The controller runs lua-openssl 0.8.5, below the 0.9.2
Feature.AADrequires, so ChaCha20-Poly1305 stays pure Lua there before and after this fix. That is correct, not a missed optimisation: on 0.8.5cipher:update(aad, true)is byte-identical tocipher:update(aad)— the AAD flag is ignored and the AAD is encrypted as plaintext. Confirmed three ways (fails the RFC 8439 vector; reproduces exactly the stream of encryptingaad .. plaintextwith no AAD; the same cipher without AAD does match RFC 8439), and reproduced locally on that same rock version against a different OpenSSL, so the defect tracks the binding, not the hardware.My BN probe named a function that does not exist. I wrote it against
bn.mod_exp; every build tested spells itbn.powmod. It would have closed the gate on the one path where OpenSSL is a large real win.On lua-openssl 0.11.1 the gate does open, and the AEAD branch that had never executed was verified to run, reproduce RFC 8439, round-trip, reject a tampered AAD, and produce byte-identical output to the pure-Lua path.
Modules
crypto.ed25519(RFC 8032) — keygen, sign, verify, and the expanded private-key form.sign_expandedskips both the seed hash and the public-key scalar multiplication, halving signing cost. Always pure Lua: no tested lua-openssl can sign with an Ed25519 key, including 0.11.1 over OpenSSL 3.6.3, which raisesnot support ed25519.crypto.hkdf(RFC 5869) — Extract/Expand over SHA-256 and SHA-512. Not routed toopenssl.kdfon purpose; the HMACs underneath are already accelerated and a HAP derivation is two HMAC calls, so a separate route would add an untestable path for no measurable gain.crypto.bignum— 24-bit limbs, CIOS Montgomery with a sliding window, OpenSSL-preferredmod_exp. The canonical representation is always the pure-Lua table and OpenSSL is used only insidemod_exp, so a runtimeuse_openssl()toggle can never produce mixed-type operands.crypto.srp— SRP-6a client side, RFC 5054 group 15 + SHA-512, usernamePair-Setup. Client only, stated in the LuaDoc.Verification
Vectors are RFC where RFC vectors exist, and generated from a validated oracle where they do not:
crypto.hkdfSyncafter confirming it reproduces A.1–A.3. Also pins all six HAP/Companion derivations with their real salt and info strings.srptools, the library pyatv drives for HAP, so they encode the convention that actually interoperates rather than a fresh reading of RFC 5054.tools/generate_srp_vectors.pyis committed and deterministic. Beyond the committed vectors, 8 fresh random sessions (varied PIN formats, leading-zero salts) were cross-checked against srptools — 32/32 — which is the check that catches a self-consistent-but-wrong implementation.The SRP conventions are worth knowing: most values are hashed in minimal big-endian form with leading zero bytes stripped, and
PAD()applies only insidekandu. The salt is hashed exactly as received. One vector uses a leading-zero salt to pin that, and I confirmed it genuinely discriminates rather than trusting that it would.Three tests were confirmed load-bearing by breaking the thing they guard: Ed25519's
S >= Lrejection (removing it makes theS+Lsignature verify), the gate's regression case (it fails against the pre-fix file), and SRP's leading-zero-salt vector.CI
New
openssl-matrixjob covering lua-openssl 0.8.5 (Control4), 0.9.2 (the AAD floor), and 0.11.1 (upstream). Each asserts the feature map resolves as measured and runs the suite with acceleration both on and off. Every leg was run locally against real bindings first. This is the other axis the existing matrix never covered: all six interpreter legs run with no binding installed, which is how a dead OpenSSL branch stayed green for its whole life.The number you asked for
Pure-Lua
mod_expon the controller, fitted across 4/8/16/32/64/96-bit exponents (R² = 0.9962):ms = 4982 + 669 × bits.bn.powmodA = g^a mod N)A whole Pair-Setup client is roughly 15 minutes pure Lua against under 0.05 s with OpenSSL. It is also not a graceful degradation: a direct 256-bit attempt blocked the driver's Lua thread until the driver was reset, and starved other drivers while it ran.
So the pure-Lua bignum is a correctness reference and portability fallback, not a shippable path on Control4.
Feature.BNis effectively a precondition for HAP pairing there. Recorded in CLAUDE.md with the recommendation that callers checkopenssl_wrapper.features().BNand fail loudly. I did not makebignumitself refuse to run — that would cost the portability the tests depend on — but say the word if you want that.Test surface
14/14modules green on Lua 5.4, 5.5 and LuaJIT, with OpenSSL acceleration both on and off.make checkclean across 18 files.make buildproduces workingcrypto.luaandcrypto-portable.lua; the portable artifact was verified standalone in an empty directory with zero external requires.Notes
reviewDecision.