From 4acf84c24bd6346039d27cf9635a4c5a3b714a36 Mon Sep 17 00:00:00 2001 From: Mark Xue Date: Wed, 23 Sep 2026 16:34:05 -0700 Subject: [PATCH] Address non-security review follow-ups establish rejects duplicate classical extension types and forgets the folded apq_psk; attestation verification decodes at the codepoints' wire width; exportSecret throws on out-of-range lengths; ProposalStore.insert no longer overwrites a migrated entry. Also fixes two stale docs and zeroizes the exporter's first-stage secret. Co-Authored-By: Claude Opus 5.5 --- .changeset/review-hardening.md | 5 + Package.swift | 12 +- Sources/MLSCombiner/ApqInfoUpdate.swift | 43 +++--- Sources/MLSCombiner/CombinerError.swift | 8 ++ Sources/MLSCombiner/CombinerGroup.swift | 43 ++++-- Sources/MLSCombiner/ExportedPsk.swift | 12 +- Sources/MLSKeySchedule/KeySchedule.swift | 2 +- .../MLSProfileRFC9420/CommitProcessing.swift | 38 ++++- Sources/MLSProfileRFC9420/Errors.swift | 19 ++- Sources/MLSProfileRFC9420/ExportSecret.swift | 11 +- Tests/MLSCombinerTests/AttestationTests.swift | 68 ++++++++- .../MLSCombinerTests/CombinerGroupTests.swift | 131 ++++++++++++++++++ .../ExportSecretTests.swift | 35 +++++ .../MigratedOwnUpdateTests.swift | 89 +++++++++++- 14 files changed, 465 insertions(+), 51 deletions(-) create mode 100644 .changeset/review-hardening.md diff --git a/.changeset/review-hardening.md b/.changeset/review-hardening.md new file mode 100644 index 0000000..f033014 --- /dev/null +++ b/.changeset/review-hardening.md @@ -0,0 +1,5 @@ +--- +"@germ-network/swift-mls": patch +--- + +`CombinerGroup.establish` rejects a classical GroupContext extension list that repeats a type (RFC 9420 §13.4), and `establish`/`join` forget the founding `apq_psk` once it is folded. `verifyFullCommitAttestation`/`verifyFullCommit` decode a wrapped attestation at the `Codepoints`' own wire width, so they can be called outside that scope. `Group.exportSecret` throws `exportLengthOutOfRange` instead of trapping on a length outside `1...255·Nh`. `ProposalStore.insert` no longer overwrites a migration-restored entry: a matching verified proposal leaves it in place, and a mismatched one throws `migratedUpdateRefAlreadyStored`. diff --git a/Package.swift b/Package.swift index c6dec23..195b69c 100644 --- a/Package.swift +++ b/Package.swift @@ -32,11 +32,13 @@ let package = Package( .package(url: "https://github.com/apple/swift-crypto.git", from: "5.0.0"), // Zeroizing storage for held secrets. Its floor is now iOS 18 / // macOS 15 (0.5.0), which this package's own floor matches; its one - // dependency is swift-crypto, already in this tree. Pinned to the - // minor: it is pre-1.0 and breaks at minor bumps by its own changelog. - // 0.5.0 is its swift-crypto 5 release (tracking this package's move); - // 0.4.0 added the SecretArchive plaintext-ingress SPI the mls-rs - // migration needs (germ-network/swift-secret-bytes#9). + // dependency is swift-crypto, already in this tree. `from:`, not + // `.upToNextMinor`: it is pre-1.0 and may break at a minor, but an + // `.upToNextMinor` fence would cap the whole dependency graph below + // its next minor; `from:` keeps the 0.5.0 floor and admits later 0.x + // releases. 0.5.0 is its swift-crypto 5 release (tracking this + // package's move); 0.4.0 added the SecretArchive plaintext-ingress + // SPI the mls-rs migration needs (germ-network/swift-secret-bytes#9). .package( url: "https://github.com/germ-network/swift-secret-bytes.git", from: "0.5.0"), diff --git a/Sources/MLSCombiner/ApqInfoUpdate.swift b/Sources/MLSCombiner/ApqInfoUpdate.swift index 5c0cbdb..735931a 100644 --- a/Sources/MLSCombiner/ApqInfoUpdate.swift +++ b/Sources/MLSCombiner/ApqInfoUpdate.swift @@ -92,7 +92,11 @@ extension MLS.Combiner.ApqInfoUpdate { /// draft caps it at one) or a malformed / wrong-component one. Accepts the /// attestation from either the typed `.appDataUpdate` effect or, when a receiver /// has opted `AppDataUpdate`'s proposal type into `customProposalTypes`, the - /// wrapped `.customProposal(type: .appDataUpdate, body:)` effect it sees instead. + /// wrapped `.customProposal(type: .appDataUpdate, body:)` effect it sees instead — + /// that wrapped arm decodes its `component_id` at the ambient + /// `ComponentID.componentIDWireWidth`, so call this inside a + /// `$componentIDWireWidth.withValue(codepoints.componentIDWireWidth)` scope, or + /// use [`verifyFullCommitAttestation`], which scopes itself. public static func extract( from effects: MLS.RFC9420.CommitEffects, componentID: MLS.Extensions.ComponentID @@ -145,6 +149,10 @@ extension MLS.Combiner { /// that want both checks in one place should use [`verifyFullCommit`] instead /// of calling this directly; this entry point remains for granular use (and /// testing) where only the attestation is relevant. + /// + /// Runs under `codepoints`' component-id wire width, so a wrapped + /// `.customProposal`-shaped attestation (see `extract`) decodes correctly + /// whether or not the caller is already inside that scope. public static func verifyFullCommitAttestation( classicalEffects: MLS.RFC9420.CommitEffects, pqEffects: MLS.RFC9420.CommitEffects, @@ -152,22 +160,25 @@ extension MLS.Combiner { pqEpoch: UInt64, codepoints: MLS.Combiner.Codepoints = .deployed ) throws -> MLS.Combiner.ApqInfoUpdate { - guard - let classical = try ApqInfoUpdate.extract( - from: classicalEffects, componentID: codepoints.apqComponentID), - let pq = try ApqInfoUpdate.extract( - from: pqEffects, componentID: codepoints.apqComponentID) - else { - throw MLS.Combiner.Error.attestationMismatch - } - // Both halves attest the same pair, and it is the actual post-commit epochs. - guard classical == pq, - classical.tEpoch == classicalEpoch, - classical.pqEpoch == pqEpoch - else { - throw MLS.Combiner.Error.attestationMismatch + try codepoints.withWireWidth { + guard + let classical = try ApqInfoUpdate.extract( + from: classicalEffects, + componentID: codepoints.apqComponentID), + let pq = try ApqInfoUpdate.extract( + from: pqEffects, componentID: codepoints.apqComponentID) + else { + throw MLS.Combiner.Error.attestationMismatch + } + // Both halves attest the same pair, and it is the actual post-commit epochs. + guard classical == pq, + classical.tEpoch == classicalEpoch, + classical.pqEpoch == pqEpoch + else { + throw MLS.Combiner.Error.attestationMismatch + } + return classical } - return classical } /// Verify that a FULL commit's classical half actually folded the current PQ diff --git a/Sources/MLSCombiner/CombinerError.swift b/Sources/MLSCombiner/CombinerError.swift index 2e672c9..a90b254 100644 --- a/Sources/MLSCombiner/CombinerError.swift +++ b/Sources/MLSCombiner/CombinerError.swift @@ -1,4 +1,5 @@ import MLSCodec +import MLSProfileRFC9420 extension MLS.Combiner { /// Failure categories for the generic combiner layer. A consumer (twomlspq-swift) @@ -39,5 +40,12 @@ extension MLS.Combiner { /// caller input. Distinct from the input-shaped cases above so a caller never /// mistakes an internal defect for a rejectable peer message. case internalInconsistency + + /// RFC 9420 §13.4: "Any field containing a list of extensions MUST NOT + /// have more than one extension of any given type." A classical + /// GroupContext extension list — the combiner's own `APQInfo` plus + /// `establish`'s caller-supplied `classicalExtraExtensions` — repeated a + /// type. + case duplicateExtensionType(MLS.RFC9420.ExtensionType) } } diff --git a/Sources/MLSCombiner/CombinerGroup.swift b/Sources/MLSCombiner/CombinerGroup.swift index 91c2135..05839d3 100644 --- a/Sources/MLSCombiner/CombinerGroup.swift +++ b/Sources/MLSCombiner/CombinerGroup.swift @@ -11,8 +11,12 @@ extension MLS.Combiner { /// group's id must appear inside its own creation-time `APQInfo`, so it exists /// before `create`), the `epochSecret` (RFC 9420 §11's fresh KDF.Nh value) and the /// `CommitRandomness` for the founding add-commit, and the peer's `KeyPackage` to - /// add. The caller owns all randomness — nothing is generated inside the combiner, - /// so establishment is byte-for-byte reproducible. + /// add. The caller supplies this material; the rest of the randomness `establish` + /// consumes flows through the two providers — the classical `psk_nonce` and each + /// founding commit's private-framing reuse guard via `randomBytes`, and the + /// Welcome/UpdatePath encapsulations via `hpkeSeal`, both `CipherSuiteProvider` + /// requirements — so providers that make those deterministic reproduce + /// establishment byte-for-byte. public struct HalfCreation: Sendable { public var groupID: Data public var leafNode: MLS.RFC9420.LeafNode @@ -68,10 +72,12 @@ extension MLS.Combiner { public struct CombinerGroup: Sendable { public internal(set) var classical: MLS.RFC9420.Group public internal(set) var pq: MLS.RFC9420.Group - /// The values `application` PSK references in the two halves' commits/joins - /// resolve from. Held here, not on either half — the profile resolves PSKs by - /// a caller closure, and a construction-time `apq_psk` must outlive any client - /// rotation. + /// The store `establish`/`join` resolve the founding `apq_psk` from while + /// building the pair. Empty on every `CombinerGroup` this module returns: + /// `establish` and `join` each forget the `apq_psk` once its commit or + /// Welcome has folded it, and `restore` never populates one either — there + /// is nothing left here for a caller to read, and the setter is `internal` + /// besides. public internal(set) var pskStore: PSKStore public let codepoints: Codepoints // The memberwise initializer is intentionally internal (synthesized): a @@ -101,7 +107,9 @@ extension MLS.Combiner.CombinerGroup { /// extensions (RFC 9420 §11.1) appended after the combiner's own `APQInfo` on the /// classical half only — the classical half is the message half, so that is where /// application-defined creation-time extensions belong. They are not threaded onto - /// the PQ half. Defaults to empty, so existing callers are unaffected. + /// the PQ half. Defaults to empty, so existing callers are unaffected. A type + /// repeated among them, or equal to the `APQInfo` extension type, throws + /// `duplicateExtensionType` (RFC 9420 §13.4). public static func establish( classical: MLS.Combiner.HalfCreation, pq: MLS.Combiner.HalfCreation, @@ -122,6 +130,23 @@ extension MLS.Combiner.CombinerGroup { pqEpoch: 1) let infoExtension = try info.asExtension( type: codepoints.apqInfoExtensionType) + // Compare by rawValue, not by ExtensionType itself: ExtensionType's + // synthesized Equatable/Hashable treats `.known` and `.unknown` at the + // same code point as distinct, so a Set would miss a + // hand-built `.unknown` duplicating a `.known` type's wire value. + let classicalExtensions = [infoExtension] + classicalExtraExtensions + var seenExtensionTypes: Set = [] + for classicalExtension in classicalExtensions { + guard + seenExtensionTypes.insert(classicalExtension.type.rawValue) + .inserted + else { + throw MLS.Combiner.Error.duplicateExtensionType( + MLS.RFC9420.ExtensionType( + rawValue: classicalExtension.type.rawValue)) + } + } + let attestation = MLS.Combiner.ApqInfoUpdate(tEpoch: 1, pqEpoch: 1) let attestationProposal = MLS.RFC9420.ProposalOrRef.proposal( try attestation.proposal(componentID: codepoints.apqComponentID)) @@ -143,7 +168,7 @@ extension MLS.Combiner.CombinerGroup { let nonce = classicalProvider.randomBytes(classicalProvider.hashSize) let (classicalGroup, classicalWelcome) = try createAndAdd( classicalProvider, creation: classical, - extensions: [infoExtension] + classicalExtraExtensions, + extensions: classicalExtensions, extraProposals: [ .proposal(apqPsk.proposal(nonce: nonce)), attestationProposal, @@ -153,6 +178,7 @@ extension MLS.Combiner.CombinerGroup { guard let pqWelcome, let classicalWelcome else { throw MLS.Combiner.Error.missingWelcome } + store.forget(storageID: apqPsk.storageID) let group = MLS.Combiner.CombinerGroup( classical: classicalGroup, pq: pqGroupForExport, pskStore: store, codepoints: codepoints) @@ -247,6 +273,7 @@ extension MLS.Combiner.CombinerGroup { let classicalRoster = classicalPending.roster let classicalGroup = classicalPending.apply().group + store.forget(storageID: apqPsk.storageID) let group = MLS.Combiner.CombinerGroup( classical: classicalGroup, pq: pqGroup, pskStore: store, codepoints: codepoints) diff --git a/Sources/MLSCombiner/ExportedPsk.swift b/Sources/MLSCombiner/ExportedPsk.swift index 7335615..8735e61 100644 --- a/Sources/MLSCombiner/ExportedPsk.swift +++ b/Sources/MLSCombiner/ExportedPsk.swift @@ -104,8 +104,9 @@ extension MLS.Combiner { /// `application` PSKs from, keyed by the local `storage_id`. It backs the /// resolver the combiner passes to `committing`/`joining`/`validating`, and is /// held on the `CombinerGroup` rather than on either half — the profile has no - /// per-group PSK store (resolution is a closure), and a construction-time value - /// (the `apq_psk`) must outlive any later client rotation. + /// per-group PSK store (resolution is a closure). `establish`/`join` forget the + /// founding `apq_psk` once its commit or Welcome has folded it (see `forget`), + /// so a `CombinerGroup` this module returns always holds an empty one. public struct PSKStore: Sendable { private var entries: [Data: SecretBytes] = [:] @@ -118,9 +119,10 @@ extension MLS.Combiner { /// Remove a PSK once the commit that referenced it has been applied (or it has /// been retired), keeping the store bounded by what the caller still vouches for. - /// A downstream seam: not exercised within this module, since the store is - /// ephemeral — a live `apq_psk` is already folded into the epoch secrets once - /// its referencing commit is applied, with nothing left here to forget. + /// `establish` and `join` each call this on the founding `apq_psk` right after + /// its commit or Welcome has folded it into the epoch secrets — nothing left + /// there to resolve it against — which is why every `CombinerGroup` this + /// module returns holds an empty store. public mutating func forget(storageID: Data) { entries[storageID] = nil } diff --git a/Sources/MLSKeySchedule/KeySchedule.swift b/Sources/MLSKeySchedule/KeySchedule.swift index e241cf2..2d4331a 100644 --- a/Sources/MLSKeySchedule/KeySchedule.swift +++ b/Sources/MLSKeySchedule/KeySchedule.swift @@ -215,7 +215,7 @@ extension MLS { context: Data, length: Int ) throws -> SecretBytes { - let secret = try deriveSecret( + let secret = try deriveSecretSecret( provider, secret: exporterSecret, label: label) let contextHash = try provider.hash(context) return try expandWithLabelSecret( diff --git a/Sources/MLSProfileRFC9420/CommitProcessing.swift b/Sources/MLSProfileRFC9420/CommitProcessing.swift index 82ca7a2..7f308b9 100644 --- a/Sources/MLSProfileRFC9420/CommitProcessing.swift +++ b/Sources/MLSProfileRFC9420/CommitProcessing.swift @@ -139,6 +139,10 @@ extension MLS.RFC9420 { /// sender together instead of a `VerifiedProposal`. public struct ProposalStore: Sendable { private var entries: [MLS.HashReference: StoredProposal] = [:] + /// Refs written by `insertMigratedOwnUpdate`, so `insert` can tell a + /// migrated entry apart from an ordinary verified one — see `insert`'s doc + /// comment. Private to this type; `StoredProposal` itself carries no marker. + private var migratedRefs: Set = [] // swift-format-ignore: UseSynthesizedInitializer -- the synthesized // memberwise init is `internal` (its access level follows `entries`, @@ -152,6 +156,20 @@ extension MLS.RFC9420 { /// `VerifiedProposal` (from `Group.verifying(proposal:)` or `unprotect`) /// carries `.proposal` content by construction; the guard keeps /// `notAProposal` as a defensive invariant rather than a live path. + /// + /// If `ref` names an entry `insertMigratedOwnUpdate` wrote, this does NOT + /// overwrite it outright (that would silently drop its + /// `migratedLeafSecret` — see that method's doc comment). Instead: when + /// the verified proposal's content — `proposal`, `sender`, `epoch`, + /// `groupID` — matches the stored entry exactly, the migration paired + /// `ref` correctly, so the existing entry is kept untouched (its + /// `migratedLeafSecret` survives) and this call is idempotent, like + /// verified-over-verified. If any field differs, the migrated `(ref, + /// leafNode)` pairing was wrong — the very thing `insertMigratedOwnUpdate` + /// cannot check — so this throws `migratedUpdateRefAlreadyStored` rather + /// than silently repairing it, leaving the existing entry as is. A `ref` + /// naming an ordinary verified entry is overwritten with identical + /// content, so re-inserting the same verified proposal stays harmless. @discardableResult public mutating func insert( _ verified: VerifiedProposal, _ provider: any MLS.CipherSuiteProvider @@ -161,6 +179,16 @@ extension MLS.RFC9420 { throw MLS.RFC9420.GroupError.notAProposal } let ref = try proposalRef(provider, content) + if migratedRefs.contains(ref), let existing = entries[ref] { + guard existing.proposal == proposal, + existing.sender == content.content.sender, + existing.epoch == content.content.epoch, + existing.groupID == content.content.groupID + else { + throw MLS.RFC9420.GroupError.migratedUpdateRefAlreadyStored + } + return ref + } entries[ref] = StoredProposal( proposal: proposal, sender: content.content.sender, epoch: content.content.epoch, groupID: content.content.groupID) @@ -185,9 +213,12 @@ extension MLS.RFC9420 { /// built it from ITS OWN, correct resolution, so this side's tree /// hash and confirmation tag stop matching once the wrong proposal is /// applied, and processing fails, just not necessarily with that same - /// named error. See `Group.insertMigratedOwnUpdate`'s own doc comment - /// for the different, worse risk when a SIBLING local membership is - /// the one committing a wrongly-paired entry. + /// named error. If a verified copy of the real proposal is later + /// `insert`ed into the same store, `insert` refuses it (this method + /// records `ref` as migrated for that comparison). See + /// `Group.insertMigratedOwnUpdate`'s own doc comment for the different, + /// worse risk when a SIBLING local membership is the one committing a + /// wrongly-paired entry. mutating func insertMigratedOwnUpdate( _ ref: MLS.HashReference, _ proposal: StoredProposal ) throws { @@ -195,6 +226,7 @@ extension MLS.RFC9420 { throw MLS.RFC9420.GroupError.migratedUpdateRefAlreadyStored } entries[ref] = proposal + migratedRefs.insert(ref) } public subscript(_ ref: MLS.HashReference) -> StoredProposal? { diff --git a/Sources/MLSProfileRFC9420/Errors.swift b/Sources/MLSProfileRFC9420/Errors.swift index ecf451f..130c06a 100644 --- a/Sources/MLSProfileRFC9420/Errors.swift +++ b/Sources/MLSProfileRFC9420/Errors.swift @@ -356,11 +356,20 @@ extension MLS.RFC9420 { /// `leafNode.encryptionKey`. Either way, the leaf is never installed /// on a mismatch. case migratedUpdateSecretMismatch - /// `@_spi(Migration) Group.insertMigratedOwnUpdate`: the given `ref` - /// already names an entry in the store, verified or migrated. A migrated - /// write may only add a new entry, never replace one — replacing a - /// verified entry would let unauthenticated data displace authenticated - /// data under the same key. + /// Two distinct directions share this case. `@_spi(Migration) + /// Group.insertMigratedOwnUpdate`: the given `ref` already names an entry + /// in the store, verified or migrated. A migrated write may only add a + /// new entry, never replace one — replacing a verified entry would let + /// unauthenticated data displace authenticated data under the same key. + /// `ProposalStore.insert`, the other direction: a verified proposal's + /// `ref` names an entry a migration wrote whose `proposal`, `sender`, + /// `epoch`, or `groupID` differs from the verified one — proof the + /// migrated `(ref, leafNode)` pairing was wrong, refused rather than + /// silently repaired (an identical match is kept, not rejected). case migratedUpdateRefAlreadyStored + /// `Group.exportSecret`'s `length` must be in `1...255 * Nh` — RFC 5869 + /// §2.3 caps HKDF-Expand at `255*HashLen` — checked up front because the + /// HKDF backends trap rather than throw past it. + case exportLengthOutOfRange(length: Int, maximum: Int) } } diff --git a/Sources/MLSProfileRFC9420/ExportSecret.swift b/Sources/MLSProfileRFC9420/ExportSecret.swift index b25dd2b..587c02e 100644 --- a/Sources/MLSProfileRFC9420/ExportSecret.swift +++ b/Sources/MLSProfileRFC9420/ExportSecret.swift @@ -31,13 +31,22 @@ extension MLS.RFC9420.Group { /// On a combined group (e.g. `MLS.Combiner.CombinerGroup`) this is callable /// per-half via `.classical` / `.pq`, which are public `Group`s; each half /// derives from its own `exporter_secret`. + /// + /// - Throws: `GroupError.exportLengthOutOfRange` if `length` is outside + /// `1...255 * Nh` — checked here rather than left to the HKDF backend, + /// which traps instead of throwing past that bound. public func exportSecret( _ provider: any MLS.CipherSuiteProvider, label: String, context: Data, length: Int ) throws -> SecretBytes { - try MLS.KeySchedule.exportSecret( + let maximum = 255 * provider.hashSize + guard length > 0, length <= maximum else { + throw MLS.RFC9420.GroupError.exportLengthOutOfRange( + length: length, maximum: maximum) + } + return try MLS.KeySchedule.exportSecret( provider, exporterSecret: epoch.exporterSecret, label: label, context: context, length: length) } diff --git a/Tests/MLSCombinerTests/AttestationTests.swift b/Tests/MLSCombinerTests/AttestationTests.swift index 2a04dbb..533daf1 100644 --- a/Tests/MLSCombinerTests/AttestationTests.swift +++ b/Tests/MLSCombinerTests/AttestationTests.swift @@ -265,13 +265,17 @@ import Testing } } - /// `verifyFullCommitAttestation` succeeds when both halves surface the wrapped - /// `.customProposal(type: .appDataUpdate)` shape, built by hand like the other - /// `CommitEffects`-constructing tests above (no need to drive a real commit under - /// `customProposalTypes` for this unit-level check). - @Test func verifyFullCommitAttestationAcceptsCustomProposalShape() throws { + /// `verifyFullCommitAttestation` decodes the wrapped `.customProposal(type: + /// .appDataUpdate)` shape under `codepoints`' own component-id wire width, not + /// whatever happens to be ambient at the call site: the body is built inside a + /// `.uint32` scope (what a deployed wire commit actually carries), and + /// `verifyFullCommitAttestation` is called OUTSIDE that scope, with the default + /// (`.deployed`, `.uint32`) codepoints. + @Test func verifyFullCommitAttestationDecodesOutsideItsOwnWireWidthScope() throws { let update = MLS.Combiner.ApqInfoUpdate(tEpoch: 2, pqEpoch: 2) - let body = try update.appDataUpdate(componentID: Self.componentID).mlsEncoded() + let body = try MLS.Extensions.ComponentID.$componentIDWireWidth.withValue(.uint32) { + try update.appDataUpdate(componentID: Self.componentID).mlsEncoded() + } let effect = MLS.RFC9420.CommitEffect.customProposal( type: MLS.RFC9420.ProposalType(.appDataUpdate), body: body) let classicalEffects = MLS.RFC9420.CommitEffects([effect]) @@ -283,6 +287,58 @@ import Testing #expect(verified == update) } + /// The symmetric case: `codepoints` naming `.uint16` decodes correctly even + /// from INSIDE an ambient `.uint32` scope — pinning that the function uses + /// `codepoints`' own width, not simply "always `.uint32`". + @Test func verifyFullCommitAttestationUsesCodepointsWidthNotAlwaysUint32() throws { + let codepoints = MLS.Combiner.Codepoints( + apqInfoExtensionType: MLS.Combiner.Codepoints.deployed.apqInfoExtensionType, + apqComponentID: Self.componentID, componentIDWireWidth: .uint16) + let update = MLS.Combiner.ApqInfoUpdate(tEpoch: 2, pqEpoch: 2) + let body = try update.appDataUpdate(componentID: Self.componentID).mlsEncoded() + let effect = MLS.RFC9420.CommitEffect.customProposal( + type: MLS.RFC9420.ProposalType(.appDataUpdate), body: body) + let classicalEffects = MLS.RFC9420.CommitEffects([effect]) + let pqEffects = MLS.RFC9420.CommitEffects([effect]) + + let verified = try MLS.Extensions.ComponentID.$componentIDWireWidth.withValue( + .uint32 + ) { + try MLS.Combiner.verifyFullCommitAttestation( + classicalEffects: classicalEffects, pqEffects: pqEffects, + classicalEpoch: 2, pqEpoch: 2, codepoints: codepoints) + } + #expect(verified == update) + } + + /// The same outside-scope property, through `verifyFullCommit`: the PSK-bound + /// check is width-independent (`ExportedPsk.storageID` is `uint16`-pinned), so + /// this pins that `verifyFullCommit`'s attestation half inherits the fix via + /// delegation, without driving a real two-half commit. + @Test func verifyFullCommitDecodesAttestationOutsideItsOwnWireWidthScope() throws { + let psk = try MLS.Combiner.ExportedPsk.fromParts( + componentID: Self.componentID, pskID: Data([0x01]), + psk: SecretBytes(randomByteCount: 32)) + var store = MLS.Combiner.PSKStore() + store.register(psk) + let (resolver, record) = store.recordingResolver() + _ = try resolver(psk.preSharedKeyID(nonce: Data(repeating: 0, count: 32))) + + let update = MLS.Combiner.ApqInfoUpdate(tEpoch: 2, pqEpoch: 2) + let body = try MLS.Extensions.ComponentID.$componentIDWireWidth.withValue(.uint32) { + try update.appDataUpdate(componentID: Self.componentID).mlsEncoded() + } + let effect = MLS.RFC9420.CommitEffect.customProposal( + type: MLS.RFC9420.ProposalType(.appDataUpdate), body: body) + let classicalEffects = MLS.RFC9420.CommitEffects([effect]) + let pqEffects = MLS.RFC9420.CommitEffects([effect]) + + let verified = try MLS.Combiner.verifyFullCommit( + classicalEffects: classicalEffects, pqEffects: pqEffects, + classicalEpoch: 2, pqEpoch: 2, record: record, expected: psk) + #expect(verified == update) + } + // MARK: commit/process helpers (two-step handshake) private func commitAttesting( diff --git a/Tests/MLSCombinerTests/CombinerGroupTests.swift b/Tests/MLSCombinerTests/CombinerGroupTests.swift index aee0431..d1e05ec 100644 --- a/Tests/MLSCombinerTests/CombinerGroupTests.swift +++ b/Tests/MLSCombinerTests/CombinerGroupTests.swift @@ -98,6 +98,97 @@ import Testing == MLS.Combiner.Codepoints.deployed.apqInfoExtensionType) } + /// RFC 9420 §13.4: an extra classical extension carrying the SAME type as the + /// combiner's own `APQInfo` extension throws `duplicateExtensionType`. + @Test func establishRejectsExtraExtensionDuplicatingApqInfo() throws { + let alice = try Support.member("alice") + let bob = try Support.member("bob") + let apqInfoType = MLS.Combiner.Codepoints.deployed.apqInfoExtensionType + let extra = MLS.RFC9420.Extension(type: apqInfoType, data: Data([0xAA])) + + #expect( + throws: MLS.Combiner.Error.duplicateExtensionType( + MLS.RFC9420.ExtensionType(rawValue: 0xF0A1)) + ) { + _ = try MLS.Combiner.CombinerGroup.establish( + classical: try Support.halfCreation(founder: alice, peer: bob), + pq: try Support.halfCreation(founder: alice, peer: bob), + mode: 0, classicalProvider: Support.provider, + pqProvider: Support.provider, classicalExtraExtensions: [extra]) + } + } + + /// Two extras of the same type among `classicalExtraExtensions` themselves — + /// no `APQInfo` involvement — throw the same way. + @Test func establishRejectsTwoExtrasOfTheSameType() throws { + let alice = try Support.member("alice") + let bob = try Support.member("bob") + let placeholderType = MLS.RFC9420.ExtensionType(rawValue: 0xFF00) + let first = MLS.RFC9420.Extension(type: placeholderType, data: Data([0x01])) + let second = MLS.RFC9420.Extension(type: placeholderType, data: Data([0x02])) + + #expect( + throws: MLS.Combiner.Error.duplicateExtensionType( + MLS.RFC9420.ExtensionType(rawValue: 0xFF00)) + ) { + _ = try MLS.Combiner.CombinerGroup.establish( + classical: try Support.halfCreation(founder: alice, peer: bob), + pq: try Support.halfCreation(founder: alice, peer: bob), + mode: 0, classicalProvider: Support.provider, + pqProvider: Support.provider, + classicalExtraExtensions: [first, second]) + } + } + + /// A hand-built `.unknown` extra at `APQInfo`'s own raw code point still + /// collides. `0xF0A1` is private-use, so `ExtensionType(rawValue:)` already + /// normalizes to `.unknown` there too — this pins the comparison being over + /// `rawValue`, not case identity. + @Test func establishRejectsUnknownExtraDuplicatingApqInfoRawValue() throws { + let alice = try Support.member("alice") + let bob = try Support.member("bob") + let extra = MLS.RFC9420.Extension( + type: MLS.RFC9420.ExtensionType.unknown(0xF0A1), data: Data([0xAA])) + + #expect( + throws: MLS.Combiner.Error.duplicateExtensionType( + MLS.RFC9420.ExtensionType(rawValue: 0xF0A1)) + ) { + _ = try MLS.Combiner.CombinerGroup.establish( + classical: try Support.halfCreation(founder: alice, peer: bob), + pq: try Support.halfCreation(founder: alice, peer: bob), + mode: 0, classicalProvider: Support.provider, + pqProvider: Support.provider, classicalExtraExtensions: [extra]) + } + } + + /// The genuine `.known`/`.unknown` hazard: a `.known(.applicationID)` extra + /// and an `.unknown` extra at the SAME raw code point are distinct + /// `ExtensionType` values (`ExtensibleEnum`'s synthesized `Equatable` treats + /// the two cases as different), so a `Set` would miss this + /// duplicate — comparing by `rawValue` catches it. + @Test func establishRejectsKnownAndUnknownExtrasSharingARawValue() throws { + let alice = try Support.member("alice") + let bob = try Support.member("bob") + let known = MLS.RFC9420.Extension( + type: MLS.RFC9420.ExtensionType(.applicationID), data: Data([0x01])) + let unknown = MLS.RFC9420.Extension( + type: MLS.RFC9420.ExtensionType.unknown(1), data: Data([0x02])) + #expect(known.type != unknown.type) + + #expect( + throws: MLS.Combiner.Error.duplicateExtensionType( + MLS.RFC9420.ExtensionType(rawValue: 1)) + ) { + _ = try MLS.Combiner.CombinerGroup.establish( + classical: try Support.halfCreation(founder: alice, peer: bob), + pq: try Support.halfCreation(founder: alice, peer: bob), + mode: 0, classicalProvider: Support.provider, + pqProvider: Support.provider, + classicalExtraExtensions: [known, unknown]) + } + } + /// `APQInfo` rides both halves' Welcomes: the joiner reads a `0xF0A1` extension out /// of each half's GroupContext, the identity fields agree across halves, each names /// the joined group and epoch — i.e. `verifyPair()` holds for the joiner. @@ -267,4 +358,44 @@ import Testing #expect(restored.pq.context == founder.pq.context) try restored.verifyPair() } + + /// `establish`/`join` each forget the founding `apq_psk` once its commit or + /// Welcome has folded it, so the store on the returned `CombinerGroup` is + /// empty on BOTH sides — nothing left there to resolve against, not merely + /// nothing referenced. + @Test func pskStoreIsEmptyAfterEstablishAndJoin() throws { + let alice = try Support.member("alice") + let bob = try Support.member("bob") + + let (founder, welcome) = try MLS.Combiner.CombinerGroup.establish( + classical: try Support.halfCreation(founder: alice, peer: bob), + pq: try Support.halfCreation(founder: alice, peer: bob), + mode: 0, classicalProvider: Support.provider, pqProvider: Support.provider) + let peer = try MLS.Combiner.CombinerGroup.join( + welcome: welcome, classicalCredentials: bob.joinCredentials, + pqCredentials: bob.joinCredentials, + classicalProvider: Support.provider, pqProvider: Support.provider) + + // Independently re-derive the SAME epoch-1 apq_psk by joining the PQ + // Welcome a second time, into a fresh Group instance — exporting from it + // doesn't touch either party's actual PQ half. + let independentPqPending = try MLS.RFC9420.Group.joining( + Support.provider, welcome: welcome.pqWelcome, + credentials: bob.joinCredentials, psk: { _ in nil }) + var independentPq = independentPqPending.apply().group + let apqPsk = try MLS.Combiner.ExportedPsk.export( + from: &independentPq, Support.provider, + componentID: MLS.Combiner.Codepoints.deployed.apqComponentID) + let id = apqPsk.preSharedKeyID( + nonce: Support.provider.randomBytes(Support.provider.hashSize)) + + #expect(try founder.pskStore.resolver()(id) == nil) + #expect(try peer.pskStore.resolver()(id) == nil) + + // Control: the SAME id resolves non-nil from a store that actually holds + // it, so the nil results above aren't a wrong-id artifact. + var controlStore = MLS.Combiner.PSKStore() + controlStore.register(apqPsk) + #expect(try controlStore.resolver()(id) != nil) + } } diff --git a/Tests/MLSProfileRFC9420Tests/ExportSecretTests.swift b/Tests/MLSProfileRFC9420Tests/ExportSecretTests.swift index b14cd3e..376690a 100644 --- a/Tests/MLSProfileRFC9420Tests/ExportSecretTests.swift +++ b/Tests/MLSProfileRFC9420Tests/ExportSecretTests.swift @@ -142,4 +142,39 @@ struct ExportSecretTests { #expect(atEpoch1 != atEpoch0) } + + /// RFC 5869 §2.3 caps HKDF-Expand's output length at `255*HashLen`; the + /// bound is inclusive, so `length == maximum` still succeeds and exports + /// exactly that many bytes. + @Test("length == maximum succeeds and exports exactly maximum bytes") + func maximumLengthSucceeds() throws { + let provider = Self.provider + let group = try SelfInteropTests.createGroup(try SelfInteropTests.member("solo")) + let maximum = 255 * provider.hashSize + + let exported = try group.exportSecret( + provider, label: "rendezvous", context: Data(), length: maximum) + #expect(exported.byteCount == maximum) + } + + /// `0`, a negative length, one past the maximum, and `Int.max` are all + /// rejected by the same guard before ever reaching the HKDF backend, which + /// traps rather than throws past `255*HashLen`. + @Test("length outside 1...maximum throws exportLengthOutOfRange") + func outOfRangeLengthThrows() throws { + let provider = Self.provider + let group = try SelfInteropTests.createGroup(try SelfInteropTests.member("solo")) + let maximum = 255 * provider.hashSize + + for length in [0, -1, maximum + 1, Int.max] { + #expect( + throws: MLS.RFC9420.GroupError.exportLengthOutOfRange( + length: length, maximum: maximum) + ) { + _ = try group.exportSecret( + provider, label: "rendezvous", context: Data(), + length: length) + } + } + } } diff --git a/Tests/MLSProfileRFC9420Tests/MigratedOwnUpdateTests.swift b/Tests/MLSProfileRFC9420Tests/MigratedOwnUpdateTests.swift index aa90a42..21a9c58 100644 --- a/Tests/MLSProfileRFC9420Tests/MigratedOwnUpdateTests.swift +++ b/Tests/MLSProfileRFC9420Tests/MigratedOwnUpdateTests.swift @@ -692,6 +692,10 @@ struct MigratedOwnUpdateTests { /// migration archive keeps apart from the group snapshot. var leafSecret: MLS.HpkeSecretKey var ref: MLS.HashReference + /// Alice's own `PublicMessage` framing of the proposal `ref` names — kept + /// so a test can re-verify it through `groupA.verifying(proposal:)`, the + /// same way a peer's own copy of the proposal would arrive. + var framed: MLS.RFC9420.PublicMessage var bobStore: MLS.RFC9420.ProposalStore var baselineStore: MLS.RFC9420.ProposalStore var baselineRef: MLS.HashReference @@ -756,7 +760,7 @@ struct MigratedOwnUpdateTests { return SecretFixture( alice: alice, bob: bob, groupA: groupA, groupB: groupB, aliceLeaf: aliceLeaf, bobLeaf: bobLeaf, updateLeaf: updateLeaf, - leafSecret: leafSecret, ref: ref, bobStore: bobStore, + leafSecret: leafSecret, ref: ref, framed: framed, bobStore: bobStore, baselineStore: baselineStore, baselineRef: baselineRef, baselineProposal: baselineProposal) } @@ -1502,4 +1506,87 @@ struct MigratedOwnUpdateTests { #expect(installed == f.updateLeaf) SelfInteropTests.assertConverged(appliedGroupA, groupB) } + + // MARK: - `ProposalStore.insert` versus a migrated entry (keep-on-match, + // throw-on-mismatch) + + /// MATCH: a verified proposal arriving under a ref the migration already + /// populated, with content matching exactly, is idempotent — the existing + /// (migrated) entry is kept untouched, so its `migratedLeafSecret` survives. + @Test( + "insert keeps a migrated entry untouched when the verified proposal's content matches" + ) + func insertKeepsMigratedEntryOnMatch() throws { + let f = try Self.secretFixture() + var store = MLS.RFC9420.ProposalStore() + try f.groupA.insertMigratedOwnUpdate( + as: f.aliceLeaf, Self.provider, into: &store, ref: f.ref, + leafNode: f.updateLeaf, epoch: f.groupA.context.epoch, + groupID: f.groupA.context.groupID, leafSecret: f.leafSecret) + + let returnedRef = try store.insert( + f.groupA.verifying(Self.provider, proposal: f.framed), Self.provider) + + #expect(returnedRef == f.ref) + #expect(store.count == 1) + #expect(store[f.ref]?.migratedLeafSecret != nil) + } + + /// MISMATCH: a verified proposal arriving under a ref the migration + /// mispaired — same ref, different content — is refused rather than + /// silently accepted or used to overwrite: the migrated `(ref, leafNode)` + /// pairing was wrong, which is exactly the mistake `insertMigratedOwnUpdate` + /// itself cannot check (see its doc comment). + @Test( + "insert throws when a verified proposal's content mismatches a migrated entry under the same ref" + ) + func insertThrowsOnMismatchedMigratedEntry() throws { + let f = try Self.fixture() + var groupA = f.groupA + + // Alice proposes a SECOND Update (U2/R2) — retained alongside U1, same as + // `noOverwriteOfMigratedEntry`. + let (message2, ref2) = try groupA.proposeUpdate( + Self.provider, signingKey: f.alice.signingKey, framing: .publicMessage) + guard case .publicMessage(let framed2) = message2 else { throw Failure.shape } + guard case .proposal(.update(let updateLeaf2)) = framed2.content.content else { + throw Failure.shape + } + #expect(updateLeaf2 != f.updateLeaf) + + // The migration mispairs R2 with U1 (`f.updateLeaf`) instead of U2 — + // individually genuine, but the wrong leaf for this ref. + var store = MLS.RFC9420.ProposalStore() + try groupA.insertMigratedOwnUpdate( + as: f.aliceLeaf, Self.provider, into: &store, ref: ref2, + leafNode: f.updateLeaf, epoch: groupA.context.epoch, + groupID: groupA.context.groupID) + + // A genuinely verified copy of the REAL proposal (U2) later arrives under + // R2 — its content disagrees with what the migration stored there. + #expect(throws: MLS.RFC9420.GroupError.migratedUpdateRefAlreadyStored) { + try store.insert( + groupA.verifying(Self.provider, proposal: framed2), Self.provider) + } + let stillStored = try #require(store[ref2]) + #expect(stillStored.proposal == .update(f.updateLeaf)) + #expect(store.count == 1) + } + + /// CONTROL: with no migrated entry involved, inserting the same verified + /// proposal twice is an idempotent overwrite, so the MATCH/MISMATCH results + /// above aren't just "insert always throws/keeps". + @Test("insert accepts the same verified proposal twice when nothing is migrated") + func insertAcceptsRepeatVerifiedInsertWithNoMigratedEntry() throws { + let f = try Self.secretFixture() + var store = MLS.RFC9420.ProposalStore() + let firstRef = try store.insert( + f.groupA.verifying(Self.provider, proposal: f.framed), Self.provider) + let secondRef = try store.insert( + f.groupA.verifying(Self.provider, proposal: f.framed), Self.provider) + + #expect(firstRef == f.ref) + #expect(secondRef == f.ref) + #expect(store.count == 1) + } }