shill: Base64 encode/decode crypto_util arguments
Initially, I thought this would be easier to do in the shim, where
things might play nicely with OpenSSL. However, it turns out that the
OpenSSL API for base64 encoding/decoding is far more painful that it's
worth. Add a convenient wrapper around the raw GLib calls so that
people have less opportunity to shoot themselves in the foot.
BUG=chromium-os:221168
TEST=Unit tests pass.
Change-Id: I69732af03fdf729519783a3440213ef6adf8a630
Reviewed-on: https://gerrit.chromium.org/gerrit/46049
Tested-by: Christopher Wiley <wiley@chromium.org>
Reviewed-by: Paul Stewart <pstew@chromium.org>
Commit-Queue: Christopher Wiley <wiley@chromium.org>
diff --git a/crypto_util_proxy.cc b/crypto_util_proxy.cc
index 060a4bb..0727345 100644
--- a/crypto_util_proxy.cc
+++ b/crypto_util_proxy.cc
@@ -12,8 +12,9 @@
#include <base/stringprintf.h>
#include "shill/event_dispatcher.h"
-#include "shill/process_killer.h"
#include "shill/file_io.h"
+#include "shill/glib.h"
+#include "shill/process_killer.h"
using base::Bind;
using base::Callback;
@@ -35,8 +36,9 @@
const char CryptoUtilProxy::kDestinationVerificationUser[] = "chronos";
const int CryptoUtilProxy::kShimJobTimeoutMilliseconds = 30 * 1000;
-CryptoUtilProxy::CryptoUtilProxy(EventDispatcher *dispatcher)
+CryptoUtilProxy::CryptoUtilProxy(EventDispatcher *dispatcher, GLib *glib)
: dispatcher_(dispatcher),
+ glib_(glib),
minijail_(Minijail::GetInstance()),
process_killer_(ProcessKiller::GetInstance()),
file_io_(FileIO::GetInstance()),
@@ -63,7 +65,6 @@
const string &bssid,
const ResultBoolCallback &result_callback,
Error *error) {
- // TODO(wiley) Check that the MAC address looks right.
string unsigned_data(reinterpret_cast<const char *>(&ssid[0]),
ssid.size());
unsigned_data.append(StringPrintf(",%s,%s,%s,%s",
@@ -71,9 +72,16 @@
bssid.c_str(),
public_key.c_str(),
nonce.c_str()));
+ string decoded_signed_data;
+ if (!glib_->B64Decode(signed_data, &decoded_signed_data)) {
+ Error::PopulateAndLog(error, Error::kOperationFailed,
+ "Failed to decode signed data.");
+ return false;
+ }
+
VerifyCredentialsMessage message;
message.set_certificate(certificate);
- message.set_signed_data(signed_data);
+ message.set_signed_data(decoded_signed_data);
message.set_unsigned_data(unsigned_data);
message.set_mac_address(bssid);
@@ -101,10 +109,16 @@
const string &data,
const ResultStringCallback &result_callback,
Error *error) {
- EncryptDataMessage message;
- message.set_public_key(public_key);
- message.set_data(data);
+ string decoded_public_key;
+ if (!glib_->B64Decode(public_key, &decoded_public_key)) {
+ Error::PopulateAndLog(error, Error::kOperationFailed,
+ "Unable to decode public key.");
+ return false;
+ }
+ EncryptDataMessage message;
+ message.set_public_key(decoded_public_key);
+ message.set_data(data);
string raw_bytes;
if (!message.SerializeToString(&raw_bytes)) {
Error::PopulateAndLog(error, Error::kOperationFailed,
@@ -350,7 +364,14 @@
return;
}
- result_handler.Run(e, response.encrypted_data());
+ string encoded_data;
+ if (!glib_->B64Encode(response.encrypted_data(), &encoded_data)) {
+ e.Populate(Error::kInternalError, "Failed to encode result.");
+ result_handler.Run(e, "");
+ return;
+ }
+
+ result_handler.Run(e, encoded_data);
}
} // namespace shill
diff --git a/crypto_util_proxy.h b/crypto_util_proxy.h
index 59b517e..e894766 100644
--- a/crypto_util_proxy.h
+++ b/crypto_util_proxy.h
@@ -24,6 +24,7 @@
class EventDispatcher;
class FileIO;
+class GLib;
class ProcessKiller;
class CryptoUtilProxy : public base::SupportsWeakPtr<CryptoUtilProxy> {
@@ -32,7 +33,7 @@
static const char kCommandEncrypt[];
static const char kCryptoUtilShimPath[];
- CryptoUtilProxy(EventDispatcher *dispatcher);
+ CryptoUtilProxy(EventDispatcher *dispatcher, GLib *glib);
virtual ~CryptoUtilProxy();
// Verify credentials for the currently connected endpoint of
@@ -40,6 +41,14 @@
// Returns true if we've succeeded in kicking off a job to an external shim
// to verify credentials. |result_callback| will be called with the actual
// result of the job, either true, or false with a descriptive error.
+ //
+ // |certificate| should be a device certificate in PEM format.
+ // |public_key| is a base64 encoded DER RSAPublicKey format public key.
+ // |nonce| has no particular format requirements.
+ // |signed_data| is the base64 encoded signed string given by the device.
+ // |destination_udn| has no format requirements.
+ // |ssid| has no constraints.
+ // |bssid| should be in the human readable format: 00:11:22:33:44:55.
virtual bool VerifyDestination(const std::string &certificate,
const std::string &public_key,
const std::string &nonce,
@@ -50,12 +59,14 @@
const ResultBoolCallback &result_callback,
Error *error);
- // Encrypt |data| under |public_key|. |public_key| is expected to be a
- // base64 encode public half of an RSA key. This is a fairly time consuming
+ // Encrypt |data| under |public_key|. This is a fairly time consuming
// process. Returns true if we've succeeded in kicking off a job to an
// external shim to sign the data. |result_callback| will be called with the
// results of the operation: an empty string and a descriptive error or the
// base64 encoded bytes of the encrypted data.
+ //
+ // |public_key| is a base64 encoded DER RSAPublicKey format public key.
+ // |data| has no particular format requirements.
virtual bool EncryptData(const std::string &public_key,
const std::string &data,
const ResultStringCallback &result_callback,
@@ -107,6 +118,7 @@
const Error &error);
EventDispatcher *dispatcher_;
+ GLib *glib_;
Minijail *minijail_;
ProcessKiller *process_killer_;
FileIO *file_io_;
diff --git a/crypto_util_proxy_unittest.cc b/crypto_util_proxy_unittest.cc
index 3689a46..490c3da 100644
--- a/crypto_util_proxy_unittest.cc
+++ b/crypto_util_proxy_unittest.cc
@@ -14,6 +14,7 @@
#include "shill/minijail.h"
#include "shill/mock_crypto_util_proxy.h"
#include "shill/mock_event_dispatcher.h"
+#include "shill/mock_glib.h"
#include "shill/mock_file_io.h"
#include "shill/mock_minijail.h"
#include "shill/mock_process_killer.h"
@@ -39,7 +40,7 @@
static const char kTestData[] = "thisisthetestdata";
static const char kTestDestinationUDN[] = "TEST1234-5678-ABCD";
static const char kTestNonce[] = "abort abort abort";
- static const char kTestPublicKey[] = "testkeygoeshere";
+ static const char kTestPublicKey[] = "YWJvcnQgYWJvcnQgYWJvcnQK";
static const char kTestSSID[] = "SomeDestinationSSID";
static const char kTestSerializedCommandMessage[] =
"Since we're not testing protocol buffer seriallization, and no data "
@@ -82,7 +83,7 @@
class CryptoUtilProxyTest : public testing::Test {
public:
CryptoUtilProxyTest()
- : crypto_util_proxy_(&dispatcher_) {
+ : crypto_util_proxy_(&dispatcher_, &glib_) {
test_ssid_.push_back(78);
test_ssid_.push_back(69);
test_ssid_.push_back(80);
@@ -191,6 +192,7 @@
MockMinijail minijail_;
MockProcessKiller process_killer_;
MockEventDispatcher dispatcher_;
+ MockGLib glib_;
MockFileIO file_io_;
MockCryptoUtilProxy crypto_util_proxy_;
std::vector<uint8_t> test_ssid_;
@@ -204,6 +206,8 @@
VerifyDestination(_, _, _, _, _, _, _, _, _))
.WillOnce(Invoke(&crypto_util_proxy_,
&MockCryptoUtilProxy::RealVerifyDestination));
+ EXPECT_CALL(glib_, B64Decode(StrEq(kTestSignedData), _))
+ .WillOnce(Return(true));
// API calls are just thin wrappers that write up a message to a shim, then
// send it via StartShimForCommand. Expect that a shim will be started in
// response to the API being called.
@@ -232,6 +236,8 @@
EXPECT_CALL(crypto_util_proxy_, EncryptData(_, _, _, _))
.WillOnce(Invoke(&crypto_util_proxy_,
&MockCryptoUtilProxy::RealEncryptData));
+ EXPECT_CALL(glib_, B64Decode(StrEq(kTestPublicKey), _))
+ .WillOnce(Return(true));
EXPECT_CALL(crypto_util_proxy_,
StartShimForCommand(CryptoUtilProxy::kCommandEncrypt, _, _))
.WillOnce(Return(true));
diff --git a/glib.cc b/glib.cc
index 4e99853..d5c0f91 100644
--- a/glib.cc
+++ b/glib.cc
@@ -2,9 +2,15 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+#include "shill/glib.h"
+
+#include <string>
+
#include <base/stringprintf.h>
-#include "shill/glib.h"
+#include "shill/logging.h"
+
+using std::string;
namespace shill {
@@ -28,6 +34,40 @@
return g_base64_encode(data, len);
}
+bool GLib::B64Decode(const string &input, string *output) {
+ CHECK(output);
+ gsize result_len = 0;
+ guchar *result = Base64Decode(input.c_str(), &result_len);
+ if (!result) {
+ LOG(ERROR) << "Failed in encoding.";
+ return false;
+ }
+
+ if (!result_len) {
+ LOG(ERROR) << "Failed in encoding.";
+ Free(result);
+ return false;
+ }
+
+ output->assign(reinterpret_cast<char *>(result), result_len);
+ Free(result);
+ return true;
+}
+
+bool GLib::B64Encode(const string &input, string *output) {
+ CHECK(output);
+ gchar *result = Base64Encode(
+ reinterpret_cast<const unsigned char *>(input.c_str()), input.length());
+ if (!result) {
+ LOG(ERROR) << "Failed in encoding.";
+ return false;
+ }
+
+ output->assign(result);
+ Free(result);
+ return true;
+}
+
void GLib::BusUnwatchName(guint watcher_id) {
g_bus_unwatch_name(watcher_id);
}
diff --git a/glib.h b/glib.h
index b2777bc..6e8e2cd 100644
--- a/glib.h
+++ b/glib.h
@@ -24,6 +24,11 @@
virtual guchar *Base64Decode(const gchar *text, gsize *out_len);
// g_base64_encode
virtual gchar *Base64Encode(const guchar *data, gsize len);
+
+ // Thin wrappers around Base64Decode/Encode. Return true on success.
+ virtual bool B64Decode(const std::string &input, std::string *output);
+ virtual bool B64Encode(const std::string &input, std::string *output);
+
// g_bus_unwatch_name
virtual void BusUnwatchName(guint watcher_id);
// g_bus_watch_name
diff --git a/mock_crypto_util_proxy.h b/mock_crypto_util_proxy.h
index 39e2a84..7f9f3e1 100644
--- a/mock_crypto_util_proxy.h
+++ b/mock_crypto_util_proxy.h
@@ -23,8 +23,8 @@
: public CryptoUtilProxy,
public base::SupportsWeakPtr<MockCryptoUtilProxy> {
public:
- MockCryptoUtilProxy(EventDispatcher *dispatcher)
- : CryptoUtilProxy(dispatcher) {}
+ MockCryptoUtilProxy(EventDispatcher *dispatcher, GLib *glib)
+ : CryptoUtilProxy(dispatcher, glib) {}
virtual ~MockCryptoUtilProxy() {}
diff --git a/mock_glib.h b/mock_glib.h
index 7342b86..f82aad8 100644
--- a/mock_glib.h
+++ b/mock_glib.h
@@ -19,6 +19,8 @@
MOCK_METHOD2(Base64Decode, guchar *(const gchar *text, gsize *out_len));
MOCK_METHOD2(Base64Encode, gchar *(const guchar *data, gsize len));
+ MOCK_METHOD2(B64Decode, bool(const std::string &input, std::string *output));
+ MOCK_METHOD2(B64Encode, bool(const std::string &input, std::string *output));
MOCK_METHOD1(BusUnwatchName, void(guint watcher_id));
MOCK_METHOD7(BusWatchName,
guint(GBusType bus_type,