| Message ID | 1441224160-11790-2-git-send-email-gshetty@nicira.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Wed, Sep 02, 2015 at 01:02:39PM -0700, Gurucharan Shetty wrote: > When --certificate option is provided, we currently use > SSL_CTX_use_certificate_chain_file() function to add > that certificate. If our single certificate file had multiple > certificates (as a chain), all of them would get added and sent > to the remote peer. But once you call > SSL_CTX_use_certificate_chain_file(), any future calls to > SSL_CTX_add_extra_chain_cert() (called when --peer-ca-cert option > is used) had no effect. > > Since our man pages and INSTALL.SSL.md say that --certificate > is used to specify one certificate and additional certificates > are sent via --peer-ca-cert, this commit changes > SSL_CTX_use_certificate_chain_file() use to > SSL_CTX_use_certificate_file(). With this, additional certificates > can now be added via --peer-ca-cert option. > > The test case added with this commit would fail without the > above changes. > > Signed-off-by: Gurucharan Shetty <gshetty@nicira.com> The use of "command pwd" is puzzling here, does it have something to do with Windows? But I thought we'd fixed the problem that ovs-pki had with Windows, so is it necessary? +AT_SETUP([peer ca cert]) +AT_KEYWORDS([ovs-vsctl ssl]) +AT_SKIP_IF([test "$HAVE_OPENSSL" = no]) +PKIDIR=`command pwd` The &&s and \s here are a little puzzling too. Do they do something useful? (Should we be checking return values by using AT_CHECK?) $OVS_PKI -B 1024 init && \ $OVS_PKI -B 1024 req+sign vsctl switch && \ $OVS_PKI -B 1024 req+sign ovsdbserver controller I see why the initial execution of ovs-vsctl ignores the output, but could the post-bootstrap connection check the output? It would be a better test if it did. Thanks, Ben.
On Tue, Sep 8, 2015 at 3:36 PM, Ben Pfaff <blp@nicira.com> wrote: > On Wed, Sep 02, 2015 at 01:02:39PM -0700, Gurucharan Shetty wrote: >> When --certificate option is provided, we currently use >> SSL_CTX_use_certificate_chain_file() function to add >> that certificate. If our single certificate file had multiple >> certificates (as a chain), all of them would get added and sent >> to the remote peer. But once you call >> SSL_CTX_use_certificate_chain_file(), any future calls to >> SSL_CTX_add_extra_chain_cert() (called when --peer-ca-cert option >> is used) had no effect. >> >> Since our man pages and INSTALL.SSL.md say that --certificate >> is used to specify one certificate and additional certificates >> are sent via --peer-ca-cert, this commit changes >> SSL_CTX_use_certificate_chain_file() use to >> SSL_CTX_use_certificate_file(). With this, additional certificates >> can now be added via --peer-ca-cert option. >> >> The test case added with this commit would fail without the >> above changes. >> >> Signed-off-by: Gurucharan Shetty <gshetty@nicira.com> > > The use of "command pwd" is puzzling here, does it have something to do > with Windows? But I thought we'd fixed the problem that ovs-pki had > with Windows, so is it necessary? Ugh, I had this test in my tree before the ovs-pki fix went in, and I forgot. > > +AT_SETUP([peer ca cert]) > +AT_KEYWORDS([ovs-vsctl ssl]) > +AT_SKIP_IF([test "$HAVE_OPENSSL" = no]) > +PKIDIR=`command pwd` > > The &&s and \s here are a little puzzling too. Do they do something > useful? (Should we be checking return values by using AT_CHECK?) > > $OVS_PKI -B 1024 init && \ > $OVS_PKI -B 1024 req+sign vsctl switch && \ > $OVS_PKI -B 1024 req+sign ovsdbserver controller I will do that. > > I see why the initial execution of ovs-vsctl ignores the output, but > could the post-bootstrap connection check the output? It would be a > better test if it did. I will do this. This also has to be done for the previous patch. So I will resend the series. > > Thanks, > > Ben.
diff --git a/lib/stream-ssl.c b/lib/stream-ssl.c index 8b063ba..564c94c 100644 --- a/lib/stream-ssl.c +++ b/lib/stream-ssl.c @@ -1071,7 +1071,7 @@ stream_ssl_set_private_key_file(const char *file_name) static void stream_ssl_set_certificate_file__(const char *file_name) { - if (SSL_CTX_use_certificate_chain_file(ctx, file_name) == 1) { + if (SSL_CTX_use_certificate_file(ctx, file_name, SSL_FILETYPE_PEM) == 1) { certificate.read = true; } else { VLOG_ERR("SSL_use_certificate_file: %s", diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at index cbfa6c2..1ec06e7 100644 --- a/tests/ovs-vsctl.at +++ b/tests/ovs-vsctl.at @@ -1336,3 +1336,30 @@ AT_CHECK([ovs-vsctl -t 5 --db=ssl:127.0.0.1:$SSL_PORT --private-key=$PKIDIR/vsct OVSDB_SERVER_SHUTDOWN AT_CLEANUP + +AT_SETUP([peer ca cert]) +AT_KEYWORDS([ovs-vsctl ssl]) +AT_SKIP_IF([test "$HAVE_OPENSSL" = no]) +PKIDIR=`command pwd` +OVS_PKI="sh $abs_top_srcdir/utilities/ovs-pki.in --dir=$PKIDIR/pki --log=$PKIDIR/ovs-pki.log" +$OVS_PKI -B 1024 init && \ +$OVS_PKI -B 1024 req+sign vsctl switch && \ +$OVS_PKI -B 1024 req+sign ovsdbserver controller + +dnl Create database. +OVSDB_INIT([conf.db]) +AT_CHECK([ovsdb-server --detach --no-chdir --pidfile="`pwd`"/pid --private-key=$PKIDIR/ovsdbserver-privkey.pem --certificate=$PKIDIR/ovsdbserver-cert.pem --ca-cert=$PKIDIR/pki/switchca/cacert.pem --peer-ca-cert=$PKIDIR/pki/controllerca/cacert.pem --remote=pssl:0:127.0.0.1 --unixctl="`pwd`"/unixctl --log-file="`pwd`"/ovsdb-server.log conf.db], [0], [ignore], [ignore]) +ON_EXIT_UNQUOTED([kill `cat pid`]) +SSL_PORT=`parse_listening_port < ovsdb-server.log` + +# During bootstrap, the connection gets torn down. So the o/p of ovs-vsctl is error. +AT_CHECK([ovs-vsctl -t 5 --db=ssl:127.0.0.1:$SSL_PORT --private-key=$PKIDIR/vsctl-privkey.pem --certificate=$PKIDIR/vsctl-cert.pem --bootstrap-ca-cert=$PKIDIR/cacert.pem show], [1], [ignore], [ignore]) + +# If the bootstrap was successful, the following file should exist. +OVS_WAIT_UNTIL([test -e $PKIDIR/cacert.pem]) + +# After bootstrap, the connection should be successful. +AT_CHECK([ovs-vsctl -t 5 --db=ssl:127.0.0.1:$SSL_PORT --private-key=$PKIDIR/vsctl-privkey.pem --certificate=$PKIDIR/vsctl-cert.pem --bootstrap-ca-cert=$PKIDIR/cacert.pem show], [0], [ignore], [ignore]) + +OVSDB_SERVER_SHUTDOWN +AT_CLEANUP
When --certificate option is provided, we currently use SSL_CTX_use_certificate_chain_file() function to add that certificate. If our single certificate file had multiple certificates (as a chain), all of them would get added and sent to the remote peer. But once you call SSL_CTX_use_certificate_chain_file(), any future calls to SSL_CTX_add_extra_chain_cert() (called when --peer-ca-cert option is used) had no effect. Since our man pages and INSTALL.SSL.md say that --certificate is used to specify one certificate and additional certificates are sent via --peer-ca-cert, this commit changes SSL_CTX_use_certificate_chain_file() use to SSL_CTX_use_certificate_file(). With this, additional certificates can now be added via --peer-ca-cert option. The test case added with this commit would fail without the above changes. Signed-off-by: Gurucharan Shetty <gshetty@nicira.com> --- lib/stream-ssl.c | 2 +- tests/ovs-vsctl.at | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-)