From dbca326f9b69d15dad2e176db677ac36e87d4953 Mon Sep 17 00:00:00 2001 From: James Short Date: Fri, 2 Dec 2022 14:58:58 -0800 Subject: [PATCH] Fix tunnel parsing exception handling. Rather than using STFATAL and exiting, bubble up tunnel parsing exception to main and display the responsible option arg and print help menu so the user need not dig into /tmp/etclient-* to determine why et aborted. Fixes #491 --- codecov.yml | 1 + src/terminal/TerminalClient.cpp | 18 ++++++++++++------ src/terminal/TerminalClient.hpp | 9 +++++++++ src/terminal/TerminalClientMain.cpp | 26 ++++++++++++++++++-------- test/TerminalTest.cpp | 16 ++++++++++++++++ 5 files changed, 56 insertions(+), 14 deletions(-) diff --git a/codecov.yml b/codecov.yml index 4ad267e66..e5d3a74b9 100644 --- a/codecov.yml +++ b/codecov.yml @@ -7,6 +7,7 @@ ignore: - "src/terminal/TelemetryService*" - "src/terminal/PsuedoTerminalConsole.hpp" - "src/terminal/PsuedoUserTerminal.hpp" + - "src/terminal/*Main.cpp" - "src/base/TcpSocketHandler*" - "src/base/SubprocessToString*" coverage: diff --git a/src/terminal/TerminalClient.cpp b/src/terminal/TerminalClient.cpp index a809aeae9..29f2df5c3 100644 --- a/src/terminal/TerminalClient.cpp +++ b/src/terminal/TerminalClient.cpp @@ -8,6 +8,10 @@ vector parseRangesToRequests(const string& input) { auto j = split(input, ','); for (auto& pair : j) { vector sourceDestination = split(pair, ':'); + if (sourceDestination.size() < 2) { + throw TunnelParseException( + "Tunnel argument must have source and destination between a ':'"); + } try { if (sourceDestination[0].find_first_not_of("0123456789-") != string::npos && @@ -29,8 +33,8 @@ vector parseRangesToRequests(const string& input) { if (sourcePortEnd - sourcePortStart != destinationPortEnd - destinationPortStart) { - STFATAL << "source/destination port range mismatch"; - exit(1); + throw TunnelParseException( + "source/destination port range must have same length"); } else { int portRangeLength = sourcePortEnd - sourcePortStart + 1; for (int i = 0; i < portRangeLength; ++i) { @@ -42,17 +46,19 @@ vector parseRangesToRequests(const string& input) { } } else if (sourceDestination[0].find('-') != string::npos || sourceDestination[1].find('-') != string::npos) { - STFATAL << "Invalid port range syntax: if source is range, " - "destination must be range"; + throw TunnelParseException( + "Invalid port range syntax: if source is range, " + "destination must be range"); } else { PortForwardSourceRequest pfsr; pfsr.mutable_source()->set_port(stoi(sourceDestination[0])); pfsr.mutable_destination()->set_port(stoi(sourceDestination[1])); pfsrs.push_back(pfsr); } + } catch (const TunnelParseException& e) { + throw e; } catch (const std::logic_error& lr) { - STFATAL << "Logic error: " << lr.what(); - exit(1); + throw TunnelParseException("Invalid tunnel argument: " + input); } } return pfsrs; diff --git a/src/terminal/TerminalClient.hpp b/src/terminal/TerminalClient.hpp index de004cfb7..7270421c4 100644 --- a/src/terminal/TerminalClient.hpp +++ b/src/terminal/TerminalClient.hpp @@ -40,5 +40,14 @@ class TerminalClient { int keepaliveDuration; }; +class TunnelParseException : public std::exception { + public: + TunnelParseException(const string& msg) : message(msg) {} + const char* what() const noexcept override { return message.c_str(); } + + private: + std::string message = " "; +}; + } // namespace et #endif // __ET_TERMINAL_CLIENT__ diff --git a/src/terminal/TerminalClientMain.cpp b/src/terminal/TerminalClientMain.cpp index edb4a4996..78ef49efc 100644 --- a/src/terminal/TerminalClientMain.cpp +++ b/src/terminal/TerminalClientMain.cpp @@ -22,6 +22,12 @@ bool ping(SocketEndpoint socketEndpoint, return true; } +void handleParseException(std::exception& e, cxxopts::Options& options) { + CLOG(INFO, "stdout") << "Exception: " << e.what() << "\n" << endl; + CLOG(INFO, "stdout") << options.help({}) << endl; + exit(1); +} + int main(int argc, char** argv) { WinsockContext context; string tmpDir = GetTempDirectory(); @@ -340,17 +346,21 @@ int main(int argc, char** argv) { } TelemetryService::get()->logToDatadog("Session Started", el::Level::Info, __FILE__, __LINE__); - TerminalClient terminalClient( - clientSocket, clientPipeSocket, socketEndpoint, id, passkey, console, - is_jumphost, result.count("t") ? result["t"].as() : "", - result.count("r") ? result["r"].as() : "", forwardAgent, - sshSocket, keepaliveDuration); + string tunnel_arg = + result.count("tunnel") ? result["tunnel"].as() : ""; + string r_tunnel_arg = result.count("reversetunnel") + ? result["reversetunnel"].as() + : ""; + TerminalClient terminalClient(clientSocket, clientPipeSocket, + socketEndpoint, id, passkey, console, + is_jumphost, tunnel_arg, r_tunnel_arg, + forwardAgent, sshSocket, keepaliveDuration); terminalClient.run(result.count("command") ? result["command"].as() : ""); + } catch (TunnelParseException& tpe) { + handleParseException(tpe, options); } catch (cxxopts::OptionException& oe) { - CLOG(INFO, "stdout") << "Exception: " << oe.what() << "\n" << endl; - CLOG(INFO, "stdout") << options.help({}) << endl; - exit(1); + handleParseException(oe, options); } #ifdef WIN32 diff --git a/test/TerminalTest.cpp b/test/TerminalTest.cpp index b8be12173..3b5c42498 100644 --- a/test/TerminalTest.cpp +++ b/test/TerminalTest.cpp @@ -255,6 +255,22 @@ class EndToEndTestFixture { bool wasShutdown = false; }; +TEST_CASE_METHOD(EndToEndTestFixture, "InvalidTunnelArg", + "[InvalidTunnelArg]") { + REQUIRE_THROWS(new TerminalClient( + clientSocketHandler, clientPipeSocketHandler, serverEndpoint, + "1234567890123456", CRYPTO_KEY, fakeConsole, false, "6010", "", false, "", + MAX_CLIENT_KEEP_ALIVE_DURATION)); + REQUIRE_THROWS(new TerminalClient( + clientSocketHandler, clientPipeSocketHandler, serverEndpoint, + "1234567890123456", CRYPTO_KEY, fakeConsole, false, "6010-6012:7000", "", + false, "", MAX_CLIENT_KEEP_ALIVE_DURATION)); + REQUIRE_THROWS(new TerminalClient( + clientSocketHandler, clientPipeSocketHandler, serverEndpoint, + "1234567890123456", CRYPTO_KEY, fakeConsole, false, "6010-6012:7000-8000", + "", false, "", MAX_CLIENT_KEEP_ALIVE_DURATION)); +} + TEST_CASE_METHOD(EndToEndTestFixture, "EndToEndTest", "[EndToEndTest]") { readWriteTest("1234567890123456", routerSocketHandler, fakeUserTerminal, serverEndpoint, clientSocketHandler, clientPipeSocketHandler,