Skip to content

agent: drop versions before 07/2024 (support of PQ encryption and securing reply queue) - #1841

Open
epoberezkin wants to merge 5 commits into
masterfrom
ep/drop-agent-versions
Open

agent: drop versions before 07/2024 (support of PQ encryption and securing reply queue)#1841
epoberezkin wants to merge 5 commits into
masterfrom
ep/drop-agent-versions

Conversation

@epoberezkin

Copy link
Copy Markdown
Member

No description provided.

@epoberezkin
epoberezkin requested a review from spaced4ndy as a code owner August 1, 2026 10:46

@simplex-chat-agent simplex-chat-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raises the minimum supported versions (SMP agent 2→6, E2E ratchet 2→3) and current SMP agent version 7→8, then deletes every branch that was conditional on the dropped-below versions: PQ-encryption gating (versionPQSupport_, pqRatchetE2EEncryptVersion guards), the old short/long header encodings (encodeLarge/largeP, encodeMsgHeader/msgHeaderP, encodeEncRatchetMessage), version-dependent padding sizes, and receipt/ratchet-sync version checks. The wire formats collapse to their PQ-capable forms and several ad-hoc encoders become plain Encoding instances.

I traced the version floor through the touched code and it is internally consistent: with agent ≥ 6 and E2E ≥ 3 every dropped predicate (>= pqdrSMPAgentVersion, >= pqRatchetE2EEncryptVersion, >= deliveryRcptsSMPAgentVersion, >= ratchetSyncSMPAgentVersion) was already always true, so removing them preserves behavior for supported peers. adjustE2EVRange in Ratchet mirrors the existing adjustAgentVRange (clamp min up to the supported floor, scoped to contact URIs), so it follows an established convention rather than inventing one. Padding functions now depend only on PQSupport, which both sides negotiate, so sender/receiver sizes still agree. Call sites, exports, and tests were all updated to match; no dangling references to the removed constants remain. This is a deliberate, breaking drop of pre-mid-2024 peers, as the title states.

Findings are minor:

  • In smpInvitation (Agent.hs) the new pqSupport <- lift $ PQSupportOn <$ compatibleInvitationUri connReq computes compatibleInvitationUri and discards it via <$, always producing PQSupportOn; the adjacent comment "assume there is no PQ encryption support" now contradicts the code. Suggested replacing with the constant and fixing the comment.
  • Two ConnectionRequestTests assertions keep "-- adjusted to v2" comments after the value moved to contactAddressV6; they now clamp to v6.

Nits (not worth a suggestion): the new agent-version changelog line in Protocol.hs (-- 8 - ...) has trailing whitespace; _pqRatchetE2EEncryptVersion / _sndAuthKeySMPAgentVersion keep an underscore prefix despite being referenced (via the minSupported… aliases), which reads as "unused" — intentional as documentation of the floor's origin, but slightly misleading.

No correctness, security, or concurrency issues found.

Comment thread src/Simplex/Messaging/Agent.hs
Comment thread tests/AgentTests/ConnectionRequestTests.hs
@epoberezkin
epoberezkin requested a review from shumvgolove August 5, 2026 21:30

@simplex-chat-agent simplex-chat-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch raises the agent minimum version to 6 (secured reply queues, 6/2024) and the e2e-encryption minimum to 3 (PQDR, 3/2024), then deletes every version gate that those minimums make unconditional. The mechanical collapse is done consistently and reads well:

  • versionPQSupport_ / addrKeysE2EVersion removed; PQ support is now the requested PQSupport directly. Correct: compatibleInvitationUri/compatibleContactUri only return Just when the peer overlaps the supported ranges, and adjustE2EVRange clamps incoming e2e ranges up to 3, so agentV >= 5 and e2eV >= 3 always held anyway.
  • Ratchet encoding gates dropped: E2ERatchetParams, MsgHeader, EncMessageHeader, EncRatchetMessage now have plain Encoding instances that always include the KEM field and always use Large (2-byte) framing; largeP/encodeLarge/encodeMsgHeader/msgHeaderP are gone. paddedHeaderLen, fullHeaderLen, e2eEncConnInfoLength, e2eEncAgentMsgLength, replyKEM_, pqEnableSupport, joinContactInitialKeys lose their version parameter. All call sites are updated. This is a deliberate wire-format break for pre-3/2024 peers, which matches the PR's stated intent.
  • A_MESSAGE gains a messageErr :: String field and callers thread short context strings through parse failures. Consistent with the existing A_PROHIBITED {prohibitedErr} pattern; A_MESSAGE is only produced locally and reported via JSON, so no cross-agent wire concern.

Blocking:

  • tests/AgentTests.hs uses fdescribe "Functional API", which focuses that group and silently skips every other test in agentTests. This must be reverted to describe before merge, otherwise CI runs a fraction of the suite.

Two earlier suggestions were marked resolved but head still disagrees:

  • Agent.hs smpInvitation still does PQSupportOn <$ compatibleInvitationUri connReq (dead computation) with a comment that contradicts the code.
  • ConnectionRequestTests.hs still carries "-- adjusted to v2" comments where the value is now v6.

Minor: trailing whitespace on the new version-history line in Protocol.hs. Non-blocking observations, no change requested: rpcAddressSMPAgentVersion (v8) is defined but currentSMPAgentVersion is hardcoded to VersionSMPA 8 rather than referencing it (matches the pre-existing convention, so fine); the underscore-prefixed but still-used _sndAuthKeySMPAgentVersion / _pqRatchetE2EEncryptVersion read oddly but compile clean; the numbered parse-context tags ("1".."6") in processSMPTransmissions are cryptic but serviceable.

Comment thread tests/AgentTests/ConnectionRequestTests.hs
Comment thread src/Simplex/Messaging/Agent.hs
Comment thread tests/AgentTests.hs
Comment thread src/Simplex/Messaging/Agent/Protocol.hs

@simplex-chat-agent simplex-chat-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Raises the minimum supported agent version to 6 (secure reply queues, sndAuthKey) and E2E version to 3 (PQDR), dropping everything before 07/2024. Because every supported peer now sits at or above the versions that gated PQ encoding, the branch removes all the v >= pqRatchetE2EEncryptVersion / v >= pqdrSMPAgentVersion conditionals and the version parameters that fed them. It also collapses the hand-written encodeMsgHeader/msgHeaderP, encRatchetMessageP, and encodeLarge/largeP pairs into plain Encoding instances, and threads user-requested PQSupport straight through instead of down-negotiating it per version.

I verified the refactor preserves wire behaviour for all still-supported peers:

  • E2E params, MsgHeader, EncMessageHeader, and EncRatchetMessage now always use the "with KEM" / Large (2-byte) encoding. For v≥3 peers this is byte-identical to the old gated encoding; only the dropped v1/v2 short encodings are removed, which is the stated intent.
  • e2eEncConnInfoLength / e2eEncAgentMsgLength lose their version arg; at min agent v6 (≥5) the reduced sizes were already selected, so no size change for supported peers.
  • versionPQSupport_ was PQSupport True for every supported (agent≥6, e2e≥3) combination, so removing the pqSupportAnd versionPQSupport_ … steps and passing PQSupport through unchanged is behaviour-preserving. The REQ event now reports PQSupportOn unconditionally, matching the earlier resolved comments.
  • adjustE2EVRange mirrors the pre-existing adjustAgentVRange (clamping a published range's min up to the new minimum); consistent with established convention.

A_MESSAGE gaining messageErr :: String is safe: SMPAgentError has no binary wire encoding (only a deriveJSON instance for the local agent↔client API), and the new field matches the shape of A_PROHIBITED/A_QUEUE/A_LINK. It turns a previously opaque parse error into a diagnostic, which is an improvement.

Tests are updated consistently (version literals 1/2→6/7/8, dropped sqSecured parameter now that reply queues are always secured, removal of the now-moot testDeliveryReceiptsVersion). The fdescribe and stale-comment issues from the prior review are resolved in head.

No correctness, security, or concurrency issues found. The change is coherent and the diff tells a single story.

Minor notes (non-blocking)

  • The parseMessage call sites in processSMPTransmissions use bare numeric context tags ("1""6", "4", "5", "6"). Given the commit's goal of adding error context, descriptive labels (e.g. "conf reply info", "client envelope") would be more useful in logs than digits; the other new contexts ("decrypt message", "parse msg header", "parse link data") already read well.
  • _sndAuthKeySMPAgentVersion and _pqRatchetE2EEncryptVersion keep an underscore prefix (conventionally "unused") while actually being used as the minSupported… bindings. Slightly misleading to a cold reader; consider dropping the underscore or inlining with a comment.
  • Ratchet.hs line 337: encodeKem kem = case has a double space.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant