Cleanup of SecretStore.

Added documentation, more failure checking and some general cleanup.

libdevcore/CommonIO.cpp

@@ -95,9 +95,11 @@ string dev::contentsString(string const& _file)
 	return contentsGeneric<string>(_file);
 }
 
-void dev::writeFile(std::string const& _file, bytesConstRef _data)
+bool dev::writeFile(std::string const& _file, bytesConstRef _data)
 {
-	ofstream(_file, ios::trunc|ios::binary).write((char const*)_data.data(), _data.size());
+	ofstream s(_file, ios::trunc | ios::binary);
+	s.write(reinterpret_cast<char const*>(_data.data()), _data.size());
+	return !!s;
 }
 
 std::string dev::getPassword(std::string const& _prompt)

libdevcore/CommonIO.h

@@ -56,10 +56,11 @@ std::string contentsString(std::string const& _file);
 bytesRef contentsNew(std::string const& _file, bytesRef _dest = bytesRef());
 
 /// Write the given binary data into the given file, replacing the file if it pre-exists.
-void writeFile(std::string const& _file, bytesConstRef _data);
+/// @returns true if writing succeeded.
+bool writeFile(std::string const& _file, bytesConstRef _data);
 /// Write the given binary data into the given file, replacing the file if it pre-exists.
-inline void writeFile(std::string const& _file, bytes const& _data) { writeFile(_file, bytesConstRef(&_data)); }
-inline void writeFile(std::string const& _file, std::string const& _data) { writeFile(_file, bytesConstRef(_data)); }
+inline bool writeFile(std::string const& _file, bytes const& _data) { return writeFile(_file, bytesConstRef(&_data)); }
+inline bool writeFile(std::string const& _file, std::string const& _data) { return writeFile(_file, bytesConstRef(_data)); }
 
 /// Nicely renders the given bytes to a string, optionally as HTML.
 /// @a _bytes: bytes array to be rendered as string. @a _width of a bytes line.

libdevcrypto/Common.cpp

@@ -178,15 +178,35 @@ bool dev::verify(Public const& _p, Signature const& _s, h256 const& _hash)
 bytes dev::pbkdf2(string const& _pass, bytes const& _salt, unsigned _iterations, unsigned _dkLen)
 {
 	bytes ret(_dkLen);
-	PKCS5_PBKDF2_HMAC<SHA256> pbkdf;
-	pbkdf.DeriveKey(ret.data(), ret.size(), 0, (byte*)_pass.data(), _pass.size(), _salt.data(), _salt.size(), _iterations);
+	if (PKCS5_PBKDF2_HMAC<SHA256>().DeriveKey(
+		ret.data(),
+		ret.size(),
+		0,
+		reinterpret_cast<byte const*>(_pass.data()),
+		_pass.size(),
+		_salt.data(),
+		_salt.size(),
+		_iterations
+	) != _iterations)
+		return bytes();
 	return ret;
 }
 
 bytes dev::scrypt(std::string const& _pass, bytes const& _salt, uint64_t _n, uint32_t _r, uint32_t _p, unsigned _dkLen)
 {
 	bytes ret(_dkLen);
-	libscrypt_scrypt((uint8_t const*)_pass.data(), _pass.size(), _salt.data(), _salt.size(), _n, _r, _p, ret.data(), ret.size());
+	if (libscrypt_scrypt(
+		reinterpret_cast<uint8_t const*>(_pass.data()),
+		_pass.size(),
+		_salt.data(),
+		_salt.size(),
+		_n,
+		_r,
+		_p,
+		ret.data(),
+		ret.size()
+	) != 0)
+		return bytes();
 	return ret;
 }
 

libdevcrypto/SecretStore.cpp

@@ -36,7 +36,8 @@ namespace fs = boost::filesystem;
 
 static const int c_keyFileVersion = 3;
 
-static js::mValue upgraded(std::string const& _s)
+/// Upgrade the json-format to the current version.
+static js::mValue upgraded(string const& _s)
 {
 	js::mValue v;
 	js::read_string(_s, v);
@@ -84,36 +85,38 @@ static js::mValue upgraded(std::string const& _s)
 	return js::mValue();
 }
 
-SecretStore::SecretStore(std::string const& _path): m_path(_path)
+SecretStore::SecretStore(string const& _path): m_path(_path)
 {
 	load();
 }
 
-SecretStore::~SecretStore()
+bytes SecretStore::secret(h128 const& _uuid, function<string()> const& _pass, bool _useCache) const
 {
-}
-
-bytes SecretStore::secret(h128 const& _uuid, function<std::string()> const& _pass, bool _useCache) const
-{
-	(void)_pass;
 	auto rit = m_cached.find(_uuid);
 	if (_useCache && rit != m_cached.end())
 		return rit->second;
 	auto it = m_keys.find(_uuid);
-	if (it == m_keys.end())
-		return bytes();
-	bytes key = decrypt(it->second.first, _pass());
-	if (!key.empty())
-		m_cached[_uuid] = key;
+	bytes key;
+	if (it != m_keys.end())
+	{
+		key = decrypt(it->second.encryptedKey, _pass());
+		if (!key.empty())
+			m_cached[_uuid] = key;
+	}
 	return key;
 }
 
-h128 SecretStore::importSecret(bytes const& _s, std::string const& _pass)
+h128 SecretStore::importSecret(bytes const& _s, string const& _pass)
 {
-	h128 r = h128::random();
-	m_cached[r] = _s;
-	m_keys[r] = make_pair(encrypt(_s, _pass), std::string());
-	save();
+	h128 r;
+	EncryptedKey key{encrypt(_s, _pass), string()};
+	if (!key.encryptedKey.empty())
+	{
+		r = h128::random();
+		m_cached[r] = _s;
+		m_keys[r] = move(key);
+		save();
+	}
 	return r;
 }
 
@@ -122,7 +125,7 @@ void SecretStore::kill(h128 const& _uuid)
 	m_cached.erase(_uuid);
 	if (m_keys.count(_uuid))
 	{
-		boost::filesystem::remove(m_keys[_uuid].second);
+		fs::remove(m_keys[_uuid].filename);
 		m_keys.erase(_uuid);
 	}
 }
@@ -132,50 +135,52 @@ void SecretStore::clearCache() const
 	m_cached.clear();
 }
 
-void SecretStore::save(std::string const& _keysPath)
+void SecretStore::save(string const& _keysPath)
 {
 	fs::path p(_keysPath);
-	boost::filesystem::create_directories(p);
+	fs::create_directories(p);
 	for (auto& k: m_keys)
 	{
-		std::string uuid = toUUID(k.first);
-		std::string filename = (p / uuid).string() + ".json";
+		string uuid = toUUID(k.first);
+		string filename = (p / uuid).string() + ".json";
 		js::mObject v;
 		js::mValue crypto;
-		js::read_string(k.second.first, crypto);
+		js::read_string(k.second.encryptedKey, crypto);
 		v["crypto"] = crypto;
 		v["id"] = uuid;
 		v["version"] = c_keyFileVersion;
-		writeFile(filename, js::write_string(js::mValue(v), true));
-		if (!k.second.second.empty() && k.second.second != filename)
-			boost::filesystem::remove(k.second.second);
-		k.second.second = filename;
+		if (writeFile(filename, js::write_string(js::mValue(v), true)))
+		{
+			swap(k.second.filename, filename);
+			if (!filename.empty() && !fs::equivalent(filename, k.second.filename))
+				fs::remove(filename);
+		}
 	}
 }
 
-void SecretStore::load(std::string const& _keysPath)
+void SecretStore::load(string const& _keysPath)
 {
 	fs::path p(_keysPath);
-	boost::filesystem::create_directories(p);
+	fs::create_directories(p);
 	for (fs::directory_iterator it(p); it != fs::directory_iterator(); ++it)
-		if (is_regular_file(it->path()))
+		if (fs::is_regular_file(it->path()))
 			readKey(it->path().string(), true);
 }
 
-h128 SecretStore::readKey(std::string const& _file, bool _deleteFile)
+h128 SecretStore::readKey(string const& _file, bool _takeFileOwnership)
 {
 	cnote << "Reading" << _file;
-	return readKeyContent(contentsString(_file), _deleteFile ? _file : string());
+	return readKeyContent(contentsString(_file), _takeFileOwnership ? _file : string());
 }
 
-h128 SecretStore::readKeyContent(std::string const& _content, std::string const& _file)
+h128 SecretStore::readKeyContent(string const& _content, string const& _file)
 {
 	js::mValue u = upgraded(_content);
 	if (u.type() == js::obj_type)
 	{
 		js::mObject& o = u.get_obj();
 		auto uuid = fromUUID(o["id"].get_str());
-		m_keys[uuid] = make_pair(js::write_string(o["crypto"], false), _file);
+		m_keys[uuid] = EncryptedKey{js::write_string(o["crypto"], false), _file};
 		return uuid;
 	}
 	else
@@ -183,62 +188,66 @@ h128 SecretStore::readKeyContent(std::string const& _content, std::string const&
 	return h128();
 }
 
-bool SecretStore::recode(h128 const& _uuid, string const& _newPass, std::function<std::string()> const& _pass, KDF _kdf)
+bool SecretStore::recode(h128 const& _uuid, string const& _newPass, function<string()> const& _pass, KDF _kdf)
 {
-//	cdebug << "recode:" << toUUID(_uuid);
 	bytes s = secret(_uuid, _pass, true);
 	if (s.empty())
 		return false;
-	m_keys[_uuid].first = encrypt(s, _newPass, _kdf);
+	m_cached.erase(_uuid);
+	m_keys[_uuid].encryptedKey = encrypt(s, _newPass, _kdf);
 	save();
 	return true;
 }
 
-std::string SecretStore::encrypt(bytes const& _v, std::string const& _pass, KDF _kdf)
+static bytes deriveNewKey(string const& _pass, KDF _kdf, js::mObject& o_ret)
 {
-	js::mObject ret;
-
-	// KDF info
 	unsigned dklen = 32;
+	unsigned iterations = 1 << 19;
 	bytes salt = h256::random().asBytes();
-	bytes derivedKey;
 	if (_kdf == KDF::Scrypt)
 	{
-		unsigned iterations = 262144;
 		unsigned p = 1;
 		unsigned r = 8;
-		ret["kdf"] = "scrypt";
+		o_ret["kdf"] = "scrypt";
 		{
 			js::mObject params;
-			params["n"] = (int64_t)iterations;
-			params["r"] = (int)r;
-			params["p"] = (int)p;
-			params["dklen"] = (int)dklen;
+			params["n"] = int64_t(iterations);
+			params["r"] = int(r);
+			params["p"] = int(p);
+			params["dklen"] = int(dklen);
 			params["salt"] = toHex(salt);
-			ret["kdfparams"] = params;
+			o_ret["kdfparams"] = params;
 		}
-		derivedKey = scrypt(_pass, salt, iterations, r, p, dklen);
+		return scrypt(_pass, salt, iterations, r, p, dklen);
 	}
 	else
 	{
-		unsigned iterations = 262144;
-		ret["kdf"] = "pbkdf2";
+		o_ret["kdf"] = "pbkdf2";
 		{
 			js::mObject params;
 			params["prf"] = "hmac-sha256";
-			params["c"] = (int)iterations;
+			params["c"] = int(iterations);
 			params["salt"] = toHex(salt);
-			params["dklen"] = (int)dklen;
-			ret["kdfparams"] = params;
+			params["dklen"] = int(dklen);
+			o_ret["kdfparams"] = params;
 		}
-		derivedKey = pbkdf2(_pass, salt, iterations, dklen);
+		return pbkdf2(_pass, salt, iterations, dklen);
+	}
+}
+
+string SecretStore::encrypt(bytes const& _v, string const& _pass, KDF _kdf)
+{
+	js::mObject ret;
+
+	bytes derivedKey = deriveNewKey(_pass, _kdf, ret);
+	if (derivedKey.empty())
+	{
+		cwarn << "Key derivation failed.";
+		return string();
 	}
-//	cdebug << "derivedKey" << toHex(derivedKey);
 
-	// cipher info
 	ret["cipher"] = "aes-128-ctr";
 	h128 key(derivedKey, h128::AlignLeft);
-//	cdebug << "cipherKey" << key.hex();
 	h128 iv = h128::random();
 	{
 		js::mObject params;
@@ -248,18 +257,21 @@ std::string SecretStore::encrypt(bytes const& _v, std::string const& _pass, KDF
 
 	// cipher text
 	bytes cipherText = encryptSymNoAuth(key, iv, &_v);
+	if (cipherText.empty())
+	{
+		cwarn << "Key encryption failed.";
+		return string();
+	}
 	ret["ciphertext"] = toHex(cipherText);
 
 	// and mac.
 	h256 mac = sha3(ref(derivedKey).cropped(16, 16).toBytes() + cipherText);
-//	cdebug << "macBody" << toHex(ref(derivedKey).cropped(16, 16).toBytes() + cipherText);
-//	cdebug << "mac" << mac.hex();
 	ret["mac"] = toHex(mac.ref());
 
-	return js::write_string((js::mValue)ret, true);
+	return js::write_string(js::mValue(ret), true);
 }
 
-bytes SecretStore::decrypt(std::string const& _v, std::string const& _pass)
+bytes SecretStore::decrypt(string const& _v, string const& _pass)
 {
 	js::mObject o;
 	{

libdevcrypto/SecretStore.h

@@ -35,41 +35,80 @@ enum class KDF {
 	Scrypt,
 };
 
+/**
+ * Manages encrypted keys stored in a certain directory on disk. The keys are read into memory
+ * and changes to the keys are automatically synced to the directory.
+ * Each file stores exactly one key in a specific JSON format whose file name is derived from the
+ * UUID of the key.
+ * @note that most of the functions here affect the filesystem and throw exceptions on failure.
+ */
 class SecretStore
 {
 public:
+	/// Construct a new SecretStore and read all keys in the given directory.
 	SecretStore(std::string const& _path = defaultPath());
-	~SecretStore();
 
+	/// @returns the secret key stored by the given @a _uuid.
+	/// @param _pass function that returns the password for the key.
+	/// @param _useCache if true, allow previously decrypted keys to be returned directly.
 	bytes secret(h128 const& _uuid, std::function<std::string()> const& _pass, bool _useCache = true) const;
+	/// Imports the (encrypted) key stored in the file @a _file and copies it to the managed directory.
 	h128 importKey(std::string const& _file) { auto ret = readKey(_file, false); if (ret) save(); return ret; }
+	/// Imports the (encrypted) key contained in the json formatted @a _content and stores it in
+	/// the managed directory.
 	h128 importKeyContent(std::string const& _content) { auto ret = readKeyContent(_content, std::string()); if (ret) save(); return ret; }
+	/// Imports the decrypted key given by @a _s and stores it, encrypted with
+	/// (a key derived from) the password @a _pass.
 	h128 importSecret(bytes const& _s, std::string const& _pass);
+	/// Decrypts and re-encrypts the key identified by @a _uuid.
 	bool recode(h128 const& _uuid, std::string const& _newPass, std::function<std::string()> const& _pass, KDF _kdf = KDF::Scrypt);
+	/// Removes the key specified by @a _uuid from both memory and disk.
 	void kill(h128 const& _uuid);
 
+	/// Returns the uuids of all stored keys.
 	std::vector<h128> keys() const { return keysOf(m_keys); }
 
-	// Clear any cached keys.
+	/// Clears all cached decrypted keys. The passwords have to be supplied in order to retrieve
+	/// secrets again after calling this function.
 	void clearCache() const;
 
-	// Doesn't save().
-	h128 readKey(std::string const& _file, bool _deleteFile);
+	/// Import the key from the file @a _file, but do not copy it to the managed directory yet.
+	/// @param _takeFileOwnership if true, deletes the file if it is not the canonical file for the
+	/// key (derived from its uuid).
+	h128 readKey(std::string const& _file, bool _takeFileOwnership);
+	/// Import the key contained in the json-encoded @a _content, but do not store it in the
+	/// managed directory.
+	/// @param _file if given, assume this file contains @a _content and delete it later, if it is
+	/// not the canonical file for the key (derived from the uuid).
 	h128 readKeyContent(std::string const& _content, std::string const& _file = std::string());
 
+	/// Store all keys in the directory @a _keysPath.
 	void save(std::string const& _keysPath);
+	/// Store all keys in the managed directory.
 	void save() { save(m_path); }
 
+	/// @returns the default path for the managed directory.
 	static std::string defaultPath() { return getDataDir("web3") + "/keys"; }
 
 private:
+	struct EncryptedKey
+	{
+		std::string encryptedKey;
+		std::string filename;
+	};
+
+	/// Loads all keys in the given directory.
 	void load(std::string const& _keysPath);
 	void load() { load(m_path); }
+	/// Encrypts @a _v with a key derived from @a _pass or the empty string on error.
 	static std::string encrypt(bytes const& _v, std::string const& _pass, KDF _kdf = KDF::Scrypt);
+	/// Decrypts @a _v with a key derived from @a _pass or the empty byte array on error.
 	static bytes decrypt(std::string const& _v, std::string const& _pass);
 
+	/// Stores decrypted keys by uuid.
 	mutable std::unordered_map<h128, bytes> m_cached;
-	std::unordered_map<h128, std::pair<std::string, std::string>> m_keys;
+	/// Stores encrypted keys together with the file they were loaded from by uuid.
+	std::unordered_map<h128, EncryptedKey> m_keys;
 
 	std::string m_path;
 };

test/libdevcrypto/SecretStore.cpp

@@ -52,7 +52,7 @@ BOOST_AUTO_TEST_CASE(basic_tests)
 	{
 		cnote << i.first;
 		js::mObject& o = i.second.get_obj();
-		SecretStore store(".");
+		SecretStore store("");
 		h128 u = store.readKeyContent(js::write_string(o["json"], false));
 		cdebug << "read uuid" << u;
 		bytes s = store.secret(u, [&](){ return o["password"].get_str(); });