From 9c4397a5191b075769a2728be990213de90b2a96 Mon Sep 17 00:00:00 2001 From: bmatusiak Date: Thu, 23 Jul 2026 13:18:33 -0400 Subject: [PATCH 1/7] Fix early-abort bug in _read_response() truncating multi-packet replies The except-handler bailed out as soon as any data had been received, on any read exception - including an ordinary 2s per-read timeout mid-stream. That truncated real multi-packet responses (e.g. the 1216-byte X-Wing pubkey) whenever one read happened to time out before the next packet arrived, well before the caller's actual deadline. Now it keeps polling until the real deadline regardless. Found while debugging onlykey-testing's TC-04 keygen test intermittently getting back short reads (e.g. 1024/1152 bytes instead of 1216). --- onlykey/age_plugin/onlykey_hid.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/onlykey/age_plugin/onlykey_hid.py b/onlykey/age_plugin/onlykey_hid.py index 929de0d..3c1569d 100644 --- a/onlykey/age_plugin/onlykey_hid.py +++ b/onlykey/age_plugin/onlykey_hid.py @@ -104,8 +104,12 @@ def _read_response(self, expected_size=0, timeout_ms=10000): if expected_size and len(result) >= expected_size: break except Exception: - if result: - break + # A single read timing out mid-stream doesn't mean the + # device is done sending - keep polling until the real + # deadline. Bailing out early here (as soon as `result` was + # non-empty) was truncating multi-packet responses like the + # 1216-byte X-Wing pubkey whenever one 2s read happened to + # time out before the next packet arrived. continue return bytes(result[:expected_size] if expected_size else result) From 8c0ae957a4b632004d15f3fa8d117cb18774635a Mon Sep 17 00:00:00 2001 From: bmatusiak Date: Thu, 23 Jul 2026 19:25:58 -0400 Subject: [PATCH 2/7] Fix multi-packet response truncation in _read_response() by ensuring all data is read before processing errors --- onlykey/age_plugin/onlykey_hid.py | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/onlykey/age_plugin/onlykey_hid.py b/onlykey/age_plugin/onlykey_hid.py index 3c1569d..31d16dc 100644 --- a/onlykey/age_plugin/onlykey_hid.py +++ b/onlykey/age_plugin/onlykey_hid.py @@ -96,21 +96,25 @@ def _read_response(self, expected_size=0, timeout_ms=10000): while time.time() < deadline: try: data = self.ok.read_bytes(64, timeout_ms=2000) - if data: - text = bytes(data).decode("ascii", errors="ignore") - if text.startswith("Error"): - raise RuntimeError(f"OnlyKey: {text.strip()}") - result.extend(data) - if expected_size and len(result) >= expected_size: - break except Exception: # A single read timing out mid-stream doesn't mean the # device is done sending - keep polling until the real # deadline. Bailing out early here (as soon as `result` was # non-empty) was truncating multi-packet responses like the # 1216-byte X-Wing pubkey whenever one 2s read happened to - # time out before the next packet arrived. + # time out before the next packet arrived. This only guards + # the read() call itself - a real device-reported error + # (below) still needs to propagate immediately, not get + # silently swallowed by a broad except around both. continue + if not data: + continue + text = bytes(data).decode("ascii", errors="ignore") + if text.startswith("Error"): + raise RuntimeError(f"OnlyKey: {text.strip()}") + result.extend(data) + if expected_size and len(result) >= expected_size: + break return bytes(result[:expected_size] if expected_size else result) From 5e7b0d2c2cfe4643abd1640bab1d1e66c6d52868 Mon Sep 17 00:00:00 2001 From: bmatusiak Date: Fri, 24 Jul 2026 12:05:52 -0400 Subject: [PATCH 3/7] Refactor Bech32 encoding for derived identities into a separate module and update related functions in cli.py and derived_xwing.py to use the new implementation --- onlykey/age_plugin/bech32.py | 89 +++++++++++++++++++++++++++++ onlykey/age_plugin/cli.py | 82 +------------------------- onlykey/age_plugin/derived_xwing.py | 46 +++++++++++---- tests/test_derived_xwing.py | 17 +++++- 4 files changed, 140 insertions(+), 94 deletions(-) create mode 100644 onlykey/age_plugin/bech32.py diff --git a/onlykey/age_plugin/bech32.py b/onlykey/age_plugin/bech32.py new file mode 100644 index 0000000..7b66633 --- /dev/null +++ b/onlykey/age_plugin/bech32.py @@ -0,0 +1,89 @@ +"""Bech32 encoding for age recipients/identities. + +Simplified implementation for age1onlykey1... / AGE-PLUGIN-ONLYKEY-1... +format - deliberately has no length cap (unlike the standard `bech32` PyPI +package, which enforces BIP-173's 90-character limit), since a 1216-byte +X-Wing recipient encodes to something far longer than that. + +Extracted from cli.py (where this originated, used correctly there for the +slot-based recipient/identity encoding) into its own module so +derived_xwing.py can use the same encoder for derived identities without a +circular import between the two. +""" + +BECH32_CHARSET = "qpzry9x8gf2tvdw0s3jn54khce6mua7l" + + +def _bech32_polymod(values): + """Internal function for Bech32 checksum.""" + GEN = [0x3B6A57B2, 0x26508E6D, 0x1EA119FA, 0x3D4233DD, 0x2A1462B3] + chk = 1 + for v in values: + b = chk >> 25 + chk = ((chk & 0x1FFFFFF) << 5) ^ v + for i in range(5): + chk ^= GEN[i] if ((b >> i) & 1) else 0 + return chk + + +def _bech32_hrp_expand(hrp): + return [ord(x) >> 5 for x in hrp] + [0] + [ord(x) & 31 for x in hrp] + + +def _bech32_create_checksum(hrp, data): + values = _bech32_hrp_expand(hrp) + data + polymod = _bech32_polymod(values + [0, 0, 0, 0, 0, 0]) ^ 1 + return [(polymod >> 5 * (5 - i)) & 31 for i in range(6)] + + +def _bech32_verify_checksum(hrp, data): + return _bech32_polymod(_bech32_hrp_expand(hrp) + data) == 1 + + +def _convertbits(data, frombits, tobits, pad=True): + """General power-of-2 base conversion.""" + acc = 0 + bits = 0 + ret = [] + maxv = (1 << tobits) - 1 + for value in data: + if value < 0 or (value >> frombits): + return None + acc = (acc << frombits) | value + bits += frombits + while bits >= tobits: + bits -= tobits + ret.append((acc >> bits) & maxv) + if pad: + if bits: + ret.append((acc << (tobits - bits)) & maxv) + elif bits >= frombits or ((acc << (tobits - bits)) & maxv): + return None + return ret + + +def bech32_encode(hrp: str, data: bytes) -> str: + """Encode bytes as Bech32.""" + values = _convertbits(list(data), 8, 5) + checksum = _bech32_create_checksum(hrp, values) + return hrp + "1" + "".join(BECH32_CHARSET[d] for d in values + checksum) + + +def bech32_decode(bech: str): + """Decode Bech32 string to (hrp, data_bytes).""" + if any(ord(x) < 33 or ord(x) > 126 for x in bech): + return None, None + bech = bech.lower() + pos = bech.rfind("1") + if pos < 1 or pos + 7 > len(bech): + return None, None + hrp = bech[:pos] + data = [BECH32_CHARSET.find(x) for x in bech[pos + 1:]] + if -1 in data: + return None, None + if not _bech32_verify_checksum(hrp, data): + return None, None + decoded = _convertbits(data[:-6], 5, 8, False) + if decoded is None: + return None, None + return hrp, bytes(decoded) diff --git a/onlykey/age_plugin/cli.py b/onlykey/age_plugin/cli.py index ccb1782..94c6a8b 100644 --- a/onlykey/age_plugin/cli.py +++ b/onlykey/age_plugin/cli.py @@ -23,93 +23,13 @@ from onlykey.age_plugin import ( __version__, PLUGIN_NAME, DEFAULT_XWING_SLOT, validate_ecc_slot, ) +from onlykey.age_plugin.bech32 import bech32_encode, bech32_decode from onlykey.age_plugin.protocol import ( Stanza, b64encode_no_pad, b64decode_no_pad, run_identity_v1, run_recipient_v1, ) -# Bech32 encoding for age recipients/identities -# Simplified implementation for age1onlykey1... format - -BECH32_CHARSET = "qpzry9x8gf2tvdw0s3jn54khce6mua7l" - - -def _bech32_polymod(values): - """Internal function for Bech32 checksum.""" - GEN = [0x3B6A57B2, 0x26508E6D, 0x1EA119FA, 0x3D4233DD, 0x2A1462B3] - chk = 1 - for v in values: - b = chk >> 25 - chk = ((chk & 0x1FFFFFF) << 5) ^ v - for i in range(5): - chk ^= GEN[i] if ((b >> i) & 1) else 0 - return chk - - -def _bech32_hrp_expand(hrp): - return [ord(x) >> 5 for x in hrp] + [0] + [ord(x) & 31 for x in hrp] - - -def _bech32_create_checksum(hrp, data): - values = _bech32_hrp_expand(hrp) + data - polymod = _bech32_polymod(values + [0, 0, 0, 0, 0, 0]) ^ 1 - return [(polymod >> 5 * (5 - i)) & 31 for i in range(6)] - - -def _bech32_verify_checksum(hrp, data): - return _bech32_polymod(_bech32_hrp_expand(hrp) + data) == 1 - - -def _convertbits(data, frombits, tobits, pad=True): - """General power-of-2 base conversion.""" - acc = 0 - bits = 0 - ret = [] - maxv = (1 << tobits) - 1 - for value in data: - if value < 0 or (value >> frombits): - return None - acc = (acc << frombits) | value - bits += frombits - while bits >= tobits: - bits -= tobits - ret.append((acc >> bits) & maxv) - if pad: - if bits: - ret.append((acc << (tobits - bits)) & maxv) - elif bits >= frombits or ((acc << (tobits - bits)) & maxv): - return None - return ret - - -def bech32_encode(hrp: str, data: bytes) -> str: - """Encode bytes as Bech32.""" - values = _convertbits(list(data), 8, 5) - checksum = _bech32_create_checksum(hrp, values) - return hrp + "1" + "".join(BECH32_CHARSET[d] for d in values + checksum) - - -def bech32_decode(bech: str): - """Decode Bech32 string to (hrp, data_bytes).""" - if any(ord(x) < 33 or ord(x) > 126 for x in bech): - return None, None - bech = bech.lower() - pos = bech.rfind("1") - if pos < 1 or pos + 7 > len(bech): - return None, None - hrp = bech[:pos] - data = [BECH32_CHARSET.find(x) for x in bech[pos + 1 :]] - if -1 in data: - return None, None - if not _bech32_verify_checksum(hrp, data): - return None, None - decoded = _convertbits(data[:-6], 5, 8, False) - if decoded is None: - return None, None - return hrp, bytes(decoded) - - # HRP for OnlyKey age recipients and identities RECIPIENT_HRP = "age1onlykey" IDENTITY_HRP = "age-plugin-onlykey-" # uppercase AGE-PLUGIN-ONLYKEY- in file diff --git a/onlykey/age_plugin/derived_xwing.py b/onlykey/age_plugin/derived_xwing.py index 3fafa23..e0fdc4c 100644 --- a/onlykey/age_plugin/derived_xwing.py +++ b/onlykey/age_plugin/derived_xwing.py @@ -15,7 +15,6 @@ DERIVE_SHAREDSEC -> [ ss_X(32) | mlkem_seed(32) ] """ -import base64 import hashlib from kyber_py.ml_kem import ML_KEM_768 @@ -81,24 +80,51 @@ def ct_x_of(ciphertext): # Distinguishes a derived identity from a slot identity so age-plugin-onlykey # can support BOTH models (like SSH/GPG). A derived identity carries the label; # the key is reproduced on demand from (OnlyKey web-derivation key, label, RPID). -_DERIVED_PREFIX = "AGE-PLUGIN-ONLYKEY-DERIVED-" +# +# Real bech32 (cli.py's bech32_encode/decode, extracted to bech32.py so both +# modules can share it without a circular import), matching the slot-based +# encode_identity()'s scheme - NOT the naive base32-with-no-checksum +# concatenation this used to be. That produced strings like +# "AGE-PLUGIN-ONLYKEY-DERIVED-" with no "1" bech32 separator and no +# checksum, which `age` itself rejects outright before ever handing off to +# the plugin ("invalid identity encoding: separator '1' at invalid +# position") - confirmed live running an actual `age -d -i ` against +# one, in onlykey-testing's TC-17. +# +# Second, deeper issue found the same way, fixed here too: the HRP can't be +# a distinct "age-plugin-onlykey-derived-" string either, even bech32-valid. +# `age` picks which plugin *binary* to run from the "AGE-PLUGIN--" +# prefix text itself (name -> `age-plugin-`), so a +# "AGE-PLUGIN-ONLYKEY-DERIVED-1..." identity made `age` look for a +# nonexistent `age-plugin-onlykey-derived` executable instead of invoking +# the real, installed `age-plugin-onlykey` - confirmed live +# ("couldn't start plugin: exec: ... not found in $PATH"). The HRP has to +# be *exactly* cli.py's IDENTITY_HRP (kept as a literal here, not imported, +# to avoid a cross-module dependency for one constant - the two must match, +# noted in both places). Slot vs. derived identities are instead +# distinguished by a marker byte in the decoded payload: cli.py's +# decode_identity() only ever produces `data[0]` in {a valid slot 1-132} or +# {IDENTITY_VERSION==1}, so 0xFF as data[0] is unambiguous and safe - the +# slot decoder raises ValueError on it either way (wrong length or +# unrecognized version), which callers already catch and skip. +from onlykey.age_plugin.bech32 import bech32_encode, bech32_decode + +_IDENTITY_HRP = "age-plugin-onlykey-" # MUST match cli.py's IDENTITY_HRP +_DERIVED_MARKER = 0xFF def encode_identity(label): """Encode a derived identity string for a label (used with `age -i`).""" if not isinstance(label, str) or not label: raise ValueError("derived identity needs a non-empty label") - b32 = base64.b32encode(label.encode("utf-8")).decode("ascii").rstrip("=") - return _DERIVED_PREFIX + b32.upper() + payload = bytes([_DERIVED_MARKER]) + label.encode("utf-8") + return bech32_encode(_IDENTITY_HRP, payload).upper() def decode_identity(s): """Decode a derived identity string -> {'derived': True, 'label': str}, or None if `s` is not a derived identity (caller falls back to slot decode).""" - s = str(s).strip().upper() - if not s.startswith(_DERIVED_PREFIX): + hrp, data = bech32_decode(str(s).strip().lower()) + if hrp != _IDENTITY_HRP or not data or data[0] != _DERIVED_MARKER: return None - b32 = s[len(_DERIVED_PREFIX):] - b32 += "=" * (-len(b32) % 8) - label = base64.b32decode(b32).decode("utf-8") - return {"derived": True, "label": label} + return {"derived": True, "label": data[1:].decode("utf-8")} diff --git a/tests/test_derived_xwing.py b/tests/test_derived_xwing.py index 897817c..2e6f76b 100644 --- a/tests/test_derived_xwing.py +++ b/tests/test_derived_xwing.py @@ -66,9 +66,20 @@ def test_deterministic_recipient_per_seed(): def test_derived_identity_roundtrip(): + from onlykey.age_plugin import cli + for label in ("age:personal", "alice@example.com", "work"): ident = dx.encode_identity(label) - assert ident.startswith("AGE-PLUGIN-ONLYKEY-DERIVED-") + # Must share the exact same "AGE-PLUGIN-ONLYKEY-1" prefix as a slot + # identity - `age` picks which plugin *binary* to invoke from that + # prefix text alone, so a distinct "...-DERIVED-1" HRP (bech32-valid + # or not) makes `age` look for a nonexistent + # `age-plugin-onlykey-derived` executable instead of the real, + # installed `age-plugin-onlykey` (confirmed live against a real + # `age -d` run - onlykey-testing's TC-17). + assert ident.startswith("AGE-PLUGIN-ONLYKEY-1") assert dx.decode_identity(ident) == {"derived": True, "label": label} - # a slot-style identity is not a derived identity - assert dx.decode_identity("AGE-PLUGIN-ONLYKEY-1QQQ") is None + # A real slot identity is not a derived identity, even sharing the same + # HRP - disambiguated by the marker byte in the decoded payload, not the + # prefix text (see _DERIVED_MARKER). + assert dx.decode_identity(cli.encode_identity(101)) is None From ed48ca1e1fadc0206bd948ef66fc2e024796c75c Mon Sep 17 00:00:00 2001 From: bmatusiak Date: Thu, 6 Aug 2026 17:51:27 -0400 Subject: [PATCH 4/7] Composite PQC: return real bytes, expose sign/decrypt, stop faking a load Three defects in the composite PGP-PQC path, all of which left the CLI unable to complete the flow it advertises. pqc.sign()/pqc.decrypt() could not return their own output. Both ended in read_string()[:N], and read_string() is ''.join(chr(item) for item in read_bytes(...) if item != 0) which drops every zero byte and returns str. Underneath it read_bytes() is a single self._hid.read(n) of ONE 64-byte report with no reassembly, so read_string(...)[:3309] for an ML-DSA-65 signature was impossible by construction rather than merely unreliable - 64 bytes is the most it could ever have returned. Both now use a new pqc.read_exact(), which reassembles consecutive reports the way the device actually sends them (send_transport_response() emits ceil(len/64) back-to-back reports), skips the status broadcasts that otherwise land where a signature belongs, and returns bytes. read_string() itself is untouched. 37 subcommands share it and the test kit pins its current behaviour; this is a binary path alongside it, modelled on the age plugin's OnlyKeyPQ._read_response(), which already solved the same problem for the 1216-byte X-Wing pubkey. Nothing exposed those functions. setpqc/loadpqc could load a composite key and no command could then use it, so "load a PQC key, decrypt something, sign something" had no command-line route past step one. Adds: onlykey-cli signpqc [RSA1-RSA4] [ecc|pqc] [digest hex | file] onlykey-cli decryptpqc [RSA1-RSA4] [hex | file] Both are device primitives, deliberately: a composite OpenPGP signature is the two halves concatenated and a composite session key needs the KMAC combine plus an RFC 3394 unwrap, and that framing belongs to the caller. Operands may be given as a path because an ML-KEM ciphertext is 2176 hex characters, past what several shells accept in one argument. setpqc reported success for a load the device refused. Outside config mode the device answers each of the three chunks with "Error not in config mode"; those replies were never read, so setpqc printed "Loaded composite PQC PGP key (160 bytes) into RSA1" and exited 0 having stored nothing. This is unusually bad here because there is no readback - okcrypto_getpubkey() has no KEYTYPE_PQC_PGP branch - so a caller had no second way to find out. load_composite_key() now waits for the device's acknowledgement and raises on a refusal, and setpqc/loadpqc exit non-zero. The wider exit-code problem across the other subcommands is left alone; it is its own change. Also drops pqc.py's "UNTESTED against hardware" docstring, which stopped being true on 2026-08-01. Verified against the emulator (onlykey-testing 02-cli/17-composite-cli-ops, 8/8): the refusal is reported, both signature halves verify against keys derived independently from the loaded blob - including the 3309-byte ML-DSA-65 one - and both decrypt halves match host-computed secrets. Co-Authored-By: Claude Opus 5 (1M context) --- onlykey/cli.py | 102 +++++++++++++++++++++++++++++-- onlykey/pqc.py | 162 +++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 246 insertions(+), 18 deletions(-) diff --git a/onlykey/cli.py b/onlykey/cli.py index f772dc6..720e00f 100644 --- a/onlykey/cli.py +++ b/onlykey/cli.py @@ -29,6 +29,25 @@ only_key = OnlyKey() + +def _pqc_input_bytes(arg): + """Read a PQC operand given as a hex string or as a path to a file. + + Same two-shapes rule `setpqc` already applies to its blob: a file is tried + as hex text first and taken as raw bytes if that fails, so both a + `.hex`-style dump and a raw binary file work. Accepting a path matters here + because an ML-KEM ciphertext is 1088 bytes - 2176 hex characters - which is + past what several shells will take as one argument. + """ + if os.path.isfile(arg): + raw = open(arg, 'rb').read() + try: + return bytes.fromhex(raw.decode().strip()) + except Exception: + return raw + return bytes.fromhex(arg.strip()) + + def cli(): logging.basicConfig(level=logging.DEBUG) @@ -428,7 +447,7 @@ def prompt_pin(): slot_id = slotmap.get(sys.argv[2]) if not slot_id: print('setpqc [RSA1-RSA4] [160-byte hex blob | file]') - return + sys.exit(1) arg = sys.argv[3] if os.path.isfile(arg): raw = open(arg, 'rb').read() @@ -438,12 +457,18 @@ def prompt_pin(): blob = raw else: blob = bytes.fromhex(arg.strip()) + # Raises if the device refused the load. Before it did, this + # printed the success line below for a load the device had + # answered with three "Error not in config mode" replies, and + # exited 0 - there is no readback for a composite key + # (okcrypto_getpubkey() has no KEYTYPE_PQC_PGP branch), so a + # caller had no way at all to tell the two outcomes apart. pqc.load_composite_key(only_key, slot_id, blob) print('Loaded composite PQC PGP key (%d bytes) into %s' % (len(blob), sys.argv[2])) except Exception: - print(sys.exc_info()[0]) + print(sys.exc_info()[1]) print('setpqc [RSA1-RSA4] [160-byte hex blob | file]') - return + sys.exit(1) elif sys.argv[1] == 'loadpqc': # Parse a composite PQC PGP private key FILE (via the OpenPGP.js bridge) # and load its 160-byte seed blob into an RSA slot. Needs Node.js. @@ -455,7 +480,7 @@ def prompt_pin(): slot_id = slotmap.get(sys.argv[3]) if len(sys.argv) > 3 else 1 if not slot_id: print('loadpqc [RSA1-RSA4] [passphrase]') - return + sys.exit(1) passphrase = sys.argv[4] if len(sys.argv) > 4 else None blob = pgp_bridge.composite_blob(path=keyfile, passphrase=passphrase) pqc.load_composite_key(only_key, slot_id, blob) @@ -464,7 +489,74 @@ def prompt_pin(): except Exception: print(sys.exc_info()[1]) print('loadpqc [RSA1-RSA4] [passphrase]') - return + sys.exit(1) + elif sys.argv[1] == 'signpqc': + # Sign a digest with ONE half of a composite PQC PGP key. + # signpqc [RSA1-RSA4] [ecc|pqc] [digest hex | file] + # ecc -> Ed25519, 64-byte signature + # pqc -> ML-DSA-65, 3309-byte signature + # + # This is the device PRIMITIVE, not a PGP message signer: a + # composite OpenPGP signature is the two halves concatenated, and + # assembling that packet is the caller's job (openpgp.js does it + # for the web app). Exposing the primitive is what lets a shell + # script, or an independent implementation's test harness, get a + # real signature out of the device at all. + try: + from . import pqc + slotmap = {'RSA1': 1, 'RSA2': 2, 'RSA3': 3, 'RSA4': 4} + halfmap = {'ecc': pqc.HALF_ECC, 'pqc': pqc.HALF_PQC} + if len(sys.argv) < 5: + print('signpqc [RSA1-RSA4] [ecc|pqc] [digest hex | file]') + sys.exit(1) + slot_id = slotmap.get(sys.argv[2]) + half = halfmap.get(sys.argv[3].lower()) + if not slot_id or half is None: + print('signpqc [RSA1-RSA4] [ecc|pqc] [digest hex | file]') + sys.exit(1) + digest = _pqc_input_bytes(sys.argv[4]) + print('Press the three buttons shown on your OnlyKey to confirm signing...', + file=sys.stderr) + sig = pqc.sign(only_key, slot_id, half, digest) + print(binascii.hexlify(sig).decode()) + except SystemExit: + raise + except Exception: + print(sys.exc_info()[1]) + print('signpqc [RSA1-RSA4] [ecc|pqc] [digest hex | file]') + sys.exit(1) + elif sys.argv[1] == 'decryptpqc': + # Decapsulate with ONE half of a composite PQC PGP key. + # decryptpqc [RSA1-RSA4] [hex | file] + # + # The device picks the half by INPUT SIZE - there is no selector: + # 32 bytes -> X25519 ephemeral point -> 32-byte shared secret + # 1088 bytes -> ML-KEM-768 ciphertext -> 32-byte shared secret + # + # Again a primitive. Recovering an OpenPGP session key from these + # needs the KMAC256("OpenPGPCompositeKDFv1") combine and an RFC 3394 + # AES key-unwrap on top, which the caller does. + try: + from . import pqc + slotmap = {'RSA1': 1, 'RSA2': 2, 'RSA3': 3, 'RSA4': 4} + if len(sys.argv) < 4: + print('decryptpqc [RSA1-RSA4] [32-byte X25519 point or 1088-byte ML-KEM ct: hex | file]') + sys.exit(1) + slot_id = slotmap.get(sys.argv[2]) + if not slot_id: + print('decryptpqc [RSA1-RSA4] [hex | file]') + sys.exit(1) + data = _pqc_input_bytes(sys.argv[3]) + print('Press the three buttons shown on your OnlyKey to confirm decryption...', + file=sys.stderr) + shared = pqc.decrypt(only_key, slot_id, data) + print(binascii.hexlify(shared).decode()) + except SystemExit: + raise + except Exception: + print(sys.exc_info()[1]) + print('decryptpqc [RSA1-RSA4] [hex | file]') + sys.exit(1) elif sys.argv[1] == 'wipekey': try: if sys.argv[2] == 'RSA1': diff --git a/onlykey/pqc.py b/onlykey/pqc.py index 48e8243..d35b83d 100644 --- a/onlykey/pqc.py +++ b/onlykey/pqc.py @@ -23,9 +23,19 @@ [64:96] X25519 secret (decrypt, ecc half) [96:160] ML-KEM-768 seed (decrypt, pqc half) FIPS 203 64-byte seed (d||z) -UNTESTED against hardware — by inspection. Validate the framing on a device. +Every operation here raises the device's three-button confirmation: okpqc_sign() +and okpqc_decrypt() both prime on their first call and do nothing at all until +CRYPTO_AUTH reaches 4. The caller sends once and then waits — the firmware +re-runs the operation itself from the third button press (OnlyKey.ino's +OKSIGN/OKDECRYPT branches), so the request is NOT resent. + +Status: the load path and both operations are verified end to end — on a Teensy +3.2 on 2026-08-01 (alpha kit TC-11) and against the emulator on 2026-08-06 +(onlykey-testing 02-cli/05-composite-load, 06-composite-ops, 16-cli-key-files). """ -from .client import Message +import time + +from .client import Message, MAX_INPUT_REPORT_SIZE # --- key type + layout (mirror okpqc.h) --------------------------------------- KEYTYPE_PQC_PGP = 7 @@ -92,29 +102,155 @@ def load_composite_key(ok, slot, blob): ok.send_message(msg=Message.OKSETPRIV, slot_id=slot, payload=bytearray([PQC_KEY_TYPE_BYTE]) + bytearray(chunk)) + _await_load_reply(ok) + + +# rsa_priv_flash()'s acknowledgement, printed once the accumulated chunks reach +# the declared key size. The composite branch declares 160. +_LOAD_OK = "Successfully set RSA Key" + + +def _await_load_reply(ok, timeout_ms=6000): + """Require the device to acknowledge the load, and raise if it refused. + + Without this the load is unverifiable from the host: okcrypto_getpubkey() + has no KEYTYPE_PQC_PGP branch, so a composite key cannot be read back, and + the only other evidence is asking the device to sign - which needs a button + press and so cannot be part of a load call. + + It matters most for the refusal that is easy to hit by accident. OKSETPRIV + is permitted only in config mode or on first use, and outside it the device + answers "Error not in config mode" to each of the three chunks. Those + replies were never read, so `setpqc` printed "Loaded composite PQC PGP key + (160 bytes) into RSA1" and exited 0 having loaded nothing at all. + """ + seen = [] + last_error = None + deadline = time.time() + timeout_ms / 1000.0 + while time.time() < deadline: + try: + data = ok.read_bytes(MAX_INPUT_REPORT_SIZE, to_bytes=True, timeout_ms=500) + except Exception as e: + # read_bytes() raises for a locked or uninitialised device and for + # several named device errors - each of those is a refused load - + # but it can also throw transiently on the read itself. Keep + # polling and report this only if nothing conclusive arrives, so a + # blip cannot fail a load that actually succeeded. + last_error = e + continue + if not data: + continue + text = bytes(data).split(b"\x00")[0].decode("ascii", "ignore").strip() + if not text: + continue + seen.append(text) + if text.startswith("Error"): + raise RuntimeError("OnlyKey refused the key load: %s" % text) + if _LOAD_OK in text: + return text + + if last_error is not None and not seen: + raise RuntimeError("OnlyKey: %s" % last_error) + raise RuntimeError( + "OnlyKey did not acknowledge the key load (expected %r, saw %r)" + % (_LOAD_OK, seen)) + + +# Status broadcasts the device emits on its own schedule. A single read taken +# right after OKSIGN/OKDECRYPT gets whichever report is first, which is how the +# ASCII of "UNLOCKED" ends up where a signature belongs. +_STATUS_PREFIXES = (b"UNLOCKED", b"INITIALIZED", b"UNINITIALIZED") -def decrypt(ok, slot, data): + +def read_exact(ok, want, timeout_ms=30000): + """Read exactly ``want`` bytes of BINARY response, reassembled across reports. + + ``read_string()`` cannot be used for any of this, for two independent + reasons, and neither is a matter of probability: + + * it is ``''.join(chr(b) for b in ... if b != 0)`` — it DROPS EVERY ZERO + BYTE and returns str, so any signature or shared secret containing a + 0x00 comes back short and shifted; and + * ``read_bytes()`` underneath it is a SINGLE ``self._hid.read(n)`` of one + 64-byte report with no reassembly, so ``read_string(...)[:3309]`` for an + ML-DSA-65 signature is impossible by construction rather than merely + unreliable — 64 bytes is the most it can ever return. + + The device sends a large response as consecutive 64-byte reports in one + tight loop (``send_transport_response()``, okcore.cpp, ``outputmode == 0``), + so a 3309-byte signature arrives as 52 reports and nothing interleaves with + them. This is the same collect-until-expected-size loop the age plugin's + ``OnlyKeyPQ._read_response()`` uses for the 1216-byte X-Wing pubkey, with + one addition it does not need: leading status broadcasts are skipped. + + Skipping is deliberately confined to the reports BEFORE the first data byte. + Once the response has started, a report may legitimately be all zeros or + read as text, and dropping one of those would silently corrupt the result. + + ``read_string()``'s own behaviour is untouched — 37 subcommands share it. + """ + out = bytearray() + started = False + last_error = None + deadline = time.time() + timeout_ms / 1000.0 + while time.time() < deadline and len(out) < want: + try: + data = ok.read_bytes(MAX_INPUT_REPORT_SIZE, to_bytes=True, timeout_ms=2000) + except Exception as e: + # A read timing out mid-stream does not mean the device is done; + # keep going to the real deadline. read_bytes() also raises for a + # locked/uninitialised device, which is worth reporting if nothing + # ever arrives - so remember it rather than discarding it. + last_error = e + continue + if not data: + continue + data = bytes(data) + if data.startswith(b"Error"): + raise RuntimeError("OnlyKey: %s" % data.split(b"\x00")[0].decode("ascii", "ignore").strip()) + if not started: + if data.startswith(_STATUS_PREFIXES) or not any(data): + continue + started = True + out.extend(data) + + if len(out) < want: + if not started and last_error is not None: + raise last_error + raise RuntimeError("OnlyKey: got %d of %d bytes" % (len(out), want)) + return bytes(out[:want]) + + +def decrypt(ok, slot, data, timeout_ms=None): """Composite decrypt. Send either the 32-byte X25519 ephemeral point (ECC half) or the 1088-byte ML-KEM ciphertext (PQC half); the device picks by size and - returns the 32-byte shared secret. openpgp.js does the KMAC combine + unwrap.""" + returns the 32-byte shared secret as BYTES. The caller does the KMAC combine + + AES key-unwrap (openpgp.js's kem.js does this for the web app). + + Raises the three-button confirmation on the device.""" if len(data) not in (X25519_PT_LEN, MLKEM_CT_LEN): raise ValueError("decrypt input must be %d (X25519 point) or %d (ML-KEM ct) bytes" % (X25519_PT_LEN, MLKEM_CT_LEN)) - ok.send_large_message2(msg=Message.OKDECRYPT, slot_id=slot, payload=data) - return ok.read_string(timeout_ms=_op_timeout(len(data)))[:SS_LEN] + ok.send_large_message2(msg=Message.OKDECRYPT, slot_id=slot, payload=list(bytes(data))) + return read_exact(ok, SS_LEN, timeout_ms or _op_timeout()) -def sign(ok, slot, component, digest): +def sign(ok, slot, component, digest, timeout_ms=None): """Composite sign. component = HALF_ECC (Ed25519) or HALF_PQC (ML-DSA-65). - Payload is [selector] + digest; returns the 64-byte or 3309-byte signature.""" + Payload is [selector] + digest; returns the 64-byte or 3309-byte signature + as BYTES. + + Raises the three-button confirmation on the device.""" if component not in (HALF_ECC, HALF_PQC): raise ValueError("component must be HALF_ECC(0) or HALF_PQC(1)") payload = bytes([component]) + bytes(digest) - ok.send_large_message2(msg=Message.OKSIGN, slot_id=slot, payload=payload) + ok.send_large_message2(msg=Message.OKSIGN, slot_id=slot, payload=list(payload)) want = ED25519_SIG_LEN if component == HALF_ECC else MLDSA_SIG_LEN - return ok.read_string(timeout_ms=_op_timeout(want))[:want] + return read_exact(ok, want, timeout_ms or _op_timeout()) -def _op_timeout(nbytes): - # ML-DSA keygen-from-seed + sign, or ML-KEM keygen + decaps, take a few 100 ms on the M4. - return 8000 if nbytes >= MLKEM_CT_LEN or nbytes >= MLDSA_SIG_LEN else 4000 +def _op_timeout(): + # Dominated by the HUMAN, not the device: every composite operation waits on + # a three-button confirmation. ML-DSA keygen-from-seed + sign and ML-KEM + # keygen + decaps take a few hundred ms on the M4 either side of that. + return 30000 From 4875e24ba1e479dec703894185579a8c9db6c591 Mon Sep 17 00:00:00 2001 From: bmatusiak Date: Thu, 6 Aug 2026 19:25:19 -0400 Subject: [PATCH 5/7] Composite PQC: conform to draft-ietf-openpgp-pqc-10 (vendored bridge copy) The Node/OpenPGP.js bridge behind `loadpqc` vendors the PQC fork, and it is required to stay BYTE-IDENTICAL to the copy in onlykey.github.io. This is that copy, resynced after the conformance change there (md5 verified). The fork implemented an earlier revision of the draft in three places, so nothing outside our own code could read what it produced: - codepoints: pqc_mldsa_ed25519 107 -> 30, pqc_mlkem_x25519 105 -> 35 (IANA assigns 30/35; 105/107 are private/experimental) - ECC key share: the raw X25519 shared secret, not SHA3-256(ss||ct||pub) - key combiner: SHA3-256(... || algId || domSep || len(domSep)), not KMAC256 over data that also carried mlkemCipherText and mlkemPublicKey All three alter derived session keys, so they land together - one flag day rather than three. Nothing on the device changes; the firmware never sees an OpenPGP algorithm ID and every hash and combine is host-side. For this repo specifically, the bridge is used by `loadpqc` to PARSE a composite private key file and extract the 160-byte blob. That path is unaffected by the KEM changes - the blob layout is untouched - and 02-cli/16-cli-key-files still passes. The codepoints do matter here, because a key file written by a conformant tool would otherwise not be recognised as composite at all. Verified end to end against rpgp 0.20 (Rust, independent implementation): all four directions pass, where before it could not parse our keys. Composite kit suites: 30/30. Co-Authored-By: Claude Opus 5 (1M context) --- onlykey/openpgp_bridge/openpgp.js | 66 +++++++++++++++++++------------ 1 file changed, 40 insertions(+), 26 deletions(-) diff --git a/onlykey/openpgp_bridge/openpgp.js b/onlykey/openpgp_bridge/openpgp.js index d8da599..a54b19f 100644 --- a/onlykey/openpgp_bridge/openpgp.js +++ b/onlykey/openpgp_bridge/openpgp.js @@ -1053,10 +1053,10 @@ var openpgp = (function (exports) { ed25519: 27, /** Ed448 (Sign only) */ ed448: 28, - /** Post-quantum ML-KEM-768 + X25519 (Encrypt only) */ - pqc_mlkem_x25519: 105, - /** Post-quantum ML-DSA-64 + Ed25519 (Sign only) */ - pqc_mldsa_ed25519: 107, + /** Post-quantum ML-DSA-65 + Ed25519 (Sign only) - IANA assigned, draft-ietf-openpgp-pqc */ + pqc_mldsa_ed25519: 30, + /** Post-quantum ML-KEM-768 + X25519 (Encrypt only) - IANA assigned, draft-ietf-openpgp-pqc */ + pqc_mlkem_x25519: 35, /** Persistent symmetric keys: encryption algorithm */ aead: 128, @@ -10593,11 +10593,12 @@ var openpgp = (function (exports) { switch (eccAlgo) { case enums.publicKey.pqc_mlkem_x25519: { const { ephemeralPublicKey: eccCipherText, sharedSecret: eccSharedSecret } = await generateEphemeralEncryptionMaterial(enums.publicKey.x25519, eccRecipientPublicKey); - const eccKeyShare = await hash$1.sha3_256(util.concatUint8Array([ - eccSharedSecret, - eccCipherText, - eccRecipientPublicKey - ])); + // draft-ietf-openpgp-pqc-10, "X25519 KEM": the ECDH key share IS the + // raw X25519 shared secret. An earlier revision of the draft hashed it + // with the ciphertext and the recipient key; that extra SHA3-256 is one + // of the two reasons this fork's KEK differed from every conforming + // implementation's. See decaps$1 for the matching change. + const eccKeyShare = eccSharedSecret; return { eccCipherText, eccKeyShare @@ -10611,13 +10612,13 @@ var openpgp = (function (exports) { async function decaps$1(eccAlgo, eccCipherText, eccSecretKey, eccPublicKey) { switch (eccAlgo) { case enums.publicKey.pqc_mlkem_x25519: { + // draft-ietf-openpgp-pqc-10, "X25519 KEM": the raw shared secret IS the + // key share - see encaps$1. recomputeSharedSecret() is where the + // hardware hook fires, so on an OnlyKey this is the device's own + // X25519 output used unchanged; the device contract is untouched by + // this correction, which is entirely host-side. const eccSharedSecret = await recomputeSharedSecret(enums.publicKey.x25519, eccCipherText, eccPublicKey, eccSecretKey); - const eccKeyShare = await hash$1.sha3_256(util.concatUint8Array([ - eccSharedSecret, - eccCipherText, - eccPublicKey - ])); - return eccKeyShare; + return eccSharedSecret; } default: throw new Error('Unsupported KEM algorithm'); @@ -10729,20 +10730,33 @@ var openpgp = (function (exports) { return sessionKey; } - async function multiKeyCombine(algo, ecdhKeyShare, ecdhCipherText, ecdhPublicKey, mlkemKeyShare, mlkemCipherText, mlkemPublicKey) { - const { kmac256 } = await Promise.resolve().then(function () { return sha3Addons; }); + // draft-ietf-openpgp-pqc-10, "Key Combiner": + // + // KEK = SHA3-256( mlkemKeyShare || ecdhKeyShare || + // ecdhCipherText || ecdhPublicKey || + // algId || domSep || len(domSep) ) + // + // An earlier revision of the draft used KMAC256 keyed on the two key shares, + // over an encData that ALSO carried mlkemCipherText and mlkemPublicKey, with + // the domain separator as the KMAC personalization. That is what this fork + // implemented, and it is why a message from any conforming implementation + // failed AES key unwrap here with "Key Data Integrity failed". + // + // `mlkemCipherText` and `mlkemPublicKey` are no longer inputs. The parameters + // are kept so the two call sites (encrypt$1 / decrypt$1) are untouched, which + // keeps this diff to the crypto it is about. + async function multiKeyCombine(algo, ecdhKeyShare, ecdhCipherText, ecdhPublicKey, mlkemKeyShare, mlkemCipherText, mlkemPublicKey) { // eslint-disable-line no-unused-vars + const domainSeparation = util.encodeUTF8('OpenPGPCompositeKDFv1'); - const key = util.concatUint8Array([mlkemKeyShare, ecdhKeyShare]); - const encData = util.concatUint8Array([ - mlkemCipherText, + const kek = await hash$1.sha3_256(util.concatUint8Array([ + mlkemKeyShare, + ecdhKeyShare, ecdhCipherText, - mlkemPublicKey, ecdhPublicKey, - new Uint8Array([algo]) - ]); - const domainSeparation = util.encodeUTF8('OpenPGPCompositeKDFv1'); - - const kek = kmac256(key, encData, { personalization: domainSeparation }); // output length: 256 bits + new Uint8Array([algo]), + domainSeparation, + new Uint8Array([domainSeparation.length]) + ])); return kek; } From 5dee6bd46a5f78affca724798bcd7267369342d1 Mon Sep 17 00:00:00 2001 From: bmatusiak Date: Thu, 6 Aug 2026 20:01:29 -0400 Subject: [PATCH 6/7] Tidy comments for review No behaviour change. Comment and docstring wording only, plus the resynced vendored bridge copy. - pqc.py: the module docstring cited a test-kit run and specific test files by path. Those live in a separate repo and cannot be resolved from here, so the claim is now just that the path has been exercised against hardware. - pqc.py: _await_load_reply() and read_exact() described what the code used to do rather than what it does. Rewritten to explain the current behaviour. - cli.py: same for the setpqc comment. - openpgp_bridge/openpgp.js: resynced with the copy in onlykey.github.io, which had the same comment tidy-up. The two must stay byte-identical. --- onlykey/cli.py | 12 ++++++------ onlykey/openpgp_bridge/openpgp.js | 32 ++++++++++++++----------------- onlykey/pqc.py | 14 +++++++------- 3 files changed, 27 insertions(+), 31 deletions(-) diff --git a/onlykey/cli.py b/onlykey/cli.py index 720e00f..3086e57 100644 --- a/onlykey/cli.py +++ b/onlykey/cli.py @@ -457,12 +457,12 @@ def prompt_pin(): blob = raw else: blob = bytes.fromhex(arg.strip()) - # Raises if the device refused the load. Before it did, this - # printed the success line below for a load the device had - # answered with three "Error not in config mode" replies, and - # exited 0 - there is no readback for a composite key - # (okcrypto_getpubkey() has no KEYTYPE_PQC_PGP branch), so a - # caller had no way at all to tell the two outcomes apart. + # Raises if the device refused the load, so the success line + # below is only ever printed for a load that happened. There is + # no readback for a composite key - okcrypto_getpubkey() has no + # KEYTYPE_PQC_PGP branch - so the device's own acknowledgement + # is the only thing that distinguishes a stored key from an + # empty slot. pqc.load_composite_key(only_key, slot_id, blob) print('Loaded composite PQC PGP key (%d bytes) into %s' % (len(blob), sys.argv[2])) except Exception: diff --git a/onlykey/openpgp_bridge/openpgp.js b/onlykey/openpgp_bridge/openpgp.js index a54b19f..95334c0 100644 --- a/onlykey/openpgp_bridge/openpgp.js +++ b/onlykey/openpgp_bridge/openpgp.js @@ -1053,9 +1053,9 @@ var openpgp = (function (exports) { ed25519: 27, /** Ed448 (Sign only) */ ed448: 28, - /** Post-quantum ML-DSA-65 + Ed25519 (Sign only) - IANA assigned, draft-ietf-openpgp-pqc */ + /** Post-quantum ML-DSA-65 + Ed25519 (Sign only) - IANA assigned, draft-ietf-openpgp-pqc-10 */ pqc_mldsa_ed25519: 30, - /** Post-quantum ML-KEM-768 + X25519 (Encrypt only) - IANA assigned, draft-ietf-openpgp-pqc */ + /** Post-quantum ML-KEM-768 + X25519 (Encrypt only) - IANA assigned, draft-ietf-openpgp-pqc-10 */ pqc_mlkem_x25519: 35, /** Persistent symmetric keys: encryption algorithm */ @@ -10594,10 +10594,9 @@ var openpgp = (function (exports) { case enums.publicKey.pqc_mlkem_x25519: { const { ephemeralPublicKey: eccCipherText, sharedSecret: eccSharedSecret } = await generateEphemeralEncryptionMaterial(enums.publicKey.x25519, eccRecipientPublicKey); // draft-ietf-openpgp-pqc-10, "X25519 KEM": the ECDH key share IS the - // raw X25519 shared secret. An earlier revision of the draft hashed it - // with the ciphertext and the recipient key; that extra SHA3-256 is one - // of the two reasons this fork's KEK differed from every conforming - // implementation's. See decaps$1 for the matching change. + // raw X25519 shared secret - it is NOT hashed with the ciphertext and + // recipient key first, as an earlier revision of the draft specified. + // decaps$1 must agree with this exactly. const eccKeyShare = eccSharedSecret; return { eccCipherText, @@ -10613,10 +10612,10 @@ var openpgp = (function (exports) { switch (eccAlgo) { case enums.publicKey.pqc_mlkem_x25519: { // draft-ietf-openpgp-pqc-10, "X25519 KEM": the raw shared secret IS the - // key share - see encaps$1. recomputeSharedSecret() is where the - // hardware hook fires, so on an OnlyKey this is the device's own - // X25519 output used unchanged; the device contract is untouched by - // this correction, which is entirely host-side. + // key share - see encaps$1, which must agree. recomputeSharedSecret() + // is where the hardware hook fires, so on a hardware-backed key this is + // the device's own X25519 output used unchanged; everything above the + // raw shared secret is host-side. const eccSharedSecret = await recomputeSharedSecret(enums.publicKey.x25519, eccCipherText, eccPublicKey, eccSecretKey); return eccSharedSecret; } @@ -10736,15 +10735,12 @@ var openpgp = (function (exports) { // ecdhCipherText || ecdhPublicKey || // algId || domSep || len(domSep) ) // - // An earlier revision of the draft used KMAC256 keyed on the two key shares, - // over an encData that ALSO carried mlkemCipherText and mlkemPublicKey, with - // the domain separator as the KMAC personalization. That is what this fork - // implemented, and it is why a message from any conforming implementation - // failed AES key unwrap here with "Key Data Integrity failed". + // NOT KMAC256, and NOT over the ML-KEM ciphertext or public key - an earlier + // revision of the draft specified both, and a KEK built that way fails AES + // key unwrap against any conforming implementation. // - // `mlkemCipherText` and `mlkemPublicKey` are no longer inputs. The parameters - // are kept so the two call sites (encrypt$1 / decrypt$1) are untouched, which - // keeps this diff to the crypto it is about. + // `mlkemCipherText` and `mlkemPublicKey` are therefore unused. They stay in + // the signature so the two call sites (encrypt$1 / decrypt$1) need no change. async function multiKeyCombine(algo, ecdhKeyShare, ecdhCipherText, ecdhPublicKey, mlkemKeyShare, mlkemCipherText, mlkemPublicKey) { // eslint-disable-line no-unused-vars const domainSeparation = util.encodeUTF8('OpenPGPCompositeKDFv1'); diff --git a/onlykey/pqc.py b/onlykey/pqc.py index d35b83d..cdc3801 100644 --- a/onlykey/pqc.py +++ b/onlykey/pqc.py @@ -29,9 +29,8 @@ re-runs the operation itself from the third button press (OnlyKey.ino's OKSIGN/OKDECRYPT branches), so the request is NOT resent. -Status: the load path and both operations are verified end to end — on a Teensy -3.2 on 2026-08-01 (alpha kit TC-11) and against the emulator on 2026-08-06 -(onlykey-testing 02-cli/05-composite-load, 06-composite-ops, 16-cli-key-files). +The load path and both operations have been exercised end to end against +hardware. """ import time @@ -120,9 +119,9 @@ def _await_load_reply(ok, timeout_ms=6000): It matters most for the refusal that is easy to hit by accident. OKSETPRIV is permitted only in config mode or on first use, and outside it the device - answers "Error not in config mode" to each of the three chunks. Those - replies were never read, so `setpqc` printed "Loaded composite PQC PGP key - (160 bytes) into RSA1" and exited 0 having loaded nothing at all. + answers "Error not in config mode" to each of the three chunks. A caller + that does not read those replies cannot distinguish a stored key from an + empty slot, and will report success for a load that did nothing. """ seen = [] last_error = None @@ -187,7 +186,8 @@ def read_exact(ok, want, timeout_ms=30000): Once the response has started, a report may legitimately be all zeros or read as text, and dropping one of those would silently corrupt the result. - ``read_string()``'s own behaviour is untouched — 37 subcommands share it. + ``read_string()`` is deliberately left alone rather than fixed in place: + every other subcommand in the CLI depends on its current behaviour. """ out = bytearray() started = False From 78dbbca12b7705d19efd17a87b48251cea8ba80f Mon Sep 17 00:00:00 2001 From: bmatusiak Date: Thu, 6 Aug 2026 20:49:58 -0400 Subject: [PATCH 7/7] Correct false claims in comments Comments only; no code changed. Found by auditing every factual claim in the diff against the firmware sources, RFC/IANA, and draft-ietf-openpgp-pqc-10. pqc.py, decrypt(): said the caller does "the KMAC combine". It does not - the combiner is SHA3-256 (draft section 4.2.1). This was left behind when the KDF was corrected and is the same class of error as the codepoints: a confident, wrong crypto claim. Same wording fixed in cli.py decryptpqc, which additionally named KMAC256 with the domain separator as though that were the construction. pqc.py module docstring: claimed the load path AND both operations had been exercised end to end against hardware. Only the load path has. read_exact, and therefore sign() and decrypt(), are new and have run against an emulated device only. The docstring now separates the two. pqc.py read_exact(): "64 bytes is the most it can ever return" is true only where MAX_INPUT_REPORT_SIZE is 64; it is 65 on Windows. Restated in terms of one report. pqc.py _op_timeout(): dropped an unsourced "a few hundred ms on the M4" timing figure. The point it supports - that the budget is sized for a human, not the device - stands without it. derived_xwing.py and tests/test_derived_xwing.py: both cited a test case in a separate test-kit repo as evidence. The observations are real; the citation cannot be resolved from this repo, so it is dropped and the claim kept. openpgp_bridge/openpgp.js: resynced with the copy in onlykey.github.io, which had the same comment corrections. The two must stay byte-identical. Verified after: py_compile clean, composite suites 30/30 unchanged. --- onlykey/age_plugin/derived_xwing.py | 3 +-- onlykey/cli.py | 7 +++++-- onlykey/openpgp_bridge/openpgp.js | 19 +++++++++++-------- onlykey/pqc.py | 27 +++++++++++++++++---------- tests/test_derived_xwing.py | 3 +-- 5 files changed, 35 insertions(+), 24 deletions(-) diff --git a/onlykey/age_plugin/derived_xwing.py b/onlykey/age_plugin/derived_xwing.py index e0fdc4c..efc878c 100644 --- a/onlykey/age_plugin/derived_xwing.py +++ b/onlykey/age_plugin/derived_xwing.py @@ -88,8 +88,7 @@ def ct_x_of(ciphertext): # "AGE-PLUGIN-ONLYKEY-DERIVED-" with no "1" bech32 separator and no # checksum, which `age` itself rejects outright before ever handing off to # the plugin ("invalid identity encoding: separator '1' at invalid -# position") - confirmed live running an actual `age -d -i ` against -# one, in onlykey-testing's TC-17. +# position") - observed running an actual `age -d -i ` against one. # # Second, deeper issue found the same way, fixed here too: the HRP can't be # a distinct "age-plugin-onlykey-derived-" string either, even bech32-valid. diff --git a/onlykey/cli.py b/onlykey/cli.py index 3086e57..a89b43e 100644 --- a/onlykey/cli.py +++ b/onlykey/cli.py @@ -534,8 +534,11 @@ def prompt_pin(): # 1088 bytes -> ML-KEM-768 ciphertext -> 32-byte shared secret # # Again a primitive. Recovering an OpenPGP session key from these - # needs the KMAC256("OpenPGPCompositeKDFv1") combine and an RFC 3394 - # AES key-unwrap on top, which the caller does. + # needs the SHA3-256 key combine of draft-ietf-openpgp-pqc-10 + # section 4.2.1 - over both key shares, the ECDH ciphertext and + # public key, the algorithm ID, and "OpenPGPCompositeKDFv1" with its + # length - and an RFC 3394 AES-256 key-unwrap on top, which the + # caller does. try: from . import pqc slotmap = {'RSA1': 1, 'RSA2': 2, 'RSA3': 3, 'RSA4': 4} diff --git a/onlykey/openpgp_bridge/openpgp.js b/onlykey/openpgp_bridge/openpgp.js index 95334c0..3f18d87 100644 --- a/onlykey/openpgp_bridge/openpgp.js +++ b/onlykey/openpgp_bridge/openpgp.js @@ -10593,10 +10593,10 @@ var openpgp = (function (exports) { switch (eccAlgo) { case enums.publicKey.pqc_mlkem_x25519: { const { ephemeralPublicKey: eccCipherText, sharedSecret: eccSharedSecret } = await generateEphemeralEncryptionMaterial(enums.publicKey.x25519, eccRecipientPublicKey); - // draft-ietf-openpgp-pqc-10, "X25519 KEM": the ECDH key share IS the - // raw X25519 shared secret - it is NOT hashed with the ciphertext and - // recipient key first, as an earlier revision of the draft specified. - // decaps$1 must agree with this exactly. + // draft-ietf-openpgp-pqc-10 section 4.1.1.1, x25519Kem.Encaps(): "Set + // the output ecdhKeyShare to X", the raw shared coordinate. It is NOT + // hashed with the ciphertext and recipient key first, which is what + // this fork used to do. decaps$1 must agree with this exactly. const eccKeyShare = eccSharedSecret; return { eccCipherText, @@ -10729,15 +10729,18 @@ var openpgp = (function (exports) { return sessionKey; } - // draft-ietf-openpgp-pqc-10, "Key Combiner": + // draft-ietf-openpgp-pqc-10 section 4.2.1, "Key combiner", verbatim: // // KEK = SHA3-256( mlkemKeyShare || ecdhKeyShare || // ecdhCipherText || ecdhPublicKey || // algId || domSep || len(domSep) ) // - // NOT KMAC256, and NOT over the ML-KEM ciphertext or public key - an earlier - // revision of the draft specified both, and a KEK built that way fails AES - // key unwrap against any conforming implementation. + // domSep is the UTF-8 encoding of "OpenPGPCompositeKDFv1" and len(domSep) is + // a single octet, decimal 21. + // + // NOT KMAC256, and NOT over the ML-KEM ciphertext or public key. This fork + // previously used KMAC256 over data that included both, and a KEK built that + // way fails AES key unwrap against any conforming implementation. // // `mlkemCipherText` and `mlkemPublicKey` are therefore unused. They stay in // the signature so the two call sites (encrypt$1 / decrypt$1) need no change. diff --git a/onlykey/pqc.py b/onlykey/pqc.py index cdc3801..7a3bf06 100644 --- a/onlykey/pqc.py +++ b/onlykey/pqc.py @@ -29,8 +29,11 @@ re-runs the operation itself from the third button press (OnlyKey.ino's OKSIGN/OKDECRYPT branches), so the request is NOT resent. -The load path and both operations have been exercised end to end against -hardware. +Exercise status, because the two halves differ. The LOAD path - the chunked +OKSETPRIV send in load_composite_key() - has run against a physical OnlyKey via +`onlykey-cli setpqc`. The binary READ path below (read_exact, and therefore +sign() and decrypt()) has been exercised against an emulated device only; it has +not yet run against hardware. """ import time @@ -170,10 +173,11 @@ def read_exact(ok, want, timeout_ms=30000): * it is ``''.join(chr(b) for b in ... if b != 0)`` — it DROPS EVERY ZERO BYTE and returns str, so any signature or shared secret containing a 0x00 comes back short and shifted; and - * ``read_bytes()`` underneath it is a SINGLE ``self._hid.read(n)`` of one - 64-byte report with no reassembly, so ``read_string(...)[:3309]`` for an - ML-DSA-65 signature is impossible by construction rather than merely - unreliable — 64 bytes is the most it can ever return. + * ``read_bytes()`` underneath it is a SINGLE ``self._hid.read(n)`` with no + reassembly, and ``read_string()`` calls it with MAX_INPUT_REPORT_SIZE — + one report — so ``read_string(...)[:3309]`` for an ML-DSA-65 signature + is impossible by construction rather than merely unreliable: one + report's worth is the most it can ever return. The device sends a large response as consecutive 64-byte reports in one tight loop (``send_transport_response()``, okcore.cpp, ``outputmode == 0``), @@ -224,8 +228,9 @@ def read_exact(ok, want, timeout_ms=30000): def decrypt(ok, slot, data, timeout_ms=None): """Composite decrypt. Send either the 32-byte X25519 ephemeral point (ECC half) or the 1088-byte ML-KEM ciphertext (PQC half); the device picks by size and - returns the 32-byte shared secret as BYTES. The caller does the KMAC combine - + AES key-unwrap (openpgp.js's kem.js does this for the web app). + returns the 32-byte shared secret as BYTES. The caller does the SHA3-256 key + combine + RFC 3394 AES key-unwrap (openpgp.js's kem.js does this for the web + app); see draft-ietf-openpgp-pqc-10 section 4.2.1 for the combiner. Raises the three-button confirmation on the device.""" if len(data) not in (X25519_PT_LEN, MLKEM_CT_LEN): @@ -251,6 +256,8 @@ def sign(ok, slot, component, digest, timeout_ms=None): def _op_timeout(): # Dominated by the HUMAN, not the device: every composite operation waits on - # a three-button confirmation. ML-DSA keygen-from-seed + sign and ML-KEM - # keygen + decaps take a few hundred ms on the M4 either side of that. + # a three-button confirmation, so this budget is sized for a person reading + # three digits off the display and pressing them. The device-side work + # either side of that - ML-DSA keygen-from-seed then sign, or ML-KEM keygen + # then decapsulate - is small by comparison. return 30000 diff --git a/tests/test_derived_xwing.py b/tests/test_derived_xwing.py index 2e6f76b..fc3ebd4 100644 --- a/tests/test_derived_xwing.py +++ b/tests/test_derived_xwing.py @@ -75,8 +75,7 @@ def test_derived_identity_roundtrip(): # prefix text alone, so a distinct "...-DERIVED-1" HRP (bech32-valid # or not) makes `age` look for a nonexistent # `age-plugin-onlykey-derived` executable instead of the real, - # installed `age-plugin-onlykey` (confirmed live against a real - # `age -d` run - onlykey-testing's TC-17). + # installed `age-plugin-onlykey` (observed against a real `age -d` run). assert ident.startswith("AGE-PLUGIN-ONLYKEY-1") assert dx.decode_identity(ident) == {"derived": True, "label": label} # A real slot identity is not a derived identity, even sharing the same