Fix up management of certificate chain validity.

6 files changed, 221 insertions, 102 deletions

diff --git a/src/certificate_chain.cc b/src/certificate_chain.cc
index d6b8a7a0..3ea6db60 100644
--- a/src/certificate_chain.cc
+++ b/src/certificate_chain.cc
@@ -327,9 +327,8 @@ CertificateChain::CertificateChain (string s)
 		}
 	}
 
-	if (!attempt_reorder ()) {
-		throw MiscError ("could not find certificate chain order");
-	}
+	/* This will throw an exception if the chain cannot be ordered */
+	leaf_to_root ();
 }
 
 /** @return Root certificate */
@@ -337,7 +336,7 @@ Certificate
 CertificateChain::root () const
 {
 	DCP_ASSERT (!_certificates.empty());
-	return _certificates.front ();
+	return root_to_leaf().front ();
 }
 
 /** @return Leaf certificate */
@@ -345,26 +344,25 @@ Certificate
 CertificateChain::leaf () const
 {
 	DCP_ASSERT (!_certificates.empty());
-	return _certificates.back ();
+	return root_to_leaf().back ();
 }
 
-/** @return Certificates in order from root to leaf */
+/** @return Certificates in order from leaf to root */
 CertificateChain::List
-CertificateChain::root_to_leaf () const
+CertificateChain::leaf_to_root () const
 {
-	return _certificates;
+	List l = root_to_leaf ();
+	l.reverse ();
+	return l;
 }
 
-/** @return Certificates in order from leaf to root */
 CertificateChain::List
-CertificateChain::leaf_to_root () const
+CertificateChain::unordered () const
 {
-	List c = _certificates;
-	c.reverse ();
-	return c;
+	return _certificates;
 }
 
-/** Add a certificate to the end of the chain.
+/** Add a certificate to the chain.
  *  @param c Certificate to add.
  */
 void
@@ -399,40 +397,49 @@ CertificateChain::remove (int i)
 	}
 }
 
-/** Check to see if the chain is valid (i.e. root signs the intermediate, intermediate
+bool
+CertificateChain::chain_valid () const
+{
+	return chain_valid (_certificates);
+}
+
+/** Check to see if a chain is valid (i.e. root signs the intermediate, intermediate
  *  signs the leaf and so on) and that the private key (if there is one) matches the
  *  leaf certificate.
- *  @param valid if non-0 and the CertificateChain is not valid, this is filled in with
- *  an explanation.
  *  @return true if it's ok, false if not.
  */
 bool
-CertificateChain::valid (string* reason) const
+CertificateChain::chain_valid (List const & chain) const
 {
-	/* Check the certificate chain */
+        /* Here I am taking a chain of certificates A/B/C/D and checking validity of B wrt A,
+	   C wrt B and D wrt C.  It also appears necessary to check the issuer of B/C/D matches
+	   the subject of A/B/C; I don't understand why.  I'm sure there's a better way of doing
+	   this with OpenSSL but the documentation does not appear not likely to reveal it
+	   any time soon.
+	*/
 
 	X509_STORE* store = X509_STORE_new ();
 	if (!store) {
 		throw MiscError ("could not create X509 store");
 	}
 
-	int n = 1;
-	for (List::const_iterator i = _certificates.begin(); i != _certificates.end(); ++i) {
+	/* Put all the certificates into the store */
+	for (List::const_iterator i = chain.begin(); i != chain.end(); ++i) {
+		if (!X509_STORE_add_cert (store, i->x509 ())) {
+			X509_STORE_free (store);
+			return false;
+		}
+	}
+
+	/* Verify each one */
+	for (List::const_iterator i = chain.begin(); i != chain.end(); ++i) {
 
 		List::const_iterator j = i;
 		++j;
-		if (j ==  _certificates.end ()) {
+		if (j == chain.end ()) {
 			break;
 		}
 
-		if (!X509_STORE_add_cert (store, i->x509 ())) {
-			X509_STORE_free (store);
-			if (reason) {
-				*reason = "X509_STORE_add_cert failed";
-			}
-			return false;
-		}
-
 		X509_STORE_CTX* ctx = X509_STORE_CTX_new ();
 		if (!ctx) {
 			X509_STORE_free (store);
@@ -443,34 +450,46 @@ CertificateChain::valid (string* reason) const
 		if (!X509_STORE_CTX_init (ctx, store, j->x509(), 0)) {
 			X509_STORE_CTX_free (ctx);
 			X509_STORE_free (store);
-			if (reason) {
-				*reason = "X509_STORE_CTX_init failed";
-			}
-			return false;
+			throw MiscError ("could not initialise X509 store context");
 		}
 
-		int v = X509_verify_cert (ctx);
+		int const v = X509_verify_cert (ctx);
 		X509_STORE_CTX_free (ctx);
 
-		if (v == 0) {
+		if (v != 1) {
+			X509_STORE_free (store);
+			return false;
+		}
+
+		/* I don't know why OpenSSL doesn't check this in verify_cert, but without this check
+		   the certificates_validation8 test fails.
+		*/
+		if (j->issuer() != i->subject()) {
 			X509_STORE_free (store);
-			if (reason) {
-				*reason = String::compose ("X509_verify_cert failed for certificate number %1", n);
-			}
 			return false;
 		}
 
-		++n;
 	}
 
 	X509_STORE_free (store);
 
-	/* Check that the leaf certificate matches the private key, if there is one */
+	return true;
+}
 
-	if (!_key) {
+/** Check that there is a valid private key for the leaf certificate.
+ *  Will return true if there are no certificates.
+ */
+bool
+CertificateChain::private_key_valid () const
+{
+	if (_certificates.empty ()) {
 		return true;
 	}
 
+	if (!_key) {
+		return false;
+	}
+
 	BIO* bio = BIO_new_mem_buf (const_cast<char *> (_key->c_str ()), -1);
 	if (!bio) {
 		throw MiscError ("could not create memory BIO");
@@ -491,29 +510,44 @@ CertificateChain::valid (string* reason) const
 #endif
 	BIO_free (bio);
 
-	if (!valid && reason) {
-		*reason = "leaf certificate does not match private key";
-	}
-
 	return valid;
 }
 
-/** @return true if the chain is now in order from root to leaf,
- *  false if no correct order was found.
- */
 bool
-CertificateChain::attempt_reorder ()
+CertificateChain::valid (string* reason) const
+{
+	try {
+		root_to_leaf ();
+	} catch (CertificateChainError& e) {
+		if (reason) {
+			*reason = "certificates do not form a chain";
+		}
+		return false;
+	}
+
+	if (!private_key_valid ()) {
+		if (reason) {
+			*reason = "private key does not exist, or does not match leaf certificate";
+		}
+		return false;
+	}
+
+	return true;
+}
+
+/** @return Certificates in order from root to leaf */
+CertificateChain::List
+CertificateChain::root_to_leaf () const
 {
-	List original = _certificates;
-	_certificates.sort ();
+	List rtl = _certificates;
+	rtl.sort ();
 	do {
-		if (valid ()) {
-			return true;
+		if (chain_valid (rtl)) {
+			return rtl;
 		}
-	} while (std::next_permutation (_certificates.begin(), _certificates.end ()));
+	} while (std::next_permutation (rtl.begin(), rtl.end()));
 
-	_certificates = original;
-	return false;
+	throw CertificateChainError ("certificate chain is not consistent");
 }
 
 /** Add a &lt;Signer&gt; and &lt;ds:Signature&gt; nodes to an XML node.
diff --git a/src/certificate_chain.h b/src/certificate_chain.h
index 657b9ce3..e38123eb 100644
--- a/src/certificate_chain.h
+++ b/src/certificate_chain.h
@@ -47,6 +47,15 @@ namespace xmlpp {
 	class Node;
 }
 
+struct certificates_validation1;
+struct certificates_validation2;
+struct certificates_validation3;
+struct certificates_validation4;
+struct certificates_validation5;
+struct certificates_validation6;
+struct certificates_validation7;
+struct certificates_validation8;
+
 namespace dcp {
 
 /** @class CertificateChain
@@ -87,9 +96,11 @@ public:
 
 	List leaf_to_root () const;
 	List root_to_leaf () const;
+	List unordered () const;
 
 	bool valid (std::string* reason = 0) const;
-	bool attempt_reorder ();
+	bool chain_valid () const;
+	bool private_key_valid () const;
 
 	void sign (xmlpp::Element* parent, Standard standard) const;
 	void add_signature_value (xmlpp::Node* parent, std::string ns) const;
@@ -106,6 +117,16 @@ public:
 
 private:
 	friend class ::certificates;
+	friend class ::certificates_validation1;
+	friend class ::certificates_validation2;
+	friend class ::certificates_validation3;
+	friend class ::certificates_validation4;
+	friend class ::certificates_validation5;
+	friend class ::certificates_validation6;
+	friend class ::certificates_validation7;
+	friend class ::certificates_validation8;
+
+	bool chain_valid (List const & chain) const;
 
 	/** Our certificates, not in any particular order */
 	List _certificates;
diff --git a/src/exceptions.cc b/src/exceptions.cc
index fbd8c85f..51dd6b01 100644
--- a/src/exceptions.cc
+++ b/src/exceptions.cc
@@ -102,3 +102,9 @@ KDMFormatError::KDMFormatError (std::string message)
 {
 
 }
+
+CertificateChainError::CertificateChainError (std::string message)
+	: runtime_error (message)
+{
+
+}
diff --git a/src/exceptions.h b/src/exceptions.h
index ed77ae3a..fb17801f 100644
--- a/src/exceptions.h
+++ b/src/exceptions.h
@@ -185,6 +185,12 @@ public:
 	KDMFormatError (std::string message);
 };
 
+class CertificateChainError : public std::runtime_error
+{
+public:
+	CertificateChainError (std::string message);
+};
+
 }
 
 #endif
diff --git a/src/util.cc b/src/util.cc
index 6dde5b37..6f09e8aa 100644
--- a/src/util.cc
+++ b/src/util.cc
@@ -243,6 +243,8 @@ dcp::init ()
 	if (xmlSecCryptoInit() < 0) {
 		throw MiscError ("could not initialise xmlsec-crypto");
 	}
+
+	OpenSSL_add_all_algorithms();
 }
 
 /** Decode a base64 string.  The base64 decode routine in KM_util.cpp
diff --git a/test/certificates_test.cc b/test/certificates_test.cc
index 1bec9869..d977b615 100644
--- a/test/certificates_test.cc
+++ b/test/certificates_test.cc
@@ -114,51 +114,101 @@ BOOST_AUTO_TEST_CASE (certificates2)
 	BOOST_CHECK_THROW (dcp::Certificate ("foo"), dcp::MiscError);
 }
 
-/** Check that dcp::CertificateChain::valid() and ::attempt_reorder() basically work */
-BOOST_AUTO_TEST_CASE (certificates_validation)
+/** Check that dcp::CertificateChain::chain_valid() and ::root_to_leaf() basically work */
+BOOST_AUTO_TEST_CASE (certificates_validation1)
 {
-	dcp::CertificateChain good1;
-	good1.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/ca.self-signed.pem")));
-	good1.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/intermediate.signed.pem")));
-	good1.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/leaf.signed.pem")));
-	BOOST_CHECK (good1.valid ());
-
-	dcp::CertificateChain good2;
-	good2.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/ca.self-signed.pem")));
-	BOOST_CHECK (good2.valid ());
-
-	dcp::CertificateChain bad1;
-	bad1.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/intermediate.signed.pem")));
-	bad1.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/leaf.signed.pem")));
-	BOOST_CHECK (!bad1.valid ());
-	BOOST_CHECK (!bad1.attempt_reorder ());
-
-	dcp::CertificateChain bad2;
-	bad2.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/leaf.signed.pem")));
-	bad2.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/ca.self-signed.pem")));
-	bad2.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/intermediate.signed.pem")));
-	BOOST_CHECK (!bad2.valid ());
-	BOOST_CHECK (bad2.attempt_reorder ());
-
-	dcp::CertificateChain bad3;
-	bad3.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/intermediate.signed.pem")));
-	bad3.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/leaf.signed.pem")));
-	bad3.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/ca.self-signed.pem")));
-	BOOST_CHECK (!bad3.valid ());
-	BOOST_CHECK (bad3.attempt_reorder ());
-
-	dcp::CertificateChain bad4;
-	bad4.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/leaf.signed.pem")));
-	bad4.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/intermediate.signed.pem")));
-	bad4.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/ca.self-signed.pem")));
-	BOOST_CHECK (!bad4.valid ());
-	BOOST_CHECK (bad4.attempt_reorder ());
-
-	dcp::CertificateChain bad5;
-	bad5.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/ca.self-signed.pem")));
-	bad5.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/leaf.signed.pem")));
-	BOOST_CHECK (!bad5.valid ());
-	BOOST_CHECK (!bad5.attempt_reorder ());
+	dcp::CertificateChain good;
+	good.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/ca.self-signed.pem")));
+	good.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/intermediate.signed.pem")));
+	good.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/leaf.signed.pem")));
+	BOOST_CHECK (good.chain_valid(good._certificates));
+}
+
+/** Check that dcp::CertificateChain::chain_valid() and ::root_to_leaf() basically work */
+BOOST_AUTO_TEST_CASE (certificates_validation2)
+{
+	dcp::CertificateChain good;
+	good.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/ca.self-signed.pem")));
+	BOOST_CHECK (good.chain_valid(good._certificates));
+}
+
+/** Check that dcp::CertificateChain::chain_valid() and ::root_to_leaf() basically work */
+BOOST_AUTO_TEST_CASE (certificates_validation3)
+{
+	dcp::CertificateChain bad;
+	bad.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/intermediate.signed.pem")));
+	bad.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/leaf.signed.pem")));
+	BOOST_CHECK (!bad.chain_valid(bad._certificates));
+	BOOST_CHECK_THROW (bad.root_to_leaf(), dcp::CertificateChainError);
+}
+
+/** Check that dcp::CertificateChain::chain_valid() and ::root_to_leaf() basically work */
+BOOST_AUTO_TEST_CASE (certificates_validation4)
+{
+	dcp::CertificateChain bad;
+	bad.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/leaf.signed.pem")));
+	bad.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/ca.self-signed.pem")));
+	bad.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/intermediate.signed.pem")));
+	BOOST_CHECK (!bad.chain_valid(bad._certificates));
+	BOOST_CHECK_NO_THROW (bad.root_to_leaf());
+}
+
+/** Check that dcp::CertificateChain::chain_valid() and ::root_to_leaf() basically work */
+BOOST_AUTO_TEST_CASE (certificates_validation5)
+{
+	dcp::CertificateChain bad;
+	bad.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/intermediate.signed.pem")));
+	bad.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/leaf.signed.pem")));
+	bad.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/ca.self-signed.pem")));
+	BOOST_CHECK (!bad.chain_valid(bad._certificates));
+	BOOST_CHECK_NO_THROW (bad.root_to_leaf());
+}
+
+/** Check that dcp::CertificateChain::chain_valid() and ::root_to_leaf() basically work */
+BOOST_AUTO_TEST_CASE (certificates_validation6)
+{
+	dcp::CertificateChain bad;
+	bad.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/leaf.signed.pem")));
+	bad.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/intermediate.signed.pem")));
+	bad.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/ca.self-signed.pem")));
+	BOOST_CHECK (!bad.chain_valid(bad._certificates));
+	BOOST_CHECK_NO_THROW (bad.root_to_leaf());
+}
+
+/** Check that dcp::CertificateChain::chain_valid() and ::root_to_leaf() basically work */
+BOOST_AUTO_TEST_CASE (certificates_validation7)
+{
+	dcp::CertificateChain bad;
+	bad.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/ca.self-signed.pem")));
+	bad.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/leaf.signed.pem")));
+	BOOST_CHECK (!bad.chain_valid(bad._certificates));
+	BOOST_CHECK_THROW (bad.root_to_leaf(), dcp::CertificateChainError);
+}
+
+/** Check that dcp::CertificateChain::chain_valid() and ::root_to_leaf() basically work */
+BOOST_AUTO_TEST_CASE (certificates_validation8)
+{
+	dcp::CertificateChain bad;
+	bad.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/ca.self-signed.pem")));
+	bad.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/intermediate.signed.pem")));
+	bad.add (dcp::Certificate (dcp::file_to_string ("test/ref/crypt/ca.self-signed.pem")));
+	BOOST_CHECK (!bad.chain_valid(bad._certificates));
+	BOOST_CHECK_THROW (bad.root_to_leaf(), dcp::CertificateChainError);
+}
+
+/** Check that we can create a valid chain */
+BOOST_AUTO_TEST_CASE (certificates_validation9)
+{
+	dcp::CertificateChain good (
+		boost::filesystem::path ("openssl"),
+		"dcpomatic.com",
+		"dcpomatic.com",
+		".dcpomatic.smpte-430-2.ROOT",
+		".dcpomatic.smpte-430-2.INTERMEDIATE",
+		"CS.dcpomatic.smpte-430-2.LEAF"
+		);
+
+	BOOST_CHECK_NO_THROW (good.root_to_leaf());
 }
 
 /** Check that dcp::Signer::valid() basically works */