[ovs-dev,2/2] stream-ssl: Get peer-ca-cert functionality to work.
diff mbox

Message ID 1441224160-11790-2-git-send-email-gshetty@nicira.com
State Changes Requested
Headers show

Commit Message

Gurucharan Shetty Sept. 2, 2015, 8:02 p.m. UTC
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(-)

Comments

Ben Pfaff Sept. 8, 2015, 10:36 p.m. UTC | #1
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.
Gurucharan Shetty Sept. 11, 2015, 4 p.m. UTC | #2
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.

Patch
diff mbox

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