Improve management of certificate chains to make it harder to have

an inconsistent chain / private key set.

3 files changed, 59 insertions, 24 deletions

diff --git a/cscript b/cscript
index 8a4e605ca..6fdeb8736 100644
--- a/cscript
+++ b/cscript
@@ -280,8 +280,8 @@ def dependencies(target):
         ffmpeg_options = {}
 
     return (('ffmpeg-cdist', 'd993f38', ffmpeg_options),
-            ('libdcp', 'c871326'),
-            ('libsub', 'ce09e4e'),
+            ('libdcp', '284f247'),
+            ('libsub', '424041f'),
             ('rtaudio-cdist', None))
 
 def configure_options(target):
diff --git a/src/lib/config.cc b/src/lib/config.cc
index c2c6592cb..7b2e29f15 100644
--- a/src/lib/config.cc
+++ b/src/lib/config.cc
@@ -194,12 +194,11 @@ try
 	boost::optional<bool> u = f.optional_bool_child ("UseAnyServers");
 	_use_any_servers = u.get_value_or (true);
 
-	list<cxml::NodePtr> servers = f.node_children ("Server");
-	for (list<cxml::NodePtr>::iterator i = servers.begin(); i != servers.end(); ++i) {
-		if ((*i)->node_children("HostName").size() == 1) {
-			_servers.push_back ((*i)->string_child ("HostName"));
+	BOOST_FOREACH (cxml::ConstNodePtr i, f.node_children("Server")) {
+		if (i->node_children("HostName").size() == 1) {
+			_servers.push_back (i->string_child ("HostName"));
 		} else {
-			_servers.push_back ((*i)->content ());
+			_servers.push_back (i->content ());
 		}
 	}
 
@@ -289,8 +288,8 @@ try
 #endif
 
 	list<cxml::NodePtr> his = f.node_children ("History");
-	for (list<cxml::NodePtr>::const_iterator i = his.begin(); i != his.end(); ++i) {
-		_history.push_back ((*i)->content ());
+	BOOST_FOREACH (cxml::ConstNodePtr i, f.node_children("History")) {
+		_history.push_back (i->content ());
 	}
 
 	cxml::NodePtr signer = f.optional_node_child ("Signer");
@@ -434,8 +433,8 @@ Config::write_config () const
 	root->add_child("ServerPortBase")->add_child_text (raw_convert<string> (_server_port_base));
 	root->add_child("UseAnyServers")->add_child_text (_use_any_servers ? "1" : "0");
 
-	for (vector<string>::const_iterator i = _servers.begin(); i != _servers.end(); ++i) {
-		root->add_child("Server")->add_child_text (*i);
+	BOOST_FOREACH (string i, _servers) {
+		root->add_child("Server")->add_child_text (i);
 	}
 
 	root->add_child("OnlyServersEncode")->add_child_text (_only_servers_encode ? "1" : "0");
@@ -498,20 +497,20 @@ Config::write_config () const
 
 	xmlpp::Element* signer = root->add_child ("Signer");
 	DCPOMATIC_ASSERT (_signer_chain);
-	BOOST_FOREACH (dcp::Certificate const & i, _signer_chain->root_to_leaf ()) {
+	BOOST_FOREACH (dcp::Certificate const & i, _signer_chain->unordered()) {
 		signer->add_child("Certificate")->add_child_text (i.certificate (true));
 	}
 	signer->add_child("PrivateKey")->add_child_text (_signer_chain->key().get ());
 
 	xmlpp::Element* decryption = root->add_child ("Decryption");
 	DCPOMATIC_ASSERT (_decryption_chain);
-	BOOST_FOREACH (dcp::Certificate const & i, _decryption_chain->root_to_leaf ()) {
+	BOOST_FOREACH (dcp::Certificate const & i, _decryption_chain->unordered()) {
 		decryption->add_child("Certificate")->add_child_text (i.certificate (true));
 	}
 	decryption->add_child("PrivateKey")->add_child_text (_decryption_chain->key().get ());
 
-	for (vector<boost::filesystem::path>::const_iterator i = _history.begin(); i != _history.end(); ++i) {
-		root->add_child("History")->add_child_text (i->string ());
+	BOOST_FOREACH (boost::filesystem::path i, _history) {
+		root->add_child("History")->add_child_text (i.string ());
 	}
 
 	_dkdms->as_xml (root);
@@ -551,8 +550,8 @@ Config::write_cinemas () const
 	xmlpp::Element* root = doc.create_root_node ("Cinemas");
 	root->add_child("Version")->add_child_text ("1");
 
-	for (list<shared_ptr<Cinema> >::const_iterator i = _cinemas.begin(); i != _cinemas.end(); ++i) {
-		(*i)->as_xml (root->add_child ("Cinema"));
+	BOOST_FOREACH (shared_ptr<Cinema> i, _cinemas) {
+		i->as_xml (root->add_child ("Cinema"));
 	}
 
 	try {
@@ -664,12 +663,12 @@ Config::read_cinemas (cxml::Document const & f)
 {
 	_cinemas.clear ();
 	list<cxml::NodePtr> cin = f.node_children ("Cinema");
-	for (list<cxml::NodePtr>::iterator i = cin.begin(); i != cin.end(); ++i) {
+	BOOST_FOREACH (cxml::ConstNodePtr i, f.node_children("Cinema")) {
 		/* Slightly grotty two-part construction of Cinema here so that we can use
 		   shared_from_this.
 		*/
-		shared_ptr<Cinema> cinema (new Cinema (*i));
-		cinema->read_screens (*i);
+		shared_ptr<Cinema> cinema (new Cinema (i));
+		cinema->read_screens (i);
 		_cinemas.push_back (cinema);
 	}
 }
diff --git a/src/wx/config_dialog.cc b/src/wx/config_dialog.cc
index a62164896..796972384 100644
--- a/src/wx/config_dialog.cc
+++ b/src/wx/config_dialog.cc
@@ -881,6 +881,13 @@ public:
 		table->Add (_button_sizer, wxGBPosition (r, 0), wxGBSpan (1, 4));
 		++r;
 
+		_private_key_bad = new wxStaticText (this, wxID_ANY, _("Leaf private key does not match leaf certificate!"));
+	        font = *wxSMALL_FONT;
+		font.SetWeight (wxFONTWEIGHT_BOLD);
+		_private_key_bad->SetFont (font);
+		table->Add (_private_key_bad, wxGBPosition (r, 0), wxGBSpan (1, 3));
+		++r;
+
 		_add_certificate->Bind     (wxEVT_BUTTON,       boost::bind (&CertificateChainEditor::add_certificate, this));
 		_remove_certificate->Bind  (wxEVT_BUTTON,       boost::bind (&CertificateChainEditor::remove_certificate, this));
 		_export_certificate->Bind  (wxEVT_BUTTON,       boost::bind (&CertificateChainEditor::export_certificate, this));
@@ -916,7 +923,15 @@ private:
 		if (d->ShowModal() == wxID_OK) {
 			try {
 				dcp::Certificate c;
-				string const extra = c.read_string (dcp::file_to_string (wx_to_std (d->GetPath ())));
+				string extra;
+				try {
+					extra = c.read_string (dcp::file_to_string (wx_to_std (d->GetPath ())));
+				} catch (boost::filesystem::filesystem_error& e) {
+					error_dialog (this, wxString::Format (_("Could not load certificate (%s)"), d->GetPath().data()));
+					d->Destroy ();
+					return;
+				}
+
 				if (!extra.empty ()) {
 					message_dialog (
 						this,
@@ -925,8 +940,17 @@ private:
 						);
 				}
 				_chain->add (c);
-				_set (_chain);
-				update_certificate_list ();
+				if (!_chain->chain_valid ()) {
+					error_dialog (
+						this,
+						_("Adding this certificate would make the chain inconsistent, so it will not be added. "
+						  "Add certificates in order from root to intermediate to leaf.")
+						);
+					_chain->remove (c);
+				} else {
+					_set (_chain);
+					update_certificate_list ();
+				}
 			} catch (dcp::MiscError& e) {
 				error_dialog (this, wxString::Format (_("Could not read certificate file (%s)"), e.what ()));
 			}
@@ -1003,6 +1027,16 @@ private:
 
 			++n;
 		}
+
+		static wxColour normal = _private_key_bad->GetForegroundColour ();
+
+		if (_chain->private_key_valid ()) {
+			_private_key_bad->Hide ();
+			_private_key_bad->SetForegroundColour (normal);
+		} else {
+			_private_key_bad->Show ();
+			_private_key_bad->SetForegroundColour (wxColour (255, 0, 0));
+		}
 	}
 
 	void remake_certificates ()
@@ -1069,7 +1103,8 @@ private:
 
 	void update_sensitivity ()
 	{
-		_remove_certificate->Enable (_certificates->GetNextItem (-1, wxLIST_NEXT_ALL, wxLIST_STATE_SELECTED) != -1);
+		/* We can only remove the leaf certificate */
+		_remove_certificate->Enable (_certificates->GetNextItem (-1, wxLIST_NEXT_ALL, wxLIST_STATE_SELECTED) == (_certificates->GetItemCount() - 1));
 		_export_certificate->Enable (_certificates->GetNextItem (-1, wxLIST_NEXT_ALL, wxLIST_STATE_SELECTED) != -1);
 	}
 
@@ -1140,6 +1175,7 @@ private:
 	wxStaticText* _private_key;
 	wxButton* _load_private_key;
 	wxButton* _export_private_key;
+	wxStaticText* _private_key_bad;
 	wxSizer* _sizer;
 	wxBoxSizer* _button_sizer;
 	shared_ptr<dcp::CertificateChain> _chain;