shill: openvpn: Assume user/pass client auth if no cert set or username is set.
BUG=chromium-os:33096
TEST=unit tests, tested on device
Change-Id: Ib77d99331efac93c2570fa9661f1ddd1bf0e64d3
Reviewed-on: https://gerrit.chromium.org/gerrit/34186
Tested-by: Darin Petkov <petkov@chromium.org>
Reviewed-by: Paul Stewart <pstew@chromium.org>
Commit-Ready: Darin Petkov <petkov@chromium.org>
diff --git a/openvpn_driver.cc b/openvpn_driver.cc
index 59c0500..aa86f8d 100644
--- a/openvpn_driver.cc
+++ b/openvpn_driver.cc
@@ -51,8 +51,6 @@
const char kDefaultPKCS11Provider[] = "libchaps.so";
// TODO(petkov): Move to chromeos/dbus/service_constants.h.
-const char kOpenVPNCertProperty[] = "OpenVPN.Cert";
-const char kOpenVPNKeyProperty[] = "OpenVPN.Key";
const char kOpenVPNPingProperty[] = "OpenVPN.Ping";
const char kOpenVPNPingExitProperty[] = "OpenVPN.PingExit";
const char kOpenVPNPingRestartProperty[] = "OpenVPN.PingRestart";
@@ -62,6 +60,9 @@
} // namespace
+// TODO(petkov): Move to chromeos/dbus/service_constants.h.
+const char OpenVPNDriver::kOpenVPNCertProperty[] = "OpenVPN.Cert";
+const char OpenVPNDriver::kOpenVPNKeyProperty[] = "OpenVPN.Key";
// static
const char OpenVPNDriver::kDefaultCACertificatesPath[] = "/etc/ssl/certs";
// static
@@ -560,11 +561,10 @@
AppendValueOption(kOpenVPNPingExitProperty, "--ping-exit", options);
AppendValueOption(kOpenVPNPingRestartProperty, "--ping-restart", options);
- AppendValueOption(kOpenVPNCertProperty, "--cert", options);
AppendValueOption(
flimflam::kOpenVPNNsCertTypeProperty, "--ns-cert-type", options);
- AppendValueOption(kOpenVPNKeyProperty, "--key", options);
+ InitClientAuthOptions(options);
InitPKCS11Options(options);
// TLS suport.
@@ -674,6 +674,19 @@
}
}
+void OpenVPNDriver::InitClientAuthOptions(vector<string> *options) {
+ bool has_cert = AppendValueOption(kOpenVPNCertProperty, "--cert", options);
+ bool has_key = AppendValueOption(kOpenVPNKeyProperty, "--key", options);
+ // If the AuthUserPass property is set, or the User property is non-empty, or
+ // there's neither a key, nor a cert available, specify user-password client
+ // authentication.
+ if (args()->ContainsString(flimflam::kOpenVPNAuthUserPassProperty) ||
+ !args()->LookupString(flimflam::kOpenVPNUserProperty, "").empty() ||
+ (!has_cert && !has_key)) {
+ options->push_back("--auth-user-pass");
+ }
+}
+
bool OpenVPNDriver::InitManagementChannelOptions(
vector<string> *options, Error *error) {
if (!management_server_->Start(dispatcher(), &sockets_, options)) {
diff --git a/openvpn_driver.h b/openvpn_driver.h
index 6f788d4..a84f76a 100644
--- a/openvpn_driver.h
+++ b/openvpn_driver.h
@@ -86,6 +86,7 @@
FRIEND_TEST(OpenVPNDriverTest, Disconnect);
FRIEND_TEST(OpenVPNDriverTest, GetRouteOptionEntry);
FRIEND_TEST(OpenVPNDriverTest, InitCAOptions);
+ FRIEND_TEST(OpenVPNDriverTest, InitClientAuthOptions);
FRIEND_TEST(OpenVPNDriverTest, InitEnvironment);
FRIEND_TEST(OpenVPNDriverTest, InitLoggingOptions);
FRIEND_TEST(OpenVPNDriverTest, InitManagementChannelOptions);
@@ -109,6 +110,9 @@
FRIEND_TEST(OpenVPNDriverTest, SplitPortFromHost);
FRIEND_TEST(OpenVPNDriverTest, VerifyPaths);
+ static const char kOpenVPNCertProperty[];
+ static const char kOpenVPNKeyProperty[];
+
static const char kDefaultCACertificatesPath[];
static const char kOpenVPNPath[];
@@ -149,6 +153,7 @@
void InitOptions(std::vector<std::string> *options, Error *error);
bool InitCAOptions(std::vector<std::string> *options, Error *error);
+ void InitClientAuthOptions(std::vector<std::string> *options);
void InitPKCS11Options(std::vector<std::string> *options);
bool InitManagementChannelOptions(
std::vector<std::string> *options, Error *error);
diff --git a/openvpn_driver_unittest.cc b/openvpn_driver_unittest.cc
index b86faf4..5067979 100644
--- a/openvpn_driver_unittest.cc
+++ b/openvpn_driver_unittest.cc
@@ -107,9 +107,15 @@
return driver_->args();
}
+ void RemoveStringArg(const string &arg) {
+ driver_->args()->RemoveString(arg);
+ }
+
// Used to assert that a flag appears in the options.
void ExpectInFlags(const vector<string> &options, const string &flag,
const string &value);
+ void ExpectInFlags(const vector<string> &options, const string &flag);
+ void ExpectNotInFlags(const vector<string> &options, const string &flag);
void SetupLSBRelease();
@@ -171,6 +177,18 @@
EXPECT_EQ(value, *it);
}
+void OpenVPNDriverTest::ExpectInFlags(const vector<string> &options,
+ const string &flag) {
+ EXPECT_TRUE(std::find(options.begin(), options.end(), flag) !=
+ options.end());
+}
+
+void OpenVPNDriverTest::ExpectNotInFlags(const vector<string> &options,
+ const string &flag) {
+ EXPECT_TRUE(std::find(options.begin(), options.end(), flag) ==
+ options.end());
+}
+
void OpenVPNDriverTest::SetupLSBRelease() {
static const char kLSBReleaseContents[] =
"\n"
@@ -477,8 +495,8 @@
EXPECT_EQ(kTLSAuthContents, contents);
ExpectInFlags(options, "--pkcs11-id", kID);
ExpectInFlags(options, "--capath", OpenVPNDriver::kDefaultCACertificatesPath);
- EXPECT_TRUE(std::find(options.begin(), options.end(), "--syslog") !=
- options.end());
+ ExpectInFlags(options, "--syslog");
+ ExpectInFlags(options, "--auth-user-pass");
}
TEST_F(OpenVPNDriverTest, InitOptionsHostWithPort) {
@@ -547,6 +565,47 @@
EXPECT_TRUE(error.IsSuccess());
}
+TEST_F(OpenVPNDriverTest, InitClientAuthOptions) {
+ static const char kTestValue[] = "foo";
+ vector<string> options;
+
+ // No key or cert, assume user/password authentication.
+ driver_->InitClientAuthOptions(&options);
+ ExpectInFlags(options, "--auth-user-pass");
+ ExpectNotInFlags(options, "--key");
+ ExpectNotInFlags(options, "--cert");
+
+ // Cert available, no user/password.
+ options.clear();
+ SetArg(OpenVPNDriver::kOpenVPNCertProperty, kTestValue);
+ driver_->InitClientAuthOptions(&options);
+ ExpectNotInFlags(options, "--auth-user-pass");
+ ExpectNotInFlags(options, "--key");
+ ExpectInFlags(options, "--cert", kTestValue);
+
+ // Key available, no user/password.
+ options.clear();
+ SetArg(OpenVPNDriver::kOpenVPNKeyProperty, kTestValue);
+ driver_->InitClientAuthOptions(&options);
+ ExpectNotInFlags(options, "--auth-user-pass");
+ ExpectInFlags(options, "--key", kTestValue);
+
+ // Key available, AuthUserPass set.
+ options.clear();
+ SetArg(flimflam::kOpenVPNAuthUserPassProperty, kTestValue);
+ driver_->InitClientAuthOptions(&options);
+ ExpectInFlags(options, "--auth-user-pass");
+ ExpectInFlags(options, "--key", kTestValue);
+
+ // Key available, User set.
+ options.clear();
+ RemoveStringArg(flimflam::kOpenVPNAuthUserPassProperty);
+ SetArg(flimflam::kOpenVPNUserProperty, "user");
+ driver_->InitClientAuthOptions(&options);
+ ExpectInFlags(options, "--auth-user-pass");
+ ExpectInFlags(options, "--key", kTestValue);
+}
+
TEST_F(OpenVPNDriverTest, InitPKCS11Options) {
vector<string> options;
driver_->InitPKCS11Options(&options);
diff --git a/openvpn_management_server.cc b/openvpn_management_server.cc
index 9349cfb..32ac8a2 100644
--- a/openvpn_management_server.cc
+++ b/openvpn_management_server.cc
@@ -97,8 +97,6 @@
hold_waiting_ = false;
options->push_back("--management-query-passwords");
- driver_->AppendFlag(
- flimflam::kOpenVPNAuthUserPassProperty, "--auth-user-pass", options);
if (driver_->AppendValueOption(flimflam::kOpenVPNStaticChallengeProperty,
"--static-challenge",
options)) {