Allow AEAD ciphers to ignore compatibility MAC negotiation - #236
Open
mjc wants to merge 2 commits into
Open
Conversation
Motivation: OpenSSH-style AEAD ciphers such as aes128-gcm@openssh.com ignore SSH MAC negotiation, but SwiftNIO SSH still requires KEXINIT MAC name-lists to be non-empty. When a peer offers an AEAD cipher with only MAC algorithms that are unused by AEAD, negotiation should not fail because the MAC lists do not overlap. Modifications: Negotiate transport protection as a concrete cipher/MAC scheme. AEAD schemes with no MAC name are selected by cipher and ignore the peer MAC list, while non-AEAD schemes still use the RFC first mutual MAC and fail if that cipher/MAC pair is unsupported. Keep the symmetric-protection requirement by comparing the selected protection types in both directions. Add AEAD regression coverage for the compatibility MAC proposal and for completing a handshake when the peer offers only unused ETM MACs. Result: OpenSSH-style AEAD negotiation succeeds without requiring MAC-list overlap, while non-AEAD MAC negotiation remains strict and symmetric.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates SSH key exchange algorithm negotiation to correctly handle OpenSSH-style AEAD ciphers by selecting transport protection schemes directly (rather than cipher/MAC tuples), while still advertising a compatibility MAC in KEXINIT for RFC 4253 compliance.
Changes:
- Return a concrete
NIOSSHTransportProtection.Typefrom transport protection negotiation and use it as the negotiated protection result. - Ignore MAC negotiation when the selected cipher corresponds to an AEAD scheme (
macName == nil), enabling negotiation even without a mutually supported MAC. - Add tests to validate compatibility MAC advertisement and AEAD negotiation behavior with mismatched MAC lists.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Tests/NIOSSHTests/SSHKeyExchangeStateMachineTests.swift | Adds regression tests covering AEAD-only KEXINIT MAC advertisement and AEAD negotiation when peer MAC lists don’t intersect. |
| Sources/NIOSSH/Key Exchange/SSHKeyExchangeStateMachine.swift | Refactors transport protection negotiation to return scheme types and implements AEAD-aware MAC negotiation behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Motivation: Transport protection negotiation checks each mutually supported cipher while selecting a concrete protection scheme. The first version of the AEAD negotiation fix built an intermediate array of candidate schemes for each cipher, which is unnecessary work during key exchange. Modifications: Search the configured transport protection schemes directly when looking for AEAD and selected cipher/MAC schemes. Add test-only transport protection metadata overrides and a regression test that advertises a first mutual MAC for a different cipher, then verifies negotiation fails instead of skipping to a later MAC. Result: Transport protection negotiation avoids the extra allocation and the intended RFC-driven MAC selection behavior is covered by a focused regression test.
mjc
force-pushed
the
fix-aead-mac-negotiation
branch
from
May 26, 2026 00:22
635ed69 to
b1feaef
Compare
Author
|
noting that I ran into this trying to connect chatgpt's codex ssh feature to my nixos server, OpenSSH_10.3p1, it would just hang instead of progressing the connection. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
SwiftNIO SSH advertises a compatibility MAC when all configured transport protections are OpenSSH-style AEAD ciphers, because RFC 4253 KEXINIT name-lists must contain at least one algorithm name. That advertised MAC is not actually used by AEAD ciphers such as
aes128-gcm@openssh.com, but negotiation still treated the MAC lists as if overlap was required.This made AEAD negotiation fail against peers that agreed on the AEAD cipher but offered only otherwise-unused MAC algorithms, such as ETM MACs.
Modifications
macName == nil) to be selected by cipher name without requiring overlap with the compatibility MAC proposal.Result
OpenSSH-style AEAD key exchange can complete when both peers agree on the AEAD cipher, even when the advertised compatibility MAC does not overlap with the peer's MAC list. Non-AEAD MAC negotiation remains strict and still fails when the first mutual MAC does not correspond to a supported protection scheme.
Testing
SSHKeyExchangeStateMachineTests.testAEADOnlyKeyExchangeAdvertisesCompatibilityMACsSSHKeyExchangeStateMachineTests.testAEADNegotiatesWhenPeerOffersOnlyUnusedMACsSSHKeyExchangeStateMachineTests.testMACNegotiationFailsWhenFirstMutualMACHasNoSchemeForSelectedCipherswift test --filter SSHKeyExchangeStateMachineTestsswift test