On Tue, Dec 20, 2022 at 11:51:33AM -0500, Xavier Simonart wrote:
> stream_ssl_set_key_and_cert is supposed to, whenever either the certificate or
> the private key file changes, re-read both of them.
> It was re-reading them only when both changed.
> So, if, for instance, certificate was changed a few seconds only after
> changing
> the key, the new key and certificate were never applied.
>
> A few patches have been proposed on similar issues.
> This patch tries to take into account the inputs/comments from them i.e.
> - avoid crash on NULL private key and valid certificate
> (from d5d0c94551b6 ("stream-ssl: Fix crash on NULL private key and valid
> certificate."))
> - avoid breaking setup while the second component is not updated
> (from
> https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/
> - update key and cert, if they are valid.
>
> Fixes: d5d0c94551b6 ("stream-ssl: Fix crash on NULL private key and valid
> certificate.")
>
> Signed-off-by: Xavier Simonart <[email protected]>
Hi,
I'm not sure if this patch is still in-flight.
Nonetheless, I've provided some review below.
> ---
> v2: fix 'rl' shadows an earlier one
> v3: fix uggly memory leak
> ---
> lib/stream-ssl.c | 120 +++++++++++++++++++++++++++++++-----------
> tests/ovsdb-server.at | 36 +++++++++++++
> 2 files changed, 126 insertions(+), 30 deletions(-)
>
> diff --git a/lib/stream-ssl.c b/lib/stream-ssl.c
> index 62da9febb..f56cb1ec7 100644
> --- a/lib/stream-ssl.c
> +++ b/lib/stream-ssl.c
> @@ -76,6 +76,12 @@ enum session_type {
> SERVER
> };
>
> +enum ssl_update_result {
> + SSL_UPDATE_ERROR,
> + SSL_NOT_UPDATED,
> + SSL_UPDATED
> +};
> +
...
> @@ -1224,16 +1248,52 @@ stream_ssl_set_certificate_file(const char *file_name)
> *
> * 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.
> + *
> + * If either the certificate or the private key file failed to update (e.g.
> + * empty file), then those files are not applied as this used to cause segv.
> + * This function also avoids breaking setup while the second component is not
> + * updated.
> */
> 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)) {
> + int priv_key_updated = update_ssl_config(&private_key, private_key_file);
enum ssl_update_result might be a better type for priv_key_updated.
> + SSL_CTX *tmp_ctx = NULL;
> +
> + if (((priv_key_updated == SSL_UPDATED) &&
> + (update_ssl_config(&certificate,certificate_file)
> + != SSL_UPDATE_ERROR)) ||
> + ((priv_key_updated != SSL_UPDATE_ERROR) &&
> + (update_ssl_config(&certificate, certificate_file) ==
> SSL_UPDATED))) {
This is a but hard to parse mentally,
so I wrote it out for myself.
1st pass:
priv_key_updated and cert_updated => result
---------------- ------------ ------
1. SSL_UPDATED SSL_UPDATED perform update
2. SSL_UPDATED SSL_NOT_UPDATED perform update
all other cases go to 2nd chance draw
2nd chance draw:
priv_key_updated and cert_updated => action
---------------- ------------ ------
2. (SSL_UPDATED SSL_UPDATED perform update) [*]
SSL_UPDATED SSL_UPDATE_ERROR do not perform update
3. SSL_NOT_UPDATED SSL_UPDATED perform update
SSL_NOT_UPDATED SSL_NOT_UPDATED do not perform update
SSL_NOT_UPDATED SSL_UPDATE_ERROR do not perform update
SSL_UPDATE_ERROR * do not perform update
[*] Not evaluated, as alreaady evaluated in first pass
This leads to the following truth table
priv_key_updated and cert_updated => result
---------------- ------------ ------
1. SSL_UPDATED SSL_UPDATED perform update
2. SSL_UPDATED SSL_NOT_UPDATED perform update
3. SSL_NOT_UPDATED SSL_UPDATED perform update
all othere cases do not perform update
Or in other words, update if:
* There is no error
* At least one SSL_UPDATED
Maybe this is a bit easier on the eyes.
It also avoids the case where update_ssl_config() is called
twice for the certificate.
* completely untested! *
enum ssl_update_result key_updated;
bool do_update = false;
priv_key_updated = update_ssl_config(&private_key, private_key_file);
if (priv_key_updated != SSL_UPDATE_ERROR) {
enum cert_updated;
cert_updated = update_ssl_config(&certificate,
certificate_file);
do_update = priv_key_updated == SSL_UPDATED ||
cert_updated == SSL_UPDATED;
}
if (do_update) {
> + tmp_ctx = new_ssl_ctx();
> + if (tmp_ctx == NULL) {
> + goto error;
> + }
> + if (SSL_CTX_use_certificate_file(tmp_ctx, certificate_file,
> + SSL_FILETYPE_PEM) != 1) {
> + VLOG_ERR_RL(&rl, "SSL_use_certificate_file: %s",
> + ERR_error_string(ERR_get_error(), NULL));
> + goto error;
> + }
> + if (SSL_CTX_use_PrivateKey_file(tmp_ctx, private_key_file,
> + SSL_FILETYPE_PEM) != 1) {
> + VLOG_ERR_RL(&rl, "SSL_use_PrivateKey_file: %s",
> + ERR_error_string(ERR_get_error(), NULL));
> + goto error;
> + }
> + if (SSL_CTX_check_private_key(tmp_ctx) != 1) {
> + VLOG_ERR_RL(&rl, "SSL_check_private_key: %s",
> + ERR_error_string(ERR_get_error(), NULL));
> + goto error;
> + }
> stream_ssl_set_certificate_file__(certificate_file);
> stream_ssl_set_private_key_file__(private_key_file);
> }
> +error:
> + if (tmp_ctx) {
The check on tmp_ctx is not needed,
as SSL_CTX_free does nothing when passed NULL.
Ref: man SSL_CTX_free(3ssl)
> + SSL_CTX_free(tmp_ctx);
> + }
> }
>
> /* Sets SSL ciphers based on string input. Aborts with an error message
> @@ -1427,7 +1487,7 @@ stream_ssl_set_ca_cert_file__(const char *file_name,
> {
> struct stat s;
>
> - if (!update_ssl_config(&ca_cert, file_name) && !force) {
> + if ((update_ssl_config(&ca_cert, file_name) != SSL_UPDATED) && !force) {
> return;
> }
>
> diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at
> index c7b2fe3ae..c2b80748b 100644
> --- a/tests/ovsdb-server.at
> +++ b/tests/ovsdb-server.at
> @@ -1311,6 +1311,42 @@ AT_CHECK([test $(get_memory_value atoms) -eq
> $initial_db_atoms])
> OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> AT_CLEANUP
>
> +
> +AT_SETUP([ssl certificate update])
> +AT_KEYWORDS([ovsdb server ssl])
> +AT_SKIP_IF([test "$HAVE_OPENSSL" = no])
> +PKIDIR=$abs_top_builddir/tests
> +AT_CHECK([ovsdb-tool create db $abs_top_srcdir/vswitchd/vswitch.ovsschema],
> [0], [stdout], [ignore])
> +on_exit 'kill `cat *.pid`'
> +
> +# ssl certificate update with empty private_key
> +AT_CHECK([ovsdb-server --log-file --detach --no-chdir --pidfile
> --private-key=db:Open_vSwitch,SSL,private_key
> --certificate=db:Open_vSwitch,SSL,certificate
> --bootstrap-ca-cert=db:Open_vSwitch,SSL,ca_cert --remote=punix:./db.sock
> --remote=db:Open_vSwitch,Open_vSwitch,manager_options db], [0], [ignore],
> [ignore])
> +AT_CHECK([ovs-vsctl --no-wait init])
> +AT_CHECK([ovs-vsctl --no-wait set-ssl pkey.key cert.cert ca.cert])
> +AT_CHECK([ovs-vsctl --no-wait set SSL . private_key='""'])
> +AT_CHECK([ovs-vsctl --no-wait set SSL . certificate='cert.new'])
> +
> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> +
> +# Use wrong key/cert combination
> +cp $PKIDIR/testpki-privkey2.pem key.pem
> +cp $PKIDIR/testpki-cert.pem cert.pem
> +AT_CHECK([ovsdb-server --log-file --detach --no-chdir --pidfile
> --private-key=key.pem --certificate=cert.pem
> --ca-cert=$PKIDIR/testpki-cacert.pem --remote=pssl:0:127.0.0.1 db], [0],
> [ignore], [ignore])
> +PARSE_LISTENING_PORT([ovsdb-server.log], [SSL_PORT])
> +OVS_WAIT_UNTIL([grep "ERR|SSL_use_PrivateKey_file" ovsdb-server.log | grep
> mismatch])
> +
> +# Use good combination and try to connect
> +cp $PKIDIR/testpki-cert2.pem cert.pem
> +AT_CHECK([ovsdb-client --private-key=$PKIDIR/testpki-privkey.pem
> --certificate=$PKIDIR/testpki-cert.pem --ca-cert=$PKIDIR/testpki-cacert.pem
> wait ssl:127.0.0.1:$SSL_PORT _Server connected > ovsdb-client.stdout 2>
> ovsdb-client.stderr])
> +
> +# Set wrong key/cert combination
> +cp $PKIDIR/testpki-privkey.pem key.pem
> +# Check that we can still connect
> +AT_CHECK([ovsdb-client --private-key=$PKIDIR/testpki-privkey.pem
> --certificate=$PKIDIR/testpki-cert.pem --ca-cert=$PKIDIR/testpki-cacert.pem
> wait ssl:127.0.0.1:$SSL_PORT _Server connected > ovsdb-client.stdout 2>
> ovsdb-client.stderr])
> +
> +OVSDB_SERVER_SHUTDOWN
> +AT_CLEANUP
> +
> AT_BANNER([OVSDB -- ovsdb-server transactions (SSL IPv4 sockets)])
>
> # OVSDB_CHECK_EXECUTION(TITLE, SCHEMA, TRANSACTIONS, OUTPUT, [KEYWORDS])
> --
> 2.31.1
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
