[ovs-dev] [PATCH] stream-ssl: Improve messages when configuring SSL if it is unsupported.

Justin Pettit jpettit at nicira.com
Tue May 10 07:43:36 UTC 2011


Looks good.  Don't know how much you care, but the commit message has "a error" in it.

--Justin


On May 9, 2011, at 12:44 PM, Ben Pfaff wrote:

> Previously, if --private-key or another option that requires SSL support
> was used, but OVS was built without OpenSSL support, then OVS would fail
> with a error message that the specified option was not supported.  This
> confused users because it made them think that the option had been removed:
>    http://openvswitch.org/pipermail/discuss/2011-April/005034.html
> 
> This commit improves the error message: OVS will now report that it was
> built without SSL support.  This should be make the problem clear to users.
> 
> Reported-by: Aaron Rosen <arosen at clemson.edu>
> Feature #5325.
> ---
> lib/automake.mk            |    2 +
> lib/stream-nossl.c         |   76 ++++++++++++++++++++++++++++++++++++++++++++
> lib/stream-ssl.h           |   26 ++-------------
> ovsdb/ovsdb-client.c       |    6 +--
> ovsdb/ovsdb-server.c       |    8 -----
> tests/test-jsonrpc.c       |    6 +---
> utilities/ovs-controller.c |    6 +---
> utilities/ovs-ofctl.c      |    2 +-
> utilities/ovs-openflowd.c  |    6 +---
> utilities/ovs-vsctl.c      |    6 +---
> vswitchd/bridge.c          |    4 +-
> vswitchd/ovs-vswitchd.c    |    7 +---
> 12 files changed, 92 insertions(+), 63 deletions(-)
> create mode 100644 lib/stream-nossl.c
> 
> diff --git a/lib/automake.mk b/lib/automake.mk
> index 7c1977f..efc1fd7 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -220,6 +220,8 @@ lib/dhparams.c: lib/dh1024.pem lib/dh2048.pem lib/dh4096.pem
> 	 openssl dhparam -C -in $(srcdir)/lib/dh4096.pem -noout)	\
> 	| sed 's/\(get_dh[0-9]*\)()/\1(void)/' > lib/dhparams.c.tmp
> 	mv lib/dhparams.c.tmp lib/dhparams.c
> +else
> +lib_libopenvswitch_a_SOURCES += lib/stream-nossl.c
> endif
> 
> EXTRA_DIST += \
> diff --git a/lib/stream-nossl.c b/lib/stream-nossl.c
> new file mode 100644
> index 0000000..cdbbf5d
> --- /dev/null
> +++ b/lib/stream-nossl.c
> @@ -0,0 +1,76 @@
> +/*
> + * Copyright (c) 2011 Nicira Networks.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include <config.h>
> +#include "stream-ssl.h"
> +#include "vlog.h"
> +
> +VLOG_DEFINE_THIS_MODULE(stream_nossl);
> +
> +/* Dummy function definitions, used when OVS is built without OpenSSL. */
> +
> +bool
> +stream_ssl_is_configured(void)
> +{
> +    return false;
> +}
> +
> +static void NO_RETURN
> +nossl_option(const char *detail)
> +{
> +    VLOG_FATAL("%s specified but Open vSwitch was built without SSL support",
> +               detail);
> +}
> +
> +void
> +stream_ssl_set_private_key_file(const char *file_name)
> +{
> +    if (file_name != NULL) {
> +        nossl_option("Private key");
> +    }
> +}
> +
> +void
> +stream_ssl_set_certificate_file(const char *file_name)
> +{
> +    if (file_name != NULL) {
> +        nossl_option("Certificate");
> +    }
> +}
> +
> +void
> +stream_ssl_set_ca_cert_file(const char *file_name, bool bootstrap OVS_UNUSED)
> +{
> +    if (file_name != NULL) {
> +        nossl_option("CA certificate");
> +    }
> +}
> +
> +void
> +stream_ssl_set_peer_ca_cert_file(const char *file_name)
> +{
> +    if (file_name != NULL) {
> +        nossl_option("Peer CA certificate");
> +    }
> +}
> +
> +void
> +stream_ssl_set_key_and_cert(const char *private_key_file,
> +                            const char *certificate_file)
> +{
> +    stream_ssl_set_private_key_file(private_key_file);
> +    stream_ssl_set_certificate_file(certificate_file);
> +}
> diff --git a/lib/stream-ssl.h b/lib/stream-ssl.h
> index 6bea577..29c3120 100644
> --- a/lib/stream-ssl.h
> +++ b/lib/stream-ssl.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2008, 2009, 2010 Nicira Networks.
> + * Copyright (c) 2008, 2009, 2010, 2011 Nicira Networks.
>  *
>  * Licensed under the Apache License, Version 2.0 (the "License");
>  * you may not use this file except in compliance with the License.
> @@ -18,28 +18,18 @@
> 
> #include <stdbool.h>
> 
> -#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_peer_ca_cert_file(const char *file_name);
> 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.
> - *
> - * Note that the definition includes a final comma, and therefore a comma
> - * must not be supplied when using the definition.  This is done so that
> - * compilation succeeds whether or not HAVE_OPENSSL is defined. */
> -#define STREAM_SSL_LONG_OPTIONS                      \
> +#define STREAM_SSL_LONG_OPTIONS                     \
>         {"private-key", required_argument, 0, 'p'}, \
>         {"certificate", required_argument, 0, 'c'}, \
> -        {"ca-cert",     required_argument, 0, 'C'},
> +        {"ca-cert",     required_argument, 0, 'C'}
> 
> #define STREAM_SSL_OPTION_HANDLERS                      \
>         case 'p':                                       \
> @@ -53,13 +43,5 @@ void stream_ssl_set_peer_ca_cert_file(const char *file_name);
>         case 'C':                                       \
>             stream_ssl_set_ca_cert_file(optarg, false); \
>             break;
> -#else /* !HAVE_OPENSSL */
> -static inline bool stream_ssl_is_configured(void)
> -{
> -    return false;
> -}
> -#define STREAM_SSL_LONG_OPTIONS
> -#define STREAM_SSL_OPTION_HANDLERS
> -#endif /* !HAVE_OPENSSL */
> 
> #endif /* stream-ssl.h */
> diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c
> index a66b013..e8afdd6 100644
> --- a/ovsdb/ovsdb-client.c
> +++ b/ovsdb/ovsdb-client.c
> @@ -80,9 +80,9 @@ parse_options(int argc, char *argv[])
>         DAEMON_LONG_OPTIONS,
> #ifdef HAVE_OPENSSL
>         {"bootstrap-ca-cert", required_argument, 0, OPT_BOOTSTRAP_CA_CERT},
> -        TABLE_LONG_OPTIONS,
> -        STREAM_SSL_LONG_OPTIONS
> +        STREAM_SSL_LONG_OPTIONS,
> #endif
> +        TABLE_LONG_OPTIONS,
>         {0, 0, 0, 0},
>     };
>     char *short_options = long_options_to_short_options(long_options);
> @@ -111,13 +111,11 @@ parse_options(int argc, char *argv[])
> 
>         TABLE_OPTION_HANDLERS(&table_style)
> 
> -#ifdef HAVE_OPENSSL
>         STREAM_SSL_OPTION_HANDLERS
> 
>         case OPT_BOOTSTRAP_CA_CERT:
>             stream_ssl_set_ca_cert_file(optarg, true);
>             break;
> -#endif
> 
>         case '?':
>             exit(EXIT_FAILURE);
> diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
> index c9b0fdd..14f0fbf 100644
> --- a/ovsdb/ovsdb-server.c
> +++ b/ovsdb/ovsdb-server.c
> @@ -51,13 +51,11 @@
> 
> VLOG_DEFINE_THIS_MODULE(ovsdb_server);
> 
> -#if HAVE_OPENSSL
> /* SSL configuration. */
> static char *private_key_file;
> static char *certificate_file;
> static char *ca_cert_file;
> static bool bootstrap_ca_cert;
> -#endif
> 
> static unixctl_cb_func ovsdb_server_exit;
> static unixctl_cb_func ovsdb_server_compact;
> @@ -598,13 +596,11 @@ reconfigure_from_db(struct ovsdb_jsonrpc_server *jsonrpc,
>     ovsdb_jsonrpc_server_set_remotes(jsonrpc, &resolved_remotes);
>     shash_destroy_free_data(&resolved_remotes);
> 
> -#if HAVE_OPENSSL
>     /* Configure SSL. */
>     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
> }
> 
> static void
> @@ -671,12 +667,10 @@ parse_options(int argc, char *argv[], char **file_namep,
>         DAEMON_LONG_OPTIONS,
>         VLOG_LONG_OPTIONS,
>         LEAK_CHECKER_LONG_OPTIONS,
> -#ifdef HAVE_OPENSSL
>         {"bootstrap-ca-cert", required_argument, 0, OPT_BOOTSTRAP_CA_CERT},
>         {"private-key", required_argument, 0, 'p'},
>         {"certificate", required_argument, 0, 'c'},
>         {"ca-cert",     required_argument, 0, 'C'},
> -#endif
>         {0, 0, 0, 0},
>     };
>     char *short_options = long_options_to_short_options(long_options);
> @@ -714,7 +708,6 @@ parse_options(int argc, char *argv[], char **file_namep,
>         DAEMON_OPTION_HANDLERS
>         LEAK_CHECKER_OPTION_HANDLERS
> 
> -#ifdef HAVE_OPENSSL
>         case 'p':
>             private_key_file = optarg;
>             break;
> @@ -732,7 +725,6 @@ parse_options(int argc, char *argv[], char **file_namep,
>             ca_cert_file = optarg;
>             bootstrap_ca_cert = true;
>             break;
> -#endif
> 
>         case '?':
>             exit(EXIT_FAILURE);
> diff --git a/tests/test-jsonrpc.c b/tests/test-jsonrpc.c
> index 12bbc97..5d93850 100644
> --- a/tests/test-jsonrpc.c
> +++ b/tests/test-jsonrpc.c
> @@ -60,10 +60,8 @@ parse_options(int argc, char *argv[])
>         {"verbose", optional_argument, 0, 'v'},
>         {"help", no_argument, 0, 'h'},
>         DAEMON_LONG_OPTIONS,
> -#ifdef HAVE_OPENSSL
>         {"bootstrap-ca-cert", required_argument, 0, OPT_BOOTSTRAP_CA_CERT},
> -        STREAM_SSL_LONG_OPTIONS
> -#endif
> +        STREAM_SSL_LONG_OPTIONS,
>         {0, 0, 0, 0},
>     };
>     char *short_options = long_options_to_short_options(long_options);
> @@ -84,13 +82,11 @@ parse_options(int argc, char *argv[])
> 
>         DAEMON_OPTION_HANDLERS
> 
> -#ifdef HAVE_OPENSSL
>         STREAM_SSL_OPTION_HANDLERS
> 
>         case OPT_BOOTSTRAP_CA_CERT:
>             stream_ssl_set_ca_cert_file(optarg, true);
>             break;
> -#endif
> 
>         case '?':
>             exit(EXIT_FAILURE);
> diff --git a/utilities/ovs-controller.c b/utilities/ovs-controller.c
> index ae0ea3d..3844666 100644
> --- a/utilities/ovs-controller.c
> +++ b/utilities/ovs-controller.c
> @@ -324,10 +324,8 @@ parse_options(int argc, char *argv[])
>         {"version",     no_argument, 0, 'V'},
>         DAEMON_LONG_OPTIONS,
>         VLOG_LONG_OPTIONS,
> -#ifdef HAVE_OPENSSL
> -        STREAM_SSL_LONG_OPTIONS
> +        STREAM_SSL_LONG_OPTIONS,
>         {"peer-ca-cert", required_argument, 0, OPT_PEER_CA_CERT},
> -#endif
>         {0, 0, 0, 0},
>     };
>     char *short_options = long_options_to_short_options(long_options);
> @@ -400,13 +398,11 @@ parse_options(int argc, char *argv[])
>         VLOG_OPTION_HANDLERS
>         DAEMON_OPTION_HANDLERS
> 
> -#ifdef HAVE_OPENSSL
>         STREAM_SSL_OPTION_HANDLERS
> 
>         case OPT_PEER_CA_CERT:
>             stream_ssl_set_peer_ca_cert_file(optarg);
>             break;
> -#endif
> 
>         case '?':
>             exit(EXIT_FAILURE);
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index c0ff4dc..6c2ca17 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -92,7 +92,7 @@ parse_options(int argc, char *argv[])
>         {"help", no_argument, 0, 'h'},
>         {"version", no_argument, 0, 'V'},
>         VLOG_LONG_OPTIONS,
> -        STREAM_SSL_LONG_OPTIONS
> +        STREAM_SSL_LONG_OPTIONS,
>         {0, 0, 0, 0},
>     };
>     char *short_options = long_options_to_short_options(long_options);
> diff --git a/utilities/ovs-openflowd.c b/utilities/ovs-openflowd.c
> index 86f5ca5..33ebc68 100644
> --- a/utilities/ovs-openflowd.c
> +++ b/utilities/ovs-openflowd.c
> @@ -277,10 +277,8 @@ parse_options(int argc, char *argv[], struct ofsettings *s)
>         DAEMON_LONG_OPTIONS,
>         VLOG_LONG_OPTIONS,
>         LEAK_CHECKER_LONG_OPTIONS,
> -#ifdef HAVE_OPENSSL
> -        STREAM_SSL_LONG_OPTIONS
> +        STREAM_SSL_LONG_OPTIONS,
>         {"bootstrap-ca-cert", required_argument, 0, OPT_BOOTSTRAP_CA_CERT},
> -#endif
>         {0, 0, 0, 0},
>     };
>     char *short_options = long_options_to_short_options(long_options);
> @@ -448,13 +446,11 @@ parse_options(int argc, char *argv[], struct ofsettings *s)
> 
>         LEAK_CHECKER_OPTION_HANDLERS
> 
> -#ifdef HAVE_OPENSSL
>         STREAM_SSL_OPTION_HANDLERS
> 
>         case OPT_BOOTSTRAP_CA_CERT:
>             stream_ssl_set_ca_cert_file(optarg, true);
>             break;
> -#endif
> 
>         case '?':
>             exit(EXIT_FAILURE);
> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> index 2c1ba6d..2b58b15 100644
> --- a/utilities/ovs-vsctl.c
> +++ b/utilities/ovs-vsctl.c
> @@ -216,10 +216,8 @@ parse_options(int argc, char *argv[])
>         {"version", no_argument, 0, 'V'},
>         VLOG_LONG_OPTIONS,
>         TABLE_LONG_OPTIONS,
> -#ifdef HAVE_OPENSSL
> -        STREAM_SSL_LONG_OPTIONS
> +        STREAM_SSL_LONG_OPTIONS,
>         {"peer-ca-cert", required_argument, 0, OPT_PEER_CA_CERT},
> -#endif
>         {0, 0, 0, 0},
>     };
>     char *tmp, *short_options;
> @@ -277,13 +275,11 @@ parse_options(int argc, char *argv[])
>         VLOG_OPTION_HANDLERS
>         TABLE_OPTION_HANDLERS(&table_style)
> 
> -#ifdef HAVE_OPENSSL
>         STREAM_SSL_OPTION_HANDLERS
> 
>         case OPT_PEER_CA_CERT:
>             stream_ssl_set_peer_ca_cert_file(optarg);
>             break;
> -#endif
> 
>         case '?':
>             exit(EXIT_FAILURE);
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index d883596..6bf4f8f 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -1401,7 +1401,7 @@ bridge_run(void)
>     /* (Re)configure if necessary. */
>     database_changed = ovsdb_idl_run(idl);
>     cfg = ovsrec_open_vswitch_first(idl);
> -#ifdef HAVE_OPENSSL
> +
>     /* Re-configure SSL.  We do this on every trip through the main loop,
>      * instead of just when the database changes, because the contents of the
>      * key and certificate files can change without the database changing.
> @@ -1414,7 +1414,7 @@ bridge_run(void)
>         stream_ssl_set_key_and_cert(ssl->private_key, ssl->certificate);
>         stream_ssl_set_ca_cert_file(ssl->ca_cert, ssl->bootstrap_ca_cert);
>     }
> -#endif
> +
>     if (database_changed || datapath_destroyed) {
>         if (cfg) {
>             struct ovsdb_idl_txn *txn = ovsdb_idl_txn_create(idl);
> diff --git a/vswitchd/ovs-vswitchd.c b/vswitchd/ovs-vswitchd.c
> index b9a2461..626cba4 100644
> --- a/vswitchd/ovs-vswitchd.c
> +++ b/vswitchd/ovs-vswitchd.c
> @@ -128,11 +128,9 @@ parse_options(int argc, char *argv[])
>         DAEMON_LONG_OPTIONS,
>         VLOG_LONG_OPTIONS,
>         LEAK_CHECKER_LONG_OPTIONS,
> -#ifdef HAVE_OPENSSL
> -        STREAM_SSL_LONG_OPTIONS
> +        STREAM_SSL_LONG_OPTIONS,
>         {"peer-ca-cert", required_argument, 0, OPT_PEER_CA_CERT},
>         {"bootstrap-ca-cert", required_argument, 0, OPT_BOOTSTRAP_CA_CERT},
> -#endif
>         {"enable-dummy", no_argument, 0, OPT_ENABLE_DUMMY},
>         {0, 0, 0, 0},
>     };
> @@ -168,8 +166,6 @@ parse_options(int argc, char *argv[])
>         VLOG_OPTION_HANDLERS
>         DAEMON_OPTION_HANDLERS
>         LEAK_CHECKER_OPTION_HANDLERS
> -
> -#ifdef HAVE_OPENSSL
>         STREAM_SSL_OPTION_HANDLERS
> 
>         case OPT_PEER_CA_CERT:
> @@ -179,7 +175,6 @@ parse_options(int argc, char *argv[])
>         case OPT_BOOTSTRAP_CA_CERT:
>             stream_ssl_set_ca_cert_file(optarg, true);
>             break;
> -#endif
> 
>         case OPT_ENABLE_DUMMY:
>             dummy_enable();
> -- 
> 1.7.4.4
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev




More information about the dev mailing list