Skip to content

Commit aff4ad7

Browse files
jshortjshort
committed
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
1 parent 91099f6 commit aff4ad7

File tree

7 files changed

+145
-62
lines changed

7 files changed

+145
-62
lines changed

‎CMakeLists.txt

+2
Original file line numberDiff line numberDiff line change
@@ -384,6 +384,8 @@ add_library(
384384
src/base/WinsockContext.hpp
385385
src/base/SubprocessToString.hpp
386386
src/base/SubprocessToString.cpp
387+
src/base/TunnelUtils.hpp
388+
src/base/TunnelUtils.cpp
387389

388390
${ET_HDRS}
389391
${ET_SRCS}

‎codecov.yml

+1
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ ignore:
77
- "src/terminal/TelemetryService*"
88
- "src/terminal/PsuedoTerminalConsole.hpp"
99
- "src/terminal/PsuedoUserTerminal.hpp"
10+
- "src/terminal/*Main.cpp"
1011
- "src/base/TcpSocketHandler*"
1112
- "src/base/SubprocessToString*"
1213
coverage:

‎src/base/TunnelUtils.cpp

+66
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
#include "TunnelUtils.hpp"
2+
3+
namespace et {
4+
vector<PortForwardSourceRequest> parseRangesToRequests(const string& input) {
5+
vector<PortForwardSourceRequest> pfsrs;
6+
auto j = split(input, ',');
7+
for (auto& pair : j) {
8+
vector<string> sourceDestination = split(pair, ':');
9+
if (sourceDestination.size() < 2) {
10+
throw TunnelParseException(
11+
"Tunnel argument must have source and destination between a ':'");
12+
}
13+
try {
14+
if (sourceDestination[0].find_first_not_of("0123456789-") !=
15+
string::npos &&
16+
sourceDestination[1].find_first_not_of("0123456789-") !=
17+
string::npos) {
18+
PortForwardSourceRequest pfsr;
19+
pfsr.set_environmentvariable(sourceDestination[0]);
20+
pfsr.mutable_destination()->set_name(sourceDestination[1]);
21+
pfsrs.push_back(pfsr);
22+
} else if (sourceDestination[0].find('-') != string::npos &&
23+
sourceDestination[1].find('-') != string::npos) {
24+
vector<string> sourcePortRange = split(sourceDestination[0], '-');
25+
int sourcePortStart = stoi(sourcePortRange[0]);
26+
int sourcePortEnd = stoi(sourcePortRange[1]);
27+
28+
vector<string> destinationPortRange = split(sourceDestination[1], '-');
29+
int destinationPortStart = stoi(destinationPortRange[0]);
30+
int destinationPortEnd = stoi(destinationPortRange[1]);
31+
32+
if (sourcePortEnd - sourcePortStart !=
33+
destinationPortEnd - destinationPortStart) {
34+
throw TunnelParseException(
35+
"source/destination port range must have same length");
36+
} else {
37+
int portRangeLength = sourcePortEnd - sourcePortStart + 1;
38+
for (int i = 0; i < portRangeLength; ++i) {
39+
PortForwardSourceRequest pfsr;
40+
pfsr.mutable_source()->set_port(sourcePortStart + i);
41+
pfsr.mutable_destination()->set_port(destinationPortStart + i);
42+
pfsrs.push_back(pfsr);
43+
}
44+
}
45+
} else if (sourceDestination[0].find('-') != string::npos ||
46+
sourceDestination[1].find('-') != string::npos) {
47+
throw TunnelParseException(
48+
"Invalid port range syntax: if source is a range, "
49+
"destination must be a range (and vice versa)");
50+
} else {
51+
PortForwardSourceRequest pfsr;
52+
pfsr.mutable_source()->set_port(stoi(sourceDestination[0]));
53+
pfsr.mutable_destination()->set_port(stoi(sourceDestination[1]));
54+
pfsrs.push_back(pfsr);
55+
}
56+
} catch (const TunnelParseException& e) {
57+
throw e;
58+
} catch (const std::logic_error& lr) {
59+
throw TunnelParseException("Invalid tunnel argument '" + input +
60+
"': " + lr.what());
61+
}
62+
}
63+
return pfsrs;
64+
}
65+
66+
} // namespace et

‎src/base/TunnelUtils.hpp

+20
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
#ifndef __ET_TUNNEL_UTILS__
2+
#define __ET_TUNNEL_UTILS__
3+
4+
#include "ETerminal.pb.h"
5+
6+
namespace et {
7+
8+
vector<PortForwardSourceRequest> parseRangesToRequests(const string& input);
9+
10+
class TunnelParseException : public std::exception {
11+
public:
12+
TunnelParseException(const string& msg) : message(msg) {}
13+
const char* what() const noexcept override { return message.c_str(); }
14+
15+
private:
16+
std::string message = " ";
17+
};
18+
19+
} // namespace et
20+
#endif // __ET_TUNNEL_UTILS__

‎src/terminal/TerminalClient.cpp

+1-54
Original file line numberDiff line numberDiff line change
@@ -1,62 +1,9 @@
11
#include "TerminalClient.hpp"
22

33
#include "TelemetryService.hpp"
4+
#include "TunnelUtils.hpp"
45

56
namespace et {
6-
vector<PortForwardSourceRequest> parseRangesToRequests(const string& input) {
7-
vector<PortForwardSourceRequest> pfsrs;
8-
auto j = split(input, ',');
9-
for (auto& pair : j) {
10-
vector<string> sourceDestination = split(pair, ':');
11-
try {
12-
if (sourceDestination[0].find_first_not_of("0123456789-") !=
13-
string::npos &&
14-
sourceDestination[1].find_first_not_of("0123456789-") !=
15-
string::npos) {
16-
PortForwardSourceRequest pfsr;
17-
pfsr.set_environmentvariable(sourceDestination[0]);
18-
pfsr.mutable_destination()->set_name(sourceDestination[1]);
19-
pfsrs.push_back(pfsr);
20-
} else if (sourceDestination[0].find('-') != string::npos &&
21-
sourceDestination[1].find('-') != string::npos) {
22-
vector<string> sourcePortRange = split(sourceDestination[0], '-');
23-
int sourcePortStart = stoi(sourcePortRange[0]);
24-
int sourcePortEnd = stoi(sourcePortRange[1]);
25-
26-
vector<string> destinationPortRange = split(sourceDestination[1], '-');
27-
int destinationPortStart = stoi(destinationPortRange[0]);
28-
int destinationPortEnd = stoi(destinationPortRange[1]);
29-
30-
if (sourcePortEnd - sourcePortStart !=
31-
destinationPortEnd - destinationPortStart) {
32-
STFATAL << "source/destination port range mismatch";
33-
exit(1);
34-
} else {
35-
int portRangeLength = sourcePortEnd - sourcePortStart + 1;
36-
for (int i = 0; i < portRangeLength; ++i) {
37-
PortForwardSourceRequest pfsr;
38-
pfsr.mutable_source()->set_port(sourcePortStart + i);
39-
pfsr.mutable_destination()->set_port(destinationPortStart + i);
40-
pfsrs.push_back(pfsr);
41-
}
42-
}
43-
} else if (sourceDestination[0].find('-') != string::npos ||
44-
sourceDestination[1].find('-') != string::npos) {
45-
STFATAL << "Invalid port range syntax: if source is range, "
46-
"destination must be range";
47-
} else {
48-
PortForwardSourceRequest pfsr;
49-
pfsr.mutable_source()->set_port(stoi(sourceDestination[0]));
50-
pfsr.mutable_destination()->set_port(stoi(sourceDestination[1]));
51-
pfsrs.push_back(pfsr);
52-
}
53-
} catch (const std::logic_error& lr) {
54-
STFATAL << "Logic error: " << lr.what();
55-
exit(1);
56-
}
57-
}
58-
return pfsrs;
59-
}
607

618
TerminalClient::TerminalClient(
629
shared_ptr<SocketHandler> _socketHandler,

‎src/terminal/TerminalClientMain.cpp

+19-8
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#include "PsuedoTerminalConsole.hpp"
77
#include "TelemetryService.hpp"
88
#include "TerminalClient.hpp"
9+
#include "TunnelUtils.hpp"
910
#include "WinsockContext.hpp"
1011

1112
using namespace et;
@@ -22,6 +23,12 @@ bool ping(SocketEndpoint socketEndpoint,
2223
return true;
2324
}
2425

26+
void handleParseException(std::exception& e, cxxopts::Options& options) {
27+
CLOG(INFO, "stdout") << "Exception: " << e.what() << "\n" << endl;
28+
CLOG(INFO, "stdout") << options.help({}) << endl;
29+
exit(1);
30+
}
31+
2532
int main(int argc, char** argv) {
2633
WinsockContext context;
2734
string tmpDir = GetTempDirectory();
@@ -340,17 +347,21 @@ int main(int argc, char** argv) {
340347
}
341348
TelemetryService::get()->logToDatadog("Session Started", el::Level::Info,
342349
__FILE__, __LINE__);
343-
TerminalClient terminalClient(
344-
clientSocket, clientPipeSocket, socketEndpoint, id, passkey, console,
345-
is_jumphost, result.count("t") ? result["t"].as<string>() : "",
346-
result.count("r") ? result["r"].as<string>() : "", forwardAgent,
347-
sshSocket, keepaliveDuration);
350+
string tunnel_arg =
351+
result.count("tunnel") ? result["tunnel"].as<string>() : "";
352+
string r_tunnel_arg = result.count("reversetunnel")
353+
? result["reversetunnel"].as<string>()
354+
: "";
355+
TerminalClient terminalClient(clientSocket, clientPipeSocket,
356+
socketEndpoint, id, passkey, console,
357+
is_jumphost, tunnel_arg, r_tunnel_arg,
358+
forwardAgent, sshSocket, keepaliveDuration);
348359
terminalClient.run(result.count("command") ? result["command"].as<string>()
349360
: "");
361+
} catch (TunnelParseException& tpe) {
362+
handleParseException(tpe, options);
350363
} catch (cxxopts::OptionException& oe) {
351-
CLOG(INFO, "stdout") << "Exception: " << oe.what() << "\n" << endl;
352-
CLOG(INFO, "stdout") << options.help({}) << endl;
353-
exit(1);
364+
handleParseException(oe, options);
354365
}
355366

356367
#ifdef WIN32

‎test/TerminalTest.cpp

+36
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
#include "TerminalClient.hpp"
55
#include "TerminalServer.hpp"
66
#include "TestHeaders.hpp"
7+
#include "TunnelUtils.hpp"
78

89
namespace et {
910
TEST_CASE("FakeConsoleTest", "[FakeConsoleTest]") {
@@ -255,6 +256,41 @@ class EndToEndTestFixture {
255256
bool wasShutdown = false;
256257
};
257258

259+
TEST_CASE("InvalidTunnelArgParsing", "[InvalidTunnelArgParsing]") {
260+
REQUIRE_THROWS_WITH(
261+
parseRangesToRequests("6010"),
262+
Catch::Matchers::Contains("must have source and destination"));
263+
REQUIRE_THROWS_WITH(parseRangesToRequests("6010-6012:7000"),
264+
Catch::Matchers::Contains("must be a range"));
265+
REQUIRE_THROWS_WITH(parseRangesToRequests("6010:7000-7010"),
266+
Catch::Matchers::Contains("must be a range"));
267+
REQUIRE_THROWS_WITH(parseRangesToRequests("6010-6012:7000-8000"),
268+
Catch::Matchers::Contains("must have same length"));
269+
}
270+
271+
TEST_CASE("ValidTunnelArgParsing", "[ValidTunnelArgParsing]") {
272+
// Plain port1:port2 forward
273+
auto pfsrs_single = parseRangesToRequests("6010:7010");
274+
REQUIRE(pfsrs_single.size() == 1);
275+
REQUIRE(pfsrs_single[0].has_source());
276+
REQUIRE(pfsrs_single[0].has_destination());
277+
REQUIRE((pfsrs_single[0].source().has_port() &&
278+
pfsrs_single[0].source().port() == 6010));
279+
REQUIRE((pfsrs_single[0].destination().has_port() &&
280+
pfsrs_single[0].destination().port() == 7010));
281+
282+
// range src_port1-src_port2:dest_port1-dest_port2 forward
283+
auto pfsrs_ranges = parseRangesToRequests("6010-6013:7010-7013");
284+
REQUIRE(pfsrs_ranges.size() == 4);
285+
286+
// named pipe forward
287+
auto pfsrs_named = parseRangesToRequests("envvar:/tmp/destination");
288+
REQUIRE(pfsrs_named.size() == 1);
289+
REQUIRE(!pfsrs_named[0].has_source());
290+
REQUIRE(pfsrs_named[0].has_destination());
291+
REQUIRE(pfsrs_named[0].has_environmentvariable());
292+
}
293+
258294
TEST_CASE_METHOD(EndToEndTestFixture, "EndToEndTest", "[EndToEndTest]") {
259295
readWriteTest("1234567890123456", routerSocketHandler, fakeUserTerminal,
260296
serverEndpoint, clientSocketHandler, clientPipeSocketHandler,

0 commit comments

Comments
 (0)