Conversation
…ASM compatibility improvements
…eb-compat-socket-import fix(web): make SSHSocket conditional import wasm-compatible
…c-forward-socks5 feat: add dynamic SOCKS5 forwarding API
…and cipher support
…d provide usage example
…26-aead-groundwork feat: start AEAD support with AES-GCM groundwork
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough将包版本从 2.16.0 升级到 2.17.0。新增本地 SOCKS5 动态转发（仅支持 SOCKS5 NO AUTH + CONNECT、依赖 dart:io）：在 API 层新增 Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
refactor: Renamed MAC protection checks to integrity protection checks test: Updated AEAD tests to use new variable names fix: Fixed the condition for the dynamic forwarding connection limit docs: Updated unit test comments to clarify their purpose chore: Removed duplicate entries from the CHANGELOG
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…onnection is closed Fixed an issue in the _SocksConnection class where remote resources were not properly released when the connection was closed Corrected an incorrect assignment of the client and server IV derivation keys in SSHTransport Optimized signal handling and closure logic in the sample code
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Fixed an issue with resource release when a connection is closed during dynamic forwarding, ensuring that both the remote connection and the client are properly closed Fixed logic for IV derivation errors and integrity checks during SSH transmission Added a connection closure status flag to prevent duplicate closures Optimized the key renegotiation mechanism to trigger based on data volume
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…t handling - In the _SocksConnection class, extract duplicate closure logic into a dedicated `close` method - In the SSHTransport class, refactor the AEAD encryption logic to support the ChaCha20-Poly1305 algorithm - Move the re-key trigger logic to the AEAD processing branch
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Adjust code formatting and improve line breaks in long lines to enhance readability - Extract EOF handling logic into separate methods: _handleClientEOF and _handleRemoteEOF - Add a maximum handshake size limit to _ByteBuffer
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lib/src/ssh_transport.dart (1)
1742-1749:⚠️ Potential issue | 🟠 Major
hasIntegrityProtection应检查双向完整性保护当前实现（Line 1743-1746）使用 OR 逻辑，只要任一方向的 AEAD 密钥非空就返回
true。这可能在密钥交换中间状态（仅本地或仅远端密钥已初始化）时返回true，误导调用者认为双向完整性保护已就绪。🔧 建议修复：检查双向保护
bool get hasIntegrityProtection { - final usingAead = (_localAeadKey != null || - _localChaChaEncKey != null || - _remoteAeadKey != null || - _remoteChaChaEncKey != null); - if (usingAead) return true; + final localAeadReady = + _localAeadKey != null || _localChaChaEncKey != null; + final remoteAeadReady = + _remoteAeadKey != null || _remoteChaChaEncKey != null; + if (localAeadReady && remoteAeadReady) return true; return _localMac != null && _remoteMac != null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/ssh_transport.dart` around lines 1742 - 1749, The current hasIntegrityProtection returns true if any AEAD/ChaCha key exists locally or remotely; change it to require both directions be present: compute usingAeadLocal = (_localAeadKey != null || _localChaChaEncKey != null) and usingAeadRemote = (_remoteAeadKey != null || _remoteChaChaEncKey != null) and only return true for AEAD when usingAeadLocal && usingAeadRemote; otherwise fall back to requiring both _localMac != null && _remoteMac != null for MAC-based integrity. Update the hasIntegrityProtection getter to check these paired conditions (referencing hasIntegrityProtection, _localAeadKey, _localChaChaEncKey, _remoteAeadKey, _remoteChaChaEncKey, _localMac, _remoteMac).
🧹 Nitpick comments (3)
lib/src/ssh_transport.dart (3)
780-789: 发送与接收路径使用不同的密钥字段
_sendAeadPacket（Line 417-418）使用_localCipherKey/_localIV，但_consumeAeadPacket（Line 783-784）使用_remoteAeadKey/_remoteAeadFixedNonce。虽然这两组字段在
_applyLocalKeys/_applyRemoteKeys中被设置为相同的值，但命名不一致会增加维护难度和引入 bug 的风险。♻️ 建议统一使用一组字段
try { plaintext = _processAead( - key: _remoteAeadKey!, - iv: _remoteAeadFixedNonce!, + key: _remoteCipherKey!, + iv: _remoteIV!, sequence: _remotePacketSN.value, aad: aad, input: encryptedInput, forEncryption: false, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/ssh_transport.dart` around lines 780 - 789, Unify the AEAD key/IV field names so send and receive use the same identifiers: change uses of _remoteAeadKey/_remoteAeadFixedNonce in _consumeAeadPacket to the same canonical fields used by _sendAeadPacket (or vice versa), and update _applyLocalKeys/_applyRemoteKeys to set that single pair (e.g., a single _aeadKey and _aeadNonce/_iv) instead of separate _local* and _remote* names; ensure all references in _sendAeadPacket, _consumeAeadPacket, and any other crypto helpers (_processAead, sequence handling) are updated accordingly to avoid mismatch.
439-466:_processAead每次调用创建新的 cipher 实例功能正确，但每次发送/接收 AEAD 包都会创建新的
GCMBlockCipher(AESEngine())实例。对于高吞吐量场景，可以考虑复用 cipher 实例（仅重新初始化 nonce）以减少分配开销。
_nonceForSequence的 nonce 构造方式（将序列号加到 IV 的后 8 字节）符合 OpenSSH AES-GCM 规范。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/ssh_transport.dart` around lines 439 - 466, _processAead currently allocates a new GCMBlockCipher(AESEngine()) on every call causing unnecessary allocations; change this by promoting a reusable GCMBlockCipher (or separate encrypt/decrypt cipher instances) to a surrounding class field and reuse it across calls, reinitializing it with cipher.init(forEncryption, AEADParameters(KeyParameter(key), 128, _nonceForSequence(iv, sequence), aad)) each time instead of creating a new instance; keep using _nonceForSequence to build the nonce and ensure you reinit the same cipher instance before processing input in _processAead to avoid per-call allocation.
633-639: 冗余的 AEAD 重定向检查此处的 AEAD 检查与
_consumePacket（Line 594-601）中的检查重复。当执行到_consumeEncryptedPacket时，按理说_consumePacket中的 AEAD 路径条件不满足，但这里又检查了类似的条件。此外，检查条件使用
_remoteCipherKey/_remoteIV，但_consumeAeadPacket内部（Line 783-784）使用的是_remoteAeadKey/_remoteAeadFixedNonce。虽然在_applyRemoteKeys中两组字段被设置为相同的值，但这种不一致容易引起混淆。建议统一使用一组字段，或添加注释说明两组字段的关系。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/ssh_transport.dart` around lines 633 - 639, The AEAD redirect check in _consumeEncryptedPacket duplicates the check in _consumePacket and mixes _remoteCipherKey/_remoteIV with the AEAD-specific fields used inside _consumeAeadPacket; to fix, either remove the redundant AEAD branch from _consumeEncryptedPacket so AEAD packets are only routed by _consumePacket, or change the condition to test the AEAD-specific fields used by _consumeAeadPacket (use _remoteAeadKey and _remoteAeadFixedNonce instead of _remoteCipherKey/_remoteIV), and update or add a short comment in _applyRemoteKeys and near the check explaining the relationship if both field sets are intentionally kept.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/src/ssh_transport.dart`:
- Around line 1742-1749: The current hasIntegrityProtection returns true if any
AEAD/ChaCha key exists locally or remotely; change it to require both directions
be present: compute usingAeadLocal = (_localAeadKey != null ||
_localChaChaEncKey != null) and usingAeadRemote = (_remoteAeadKey != null ||
_remoteChaChaEncKey != null) and only return true for AEAD when usingAeadLocal
&& usingAeadRemote; otherwise fall back to requiring both _localMac != null &&
_remoteMac != null for MAC-based integrity. Update the hasIntegrityProtection
getter to check these paired conditions (referencing hasIntegrityProtection,
_localAeadKey, _localChaChaEncKey, _remoteAeadKey, _remoteChaChaEncKey,
_localMac, _remoteMac).
---
Nitpick comments:
In `@lib/src/ssh_transport.dart`:
- Around line 780-789: Unify the AEAD key/IV field names so send and receive use
the same identifiers: change uses of _remoteAeadKey/_remoteAeadFixedNonce in
_consumeAeadPacket to the same canonical fields used by _sendAeadPacket (or vice
versa), and update _applyLocalKeys/_applyRemoteKeys to set that single pair
(e.g., a single _aeadKey and _aeadNonce/_iv) instead of separate _local* and
_remote* names; ensure all references in _sendAeadPacket, _consumeAeadPacket,
and any other crypto helpers (_processAead, sequence handling) are updated
accordingly to avoid mismatch.
- Around line 439-466: _processAead currently allocates a new
GCMBlockCipher(AESEngine()) on every call causing unnecessary allocations;
change this by promoting a reusable GCMBlockCipher (or separate encrypt/decrypt
cipher instances) to a surrounding class field and reuse it across calls,
reinitializing it with cipher.init(forEncryption,
AEADParameters(KeyParameter(key), 128, _nonceForSequence(iv, sequence), aad))
each time instead of creating a new instance; keep using _nonceForSequence to
build the nonce and ensure you reinit the same cipher instance before processing
input in _processAead to avoid per-call allocation.
- Around line 633-639: The AEAD redirect check in _consumeEncryptedPacket
duplicates the check in _consumePacket and mixes _remoteCipherKey/_remoteIV with
the AEAD-specific fields used inside _consumeAeadPacket; to fix, either remove
the redundant AEAD branch from _consumeEncryptedPacket so AEAD packets are only
routed by _consumePacket, or change the condition to test the AEAD-specific
fields used by _consumeAeadPacket (use _remoteAeadKey and _remoteAeadFixedNonce
instead of _remoteCipherKey/_remoteIV), and update or add a short comment in
_applyRemoteKeys and near the check explaining the relationship if both field
sets are intentionally kept.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6b15ad4-c55a-47e2-8b7c-56db89ad6953
📒 Files selected for processing (6)
lib/src/dynamic_forward_io.dartlib/src/sftp/sftp_client.dartlib/src/ssh_agent.dartlib/src/ssh_key_pair.dartlib/src/ssh_transport.darttest/src/algorithm/ssh_cipher_type_test.dart
✅ Files skipped from review due to trivial changes (3)
- lib/src/ssh_agent.dart
- lib/src/ssh_key_pair.dart
- lib/src/sftp/sftp_client.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- test/src/algorithm/ssh_cipher_type_test.dart
- lib/src/dynamic_forward_io.dart
Remove the _localAeadFixedNonce variable and related AEAD encryption logic, which are no longer in use Simplify the conditional logic in the hasIntegrityProtection method
There was a problem hiding this comment.
♻️ Duplicate comments (2)
lib/src/ssh_transport.dart (2)
239-269:⚠️ Potential issue | 🔴 Critical
NEW_KEYS前仍然在用“下一轮协商结果”驱动当前收发路径。这条和之前的 rekey state 问题是同一个根因：
localCipherType/ct/macType还是直接读_clientCipherType、_serverCipherType、_clientMacType、_serverMacType，而这些值在收到KEXINIT时就已经被下一轮结果覆盖了。当前会话还没切到新 key 时，只要 rekey 跨了 AEAD/非 AEAD 或 ETM/非 ETM，KEX 包就会被按错误路径编码/解码：例如 AEAD → CTR 会落到明文分支，CTR+ETM → GCM（这里现在允许*_MacType == null）又会让接收侧继续走旧的加密分支，但拿到和活动状态不一致的macType。另外，_applyLocalKeys()/_applyRemoteKeys()现在也没有把“另一类”AEAD 状态完整清空，后续 rekey 还会被残留的 ChaCha/GCM key material 继续放大这个误判。这里还是要把 negotiated state 和 active state 分离，只在NEW_KEYS后切换活动 cipher/MAC。Also applies to: 543-556, 988-1105, 1446-1450
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/ssh_transport.dart` around lines 239 - 269, 当前代码在发送/接收数据时直接读取 `_clientCipherType` / `_serverCipherType` / `_clientMacType` / `_serverMacType` 等“协商结果”字段（见 localCipherType 计算与使用处），导致在收到 KEXINIT 到处理 NEW_KEYS 之前会错误地用下一轮协商结果编码/解码包；请把“已协商但未激活”的状态与“活动中”的状态分离：在发送/接收路径（例如使用 localCipherType 的分支）只读活动变量（引入或使用诸如 _activeClientCipherType/_activeServerCipherType、_activeClientMacType/_activeServerMacType），不直接访问 `_client*`/`_server*`；并在 `_applyLocalKeys()` / `_applyRemoteKeys()` 中在切换到 NEW_KEYS 时才更新这些活动变量，同时完整清除与另一类 AEAD（如 ChaCha/GCM）的残留 key material（例如 `_localChaChaEncKey`/`_localChaChaLenKey` /相关 GCM keys）以避免残留影响后续 rekey。
731-748:⚠️ Potential issue | 🟠 Major先拦住
paddingLength >= packetLength，否则畸形 AEAD 包会抛出非SSHError异常。这里和 ETM 分支不一样，没有在
paddingLength越界时提前转成SSHPacketError。一旦对端发来认证通过但格式非法的 AEAD 包，payloadLength会变成负数，后面的Uint8List.sublistView会抛RangeError，_onSocketData()不会按 SSH 错误路径处理它。建议把这个上界检查补到这里，或者下沉到_verifyPacketPadding()，这样 ChaCha 分支也能一起复用。🔧 建议修复
final paddingLength = plaintext[0]; + if (paddingLength >= packetLength) { + throw SSHPacketError( + 'Padding length too large: $paddingLength (packet length is $packetLength)', + ); + } final payloadLength = packetLength - paddingLength - 1; _verifyPacketPadding(payloadLength, paddingLength); return Uint8List.sublistView(plaintext, 1, 1 + payloadLength);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/ssh_transport.dart` around lines 731 - 748, The AEAD branch must reject malformed packets where paddingLength >= packetLength before computing payloadLength to avoid a RangeError (which bypasses SSH error handling); add an explicit upper-bound check after reading paddingLength in the AEAD path that throws SSHPacketError (same as the ETM branch) when paddingLength >= packetLength, or move that validation into _verifyPacketPadding so both the AEAD code path (the block using _processAead and Uint8List.sublistView) and the ChaCha branch reuse the same check; ensure the thrown error type is SSHPacketError so _onSocketData() follows the SSH error path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/src/ssh_transport.dart`:
- Around line 239-269: 当前代码在发送/接收数据时直接读取 `_clientCipherType` /
`_serverCipherType` / `_clientMacType` / `_serverMacType` 等“协商结果”字段（见
localCipherType 计算与使用处），导致在收到 KEXINIT 到处理 NEW_KEYS
之前会错误地用下一轮协商结果编码/解码包；请把“已协商但未激活”的状态与“活动中”的状态分离：在发送/接收路径（例如使用 localCipherType
的分支）只读活动变量（引入或使用诸如
_activeClientCipherType/_activeServerCipherType、_activeClientMacType/_activeServerMacType），不直接访问
`_client*`/`_server*`；并在 `_applyLocalKeys()` / `_applyRemoteKeys()` 中在切换到
NEW_KEYS 时才更新这些活动变量，同时完整清除与另一类 AEAD（如 ChaCha/GCM）的残留 key material（例如
`_localChaChaEncKey`/`_localChaChaLenKey` /相关 GCM keys）以避免残留影响后续 rekey。
- Around line 731-748: The AEAD branch must reject malformed packets where
paddingLength >= packetLength before computing payloadLength to avoid a
RangeError (which bypasses SSH error handling); add an explicit upper-bound
check after reading paddingLength in the AEAD path that throws SSHPacketError
(same as the ETM branch) when paddingLength >= packetLength, or move that
validation into _verifyPacketPadding so both the AEAD code path (the block using
_processAead and Uint8List.sublistView) and the ChaCha branch reuse the same
check; ensure the thrown error type is SSHPacketError so _onSocketData() follows
the SSH error path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a83254d8-78f1-414e-a20e-e8ca7b13f76f
📒 Files selected for processing (2)
lib/src/dynamic_forward_io.dartlib/src/ssh_transport.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/src/dynamic_forward_io.dart
The condition for checking the number of connections should be “less than or equal to” rather than “less than” Added the _dialing flag to prevent concurrent dialing requests
| final target = _parseConnectRequest(); | ||
| if (target == null) return; |
There was a problem hiding this comment.
🔴 _dialing flag never reset when SOCKS5 CONNECT request data is incomplete, permanently blocking the connection
In _consumeHandshake(), when the SOCKS5 greeting is parsed successfully and the state transitions to request, the code sets _dialing = true (line 192) before calling _parseConnectRequest() (line 194). If _parseConnectRequest() returns null because the CONNECT request data hasn't arrived yet (which is the normal case — SOCKS5 clients send the greeting, wait for the method selection reply, then send the CONNECT in a separate TCP segment), the method returns at line 195 without resetting _dialing. When the CONNECT request data arrives in a subsequent _onClientData call, _consumeHandshake() hits if (_dialing) return; at line 191 and returns immediately, so the request is never processed. The connection hangs until the handshake timer expires (default 10 seconds), making the dynamic forwarding feature non-functional for all standard SOCKS5 clients.
| final target = _parseConnectRequest(); | |
| if (target == null) return; | |
| final target = _parseConnectRequest(); | |
| if (target == null) { | |
| _dialing = false; | |
| return; | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Summary by CodeRabbit