Address non-security review follow-ups

.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`.

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"),

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,29 +149,36 @@ 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,
 		classicalEpoch: UInt64,
 		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

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)
 	}
 }

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<ExtensionType> would miss a
+			// hand-built `.unknown` duplicating a `.known` type's wire value.
+			let classicalExtensions = [infoExtension] + classicalExtraExtensions
+			var seenExtensionTypes: Set<UInt16> = []
+			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)

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
 		}

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(

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<MLS.HashReference> = []
 
 		// 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,16 +213,20 @@ 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 {
 			guard entries[ref] == nil else {
 				throw MLS.RFC9420.GroupError.migratedUpdateRefAlreadyStored
 			}
 			entries[ref] = proposal
+			migratedRefs.insert(ref)
 		}
 
 		public subscript(_ ref: MLS.HashReference) -> StoredProposal? {

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)
 	}
 }

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)
 	}