feat: default-on transport encryption (Noise_NK over libsodium)#11
Open
Segfaultd wants to merge 33 commits into
Open
feat: default-on transport encryption (Noise_NK over libsodium)#11Segfaultd wants to merge 33 commits into
Segfaultd wants to merge 33 commits into
Conversation
Fetch libsodium via robinlinden/libsodium-cmake (FetchContent) before add_subdirectory(Source) so the core library has access unconditionally. Link it as a build-local-only PRIVATE dep using $<BUILD_LOCAL_INTERFACE:> to keep the install export clean. Initialize libsodium at the top of RakPeer::Startup(); return the new STARTUP_LIBSODIUM_INIT_FAILED enumerator (appended to StartupResult in types.h) if sodium_init() returns -1.
Add ServerSecurityKey struct and GenerateServerSecurityKey() using libsodium X25519 (randombytes_buf + crypto_scalarmult_base), and stand up CryptoUnitTest to verify keypair correctness and uniqueness.
…n useSecurity) Replace cat::AuthenticatedEncryption with the new libsodium-based SecureSession in ReliabilityLayer. Encrypt/Decrypt remain gated on the `useSecurity` runtime flag so all existing tests pass unchanged (useSecurity is still false until Task 7).
Update all Samples/Tests/ sources to the new mandatory-encryption API: add SetServerSecurityKey(GetSampleServerKey()) on every peer that accepts incoming connections, pass GetSampleServerKey().publicKey as the 5th arg to every Connect() and ConnectWithSocket() call, and rewrite SecurityFunctionsTest to use the new direct-key overload instead of the removed PublicKey/PKM_USE_KNOWN_PUBLIC_KEY struct.
…zer run Expand SecurityFunctionsTest with two network-level negative sub-tests: - Sub-test 2: client connecting with a wrong pinned key is rejected (server's ReadMessageA silently drops the bad handshake; client receives ID_ALREADY_CONNECTED/ID_CONNECTION_ATTEMPT_FAILED, never ID_CONNECTION_REQUEST_ACCEPTED). - Sub-test 3: client connecting to a server with no security key gets ID_OUR_SYSTEM_REQUIRES_SECURITY immediately, never establishes a connection. Add MAFIANET_SANITIZER CMake option (address, undefined, address+undefined). CryptoUnitTest and SecurityFunctionsTest both pass clean under -fsanitize=address,undefined with ASAN_OPTIONS=detect_leaks=0. Note on tamper/replay/MITM: PacketChangerPlugin and PacketDropPlugin operate above the encryption layer (InternalPacket level, post-decrypt). Network-level datagram tamper and replay rejection are covered by CryptoUnitTest at the SecureSession level.
Rewrite docs/basics/secure-connections.rst for the new Noise_NK / libsodium stack; update docs/basics/connecting.rst with the revised Connect signature; update docs/plugins/fully-connected-mesh-2.rst for the new SetConnectOnNewRemoteConnection parameter; add an Unreleased section to docs/changelog.rst and README.md Changelog covering all breaking changes (mandatory encryption, removed API, libsodium dep, wire-incompatibility with 0.7.x, FCM2 signature change).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughImplements always-on Noise_NK transport with libsodium: adds crypto primitives (keys/noise/securesession), stateless HMAC cookie DoS mitigation, ServerSecurityKey APIs, requires pinned 32-byte serverPublicKey in Connect()/ConnectWithSocket(), updates ReliabilityLayer, samples/tests, CMake, and docs. ChangesMandatory Noise_NK transport and API changes
Estimated code review effort Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
README.md (3)
96-96:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBasic Usage example uses the removed
Connectsignature.This PR makes a pinned
serverPublicKey[32]mandatory onConnect/ConnectWithSocketand addsSetServerSecurityKey. The quick-start snippet still callsConnect("127.0.0.1", 60000, nullptr, 0), which no longer compiles and omits key setup. Update the example to reflect the new always-on encryption flow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 96, Update the README quick-start example to use the new mandatory encryption API: call SetServerSecurityKey(...) with a 32-byte serverPublicKey before calling Connect or ConnectWithSocket, and then use the new Connect/ConnectWithSocket signature that includes the serverPublicKey parameter instead of Connect("127.0.0.1", 60000, nullptr, 0); ensure the example shows providing a non-null serverPublicKey[32] buffer (or how to obtain it) so the code compiles and demonstrates the always-on encryption flow.
40-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOpenSSL version requirement is inconsistent and violates the dependency guideline.
CMakeLists.txtline 55 usesfind_package(OpenSSL 3.0.0 REQUIRED), but the README still statesOpenSSL 1.0.0+here and in the dependency table (Line 251). Update both to 3.0+ to match the build and stay synchronized.As per coding guidelines: "Require OpenSSL 3.0 or higher as a dependency for MafiaNet".📝 Proposed fix
-- OpenSSL 1.0.0+ +- OpenSSL 3.0+-| OpenSSL | 1.0.0+ | Encryption (required, system-installed) | +| OpenSSL | 3.0+ | Encryption (required, system-installed) |Also applies to: 251-251
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 40, The README currently lists "OpenSSL 1.0.0+" which is inconsistent with the build requirement; update every README occurrence (including the dependency table entry) that says OpenSSL 1.0.0+ to "OpenSSL 3.0+" so it matches the CMakeLists.txt find_package(OpenSSL 3.0.0 REQUIRED) declaration; search for "OpenSSL 1.0.0" or similar strings in README.md and replace them with "OpenSSL 3.0+" and ensure the dependency table row reflects this change.
249-255:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd libsodium to the dependencies table.
The changelog now lists libsodium as a required auto-fetched dependency, but the Dependencies table omits it. Add a row for completeness.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 249 - 255, The Dependencies table in README.md is missing libsodium; add a new markdown row under the same table header ("Dependency | Version | Used For") similar to the existing rows (e.g., alongside OpenSSL and Opus) with the dependency name "libsodium", an appropriate version marker (e.g., "-" or a minimum version if known from the changelog), and a brief "Used For" description such as "Cryptographic primitives (auto-fetched dependency)". Update the table so it reads consistently with the other entries.Samples/CloudClient/CloudClientSample.cpp (1)
153-205:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftLoad-balancing reconnect has no way to pin the new server's identity.
The rebalance logic only learns
row->serverSystemAddress, then Line 205 reconnects with the same cached sample key regardless of which server won. That only works if every cloud server shares one long-term private key, which defeats per-server pinning and makes independently keyed deployments impossible. The cloud row needs to publish each server's public key so this reconnect can pin the selected destination correctly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Samples/CloudClient/CloudClientSample.cpp` around lines 153 - 205, The rebalance reconnect uses MafiaNet::GetSampleServerKey().publicKey instead of the selected server's actual public key, preventing per-server pinning; when parsing each cloudQueryResult.rowsReturned entry (in the loop that reads row->data via bsIn) also read the server's public key (or ensure CloudQueryRow includes a publicKey field), store it alongside lowestConnectionAddress (e.g., lowestConnectionPublicKey) when updating lowestConnectionsServer, and then pass that stored public key into rakPeer->Connect(lowestConnectionAddress.ToString(false), ..., lowestConnectionPublicKey) instead of GetSampleServerKey().publicKey so the client pins the chosen server's identity.Source/src/FullyConnectedMesh2.cpp (1)
757-782:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThis auto-connect path can only pin one key for the entire mesh.
SetConnectOnNewRemoteConnection()caches a singleconnectionServerPublicKey, andConnectToRemoteNewIncomingConnections()reuses it for every discovered peer. In a real mesh, different remote servers can have different long-term keys, so this either forces every node to share one private key or makes auto-connect fail with key mismatches. Thenullptrbranch is broken too: it zero-fills the buffer and still passes that impossible all-zero key intoConnect(). This needs per-peer key distribution (or a callback/map lookup by remote GUID/address) plus a hard guard that refuses auto-connect when no key is configured.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/src/FullyConnectedMesh2.cpp` around lines 757 - 782, SetConnectOnNewRemoteConnection/ConnectToRemoteNewIncomingConnections currently use a single cached connectionServerPublicKey for all peers and zero-fill when none is set; change this to a per-peer key lookup (e.g., map or callback keyed by remoteGuid or remoteAddress) in ConnectToRemoteNewIncomingConnections so each remote GUID/address gets its own server public key, and call rakPeerInterface->Connect(...) with the per-peer key; add a strict guard to skip/refuse auto-connect when no valid key exists for that peer (do not pass an all-zero key), and update SetConnectOnNewRemoteConnection to either register a default key or store the callback/lookup mechanism instead of a single global key.Source/include/mafianet/peer.h (1)
140-153:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMajor API-contract fix: enforce the 32-byte pinning key type.
const unsigned char serverPublicKey[32]in function parameters decays toconst unsigned char*, so the signature does not enforce that callers pass a 32-byte key (any compatible pointer can be provided). For this security-critical pinning parameter, encode the size in the type (e.g., take a reference to a 32-byte array) in the public and internal signatures (also applies to 678-679).Suggested signature change
- ConnectionAttemptResult Connect( const char* host, unsigned short remotePort, const char *passwordData, int passwordDataLength, const unsigned char serverPublicKey[32], unsigned connectionSocketIndex=0, unsigned sendConnectionAttemptCount=12, unsigned timeBetweenSendConnectionAttemptsMS=500, MafiaNet::TimeMS timeoutTime=0 ); + ConnectionAttemptResult Connect( const char* host, unsigned short remotePort, const char *passwordData, int passwordDataLength, const unsigned char (&serverPublicKey)[32], unsigned connectionSocketIndex=0, unsigned sendConnectionAttemptCount=12, unsigned timeBetweenSendConnectionAttemptsMS=500, MafiaNet::TimeMS timeoutTime=0 ); - virtual ConnectionAttemptResult ConnectWithSocket(const char* host, unsigned short remotePort, const char *passwordData, int passwordDataLength, RakNetSocket2* socket, const unsigned char serverPublicKey[32], unsigned sendConnectionAttemptCount=12, unsigned timeBetweenSendConnectionAttemptsMS=500, MafiaNet::TimeMS timeoutTime=0); + virtual ConnectionAttemptResult ConnectWithSocket(const char* host, unsigned short remotePort, const char *passwordData, int passwordDataLength, RakNetSocket2* socket, const unsigned char (&serverPublicKey)[32], unsigned sendConnectionAttemptCount=12, unsigned timeBetweenSendConnectionAttemptsMS=500, MafiaNet::TimeMS timeoutTime=0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/include/mafianet/peer.h` around lines 140 - 153, The Connect and ConnectWithSocket declarations use const unsigned char serverPublicKey[32], which decays to a pointer and doesn't enforce a 32-byte size; change the parameter type in both the public declarations and their corresponding internal/overload signatures (including the other two occurrences of the same parameter) to a reference-to-32-byte-array type (for example, const unsigned char (&serverPublicKey)[32] or an equivalent std::array<unsigned char,32> const&), update the matching implementations to use that exact type, and update all call sites to pass a 32-byte array (or compatible std::array) so the compiler enforces the 32-byte pinning key constraint.Source/src/RakPeer.cpp (1)
4604-4660:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftBad Noise messages currently leak active peer slots.
Both sides call
AssignSystemAddressToRemoteSystemList()before the peer’s Noise message is authenticated, and the failure path only callsDereferenceRemoteSystem(systemAddress). That removes the hash lookup entry, but it leaves the slot active inactiveSystemListwithisActive == trueuntil the 10s timeout path cleans it up. On the server side, a reachable client can burnmaximumIncomingConnectionswith valid cookies plus malformedMESSAGE_A; on the client side, forgedREPLY_2packets can strand local slots the same way. The fix needs to happen at the root: either validateMESSAGE_A/Bbefore allocating the remote slot, or fully roll the slot back on failure.Also applies to: 4993-5025
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/src/RakPeer.cpp` around lines 4604 - 4660, AssignSystemAddressToRemoteSystemList() is being called before Noise messages are authenticated, so failures (e.g., noise.ReadMessageB() false) only call DereferenceRemoteSystem(systemAddress) which removes the hash but leaves an activeSystemList slot with isActive==true; update the failure paths (both in the server-side branch around noise.ReadMessageB and the corresponding client-side REPLY_2 handling around lines ~4993-5025) to fully roll back the allocated remote slot by clearing/unsetting the activeSystemList entry (set isActive=false, clear GUID/socket/addresses as AssignSystemAddressToRemoteSystemList initially did) and free any RemoteSystemStruct state before returning, or alternatively move the noise authentication to occur before calling AssignSystemAddressToRemoteSystemList so no active slot is allocated until the Noise handshake (noise.ReadMessageA/B) succeeds; modify code around RakPeer::AssignSystemAddressToRemoteSystemList, RakPeer::DereferenceRemoteSystem, and the RequestedConnectionStruct removal logic to ensure the slot is either not created or fully cleaned up on handshake failure.Samples/Tests/PeerConnectDisconnectTest.cpp (1)
233-240:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle benign reconnect races here too.
This loop still treats any non-
CONNECTION_ATTEMPT_STARTEDresult as fatal, but Lines 278-287 already documentALREADY_CONNECTED_TO_ENDPOINTandCONNECTION_ATTEMPT_ALREADY_IN_PROGRESSas expected races during re-convergence. With the encrypted handshake adding more overlap, this earlier reconnect path can false-fail the test before the tolerant logic runs.Proposed fix
- if (peerList[i]->Connect("127.0.0.1", 60000+j, 0,0, MafiaNet::GetSampleServerKey().publicKey)!=CONNECTION_ATTEMPT_STARTED) + ConnectionAttemptResult car = + peerList[i]->Connect("127.0.0.1", 60000+j, 0,0, MafiaNet::GetSampleServerKey().publicKey); + if (car != CONNECTION_ATTEMPT_STARTED && + car != ALREADY_CONNECTED_TO_ENDPOINT && + car != CONNECTION_ATTEMPT_ALREADY_IN_PROGRESS) { if (isVerbose) DebugTools::ShowError("Problem while calling connect.\n",!noPauses && isVerbose,__LINE__,__FILE__); return 1;//This fails the test, don't bother going on. }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Samples/Tests/PeerConnectDisconnectTest.cpp` around lines 233 - 240, The Connect call on peerList[i] currently treats any result other than CONNECTION_ATTEMPT_STARTED as fatal; update the check around peerList[i]->Connect(...) so that it treats ALREADY_CONNECTED_TO_ENDPOINT and CONNECTION_ATTEMPT_ALREADY_IN_PROGRESS as benign races (do not fail, just continue), only logging and returning 1 for other unexpected results; keep the existing verbose DebugTools::ShowError path for true errors and preserve the existing behavior when the result equals CONNECTION_ATTEMPT_STARTED.Samples/Tests/ManyClientsOneServerBlockingTest.cpp (1)
238-247:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove
SetServerSecurityKey()ahead ofStartup().Line 246 installs the key after the server was started on Line 239. Since this PR makes the server identity part of startup, the reconnect stress loop is currently testing an unsupported initialization sequence.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Samples/Tests/ManyClientsOneServerBlockingTest.cpp` around lines 238 - 247, Move the call to server->SetServerSecurityKey(...) so it runs before calling server->Startup(...); specifically, set the server key (using MafiaNet::GetSampleServerKey()) prior to creating/using SocketDescriptor and invoking Startup, then call server->SetMaximumIncomingConnections(clientNum) after Startup as before—ensure StartupResult and serverSd usage remain unchanged but with SetServerSecurityKey placed ahead of server->Startup to reflect the new startup identity requirement.Samples/Tests/ReliableOrderedConvertedTest.cpp (1)
160-169:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStart the receiver only after its server key is installed.
Line 168 sets the key after the receiver was already started on Line 161. With default-on transport security, that makes this ordering test depend on an unsupported secure-server initialization path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Samples/Tests/ReliableOrderedConvertedTest.cpp` around lines 160 - 169, The receiver is started before its server security key is set, causing the test to rely on an unsupported secure-server init path; move the call to receiver->SetServerSecurityKey(MafiaNet::GetSampleServerKey()) so it executes before receiver->Startup(...) (reference SocketDescriptor, StartupResult, and the methods receiver->SetServerSecurityKey and receiver->Startup) to ensure the key is installed prior to starting the receiver and then call receiver->SetMaximumIncomingConnections afterward as before.Samples/Tests/ManyClientsOneServerNonBlockingTest.cpp (1)
75-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet the server key before starting the peer.
Line 83 comes after Line 76, but the new secure-server flow requires the identity key to be present during startup. Right now the non-blocking test is validating connections against a server that was initialized out of contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Samples/Tests/ManyClientsOneServerNonBlockingTest.cpp` around lines 75 - 84, The server security key is set after calling server->Startup but the secure-server flow requires the identity key to be present during startup; move the call to server->SetServerSecurityKey(MafiaNet::GetSampleServerKey()) to before invoking server->Startup(...) (i.e., set the key on the ServerPeer instance before creating the SocketDescriptor/starting the peer), then call server->SetMaximumIncomingConnections(clientNum) and proceed with Startup; ensure StartupResult handling (serverResult != RAKNET_STARTED) remains unchanged.
🟡 Minor comments (8)
CMakeLists.txt-39-47 (1)
39-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSanitizer flags will break MSVC builds.
The comment states Clang/GCC only, but there's no compiler guard. If
MAFIANET_SANITIZERis set under MSVC,-fsanitize=...is passed verbatim and the build fails with an opaque error. Consider gating on the compiler.♻️ Proposed guard
set(MAFIANET_SANITIZER "" CACHE STRING "Enable compiler sanitizers (address, undefined, address+undefined)") if(MAFIANET_SANITIZER) + if(NOT (CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU")) + message(FATAL_ERROR "MAFIANET_SANITIZER is only supported with Clang/GCC") + endif() # Parse the "+" separator: "address+undefined" -> "-fsanitize=address,undefined" string(REPLACE "+" "," _san_flags "${MAFIANET_SANITIZER}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CMakeLists.txt` around lines 39 - 47, The sanitizer flags block should be applied only for compilers that support -fsanitize (Clang/GCC/AppleClang), so guard the existing MAFIANET_SANITIZER logic with a compiler check: when MAFIANET_SANITIZER is set, first test CMAKE_CXX_COMPILER_ID (or CMAKE_C_COMPILER_ID) and only perform the string(REPLACE ...), set(_san_compile_flags ...), set(_san_link_flags ...), message(STATUS ...), add_compile_options(...) and add_link_options(...) for "GNU", "Clang", or "AppleClang"; otherwise emit a warning message and skip adding -fsanitize to avoid passing flags to MSVC. Ensure the guard wraps the use of variables _san_flags, _san_compile_flags and the add_compile_options/add_link_options calls.Samples/ChatExample/Client/Chat Example Client.cpp-115-122 (1)
115-122:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle
Startup()failure before the first secureConnect().This path still ignores
client->Startup(...)and immediately callsSetServerSecurityKey()plusConnect(...). If startup fails, the sample reports a connection problem instead of the actual initialization failure. Gate the first connect onRAKNET_STARTED, like the later"startup"command already does.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Samples/ChatExample/Client/Chat` Example Client.cpp around lines 115 - 122, The code calls client->Startup(...) and then immediately calls client->SetServerSecurityKey(...) and client->Connect(...), but it doesn't check that Startup actually succeeded; gate the initial connect on the startup state by verifying the startup result (or client state equals RAKNET_STARTED) before calling SetServerSecurityKey and Connect. Specifically, after calling client->Startup(...) check the returned status (or the client state constant RAKNET_STARTED) and only proceed to call client->SetServerSecurityKey(...) and client->Connect(...) when startup succeeded; otherwise log or handle the startup failure and skip the Connect call (use the same startup-checking approach used by the "startup" command in the codebase).Samples/BurstTest/BurstTest.cpp-78-83 (1)
78-83:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCheck
Startup()before configuring security or continuing.Both branches ignore the
Startup()result. With default-on security, startup can now fail before anyConnect()call, but the sample still sets the server key and drops into the main loop as if the peer were active. Fail fast on non-RAKNET_STARTEDhere so startup errors do not turn into misleading connection failures later.Also applies to: 98-100
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Samples/BurstTest/BurstTest.cpp` around lines 78 - 83, Check the return value of rakPeer->Startup(...) and fail fast if it is not RAKNET_STARTED: call Startup(), store its return, and if it != RAKNET_STARTED log an error and exit (or return) before calling SetServerSecurityKey or Connect; update both occurrences that call Startup() (the one near Startup()/SetServerSecurityKey/Connect and the similar block around lines 98-100) to perform this check so security setup and the main loop only run when Startup() succeeded.Source/src/crypto/securesession.cpp-71-78 (1)
71-78:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCheck the AEAD encrypt return value to actually fail closed.
The return of
crypto_aead_chacha20poly1305_ietf_encryptis ignored, yet the buffer is overwritten andlengthupdated unconditionally, so the function reports success regardless. This contradicts the stated "fail closed on encrypt failures" goal. Although this primitive returns 0 in current libsodium, guarding it is trivial and future-proofs against API/behavior changes.🛡️ Proposed fix
unsigned long long clen = 0; - crypto_aead_chacha20poly1305_ietf_encrypt( - tmp, &clen, buffer, length, nullptr, 0, nullptr, npub, txKey); + if (crypto_aead_chacha20poly1305_ietf_encrypt( + tmp, &clen, buffer, length, nullptr, 0, nullptr, npub, txKey) != 0) + return false; writeLE64(buffer, ctr);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/src/crypto/securesession.cpp` around lines 71 - 78, The AEAD encrypt call crypto_aead_chacha20poly1305_ietf_encrypt is currently invoked without checking its return value, and the code unconditionally writes tmp into buffer and updates length; change this to capture the return (e.g., int rc = crypto_aead_chacha20poly1305_ietf_encrypt(...)), and if rc != 0 fail closed by returning false and NOT modifying buffer, length, or writing the counter; only after a successful rc == 0 should you call writeLE64(buffer, ctr), memcpy(buffer + 8, tmp, (size_t)clen) and update length = (unsigned int)(8 + clen). Ensure tmp and clen are only trusted after a successful encrypt.docs/plugins/fully-connected-mesh-2.rst-20-24 (1)
20-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the local
SetServerSecurityKey()step too.This example now shows the pinned public key for outbound connects, but every
FullyConnectedMesh2participant also accepts inbound peer connections. Without also showingpeer->SetServerSecurityKey(...), the snippet suggests the public key alone is sufficient and a copied example will fail once peers start dialing each other.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plugins/fully-connected-mesh-2.rst` around lines 20 - 24, The docs show setting a pinned server public key for outbound connects via FullyConnectedMesh2::SetConnectOnNewRemoteConnection but omit the corresponding step to accept inbound connections; update the snippet to call SetServerSecurityKey(...) on the local peer (the FullyConnectedMesh2 instance or its peer object, e.g., peer->SetServerSecurityKey(serverPublicKey)) so the participant uses the same pinned key for incoming connections and successfully accepts dialed peers; ensure the call is placed before starting/listening for connections and references the same serverPublicKey buffer used in SetConnectOnNewRemoteConnection.Samples/RakVoice/main.cpp-173-173 (1)
173-173:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCheck
Connect()results at these secured call sites.A bad pinned key or other immediate connect rejection now fails synchronously, but these branches assume the attempt started and leave the sample waiting on a connection that never began.
Suggested shape
- rakPeer->Connect(facilitatorIP, NAT_PUNCHTHROUGH_FACILITATOR_PORT, 0, 0, MafiaNet::GetSampleServerKey().publicKey); - printf("Connecting to facilitator...\n"); + ConnectionAttemptResult result = + rakPeer->Connect(facilitatorIP, NAT_PUNCHTHROUGH_FACILITATOR_PORT, 0, 0, MafiaNet::GetSampleServerKey().publicKey); + if (result != CONNECTION_ATTEMPT_STARTED) + { + printf("Failed to start facilitator connection: %d\n", result); + useNatPunchthrough = false; + } + else + { + printf("Connecting to facilitator...\n"); + }Also applies to: 231-231, 335-335
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Samples/RakVoice/main.cpp` at line 173, The call sites using rakPeer->Connect(...), including the facilitator connect (using facilitatorIP and NAT_PUNCHTHROUGH_FACILITATOR_PORT) and other calls that pass MafiaNet::GetSampleServerKey().publicKey, must check the Connect() return value and handle synchronous failures: capture the result of rakPeer->Connect, and on failure log an error (include the key/IP context), update the sample state to avoid waiting for a connection (e.g., abort the connect flow or trigger a retry/fail path) and return/exit the calling function; do this for each Connect() invocation so a bad pinned key or immediate rejection will not leave the sample hung.Samples/Tests/CryptoUnitTest.h-21-23 (1)
21-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
overridetoCryptoUnitTestoverrides ofTestInterface.
TestInterfacedeclares these pure-virtual signatures, andCryptoUnitTest.h(lines 21-23) currently lacksoverride:
int RunTest(DataStructures::List<RakString> params, bool isVerbose, bool noPauses)=0RakString GetTestName()=0RakString ErrorCodeToString(int errorCode)=0♻️ Proposed change
- int RunTest(DataStructures::List<RakString> params, bool isVerbose, bool noPauses); - RakString GetTestName(); - RakString ErrorCodeToString(int errorCode); + int RunTest(DataStructures::List<RakString> params, bool isVerbose, bool noPauses) override; + RakString GetTestName() override; + RakString ErrorCodeToString(int errorCode) override;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Samples/Tests/CryptoUnitTest.h` around lines 21 - 23, The three methods in CryptoUnitTest are implementing TestInterface but lack the override specifier; update the declarations of RunTest(DataStructures::List<RakString> params, bool isVerbose, bool noPauses), GetTestName(), and ErrorCodeToString(int errorCode) in the CryptoUnitTest class to include the override keyword so the signatures explicitly override the pure-virtual methods from TestInterface.Samples/Tests/CommonFunctions.cpp-68-71 (1)
68-71:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFail fast on immediate
Connect()failures.Now that
Connect()takes a pinned public key, it can reject synchronously for security/config errors. Ignoring the return here turns those into a generic timeout and retries the same bad request every 100ms.Proposed fix
- peer->Connect(ip,port,0,0, MafiaNet::GetSampleServerKey().publicKey); + ConnectionAttemptResult car = + peer->Connect(ip, port, 0, 0, MafiaNet::GetSampleServerKey().publicKey); + if (car != CONNECTION_ATTEMPT_STARTED && + car != ALREADY_CONNECTED_TO_ENDPOINT && + car != CONNECTION_ATTEMPT_ALREADY_IN_PROGRESS) + { + return false; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Samples/Tests/CommonFunctions.cpp` around lines 68 - 71, The code currently calls peer->Connect(ip,port,0,0, MafiaNet::GetSampleServerKey().publicKey) without checking its boolean return, so synchronous rejections become masked as timeouts and cause repeated retries; update the block after CommonFunctions::ConnectionStateMatchesOptions(...) to check the return value of peer->Connect(...) and on false immediately handle the failure (e.g. log the error with context, stop retrying, and return or mark the attempt as failed) instead of allowing the retry loop to continue.
🧹 Nitpick comments (4)
Samples/DirectoryDeltaTransfer/DirectoryDeltaTransferTest.cpp (1)
144-145: ⚡ Quick winThe UDP security migration in this test is currently dead code.
Line 43 still defines
USE_TCP, so both changed UDP branches are excluded from the build. As checked in, this file won't catch signature drift or compile breaks in the new encryptedRakPeerpath. Please add a non-TCP build/test variant, or move this coverage into a test that is actually compiled.Also applies to: 301-301
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Samples/DirectoryDeltaTransfer/DirectoryDeltaTransferTest.cpp` around lines 144 - 145, The test currently compiles only under TCP because the macro USE_TCP remains defined, so the UDP/encrypted-RakPeer branches (e.g., the calls around rakPeer->SetServerSecurityKey and the new encrypted RakPeer path) are never built; update the test suite to exercise the non-TCP code by adding a non-TCP build/test variant or moving the UDP/encryption-specific assertions into a test that is compiled: remove or conditionally undefine USE_TCP for a new variant, or duplicate the test into a new test file that uses the UDP configuration and calls SetServerSecurityKey/SetMaximumIncomingConnections against the encrypted RakPeer path so signature/compile breaks are caught during CI.Source/include/mafianet/peerinterface.h (1)
32-32: ⚡ Quick winUse the public include path for
keys.h.This is a public header, so the new dependency should follow the installed
mafianet/...form too.♻️ Proposed fix
-#include "crypto/keys.h" +#include "mafianet/crypto/keys.h"As per coding guidelines, "Include paths in C++ code should use the 'mafianet/...' format for public API headers".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/include/mafianet/peerinterface.h` at line 32, Replace the local-include form for the public header with the installed public include path: change the include directive that currently pulls "crypto/keys.h" in peerinterface.h to use the public API path "mafianet/crypto/keys.h" so the header follows the project's public include convention.Source/include/mafianet/crypto/keys.h (1)
16-20: ⚖️ Poor tradeoffConsider defense-in-depth zeroization for
ServerSecurityKeycopies/temporaries
ServerSecurityKeyis a plain struct (no destructor), so short-lived copies (e.g., the return value fromGenerateServerSecurityKey()and any by-value copies) aren’t wiped when they die.RakPeer::~RakPeer()already clears the long-lived stored key viasodium_memzero(serverSecurityKey.secretKey, ...), so the main secret held byRakPeeris handled; adding aServerSecurityKeydestructor callingsodium_memzero(secretKey, sizeof secretKey)would cover remaining copies/temporaries (verify this doesn’t cause any ABI/compile issues with by-value return usage).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/include/mafianet/crypto/keys.h` around lines 16 - 20, ServerSecurityKey is a plain POD so secretKey copies/temporaries aren’t zeroed; add a destructor to ServerSecurityKey that calls sodium_memzero(secretKey, sizeof secretKey) (and optionally publicKey) to zero out secrets on destruction, ensure the struct remains RAK_DLL_EXPORT-compatible and that by-value returns from GenerateServerSecurityKey() still compile (verify ABI/compile), and keep existing RakPeer::~RakPeer() zeroing as-is.Samples/Tests/CryptoUnitTest.h (1)
14-14: 💤 Low value
using namespace MafiaNet;in a header leaks into every translation unit that includes it. Prefer qualifying types (MafiaNet::RakString) or moving the directive into the.cpp. Optional, and consistent with the existing test-header pattern if you prefer to keep it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Samples/Tests/CryptoUnitTest.h` at line 14, Remove the global using directive from the header: the line "using namespace MafiaNet;" leaks into all TUs; instead either fully qualify types in this header (e.g., use MafiaNet::RakString, MafiaNet::SomeClass) or move the using directive into the corresponding .cpp file where tests are implemented; update any unqualified symbol references in CryptoUnitTest.h to their MafiaNet::... qualified names to avoid namespace pollution.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/basics/secure-connections.rst`:
- Around line 4-7: The docs/basics/secure-connections.rst text incorrectly pins
the change to "0.8.0"; reconcile this with the canonical project version by
either (A) changing the "0.8.0" occurrences in secure-connections.rst (the
encryption/authentication-by-default, API removal/replacement, and wire-compat
notes) to "0.7.0" to match the root VERSION in CMakeLists.txt and
MAFIANET_VERSION in Source/include/mafianet/version.h and entries in
docs/changelog.rst / README.md, or (B) if the change truly landed in 0.8.0, bump
the canonical VERSION and MAFIANET_VERSION to 0.8.0 and update changelog/README
accordingly; pick one approach and apply it consistently across
secure-connections.rst and the canonical version definitions.
In `@docs/changelog.rst`:
- Around line 6-60: Replace the top-level "Unreleased" heading with a concrete
release heading "Version X.Y.Z" (substitute the actual release number) and
ensure the subsequent sections (the ".. rubric:: Breaking changes", ".. rubric::
New features", and ".. rubric:: Dependencies") remain under that new Version
X.Y.Z heading; locate the "Unreleased" title string in docs/changelog.rst and
update it to the real version so the changelog aligns with the CMake canonical
version.
In `@Samples/CommandConsoleClient/main.cpp`:
- Around line 130-136: Remove the call that loads the full sample keypair into
the client (rakPeer->SetServerSecurityKey(MafiaNet::GetSampleServerKey())) and
instead use only the server's public key; stop calling
MafiaNet::GetSampleServerKey() from the client-side branch and either call an
existing MafiaNet::GetSampleServerPublicKey() or add a helper that returns only
the public key, then keep passing MafiaNet::GetSampleServerKey().publicKey (or
the new GetSampleServerPublicKey()) to rakPeer->Connect(...). Reserve
SetServerSecurityKey() for server code so the client's Connect/Connect(...,
publicKey) path does not ship the private key.
In `@Samples/NATCompleteClient/main.cpp`:
- Line 155: The Connect call in main.cpp is hard-pinning
MafiaNet::GetSampleServerKey().publicKey for all connection attempts which
breaks connections to the hosted default server; change the Connect(...) call to
pass the correct pinned public key for the default host (compare ipAddr /
DEFAULT_SERVER_ADDRESS or the code path used by ConnectBlocking()) or omit the
server-security public key for the default-host path so the client does not
enforce the demo key. Locate the call to rakPeer->Connect(...) and replace the
third-party key argument (MafiaNet::GetSampleServerKey().publicKey) with either
the proper natpunch.slikesoft.com pinned key constant or nullptr/empty when
connecting to DEFAULT_SERVER_ADDRESS, ensuring the logic mirrors
ConnectBlocking()'s default-host behavior.
In `@Samples/ReliableOrderedTest/ReliableOrderedTest.cpp`:
- Around line 160-163: The sender branch currently calls
sender->SetServerSecurityKey(MafiaNet::GetSampleServerKey()), which bakes the
server private key into the sender binary; remove that call and only pass the
server public key to sender->Connect (use the existing
MafiaNet::GetSampleServerKey().publicKey argument or a public-key-only helper if
available) so the sender never holds the private key; keep
sender->SetServerSecurityKey(...) only on the receiver/server code path that
accepts connections to ensure the private key remains server-side.
In `@Samples/Tests/LocalIsConnectedTest.cpp`:
- Around line 57-60: Move the server key setup so it runs before starting the
peer: call server->SetServerSecurityKey(MafiaNet::GetSampleServerKey()) prior to
server->Startup(...). In other words, reorder the lines so
SetServerSecurityKey(...) executes before Startup(...) (keeping SocketDescriptor
initialization and other settings unchanged) so the test exercises the encrypted
startup path.
In `@Samples/Tests/RPC4ContextTest.cpp`:
- Around line 164-168: Move the call to
server->SetServerSecurityKey(MafiaNet::GetSampleServerKey()) so it occurs before
server->Startup(...). Specifically, set the server security key (using
MafiaNet::GetSampleServerKey and the SetServerSecurityKey method) immediately
after creating SocketDescriptor serverSd(0, 0) and before invoking
server->Startup(8, &serverSd, 1); keep SetMaximumIncomingConnections after
Startup as-is.
In `@Samples/Tests/SecurityFunctionsTest.cpp`:
- Around line 247-255: The test currently treats ID_ALREADY_CONNECTED as an
expected auth rejection (setting clientGotRejection = true); instead leave this
path failing so the cleanup bug surfaces — remove or change that assignment so
ID_ALREADY_CONNECTED does NOT mark the handshake as a handled rejection (e.g.,
clear/avoid setting clientGotRejection and optionally log/unexpected message in
the ID_ALREADY_CONNECTED branch), and add a corresponding case (value 26) to
ErrorCodeToString() so the code-to-string mapping includes this error code for
clearer diagnostics; reference the ID_ALREADY_CONNECTED case block, the
clientGotRejection variable, and the ErrorCodeToString() function when making
these edits.
In `@Samples/Tests/TestHelpers.cpp`:
- Around line 24-28: The secure-server startup sequence is wrong:
server->SetServerSecurityKey(MafiaNet::GetSampleServerKey()) must be called
before server->Startup(...) so encrypted peers are initialized correctly;
reorder the calls in the helper so you invoke server->SetServerSecurityKey(...)
(using MafiaNet::GetSampleServerKey()) immediately after creating the
SocketDescriptor/RakPeerInterface instance and before calling
server->Startup(...), then call server->SetMaximumIncomingConnections(...) as
before.
- Around line 71-72: ConnectTwoPeersLocally currently returns the raw result of
connector->Connect(...) which is a MafiaNet::ConnectionAttemptResult and
therefore flips success semantics; change the return to compare the result to
MafiaNet::CONNECTION_ATTEMPT_STARTED (e.g. return connector->Connect(...) ==
MafiaNet::CONNECTION_ATTEMPT_STARTED) so the helper returns false on failure as
intended; reference ConnectTwoPeersLocally, connector->Connect,
MafiaNet::ConnectionAttemptResult, and CONNECTION_ATTEMPT_STARTED when making
the change.
In `@Source/CMakeLists.txt`:
- Around line 321-328: The installed CMake package does not declare libsodium
for downstream consumers: update the export/install logic so the exported target
for MafiaNetStatic includes libsodium in its link interface and ensure the
generated package file (MafiaNetConfig.cmake.in) calls find_dependency(sodium)
or find_dependency(libsodium); specifically, change the link specification
currently using $<BUILD_INTERFACE:sodium> for target MafiaNetStatic to provide a
proper PUBLIC/INTERFACE dependency for installs (so consumers see libsodium),
and add a corresponding find_dependency(sodium) entry in MafiaNetConfig.cmake.in
(or document that installed static packages require consumers to provide
libsodium).
In `@Source/src/crypto/noise.cpp`:
- Around line 184-215: The WriteMessageA path currently ignores the return of
crypto_scalarmult and may mix a DH computed from an invalid low-order responder
static into ss.MixKey; update InitInitiator(responderStaticPub) to validate the
provided responder static (rs) or set an internal error flag (e.g.,
handshake_ok/valid_rs) and have WriteMessageA check that flag before performing
crypto_scalarmult, returning/aborting early on invalid state; alternatively,
check the return value of crypto_scalarmult in WriteMessageA and on non-zero
return do not call ss.MixKey, zero sensitive state, and propagate failure so the
handshake fails closed (reference InitInitiator, WriteMessageA, rs,
crypto_scalarmult, ss.MixKey).
In `@Source/src/RakPeer.cpp`:
- Around line 4801-4811: The server currently advertises HasCookie=0 and falls
back to plaintext when rakPeer->hasServerSecurityKey is false; instead, make the
server fail-closed: in the block handling the outgoing HasCookie flag (the
bsOut.Write((unsigned char) 0) branch) and the corresponding
ID_OPEN_CONNECTION_REQUEST_2 handling (around ID_OPEN_CONNECTION_REQUEST_2), do
not advertise or accept plaintext connections — reject the connection attempt
(or send an explicit refusal) when rakPeer->hasServerSecurityKey is not set so
clients cannot silently downgrade; reference rakPeer->hasServerSecurityKey,
GenerateConnectionCookie, the HasCookie write, and the
ID_OPEN_CONNECTION_REQUEST_2 handling code and ensure callers are required to
call SetServerSecurityKey() to enable connections.
In `@Source/src/ReliabilityLayer.cpp`:
- Around line 417-420: Currently the code subtracts
SecureSession::OVERHEAD_BYTES twice (once in
Reset/GetMaxDatagramSizeExcludingMessageHeaderBytes and again here), causing an
undersized MTU; remove the second subtraction by eliminating the conditional
deduction in the congestionManager initialization path: leave
SecureSession::OVERHEAD_BYTES handling inside
Reset()/GetMaxDatagramSizeExcludingMessageHeaderBytes and call
congestionManager.Init(MafiaNet::GetTimeUS(), mtuSize - UDP_HEADER_SIZE)
unmodified when computing the datagram payload size, keeping references to
_useSecurity, SecureSession::OVERHEAD_BYTES, Reset(),
GetMaxDatagramSizeExcludingMessageHeaderBytes(), and congestionManager.Init to
locate the change.
---
Outside diff comments:
In `@README.md`:
- Line 96: Update the README quick-start example to use the new mandatory
encryption API: call SetServerSecurityKey(...) with a 32-byte serverPublicKey
before calling Connect or ConnectWithSocket, and then use the new
Connect/ConnectWithSocket signature that includes the serverPublicKey parameter
instead of Connect("127.0.0.1", 60000, nullptr, 0); ensure the example shows
providing a non-null serverPublicKey[32] buffer (or how to obtain it) so the
code compiles and demonstrates the always-on encryption flow.
- Line 40: The README currently lists "OpenSSL 1.0.0+" which is inconsistent
with the build requirement; update every README occurrence (including the
dependency table entry) that says OpenSSL 1.0.0+ to "OpenSSL 3.0+" so it matches
the CMakeLists.txt find_package(OpenSSL 3.0.0 REQUIRED) declaration; search for
"OpenSSL 1.0.0" or similar strings in README.md and replace them with "OpenSSL
3.0+" and ensure the dependency table row reflects this change.
- Around line 249-255: The Dependencies table in README.md is missing libsodium;
add a new markdown row under the same table header ("Dependency | Version | Used
For") similar to the existing rows (e.g., alongside OpenSSL and Opus) with the
dependency name "libsodium", an appropriate version marker (e.g., "-" or a
minimum version if known from the changelog), and a brief "Used For" description
such as "Cryptographic primitives (auto-fetched dependency)". Update the table
so it reads consistently with the other entries.
In `@Samples/CloudClient/CloudClientSample.cpp`:
- Around line 153-205: The rebalance reconnect uses
MafiaNet::GetSampleServerKey().publicKey instead of the selected server's actual
public key, preventing per-server pinning; when parsing each
cloudQueryResult.rowsReturned entry (in the loop that reads row->data via bsIn)
also read the server's public key (or ensure CloudQueryRow includes a publicKey
field), store it alongside lowestConnectionAddress (e.g.,
lowestConnectionPublicKey) when updating lowestConnectionsServer, and then pass
that stored public key into
rakPeer->Connect(lowestConnectionAddress.ToString(false), ...,
lowestConnectionPublicKey) instead of GetSampleServerKey().publicKey so the
client pins the chosen server's identity.
In `@Samples/Tests/ManyClientsOneServerBlockingTest.cpp`:
- Around line 238-247: Move the call to server->SetServerSecurityKey(...) so it
runs before calling server->Startup(...); specifically, set the server key
(using MafiaNet::GetSampleServerKey()) prior to creating/using SocketDescriptor
and invoking Startup, then call server->SetMaximumIncomingConnections(clientNum)
after Startup as before—ensure StartupResult and serverSd usage remain unchanged
but with SetServerSecurityKey placed ahead of server->Startup to reflect the new
startup identity requirement.
In `@Samples/Tests/ManyClientsOneServerNonBlockingTest.cpp`:
- Around line 75-84: The server security key is set after calling
server->Startup but the secure-server flow requires the identity key to be
present during startup; move the call to
server->SetServerSecurityKey(MafiaNet::GetSampleServerKey()) to before invoking
server->Startup(...) (i.e., set the key on the ServerPeer instance before
creating the SocketDescriptor/starting the peer), then call
server->SetMaximumIncomingConnections(clientNum) and proceed with Startup;
ensure StartupResult handling (serverResult != RAKNET_STARTED) remains
unchanged.
In `@Samples/Tests/PeerConnectDisconnectTest.cpp`:
- Around line 233-240: The Connect call on peerList[i] currently treats any
result other than CONNECTION_ATTEMPT_STARTED as fatal; update the check around
peerList[i]->Connect(...) so that it treats ALREADY_CONNECTED_TO_ENDPOINT and
CONNECTION_ATTEMPT_ALREADY_IN_PROGRESS as benign races (do not fail, just
continue), only logging and returning 1 for other unexpected results; keep the
existing verbose DebugTools::ShowError path for true errors and preserve the
existing behavior when the result equals CONNECTION_ATTEMPT_STARTED.
In `@Samples/Tests/ReliableOrderedConvertedTest.cpp`:
- Around line 160-169: The receiver is started before its server security key is
set, causing the test to rely on an unsupported secure-server init path; move
the call to receiver->SetServerSecurityKey(MafiaNet::GetSampleServerKey()) so it
executes before receiver->Startup(...) (reference SocketDescriptor,
StartupResult, and the methods receiver->SetServerSecurityKey and
receiver->Startup) to ensure the key is installed prior to starting the receiver
and then call receiver->SetMaximumIncomingConnections afterward as before.
In `@Source/include/mafianet/peer.h`:
- Around line 140-153: The Connect and ConnectWithSocket declarations use const
unsigned char serverPublicKey[32], which decays to a pointer and doesn't enforce
a 32-byte size; change the parameter type in both the public declarations and
their corresponding internal/overload signatures (including the other two
occurrences of the same parameter) to a reference-to-32-byte-array type (for
example, const unsigned char (&serverPublicKey)[32] or an equivalent
std::array<unsigned char,32> const&), update the matching implementations to use
that exact type, and update all call sites to pass a 32-byte array (or
compatible std::array) so the compiler enforces the 32-byte pinning key
constraint.
In `@Source/src/FullyConnectedMesh2.cpp`:
- Around line 757-782:
SetConnectOnNewRemoteConnection/ConnectToRemoteNewIncomingConnections currently
use a single cached connectionServerPublicKey for all peers and zero-fill when
none is set; change this to a per-peer key lookup (e.g., map or callback keyed
by remoteGuid or remoteAddress) in ConnectToRemoteNewIncomingConnections so each
remote GUID/address gets its own server public key, and call
rakPeerInterface->Connect(...) with the per-peer key; add a strict guard to
skip/refuse auto-connect when no valid key exists for that peer (do not pass an
all-zero key), and update SetConnectOnNewRemoteConnection to either register a
default key or store the callback/lookup mechanism instead of a single global
key.
In `@Source/src/RakPeer.cpp`:
- Around line 4604-4660: AssignSystemAddressToRemoteSystemList() is being called
before Noise messages are authenticated, so failures (e.g., noise.ReadMessageB()
false) only call DereferenceRemoteSystem(systemAddress) which removes the hash
but leaves an activeSystemList slot with isActive==true; update the failure
paths (both in the server-side branch around noise.ReadMessageB and the
corresponding client-side REPLY_2 handling around lines ~4993-5025) to fully
roll back the allocated remote slot by clearing/unsetting the activeSystemList
entry (set isActive=false, clear GUID/socket/addresses as
AssignSystemAddressToRemoteSystemList initially did) and free any
RemoteSystemStruct state before returning, or alternatively move the noise
authentication to occur before calling AssignSystemAddressToRemoteSystemList so
no active slot is allocated until the Noise handshake (noise.ReadMessageA/B)
succeeds; modify code around RakPeer::AssignSystemAddressToRemoteSystemList,
RakPeer::DereferenceRemoteSystem, and the RequestedConnectionStruct removal
logic to ensure the slot is either not created or fully cleaned up on handshake
failure.
---
Minor comments:
In `@CMakeLists.txt`:
- Around line 39-47: The sanitizer flags block should be applied only for
compilers that support -fsanitize (Clang/GCC/AppleClang), so guard the existing
MAFIANET_SANITIZER logic with a compiler check: when MAFIANET_SANITIZER is set,
first test CMAKE_CXX_COMPILER_ID (or CMAKE_C_COMPILER_ID) and only perform the
string(REPLACE ...), set(_san_compile_flags ...), set(_san_link_flags ...),
message(STATUS ...), add_compile_options(...) and add_link_options(...) for
"GNU", "Clang", or "AppleClang"; otherwise emit a warning message and skip
adding -fsanitize to avoid passing flags to MSVC. Ensure the guard wraps the use
of variables _san_flags, _san_compile_flags and the
add_compile_options/add_link_options calls.
In `@docs/plugins/fully-connected-mesh-2.rst`:
- Around line 20-24: The docs show setting a pinned server public key for
outbound connects via FullyConnectedMesh2::SetConnectOnNewRemoteConnection but
omit the corresponding step to accept inbound connections; update the snippet to
call SetServerSecurityKey(...) on the local peer (the FullyConnectedMesh2
instance or its peer object, e.g., peer->SetServerSecurityKey(serverPublicKey))
so the participant uses the same pinned key for incoming connections and
successfully accepts dialed peers; ensure the call is placed before
starting/listening for connections and references the same serverPublicKey
buffer used in SetConnectOnNewRemoteConnection.
In `@Samples/BurstTest/BurstTest.cpp`:
- Around line 78-83: Check the return value of rakPeer->Startup(...) and fail
fast if it is not RAKNET_STARTED: call Startup(), store its return, and if it !=
RAKNET_STARTED log an error and exit (or return) before calling
SetServerSecurityKey or Connect; update both occurrences that call Startup()
(the one near Startup()/SetServerSecurityKey/Connect and the similar block
around lines 98-100) to perform this check so security setup and the main loop
only run when Startup() succeeded.
In `@Samples/ChatExample/Client/Chat` Example Client.cpp:
- Around line 115-122: The code calls client->Startup(...) and then immediately
calls client->SetServerSecurityKey(...) and client->Connect(...), but it doesn't
check that Startup actually succeeded; gate the initial connect on the startup
state by verifying the startup result (or client state equals RAKNET_STARTED)
before calling SetServerSecurityKey and Connect. Specifically, after calling
client->Startup(...) check the returned status (or the client state constant
RAKNET_STARTED) and only proceed to call client->SetServerSecurityKey(...) and
client->Connect(...) when startup succeeded; otherwise log or handle the startup
failure and skip the Connect call (use the same startup-checking approach used
by the "startup" command in the codebase).
In `@Samples/RakVoice/main.cpp`:
- Line 173: The call sites using rakPeer->Connect(...), including the
facilitator connect (using facilitatorIP and NAT_PUNCHTHROUGH_FACILITATOR_PORT)
and other calls that pass MafiaNet::GetSampleServerKey().publicKey, must check
the Connect() return value and handle synchronous failures: capture the result
of rakPeer->Connect, and on failure log an error (include the key/IP context),
update the sample state to avoid waiting for a connection (e.g., abort the
connect flow or trigger a retry/fail path) and return/exit the calling function;
do this for each Connect() invocation so a bad pinned key or immediate rejection
will not leave the sample hung.
In `@Samples/Tests/CommonFunctions.cpp`:
- Around line 68-71: The code currently calls peer->Connect(ip,port,0,0,
MafiaNet::GetSampleServerKey().publicKey) without checking its boolean return,
so synchronous rejections become masked as timeouts and cause repeated retries;
update the block after CommonFunctions::ConnectionStateMatchesOptions(...) to
check the return value of peer->Connect(...) and on false immediately handle the
failure (e.g. log the error with context, stop retrying, and return or mark the
attempt as failed) instead of allowing the retry loop to continue.
In `@Samples/Tests/CryptoUnitTest.h`:
- Around line 21-23: The three methods in CryptoUnitTest are implementing
TestInterface but lack the override specifier; update the declarations of
RunTest(DataStructures::List<RakString> params, bool isVerbose, bool noPauses),
GetTestName(), and ErrorCodeToString(int errorCode) in the CryptoUnitTest class
to include the override keyword so the signatures explicitly override the
pure-virtual methods from TestInterface.
In `@Source/src/crypto/securesession.cpp`:
- Around line 71-78: The AEAD encrypt call
crypto_aead_chacha20poly1305_ietf_encrypt is currently invoked without checking
its return value, and the code unconditionally writes tmp into buffer and
updates length; change this to capture the return (e.g., int rc =
crypto_aead_chacha20poly1305_ietf_encrypt(...)), and if rc != 0 fail closed by
returning false and NOT modifying buffer, length, or writing the counter; only
after a successful rc == 0 should you call writeLE64(buffer, ctr), memcpy(buffer
+ 8, tmp, (size_t)clen) and update length = (unsigned int)(8 + clen). Ensure tmp
and clen are only trusted after a successful encrypt.
---
Nitpick comments:
In `@Samples/DirectoryDeltaTransfer/DirectoryDeltaTransferTest.cpp`:
- Around line 144-145: The test currently compiles only under TCP because the
macro USE_TCP remains defined, so the UDP/encrypted-RakPeer branches (e.g., the
calls around rakPeer->SetServerSecurityKey and the new encrypted RakPeer path)
are never built; update the test suite to exercise the non-TCP code by adding a
non-TCP build/test variant or moving the UDP/encryption-specific assertions into
a test that is compiled: remove or conditionally undefine USE_TCP for a new
variant, or duplicate the test into a new test file that uses the UDP
configuration and calls SetServerSecurityKey/SetMaximumIncomingConnections
against the encrypted RakPeer path so signature/compile breaks are caught during
CI.
In `@Samples/Tests/CryptoUnitTest.h`:
- Line 14: Remove the global using directive from the header: the line "using
namespace MafiaNet;" leaks into all TUs; instead either fully qualify types in
this header (e.g., use MafiaNet::RakString, MafiaNet::SomeClass) or move the
using directive into the corresponding .cpp file where tests are implemented;
update any unqualified symbol references in CryptoUnitTest.h to their
MafiaNet::... qualified names to avoid namespace pollution.
In `@Source/include/mafianet/crypto/keys.h`:
- Around line 16-20: ServerSecurityKey is a plain POD so secretKey
copies/temporaries aren’t zeroed; add a destructor to ServerSecurityKey that
calls sodium_memzero(secretKey, sizeof secretKey) (and optionally publicKey) to
zero out secrets on destruction, ensure the struct remains
RAK_DLL_EXPORT-compatible and that by-value returns from
GenerateServerSecurityKey() still compile (verify ABI/compile), and keep
existing RakPeer::~RakPeer() zeroing as-is.
In `@Source/include/mafianet/peerinterface.h`:
- Line 32: Replace the local-include form for the public header with the
installed public include path: change the include directive that currently pulls
"crypto/keys.h" in peerinterface.h to use the public API path
"mafianet/crypto/keys.h" so the header follows the project's public include
convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…pic, handshake cleanup, sample key hygiene)
…docs
Add ServerSecurityKeyFromSecret, ServerSecurityKeyFromSecretHex/Base64,
ServerPublicKeyFromHex/Base64, and four To{Hex,Base64} encoders so that
server owners can store only the secret as a config/env string (no key
file required) and clients can pin the public key from a config string.
Extend CryptoUnitTest with round-trip and malformed-input checks (codes
50-58). Expand docs/basics/secure-connections.rst with five key-gen/load
options including config string, env var, ephemeral, and binary file.
…tizer guard, AEAD/encrypt return, slot-cleanup follow-ups)
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Source/src/crypto/keys.cpp (1)
23-29: ⚡ Quick winUse
sizeoffor the key copy to match buffer declaration.Line 26 hardcodes
32for the memcpy size. Usingsizeof(k.secretKey)(or equivalentlysizeof(secretKey)) ensures the copy matches the declared buffer size and improves maintainability if the key size ever changes.♻️ Proposed fix
ServerSecurityKey ServerSecurityKeyFromSecret(const unsigned char secretKey[32]) { ServerSecurityKey k; - memcpy(k.secretKey, secretKey, 32); + memcpy(k.secretKey, secretKey, sizeof(k.secretKey)); crypto_scalarmult_base(k.publicKey, k.secretKey); return k; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/src/crypto/keys.cpp` around lines 23 - 29, Replace the hardcoded memcpy length in ServerSecurityKeyFromSecret with the actual buffer size: change the memcpy call to use sizeof(k.secretKey) (or sizeof(secretKey)) so the copy uses the declared key buffer size instead of the literal 32; update the memcpy invocation that currently references memcpy(k.secretKey, secretKey, 32) to use sizeof(k.secretKey) to ensure correct and maintainable sizing when copying secretKey into k.secretKey.Source/include/mafianet/crypto/keys.h (1)
28-29: 💤 Low valueConsider
constexprfor compile-time constants in C++17.The
SERVER_KEY_HEX_LENandSERVER_KEY_BASE64_LENconstants could be declared asconstexprinstead ofstatic constto better express that they are compile-time constants in C++17.♻️ Proposed change
-static const size_t SERVER_KEY_HEX_LEN = 65; // 32 bytes -> 64 hex chars + NUL -static const size_t SERVER_KEY_BASE64_LEN = 45; // sodium_base64_ENCODED_LEN(32, ORIGINAL variant) +constexpr size_t SERVER_KEY_HEX_LEN = 65; // 32 bytes -> 64 hex chars + NUL +constexpr size_t SERVER_KEY_BASE64_LEN = 45; // sodium_base64_ENCODED_LEN(32, ORIGINAL variant)As per coding guidelines, MafiaNet uses C++17 standard.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/include/mafianet/crypto/keys.h` around lines 28 - 29, The two compile-time constants SERVER_KEY_HEX_LEN and SERVER_KEY_BASE64_LEN should be declared as constexpr to reflect C++17 compile-time constants; replace "static const size_t SERVER_KEY_HEX_LEN = 65;" and "static const size_t SERVER_KEY_BASE64_LEN = 45;" with "constexpr size_t SERVER_KEY_HEX_LEN = 65;" and "constexpr size_t SERVER_KEY_BASE64_LEN = 45;" respectively (leave the names unchanged so callers like SERVER_KEY_HEX_LEN and SERVER_KEY_BASE64_LEN continue to work).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Source/include/mafianet/crypto/keys.h`:
- Around line 28-29: The two compile-time constants SERVER_KEY_HEX_LEN and
SERVER_KEY_BASE64_LEN should be declared as constexpr to reflect C++17
compile-time constants; replace "static const size_t SERVER_KEY_HEX_LEN = 65;"
and "static const size_t SERVER_KEY_BASE64_LEN = 45;" with "constexpr size_t
SERVER_KEY_HEX_LEN = 65;" and "constexpr size_t SERVER_KEY_BASE64_LEN = 45;"
respectively (leave the names unchanged so callers like SERVER_KEY_HEX_LEN and
SERVER_KEY_BASE64_LEN continue to work).
In `@Source/src/crypto/keys.cpp`:
- Around line 23-29: Replace the hardcoded memcpy length in
ServerSecurityKeyFromSecret with the actual buffer size: change the memcpy call
to use sizeof(k.secretKey) (or sizeof(secretKey)) so the copy uses the declared
key buffer size instead of the literal 32; update the memcpy invocation that
currently references memcpy(k.secretKey, secretKey, 32) to use
sizeof(k.secretKey) to ensure correct and maintainable sizing when copying
secretKey into k.secretKey.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8bf003a6-35c6-458d-aebd-b9af7aa4e9fb
📒 Files selected for processing (32)
CMakeLists.txtREADME.mdSamples/CommandConsoleClient/main.cppSamples/NATCompleteClient/main.cppSamples/ReliableOrderedTest/ReliableOrderedTest.cppSamples/SampleHelpers/SampleSecurity.hSamples/Tests/CrossConnectionConvertTest.cppSamples/Tests/CryptoUnitTest.cppSamples/Tests/CryptoUnitTest.hSamples/Tests/EightPeerTest.cppSamples/Tests/LocalIsConnectedTest.cppSamples/Tests/ManyClientsOneServerBlockingTest.cppSamples/Tests/ManyClientsOneServerNonBlockingTest.cppSamples/Tests/MaximumConnectTest.cppSamples/Tests/PacketAndLowLevelTestsTest.cppSamples/Tests/PeerConnectDisconnectTest.cppSamples/Tests/PeerConnectDisconnectWithCancelPendingTest.cppSamples/Tests/PingTestsTest.cppSamples/Tests/RPC4ContextTest.cppSamples/Tests/ReliableOrderedConvertedTest.cppSamples/Tests/SecurityFunctionsTest.cppSamples/Tests/TestHelpers.cppSource/include/mafianet/crypto/keys.hSource/include/mafianet/peerinterface.hSource/src/RakPeer.cppSource/src/ReliabilityLayer.cppSource/src/crypto/keys.cppSource/src/crypto/noise.cppSource/src/crypto/securesession.cppcmake/MafiaNetConfig.cmake.indocs/basics/secure-connections.rstdocs/plugins/fully-connected-mesh-2.rst
✅ Files skipped from review due to trivial changes (2)
- cmake/MafiaNetConfig.cmake.in
- README.md
🚧 Files skipped from review as they are similar to previous changes (18)
- docs/plugins/fully-connected-mesh-2.rst
- CMakeLists.txt
- Samples/Tests/PeerConnectDisconnectTest.cpp
- Samples/Tests/MaximumConnectTest.cpp
- Samples/Tests/CrossConnectionConvertTest.cpp
- Samples/NATCompleteClient/main.cpp
- Samples/Tests/EightPeerTest.cpp
- Samples/Tests/LocalIsConnectedTest.cpp
- Samples/Tests/TestHelpers.cpp
- Samples/Tests/PeerConnectDisconnectWithCancelPendingTest.cpp
- Samples/Tests/ManyClientsOneServerNonBlockingTest.cpp
- Source/include/mafianet/peerinterface.h
- Samples/Tests/CryptoUnitTest.h
- Source/src/crypto/securesession.cpp
- Samples/Tests/ManyClientsOneServerBlockingTest.cpp
- Samples/Tests/SecurityFunctionsTest.cpp
- Samples/Tests/RPC4ContextTest.cpp
- Source/src/crypto/noise.cpp
… helpers, doc consistency) - FullyConnectedMesh2: actually fail closed without a pinned key — track hasConnectionServerPublicKey and skip auto-mesh Connect() attempts instead of dialing with an all-zero key (which burned the full retry cycle and emitted spurious ID_CONNECTION_ATTEMPT_FAILED) - crypto/keys: GenerateServerSecurityKey/ServerSecurityKeyFromSecret call sodium_init() themselves (idempotent), so the documented set-key-before- Startup order no longer uses the RNG before libsodium is initialized; fail closed with an all-zero keypair if init fails - peerinterface.h/peer.h: SetServerSecurityKey docs now say call BEFORE Startup (was 'after', contradicting README/changelog and racing the network thread's unsynchronized key reads) - docs: replace nonexistent LoadServerSecurityKey() in fully-connected-mesh-2 example with ServerSecurityKeyFromSecret; update connecting.rst Connect defaults to 12/500ms to match the headers
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Source/include/mafianet/FullyConnectedMesh2.h (1)
352-355: Initialization is already correct in FullyConnectedMesh2 constructor
FullyConnectedMesh2::FullyConnectedMesh2()(inSource/src/FullyConnectedMesh2.cpp, constructor body) zero-initializesconnectionServerPublicKeyand setshasConnectionServerPublicKey=false, so the fail-closed logic aroundhasConnectionServerPublicKey==falsecan’t read indeterminate values.Optional: consider adding C++ in-class initializers for defense-in-depth:
unsigned char connectionServerPublicKey[32] = {}; bool hasConnectionServerPublicKey = false;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/include/mafianet/FullyConnectedMesh2.h` around lines 352 - 355, The constructor already zeroes connectionServerPublicKey and sets hasConnectionServerPublicKey=false, but for defence-in-depth add C++ in-class initializers on the FullyConnectedMesh2 declaration: initialize connectionServerPublicKey to all zeros and hasConnectionServerPublicKey to false so the members are always well-defined even before the constructor runs; update the declarations of connectionServerPublicKey and hasConnectionServerPublicKey in FullyConnectedMesh2.h accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Source/include/mafianet/FullyConnectedMesh2.h`:
- Around line 352-355: The constructor already zeroes connectionServerPublicKey
and sets hasConnectionServerPublicKey=false, but for defence-in-depth add C++
in-class initializers on the FullyConnectedMesh2 declaration: initialize
connectionServerPublicKey to all zeros and hasConnectionServerPublicKey to false
so the members are always well-defined even before the constructor runs; update
the declarations of connectionServerPublicKey and hasConnectionServerPublicKey
in FullyConnectedMesh2.h accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 16b2b0d7-9c86-4f31-892f-724d0b8f0b78
📒 Files selected for processing (9)
README.mdSource/include/mafianet/FullyConnectedMesh2.hSource/include/mafianet/crypto/keys.hSource/include/mafianet/peer.hSource/include/mafianet/peerinterface.hSource/src/FullyConnectedMesh2.cppSource/src/crypto/keys.cppdocs/basics/connecting.rstdocs/plugins/fully-connected-mesh-2.rst
✅ Files skipped from review due to trivial changes (1)
- docs/plugins/fully-connected-mesh-2.rst
🚧 Files skipped from review as they are similar to previous changes (7)
- Source/src/FullyConnectedMesh2.cpp
- README.md
- Source/include/mafianet/crypto/keys.h
- docs/basics/connecting.rst
- Source/include/mafianet/peerinterface.h
- Source/src/crypto/keys.cpp
- Source/include/mafianet/peer.h
…GridSectorizer) into feat/default-encryption Conflict resolutions: - RakPeer.cpp: whole-file conflict (branch re-indented the file); took the branch version and hand-ported master's 8 hunks: disconnect-reason field init/teardown (Startup/Shutdown/slot reuse/CloseConnectionInternal2), ClearDisconnectReason, reasonData plumbing through CloseConnection -> NotifyAndFlagForShutdown, reason stash on incoming notification, reason attach on the synthesized user packet, and the CloseConnection -1-index bugfix - peer.h: kept both RemoteSystemStruct additions (disconnectReasonData/Length from master, Noise answer[48] from this branch) - Tests.cpp/IncludeAllTests.h: union of CryptoUnitTest and master's three new tests - README/changelog: Unreleased encryption section on top of 0.9.0/0.8.0; wire-incompatibility note now says 0.9.x and earlier - DisconnectReasonTest: server installs the shared sample security key (mandatory encryption would otherwise refuse its connections) Verified: full build; DisconnectReasonTest, PeerGuidTest, PointGridSectorizerTest, CryptoUnitTest, SecurityFunctionsTest, PeerConnectDisconnectTest all pass.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Source/include/mafianet/peerinterface.h (1)
128-139:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the new pinned key parameter in
ConnectWithSocket.Line 139 includes
serverPublicKey[32], but the\paramlist above it skips this argument. This leaves a security-critical API parameter undocumented.Suggested doc fix
/// \param[in] passwordDataLength The length in bytes of passwordData /// \param[in] socket A bound socket returned by another instance of RakPeerInterface + /// \param[in] serverPublicKey The 32-byte Noise_NK static public key the client pins for the server. Encryption is mandatory; this key must match the server's SetServerSecurityKey(). /// \param[in] sendConnectionAttemptCount How many datagrams to send to the other system to try to connect. /// \param[in] timeBetweenSendConnectionAttemptsMS Time to elapse before a datagram is sent to the other system to try to connect. After sendConnectionAttemptCount number of attempts, ID_CONNECTION_ATTEMPT_FAILED is returned. Under low bandwidth conditions with multiple simultaneous outgoing connections, this value should be raised to 1000 or higher, or else the MTU detection can overrun the available bandwidth. /// \param[in] timeoutTime How long to keep the connection alive before dropping it on unable to send a reliable message. 0 to use the default from SetTimeoutTime(UNASSIGNED_SYSTEM_ADDRESS);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/include/mafianet/peerinterface.h` around lines 128 - 139, The ConnectWithSocket declaration adds a new parameter serverPublicKey[32] but the docstring above lacks a corresponding \param entry; update the comment block for the ConnectWithSocket method to add a \param[in] description for serverPublicKey (explain it is the 32-byte pinned server public key used for server identity verification and that it may be nullptr/empty if not used), and ensure it mentions expected length and behavior when provided so the security-critical parameter is documented alongside host, remotePort, passwordData, etc., referencing the ConnectWithSocket method and serverPublicKey symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Source/include/mafianet/peerinterface.h`:
- Around line 128-139: The ConnectWithSocket declaration adds a new parameter
serverPublicKey[32] but the docstring above lacks a corresponding \param entry;
update the comment block for the ConnectWithSocket method to add a \param[in]
description for serverPublicKey (explain it is the 32-byte pinned server public
key used for server identity verification and that it may be nullptr/empty if
not used), and ensure it mentions expected length and behavior when provided so
the security-critical parameter is documented alongside host, remotePort,
passwordData, etc., referencing the ConnectWithSocket method and serverPublicKey
symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c18cf0bc-85cb-422c-b7c8-b2b816d839af
📒 Files selected for processing (12)
CMakeLists.txtREADME.mdSamples/Tests/DisconnectReasonTest.cppSamples/Tests/IncludeAllTests.hSamples/Tests/Tests.cppSource/CMakeLists.txtSource/include/mafianet/peer.hSource/include/mafianet/peerinterface.hSource/include/mafianet/types.hSource/src/RakPeer.cppdocs/basics/connecting.rstdocs/changelog.rst
✅ Files skipped from review due to trivial changes (2)
- docs/changelog.rst
- docs/basics/connecting.rst
🚧 Files skipped from review as they are similar to previous changes (7)
- Samples/Tests/IncludeAllTests.h
- CMakeLists.txt
- Samples/Tests/Tests.cpp
- Source/include/mafianet/types.h
- Source/CMakeLists.txt
- README.md
- Source/include/mafianet/peer.h
REQUEST_1 keeps being resent until REPLY_2 completes the attempt, so duplicate REPLY_1s are routine (RTT above the 500ms retry interval, or a lost REPLY_2). The client regenerated message A with a fresh ephemeral on every REPLY_1 while the server replays the message B it cached for the first message A, so the handshake hard-failed with a spurious ID_PUBLIC_KEY_MISMATCH on slow or lossy links - and a blind off-path spoofer could kill any in-progress attempt with one forged REPLY_2. - Generate message A exactly once per connection attempt and resend the same cached bytes on every REPLY_1 (idempotent with the server's cached message B). - Verify message B on a COPY of the handshake state (ReadMessageB mutates state even on failure); drop a stale/forged REPLY_2 silently and keep retrying instead of aborting the attempt. - Report ID_PUBLIC_KEY_MISMATCH instead of ID_CONNECTION_ATTEMPT_FAILED when the attempt exhausts after a message B failed verification. - Make NoiseHandshake copy operations explicitly defaulted (trial-verify relies on copies; user-declared dtor made implicit copies deprecated). - CryptoUnitTest: new sub-test asserting a failed ReadMessageB on a copy leaves the original state verifiable with matching transport keys.
Two hardening fixes in the client-side ID_OPEN_CONNECTION_REPLY_2 path, found in pre-landing review: - A spoofed REPLY_2 with doSecurity=0 previously skipped Noise message B verification and completed a plaintext, unauthenticated connection despite the pinned server key. Now fails closed with ID_OUR_SYSTEM_REQUIRES_SECURITY, mirroring the REPLY_1 path. - A forged REPLY_2 arriving before message A was generated was verified against a never-initialized initiator handshake (uninitialized symmetric state, zero ephemeral). Now dropped silently via a noiseMsgAGenerated guard; NoiseHandshake's constructor additionally zero-initializes the symmetric state as defense in depth.
SetServerSecurityKey documents that it must be called before Startup(): the network thread reads hasServerSecurityKey/serverSecurityKey without synchronization, so installing the key while the thread is running is a data race. Most sample/test call sites installed it on the line after Startup(); reorder all 68 sites across 47 files so the key is in place before the network thread starts, matching the README and docs examples.
- Recognize ID_OUR_SYSTEM_REQUIRES_SECURITY as an offline message so the server's reject datagram actually reaches the connecting client (cancels the attempt and surfaces the failure instead of being silently dropped as an unparseable connected-system datagram). - Remove the always-true RequestedConnectionStruct::useNoiseSecurity flag and its unreachable branches (REPLY_1 flag=0 announce, REPLY_2 ID_REMOTE_SYSTEM_REQUIRES_PUBLIC_KEY path); encryption is mandatory, so every attempt pins a key. - Check crypto_scalarmult's return in NoiseHandshake::WriteMessageB, mirroring the other DH sites, so the fail-closed behavior doesn't depend on call order. - Add CryptoUnitTest to the test list in CLAUDE.md.
- Guard SetServerSecurityKey against calls while the peer is active (assert + ignore), mirroring the old InitializeSecurity offline check; the network thread reads the key unsynchronized. - Restore password and ban-list enforcement coverage lost in the SecurityFunctionsTest rewrite as sub-test 4 (codes 40-53), now running over the encrypted handshake. - Correct changelog/migration docs: GenerateServerRSAKeys only ever appeared in documentation examples, never in code. - Drop dead _san_compile_flags/_san_link_flags variables in CMakeLists.
- ChatExample client pins the server's public key via GetSampleServerPublicKey() instead of installing the demo secret keypair. - Annotate/rename the vestigial libcat doSecurity byte in ID_CONNECTION_REQUEST as a reserved framing field on both ends. - Re-derive Noise message B when a duplicate REQUEST_2 carries a different message A (client restarted its attempt on the same half-open slot), instead of stranding it on the stale cached answer until timeout. - Add SecurityFunctionsTest sub-test 5: raw-socket stateless-cookie gate coverage (corrupted/truncated cookie and garbage message A dropped silently; server still accepts a real client afterward).
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
1 participant
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.
Summary
Makes MafiaNet encrypted and authenticated by default, replacing the dead libcat crypto stack with a modern, vetted libsodium-based design.
Noise_NK_25519_ChaChaPoly_SHA512(X25519 + ChaCha20-Poly1305 + SHA-512/HKDF), implemented over libsodium. Validated byte-for-byte against an independent OpenSSL-backed reference (known-answer vector inCryptoUnitTest).SecureHandshake.*/LIBCAT_SECURITY. Added: libsodium (pinned commit, via FetchContent).New API
MafiaNet::ServerSecurityKey,MafiaNet::GenerateServerSecurityKey()(mafianet/crypto/keys.h)RakPeerInterface::SetServerSecurityKey(const ServerSecurityKey&)Connect(host, port, pw, pwLen, const unsigned char serverPublicKey[32], ...)— pinned key now required (same forConnectWithSocket)Breaking changes (⚠️ wire-incompatible with 0.7.x)
InitializeSecurity/DisableSecurity/AddToSecurityExceptionList/GetClientPublicKeyFromSystemAddressand thePublicKey/PublicKeyModetypes.FullyConnectedMesh2::SetConnectOnNewRemoteConnectiongains aserverPublicKey[32]param (auto-mesh fail-closed without a shared key).Test Plan
CryptoUnitTest— key gen, Noise symmetric core (HKDF/AEAD), full NK handshake + independent known-answer vector, SecureSession round-trip/tamper/replaySecurityFunctionsTest— encrypted-connect happy path (payload round-trip) + wrong-pinned-key rejected + keyless-server rejectedKnown follow-ups (non-blocking)
Summary by CodeRabbit
New Features
Breaking Changes
Documentation
Build
Tests & Samples