Make key files extensible, store key name in key file

Storing the key name in the key file makes it unnecessary to pass the
--key-name option to git-crypt unlock.

This breaks compatibility with post-revamp keys.  On the plus side,
keys are now extensible so in the future it will be easier to make
changes to the format without breaking compatibility.

commands.cpp

@@ -81,20 +81,11 @@ static void configure_git_filters (const char* key_name)
 	}
 }
 
-static void validate_key_name (const char* key_name)
+static void validate_key_name_or_throw (const char* key_name)
 {
-	if (!*key_name) {
-		throw Error("Key name may not be empty");
-	}
-
-	if (std::strcmp(key_name, "default") == 0) {
-		throw Error("`default' is not a legal key name");
-	}
-	// Need to be restrictive with key names because they're used as part of a Git filter name
-	while (char c = *key_name++) {
-		if (!std::isalnum(c) && c != '-' && c != '_') {
-			throw Error("Key names may contain only A-Z, a-z, 0-9, '-', and '_'");
-		}
+	std::string			reason;
+	if (!validate_key_name(key_name, &reason)) {
+		throw Error(reason);
 	}
 }
 
@@ -330,25 +321,26 @@ static bool decrypt_repo_key (Key_file& key_file, const char* key_name, uint32_t
 			if (!this_version_entry) {
 				throw Error("GPG-encrypted keyfile is malformed because it does not contain expected key version");
 			}
-			key_file.add(key_version, *this_version_entry);
+			key_file.add(*this_version_entry);
 			return true;
 		}
 	}
 	return false;
 }
 
-static void encrypt_repo_key (const char* key_name, uint32_t key_version, const Key_file::Entry& key, const std::vector<std::string>& collab_keys, const std::string& keys_path, std::vector<std::string>* new_files)
+static void encrypt_repo_key (const char* key_name, const Key_file::Entry& key, const std::vector<std::string>& collab_keys, const std::string& keys_path, std::vector<std::string>* new_files)
 {
 	std::string	key_file_data;
 	{
 		Key_file this_version_key_file;
-		this_version_key_file.add(key_version, key);
+		this_version_key_file.set_key_name(key_name);
+		this_version_key_file.add(key);
 		key_file_data = this_version_key_file.store_to_string();
 	}
 
 	for (std::vector<std::string>::const_iterator collab(collab_keys.begin()); collab != collab_keys.end(); ++collab) {
 		std::ostringstream	path_builder;
-		path_builder << keys_path << '/' << (key_name ? key_name : "default") << '/' << key_version << '/' << *collab << ".gpg";
+		path_builder << keys_path << '/' << (key_name ? key_name : "default") << '/' << key.version << '/' << *collab << ".gpg";
 		std::string		path(path_builder.str());
 
 		if (access(path.c_str(), F_OK) == 0) {
@@ -604,7 +596,7 @@ int init (int argc, char** argv)
 	}
 
 	if (key_name) {
-		validate_key_name(key_name);
+		validate_key_name_or_throw(key_name);
 	}
 
 	std::string		internal_key_path(get_internal_key_path(key_name));
@@ -618,6 +610,7 @@ int init (int argc, char** argv)
 	// 1. Generate a key and install it
 	std::clog << "Generating key..." << std::endl;
 	Key_file		key_file;
+	key_file.set_key_name(key_name);
 	key_file.generate();
 
 	mkdir_parent(internal_key_path);
@@ -681,6 +674,12 @@ int unlock (int argc, char** argv)
 	if (symmetric_key_file) {
 		// Read from the symmetric key file
 		// TODO: command line flag to accept legacy key format?
+
+		if (key_name) {
+			std::clog << "Error: key name should not be specified when unlocking with symmetric key." << std::endl;
+			return 1;
+		}
+
 		try {
 			if (std::strcmp(symmetric_key_file, "-") == 0) {
 				key_file.load(std::cin);
@@ -713,7 +712,7 @@ int unlock (int argc, char** argv)
 			return 1;
 		}
 	}
-	std::string		internal_key_path(get_internal_key_path(key_name));
+	std::string		internal_key_path(get_internal_key_path(key_file.get_key_name()));
 	// TODO: croak if internal_key_path already exists???
 	mkdir_parent(internal_key_path);
 	if (!key_file.store_to_file(internal_key_path.c_str())) {
@@ -722,7 +721,7 @@ int unlock (int argc, char** argv)
 	}
 
 	// 4. Configure git for git-crypt
-	configure_git_filters(key_name);
+	configure_git_filters(key_file.get_key_name());
 
 	// 5. Do a force checkout so any files that were previously checked out encrypted
 	//    will now be checked out decrypted.
@@ -793,7 +792,7 @@ int add_collab (int argc, char** argv)
 	std::string			keys_path(get_repo_keys_path());
 	std::vector<std::string>	new_files;
 
-	encrypt_repo_key(key_name, key_file.latest(), *key, collab_keys, keys_path, &new_files);
+	encrypt_repo_key(key_name, *key, collab_keys, keys_path, &new_files);
 
 	// add/commit the new files
 	if (!new_files.empty()) {

key.cpp

@@ -40,9 +40,69 @@
 #include <sstream>
 #include <cstring>
 #include <stdexcept>
+#include <vector>
+
+Key_file::Entry::Entry ()
+{
+	version = 0;
+	std::memset(aes_key, 0, AES_KEY_LEN);
+	std::memset(hmac_key, 0, HMAC_KEY_LEN);
+}
 
 void		Key_file::Entry::load (std::istream& in)
 {
+	while (true) {
+		uint32_t	field_id;
+		if (!read_be32(in, field_id)) {
+			throw Malformed();
+		}
+		if (field_id == KEY_FIELD_END) {
+			break;
+		}
+		uint32_t	field_len;
+		if (!read_be32(in, field_len)) {
+			throw Malformed();
+		}
+
+		if (field_id == KEY_FIELD_VERSION) {
+			if (field_len != 4) {
+				throw Malformed();
+			}
+			if (!read_be32(in, version)) {
+				throw Malformed();
+			}
+		} else if (field_id == KEY_FIELD_AES_KEY) {
+			if (field_len != AES_KEY_LEN) {
+				throw Malformed();
+			}
+			in.read(reinterpret_cast<char*>(aes_key), AES_KEY_LEN);
+			if (in.gcount() != AES_KEY_LEN) {
+				throw Malformed();
+			}
+		} else if (field_id == KEY_FIELD_HMAC_KEY) {
+			if (field_len != HMAC_KEY_LEN) {
+				throw Malformed();
+			}
+			in.read(reinterpret_cast<char*>(hmac_key), HMAC_KEY_LEN);
+			if (in.gcount() != HMAC_KEY_LEN) {
+				throw Malformed();
+			}
+		} else if (field_id & 1) { // unknown critical field
+			throw Incompatible();
+		} else {
+			// unknown non-critical field - safe to ignore
+			in.ignore(field_len);
+			if (in.gcount() != field_len) {
+				throw Malformed();
+			}
+		}
+	}
+}
+
+void		Key_file::Entry::load_legacy (uint32_t arg_version, std::istream& in)
+{
+	version = arg_version;
+
 	// First comes the AES key
 	in.read(reinterpret_cast<char*>(aes_key), AES_KEY_LEN);
 	if (in.gcount() != AES_KEY_LEN) {
@@ -58,12 +118,28 @@ void		Key_file::Entry::load (std::istream& in)
 
 void		Key_file::Entry::store (std::ostream& out) const
 {
+	// Version
+	write_be32(out, KEY_FIELD_VERSION);
+	write_be32(out, 4);
+	write_be32(out, version);
+
+	// AES key
+	write_be32(out, KEY_FIELD_AES_KEY);
+	write_be32(out, AES_KEY_LEN);
 	out.write(reinterpret_cast<const char*>(aes_key), AES_KEY_LEN);
+
+	// HMAC key
+	write_be32(out, KEY_FIELD_HMAC_KEY);
+	write_be32(out, HMAC_KEY_LEN);
 	out.write(reinterpret_cast<const char*>(hmac_key), HMAC_KEY_LEN);
+
+	// End
+	write_be32(out, KEY_FIELD_END);
 }
 
-void		Key_file::Entry::generate ()
+void		Key_file::Entry::generate (uint32_t arg_version)
 {
+	version = arg_version;
 	random_bytes(aes_key, AES_KEY_LEN);
 	random_bytes(hmac_key, HMAC_KEY_LEN);
 }
@@ -79,15 +155,15 @@ const Key_file::Entry*	Key_file::get (uint32_t version) const
 	return it != entries.end() ? &it->second : 0;
 }
 
-void		Key_file::add (uint32_t version, const Entry& entry)
+void		Key_file::add (const Entry& entry)
 {
-	entries[version] = entry;
+	entries[entry.version] = entry;
 }
 
 
 void		Key_file::load_legacy (std::istream& in)
 {
-	entries[0].load(in);
+	entries[0].load_legacy(0, in);
 }
 
 void		Key_file::load (std::istream& in)
@@ -103,12 +179,52 @@ void		Key_file::load (std::istream& in)
 	if (load_be32(preamble + 12) != FORMAT_VERSION) {
 		throw Incompatible();
 	}
+	load_header(in);
 	while (in.peek() != -1) {
-		uint32_t	version;
-		if (!read_be32(in, version)) {
+		Entry		entry;
+		entry.load(in);
+		add(entry);
+	}
+}
+
+void		Key_file::load_header (std::istream& in)
+{
+	while (true) {
+		uint32_t	field_id;
+		if (!read_be32(in, field_id)) {
 			throw Malformed();
 		}
-		entries[version].load(in);
+		if (field_id == HEADER_FIELD_END) {
+			break;
+		}
+		uint32_t	field_len;
+		if (!read_be32(in, field_len)) {
+			throw Malformed();
+		}
+
+		if (field_id == HEADER_FIELD_KEY_NAME) {
+			if (field_len > KEY_NAME_MAX_LEN) {
+				throw Malformed();
+			}
+			std::vector<char>	bytes(field_len);
+			in.read(&bytes[0], field_len);
+			if (in.gcount() != field_len) {
+				throw Malformed();
+			}
+			key_name.assign(&bytes[0], field_len);
+			if (!validate_key_name(key_name.c_str())) {
+				key_name.clear();
+				throw Malformed();
+			}
+		} else if (field_id & 1) { // unknown critical field
+			throw Incompatible();
+		} else {
+			// unknown non-critical field - safe to ignore
+			in.ignore(field_len);
+			if (in.gcount() != field_len) {
+				throw Malformed();
+			}
+		}
 	}
 }
 
@@ -116,8 +232,13 @@ void		Key_file::store (std::ostream& out) const
 {
 	out.write("\0GITCRYPTKEY", 12);
 	write_be32(out, FORMAT_VERSION);
+	if (!key_name.empty()) {
+		write_be32(out, HEADER_FIELD_KEY_NAME);
+		write_be32(out, key_name.size());
+		out.write(key_name.data(), key_name.size());
+	}
+	write_be32(out, HEADER_FIELD_END);
 	for (Map::const_iterator it(entries.begin()); it != entries.end(); ++it) {
-		write_be32(out, it->first);
 		it->second.store(out);
 	}
 }
@@ -157,7 +278,8 @@ std::string	Key_file::store_to_string () const
 
 void		Key_file::generate ()
 {
-	entries[is_empty() ? 0 : latest() + 1].generate();
+	uint32_t	version(is_empty() ? 0 : latest() + 1);
+	entries[version].generate(version);
 }
 
 uint32_t	Key_file::latest () const
@@ -168,3 +290,29 @@ uint32_t	Key_file::latest () const
 	return entries.begin()->first;
 }
 
+bool validate_key_name (const char* key_name, std::string* reason)
+{
+	if (!*key_name) {
+		if (reason) { *reason = "Key name may not be empty"; }
+		return false;
+	}
+
+	if (std::strcmp(key_name, "default") == 0) {
+		if (reason) { *reason = "`default' is not a legal key name"; }
+		return false;
+	}
+	// Need to be restrictive with key names because they're used as part of a Git filter name
+	size_t		len = 0;
+	while (char c = *key_name++) {
+		if (!std::isalnum(c) && c != '-' && c != '_') {
+			if (reason) { *reason = "Key names may contain only A-Z, a-z, 0-9, '-', and '_'"; }
+			return false;
+		}
+		if (++len > KEY_NAME_MAX_LEN) {
+			if (reason) { *reason = "Key name is too long"; }
+			return false;
+		}
+	}
+	return true;
+}
+

key.hpp

@@ -45,12 +45,16 @@ enum {
 struct Key_file {
 public:
 	struct Entry {
+		uint32_t		version;
 		unsigned char		aes_key[AES_KEY_LEN];
 		unsigned char		hmac_key[HMAC_KEY_LEN];
 
+		Entry ();
+
 		void			load (std::istream&);
+		void			load_legacy (uint32_t version, std::istream&);
 		void			store (std::ostream&) const;
-		void			generate ();
+		void			generate (uint32_t version);
 	};
 
 	struct Malformed { }; // exception class
@@ -59,7 +63,7 @@ public:
 	const Entry*			get_latest () const;
 
 	const Entry*			get (uint32_t version) const;
-	void				add (uint32_t version, const Entry&);
+	void				add (const Entry&);
 
 	void				load_legacy (std::istream&);
 	void				load (std::istream&);
@@ -77,11 +81,33 @@ public:
 
 	uint32_t			latest () const;
 
+	void				set_key_name (const char* k) { key_name = k ? k : ""; }
+	const char*			get_key_name () const { return key_name.empty() ? 0 : key_name.c_str(); }
 private:
 	typedef std::map<uint32_t, Entry, std::greater<uint32_t> > Map;
-	enum { FORMAT_VERSION = 1 };
+	enum { FORMAT_VERSION = 2 };
 
 	Map				entries;
+	std::string			key_name;
+
+	void				load_header (std::istream&);
+
+	enum {
+		HEADER_FIELD_END	= 0,
+		HEADER_FIELD_KEY_NAME	= 1
+	};
+	enum {
+		KEY_FIELD_END		= 0,
+		KEY_FIELD_VERSION	= 1,
+		KEY_FIELD_AES_KEY	= 3,
+		KEY_FIELD_HMAC_KEY	= 5
+	};
 };
 
+enum {
+	KEY_NAME_MAX_LEN = 128
+};
+
+bool validate_key_name (const char* key_name, std::string* reason =0);
+
 #endif