From: Ben Pfaff <blp@nicira.com>
Date: Thu, 5 Aug 2010 16:24:00 +0000 (-0700)
Subject: stream-ssl: Make changing keys and certificate at runtime reliable.
X-Git-Tag: v1.1.0pre1~121
X-Git-Url: http://git.onelab.eu/?a=commitdiff_plain;h=6f1e91b1d7c058d701145080c344bbc531b394ed;p=sliver-openvswitch.git

stream-ssl: Make changing keys and certificate at runtime reliable.

OpenSSL is picky about the order in which keys and certificates are
changed: you have to change the certificate first, then the key.  It
doesn't document this, but deep in the source code, in a function that sets
a new certificate, it has this comment:

    /* don't fail for a cert/key mismatch, just free
     * current private key (when switching to a different
     * cert & key, first this function should be used,
     * then ssl_set_pkey */

Brilliant, guys, thanks a lot.

Bug #2921.
---

diff --git a/lib/stream-ssl.c b/lib/stream-ssl.c
index 851e7a76f..34dcc1f98 100644
--- a/lib/stream-ssl.c
+++ b/lib/stream-ssl.c
@@ -975,32 +975,75 @@ update_ssl_config(struct ssl_config_file *config, const char *file_name)
     return true;
 }
 
+static void
+stream_ssl_set_private_key_file__(const char *file_name)
+{
+    if (SSL_CTX_use_PrivateKey_file(ctx, file_name, SSL_FILETYPE_PEM) == 1) {
+        private_key.read = true;
+    } else {
+        VLOG_ERR("SSL_use_PrivateKey_file: %s",
+                 ERR_error_string(ERR_get_error(), NULL));
+    }
+}
+
 void
 stream_ssl_set_private_key_file(const char *file_name)
 {
-    if (!update_ssl_config(&private_key, file_name)) {
-        return;
+    if (update_ssl_config(&private_key, file_name)) {
+        stream_ssl_set_private_key_file__(file_name);
     }
-    if (SSL_CTX_use_PrivateKey_file(ctx, file_name, SSL_FILETYPE_PEM) != 1) {
-        VLOG_ERR("SSL_use_PrivateKey_file: %s",
+}
+
+static void
+stream_ssl_set_certificate_file__(const char *file_name)
+{
+    if (SSL_CTX_use_certificate_chain_file(ctx, file_name) == 1) {
+        certificate.read = true;
+    } else {
+        VLOG_ERR("SSL_use_certificate_file: %s",
                  ERR_error_string(ERR_get_error(), NULL));
-        return;
     }
-    private_key.read = true;
 }
 
 void
 stream_ssl_set_certificate_file(const char *file_name)
 {
-    if (!update_ssl_config(&certificate, file_name)) {
-        return;
+    if (update_ssl_config(&certificate, file_name)) {
+        stream_ssl_set_certificate_file__(file_name);
     }
-    if (SSL_CTX_use_certificate_chain_file(ctx, file_name) != 1) {
-        VLOG_ERR("SSL_use_certificate_file: %s",
-                 ERR_error_string(ERR_get_error(), NULL));
-        return;
+}
+
+/* Sets the private key and certificate files in one operation.  Use this
+ * interface, instead of calling stream_ssl_set_private_key_file() and
+ * stream_ssl_set_certificate_file() individually, in the main loop of a
+ * long-running program whose key and certificate might change at runtime.
+ *
+ * This is important because of OpenSSL's behavior.  If an OpenSSL context
+ * already has a certificate, and stream_ssl_set_private_key_file() is called
+ * to install a new private key, OpenSSL will report an error because the new
+ * private key does not match the old certificate.  The other order, of setting
+ * a new certificate, then setting a new private key, does work.
+ *
+ * If this were the only problem, calling stream_ssl_set_certificate_file()
+ * before stream_ssl_set_private_key_file() would fix it.  But, if the private
+ * key is changed before the certificate (e.g. someone "scp"s or "mv"s the new
+ * private key in place before the certificate), then OpenSSL would reject that
+ * change, and then the change of certificate would succeed, but there would be
+ * no associated private key (because it had only changed once and therefore
+ * there was no point in re-reading it).
+ *
+ * This function avoids both problems by, whenever either the certificate or
+ * the private key file changes, re-reading both of them, in the correct order.
+ */
+void
+stream_ssl_set_key_and_cert(const char *private_key_file,
+                            const char *certificate_file)
+{
+    if (update_ssl_config(&private_key, private_key_file)
+        || update_ssl_config(&certificate, certificate_file)) {
+        stream_ssl_set_certificate_file__(certificate_file);
+        stream_ssl_set_private_key_file__(private_key_file);
     }
-    certificate.read = true;
 }
 
 /* Reads the X509 certificate or certificates in file 'file_name'.  On success,
diff --git a/lib/stream-ssl.h b/lib/stream-ssl.h
index dd2a16ee8..ba6e422ed 100644
--- a/lib/stream-ssl.h
+++ b/lib/stream-ssl.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009 Nicira Networks.
+ * Copyright (c) 2008, 2009, 2010 Nicira Networks.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -20,9 +20,15 @@
 
 #ifdef HAVE_OPENSSL
 bool stream_ssl_is_configured(void);
+
 void stream_ssl_set_private_key_file(const char *file_name);
 void stream_ssl_set_certificate_file(const char *file_name);
 void stream_ssl_set_ca_cert_file(const char *file_name, bool bootstrap);
+
+void stream_ssl_set_key_and_cert(const char *private_key_file,
+                                 const char *certificate_file);
+
+
 void stream_ssl_set_peer_ca_cert_file(const char *file_name);
 
 /* Define the long options for SSL support.
diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
index 4ca9c2d1f..27db0702c 100644
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -283,8 +283,8 @@ reconfigure_from_db(struct ovsdb_jsonrpc_server *jsonrpc,
 
 #if HAVE_OPENSSL
     /* Configure SSL. */
-    stream_ssl_set_private_key_file(query_db_string(db, private_key_file));
-    stream_ssl_set_certificate_file(query_db_string(db, certificate_file));
+    stream_ssl_set_key_and_cert(query_db_string(db, private_key_file),
+                                query_db_string(db, certificate_file));
     stream_ssl_set_ca_cert_file(query_db_string(db, ca_cert_file),
                                 bootstrap_ca_cert);
 #endif
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 12bad0bb4..0397e0a23 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -350,8 +350,7 @@ bridge_configure_ssl(const struct ovsrec_ssl *ssl)
 {
     /* XXX SSL should be configurable on a per-bridge basis. */
     if (ssl) {
-        stream_ssl_set_private_key_file(ssl->private_key);
-        stream_ssl_set_certificate_file(ssl->certificate);
+        stream_ssl_set_key_and_cert(ssl->private_key, ssl->certificate);
         stream_ssl_set_ca_cert_file(ssl->ca_cert, ssl->bootstrap_ca_cert);
     }
 }