shill: OpenVPNDriver: Perform extra certificate validation
Support the OpenVPN.ExtraCertPEM, OpenVPN.VerifyHash and
OpenVPN.VerifyX509Name properties, passing the appropriate
arguments to the OpenVPN startup.
CQ-DEPEND=CL:171101,CL:171091
BUG=chromium:301830
TEST=Unit tests; upcoming auto-tests for verify flags
Change-Id: I25e46681638b129bcb55ae93b7a9244f134742cf
Reviewed-on: https://chromium-review.googlesource.com/171102
Reviewed-by: Ben Chan <benchan@chromium.org>
Commit-Queue: Paul Stewart <pstew@chromium.org>
Tested-by: Paul Stewart <pstew@chromium.org>
diff --git a/doc/service-api.txt b/doc/service-api.txt
index 1a83f91..8a86144 100644
--- a/doc/service-api.txt
+++ b/doc/service-api.txt
@@ -736,14 +736,13 @@
array{string} OpenVPN.ExtraCertPEM [writeonly]
- (VPN services of type OpenVPN only) The list of
- x509 CA certificates to be used to complete the
- certificate authentication chain from the CA
- certificates. None of these certificates can directly
- sign the server certificate. Each x509 CA certificate
- must be in in PEM format; specifically
- the base64 counterpart of the DER contents, optionally
- surrounded by a "-----BEGIN CERTIFICATE-----" and
+ (VPN services of type OpenVPN only) A list of
+ additonal x509 CA certificates to be used to complete
+ the certificate authentication chain from the CA
+ certificates. Each x509 CA certificate must be in in
+ PEM format; specifically the base64 counterpart of
+ the DER contents, optionally surrounded by a
+ "-----BEGIN CERTIFICATE-----" and
"-----END CERTIFICATE-----" line. The current
value of this property is readable in the "Provider"
property of this service.
@@ -967,6 +966,38 @@
The current value of this property is readable in the
"Provider" property of this service.
+ string OpenVPN.VerifyHash [writeonly]
+
+ (VPN services of type OpenVPN only) If non-empty,
+ passes this as the "--verify-hash" argument to OpenVPN,
+ which specifies the SHA1 fingerprint for level-1
+ certificate. The current value of this property is
+ readable in the "Provider" property of this service.
+
+ string OpenVPN.VerifyX509Name [writeonly]
+
+ (VPN services of type OpenVPN only) If non-empty,
+ passes this as the "--verify-x509-name" argument to
+ OpenVPN, which specifies the X509 subject distinguished
+ name we mandate the remote VPN server to have. The
+ current value of this property is readable in the
+ "Provider" property of this service.
+
+ string OpenVPN.VerifyX509Type [writeonly]
+
+ (VPN services of type OpenVPN only) If non-empty,
+ this string is passed as a second parameter to the
+ "--verify-x509-name" flag sent to OpenVPN, which
+ qualifies the type of parameter specified in the
+ "OpenVPN.VerifyX509Name" property of this service.
+ If the "OpenVPN.VerifyX509Name" property is unset or
+ empty, setting this property has no effect during
+ connection. Please see the documentation of the
+ "--verify-x509-name" flag in the OpenVPN documentation
+ to better understand how these two parameters interact.
+ The current value of this property is readable in the
+ "Provider" property of this service.
+
string Passphrase [readwrite]
If the service type is "wifi", then this property
diff --git a/openvpn_driver.cc b/openvpn_driver.cc
index 2deeb78..6e35063 100644
--- a/openvpn_driver.cc
+++ b/openvpn_driver.cc
@@ -110,6 +110,9 @@
{ kOpenVPNPingRestartProperty, 0 },
{ kOpenVPNTLSAuthProperty, 0 },
{ kOpenVPNVerbProperty, 0 },
+ { kOpenVPNVerifyHashProperty, 0 },
+ { kOpenVPNVerifyX509NameProperty, 0 },
+ { kOpenVPNVerifyX509TypeProperty, 0 },
{ kVPNMTUProperty, 0 },
};
@@ -140,6 +143,7 @@
management_server_(new OpenVPNManagementServer(this, glib)),
nss_(NSS::GetInstance()),
certificate_file_(new CertificateFile()),
+ extra_certificates_file_(new CertificateFile()),
process_killer_(ProcessKiller::GetInstance()),
lsb_release_file_(kLSBReleaseFile),
openvpn_config_directory_(kDefaultOpenVPNConfigurationDirectory),
@@ -637,6 +641,12 @@
return;
}
+ // Additional remote certificate verification options.
+ InitCertificateVerifyOptions(options);
+ if (!InitExtraCertOptions(options, error)) {
+ return;
+ }
+
// Client-side ping support.
AppendValueOption(kOpenVPNPingProperty, "ping", options);
AppendValueOption(kOpenVPNPingExitProperty, "ping-exit", options);
@@ -751,6 +761,47 @@
return true;
}
+void OpenVPNDriver::InitCertificateVerifyOptions(
+ std::vector<std::vector<std::string>> *options) {
+ AppendValueOption(kOpenVPNVerifyHashProperty, "verify-hash", options);
+ string x509_name = args()->LookupString(kOpenVPNVerifyX509NameProperty, "");
+ if (!x509_name.empty()) {
+ string x509_type = args()->LookupString(kOpenVPNVerifyX509TypeProperty, "");
+ if (x509_type.empty()) {
+ AppendOption("verify-x509-name", x509_name, options);
+ } else {
+ AppendOption("verify-x509-name", x509_name, x509_type, options);
+ }
+ }
+}
+
+bool OpenVPNDriver::InitExtraCertOptions(
+ vector<vector<string>> *options, Error *error) {
+ if (!args()->ContainsStrings(kOpenVPNExtraCertPemProperty)) {
+ // It's okay for this parameter to be unspecified.
+ return true;
+ }
+
+ vector<string> extra_certs = args()->GetStrings(kOpenVPNExtraCertPemProperty);
+ if (extra_certs.empty()) {
+ // It's okay for this parameter to be empty.
+ return true;
+ }
+
+ FilePath certfile =
+ extra_certificates_file_->CreatePEMFromStrings(extra_certs);
+ if (certfile.empty()) {
+ Error::PopulateAndLog(
+ error,
+ Error::kInvalidArguments,
+ "Unable to extract extra PEM CA certificates.");
+ return false;
+ }
+
+ AppendOption("extra-certs", certfile.value(), options);
+ return true;
+}
+
void OpenVPNDriver::InitPKCS11Options(vector<vector<string>> *options) {
string id = args()->LookupString(kOpenVPNClientCertIdProperty, "");
if (!id.empty()) {
diff --git a/openvpn_driver.h b/openvpn_driver.h
index b544225..0e2c93e 100644
--- a/openvpn_driver.h
+++ b/openvpn_driver.h
@@ -120,8 +120,10 @@
FRIEND_TEST(OpenVPNDriverTest, Disconnect);
FRIEND_TEST(OpenVPNDriverTest, GetRouteOptionEntry);
FRIEND_TEST(OpenVPNDriverTest, InitCAOptions);
+ FRIEND_TEST(OpenVPNDriverTest, InitCertificateVerifyOptions);
FRIEND_TEST(OpenVPNDriverTest, InitClientAuthOptions);
FRIEND_TEST(OpenVPNDriverTest, InitEnvironment);
+ FRIEND_TEST(OpenVPNDriverTest, InitExtraCertOptions);
FRIEND_TEST(OpenVPNDriverTest, InitLoggingOptions);
FRIEND_TEST(OpenVPNDriverTest, InitOptions);
FRIEND_TEST(OpenVPNDriverTest, InitOptionsHostWithPort);
@@ -190,7 +192,11 @@
std::vector<std::vector<std::string>> *options, Error *error);
bool InitCAOptions(
std::vector<std::vector<std::string>> *options, Error *error);
+ void InitCertificateVerifyOptions(
+ std::vector<std::vector<std::string>> *options);
void InitClientAuthOptions(std::vector<std::vector<std::string>> *options);
+ bool InitExtraCertOptions(
+ std::vector<std::vector<std::string>> *options, Error *error);
void InitPKCS11Options(std::vector<std::vector<std::string>> *options);
bool InitManagementChannelOptions(
std::vector<std::vector<std::string>> *options, Error *error);
@@ -248,6 +254,7 @@
scoped_ptr<OpenVPNManagementServer> management_server_;
NSS *nss_;
scoped_ptr<CertificateFile> certificate_file_;
+ scoped_ptr<CertificateFile> extra_certificates_file_;
ProcessKiller *process_killer_;
base::FilePath lsb_release_file_;
diff --git a/openvpn_driver_unittest.cc b/openvpn_driver_unittest.cc
index a47cf58..ee9a204 100644
--- a/openvpn_driver_unittest.cc
+++ b/openvpn_driver_unittest.cc
@@ -71,10 +71,13 @@
&control_, &dispatcher_, &metrics_, &manager_,
kInterfaceName, kInterfaceIndex, Technology::kVPN)),
certificate_file_(new MockCertificateFile()),
+ extra_certificates_file_(new MockCertificateFile()),
management_server_(new NiceMock<MockOpenVPNManagementServer>()) {
driver_->management_server_.reset(management_server_);
driver_->nss_ = &nss_;
driver_->certificate_file_.reset(certificate_file_); // Passes ownership.
+ driver_->extra_certificates_file_.reset(
+ extra_certificates_file_); // Passes ownership.
driver_->process_killer_ = &process_killer_;
temporary_directory_.CreateUniqueTempDir();
driver_->openvpn_config_directory_ =
@@ -214,6 +217,7 @@
scoped_refptr<MockVPNService> service_;
scoped_refptr<MockVirtualDevice> device_;
MockCertificateFile *certificate_file_; // Owned by |driver_|.
+ MockCertificateFile *extra_certificates_file_; // Owned by |driver_|.
MockNSS nss_;
MockProcessKiller process_killer_;
base::ScopedTempDir temporary_directory_;
@@ -699,6 +703,44 @@
EXPECT_TRUE(error.IsSuccess());
}
+TEST_F(OpenVPNDriverTest, InitCertificateVerifyOptions) {
+ {
+ Error error;
+ vector<vector<string>> options;
+ // No options supplied.
+ driver_->InitCertificateVerifyOptions(&options);
+ EXPECT_TRUE(options.empty());
+ }
+ const char kName[] = "x509-name";
+ {
+ Error error;
+ vector<vector<string>> options;
+ // With Name property alone, we should have the 1-parameter version of the
+ // "x509-verify-name" parameter provided.
+ SetArg(kOpenVPNVerifyX509NameProperty, kName);
+ driver_->InitCertificateVerifyOptions(&options);
+ ExpectInFlags(options, "verify-x509-name", kName);
+ }
+ const char kType[] = "x509-type";
+ {
+ Error error;
+ vector<vector<string>> options;
+ // With both Name property and Type property set, we should have the
+ // 2-parameter version of the "x509-verify-name" parameter provided.
+ SetArg(kOpenVPNVerifyX509TypeProperty, kType);
+ driver_->InitCertificateVerifyOptions(&options);
+ ExpectInFlags(options, vector<string> { "verify-x509-name", kName, kType });
+ }
+ {
+ Error error;
+ vector<vector<string>> options;
+ // We should ignore the Type parameter if no Name parameter is specified.
+ SetArg(kOpenVPNVerifyX509NameProperty, "");
+ driver_->InitCertificateVerifyOptions(&options);
+ EXPECT_TRUE(options.empty());
+ }
+}
+
TEST_F(OpenVPNDriverTest, InitClientAuthOptions) {
static const char kTestValue[] = "foo";
vector<vector<string>> options;
@@ -779,6 +821,49 @@
ExpectNotInFlags(options, "cert");
}
+TEST_F(OpenVPNDriverTest, InitExtraCertOptions) {
+ {
+ Error error;
+ vector<vector<string>> options;
+ // No ExtraCertOptions supplied.
+ EXPECT_TRUE(driver_->InitExtraCertOptions(&options, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ EXPECT_TRUE(options.empty());
+ }
+ {
+ Error error;
+ vector<vector<string>> options;
+ SetArgArray(kOpenVPNExtraCertPemProperty, vector<string>());
+ // Empty ExtraCertOptions supplied.
+ EXPECT_TRUE(driver_->InitExtraCertOptions(&options, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ EXPECT_TRUE(options.empty());
+ }
+ const vector<string> kExtraCerts{ "---PEM CONTENTS---" };
+ SetArgArray(kOpenVPNExtraCertPemProperty, kExtraCerts);
+ static const char kPEMCertfile[] = "/tmp/pem-cert";
+ FilePath pem_cert(kPEMCertfile);
+ EXPECT_CALL(*extra_certificates_file_, CreatePEMFromStrings(kExtraCerts))
+ .WillOnce(Return(FilePath()))
+ .WillOnce(Return(pem_cert));
+ // CreatePemFromStrings fails.
+ {
+ Error error;
+ vector<vector<string>> options;
+ EXPECT_FALSE(driver_->InitExtraCertOptions(&options, &error));
+ EXPECT_EQ(Error::kInvalidArguments, error.type());
+ EXPECT_TRUE(options.empty());
+ }
+ // CreatePemFromStrings succeeds.
+ {
+ Error error;
+ vector<vector<string>> options;
+ EXPECT_TRUE(driver_->InitExtraCertOptions(&options, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ ExpectInFlags(options, "extra-certs", kPEMCertfile);
+ }
+}
+
TEST_F(OpenVPNDriverTest, InitPKCS11Options) {
vector<vector<string>> options;
driver_->InitPKCS11Options(&options);