On Thu, 2023-04-27 at 14:10 +0200, Ilya Maximets wrote:
> On 4/27/23 11:50, Stefan Hoffmann wrote:
> > This tests stream.c and stream.py with ssl connection at
> > CHECK_STREAM_OPEN_BLOCK.
> > For the tests, ovsdb needs to be build with libssl.
> > 
> > Signed-off-by: Stefan Hoffmann <[email protected]>
> 
> Hi, Stefan.  Thanks for the patch!
> 
> A few comments inline.
> 
> Best regards, Ilya Maximets.
> 

Hi Ilya,

thanks for your feedback and pointing me to the right places. A new
version is send.

I also have some obvious unittests. Like: 

mock do_handshake() raises SSLZeroReturnError
assert connect() returns get_exception_errno()

Should I also add them or is that not needed?

Best regards
Stefan
> > ---
> >  tests/ovsdb-idl.at   | 29 ++++++++++++++++++++++++-----
> >  tests/test-stream.c  |  5 +++++
> >  tests/test-stream.py |  6 ++++++
> >  3 files changed, 35 insertions(+), 5 deletions(-)
> > 
> > diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
> > index 5a7e76eaa..ad9c1b5a1 100644
> > --- a/tests/ovsdb-idl.at
> > +++ b/tests/ovsdb-idl.at
> > @@ -8,7 +8,13 @@ m4_divert_text([PREPARE_TESTS], [
> >  # specified).
> >  ovsdb_start_idltest () {
> >      ovsdb-tool create db ${2:-$abs_srcdir/idltest.ovsschema} || return $?
> > -    ovsdb-server -vconsole:warn --log-file --detach --no-chdir --pidfile 
> > --remote=punix:socket ${1:+--remote=$1} db || return $?
> > +    SSL_FLAGS=""
> > +    if [[ "${1::4}" == "pssl" ]]; then
> > +      openssl genrsa -out testca.key 2048
> > +      openssl req -x509 -new -nodes -key testca.key -sha256 -days 3650 
> > -out testca.crt -subj "/CN=OVS-TEST"
> 
> There is no need to generate new certificates.  There are already
> a few pre-generated certificates available in the test directory.
> See the OVSDB_CHECK_IDL_SSL_PY on where to find them.
> 
> Also, these openssl commands generate unwanted output that is causing
> CI failures.
> 
> > +      SSL_FLAGS="--private-key=testca.key --certificate=testca.crt 
> > --ca-cert=testca.crt"
> > +    fi
> > +    ovsdb-server -vconsole:warn --log-file --detach --no-chdir --pidfile 
> > $SSL_FLAGS --remote=punix:socket ${1:+--remote=$1} db || return $?
> >      on_exit 'kill `cat ovsdb-server.pid`'
> >  }
> >  
> > @@ -2279,14 +2285,21 @@ m4_define([CHECK_STREAM_OPEN_BLOCK],
> >    [AT_SETUP([Check stream open block - $1 - $3])
> >     AT_SKIP_IF([test "$3" = "tcp6" && test "$IS_WIN32" = "yes"])
> >     AT_SKIP_IF([test "$3" = "tcp6" && test "$HAVE_IPV6" = "no"])
> > +   AT_SKIP_IF([test "$3" = "ssl6" && test "$IS_WIN32" = "yes"])
> > +   AT_SKIP_IF([test "$3" = "ssl6" && test "$HAVE_IPV6" = "no"])
> > +   AT_SKIP_IF([test "$3" = "ssl" && test "$HAVE_OPENSSL" != "yes"])
> > +   AT_SKIP_IF([test "$3" = "ssl6" && test "$HAVE_OPENSSL" != "yes"])
> 
> We should also check if python is built with SSL support.
> OVSDB_CHECK_IDL_SSL_PY does that by checking if it's possible
> to import the library.
> 
> >     AT_KEYWORDS([ovsdb server stream open_block $3])
> > -   AT_CHECK([ovsdb_start_idltest "ptcp:0:$4"])
> > +   PROTOCOL=$3
> > +   PROTOCOL=${PROTOCOL::3}
> > +   LISTEN_PROTOCOL=p$PROTOCOL
> > +   AT_CHECK([ovsdb_start_idltest "$LISTEN_PROTOCOL:0:$4"])
> >     PARSE_LISTENING_PORT([ovsdb-server.log], [TCP_PORT])
> >     WRONG_PORT=$(($TCP_PORT + 101))
> > -   AT_CHECK([$2 tcp:$4:$TCP_PORT], [0], [ignore])
> > -   AT_CHECK([$2 tcp:$4:$WRONG_PORT], [1], [ignore], [ignore])
> > +   AT_CHECK([$2 $PROTOCOL:$4:$TCP_PORT], [0], [ignore])
> > +   AT_CHECK([$2 $PROTOCOL:$4:$WRONG_PORT], [1], [ignore], [ignore])
> >     OVSDB_SERVER_SHUTDOWN
> > -   AT_CHECK([$2 tcp:$4:$TCP_PORT], [1], [ignore], [ignore])
> > +   AT_CHECK([$2 $PROTOCOL:$4:$TCP_PORT], [1], [ignore], [ignore])
> >     AT_CLEANUP])
> >  
> >  CHECK_STREAM_OPEN_BLOCK([C], [test-stream], [tcp], [127.0.0.1])
> > @@ -2295,6 +2308,12 @@ CHECK_STREAM_OPEN_BLOCK([Python3], [$PYTHON3 
> > $srcdir/test-stream.py],
> >                          [tcp], [127.0.0.1])
> >  CHECK_STREAM_OPEN_BLOCK([Python3], [$PYTHON3 $srcdir/test-stream.py],
> >                          [tcp6], [[[::1]]])
> > +CHECK_STREAM_OPEN_BLOCK([C], [test-stream], [ssl], [127.0.0.1])
> > +CHECK_STREAM_OPEN_BLOCK([C], [test-stream], [ssl6], [[[::1]]])
> > +CHECK_STREAM_OPEN_BLOCK([Python3], [$PYTHON3 $srcdir/test-stream.py],
> > +                        [ssl], [127.0.0.1])
> > +CHECK_STREAM_OPEN_BLOCK([Python3], [$PYTHON3 $srcdir/test-stream.py],
> > +                        [ssl6], [[[::1]]])
> >  
> >  # same as OVSDB_CHECK_IDL but uses Python IDL implementation with tcp
> >  # with multiple remotes to assert the idl connects to the leader of the 
> > Raft cluster
> > diff --git a/tests/test-stream.c b/tests/test-stream.c
> > index 68ce2c544..e70255ffe 100644
> > --- a/tests/test-stream.c
> > +++ b/tests/test-stream.c
> > @@ -19,6 +19,7 @@
> >  #include "fatal-signal.h"
> >  #include "openvswitch/vlog.h"
> >  #include "stream.h"
> > +#include "stream-ssl.h"
> >  #include "util.h"
> >  
> >  VLOG_DEFINE_THIS_MODULE(test_stream);
> > @@ -35,6 +36,10 @@ main(int argc, char *argv[])
> >      if (argc < 2) {
> >          ovs_fatal(0, "usage: %s REMOTE", argv[0]);
> >      }
> > +    if (strncmp("ssl:", argv[1], 4) == 0) {
> > +        stream_ssl_set_ca_cert_file("testca.crt", false);
> > +        stream_ssl_set_key_and_cert("testca.key", "testca.crt");
> > +    }
> >  
> >      error = stream_open_block(stream_open(argv[1], &stream, DSCP_DEFAULT),
> >                                10000, &stream);
> > diff --git a/tests/test-stream.py b/tests/test-stream.py
> > index 93d63c019..4914e3d31 100644
> > --- a/tests/test-stream.py
> > +++ b/tests/test-stream.py
> > @@ -19,6 +19,12 @@ import ovs.stream
> >  
> >  def main(argv):
> >      remote = argv[1]
> > +
> > +    if remote.startswith("ssl"):
> > +        ovs.stream.SSLStream.ssl_set_ca_cert_file('testca.crt')
> > +        ovs.stream.SSLStream.ssl_set_certificate_file('testca.crt')
> > +        ovs.stream.SSLStream.ssl_set_private_key_file('testca.key')
> 
> It's probably better to pass file names as cmdline arguments like
> tests/test-ovsdb.py does.  Same can be applied to the C version.
> No need for argparse or getopt stuff, just plain args should be
> sufficient for now.
> 
> Again, see the OVSDB_CHECK_IDL_SSL_PY for the usage example.
> 
> > +
> >      err, stream = ovs.stream.Stream.open_block(
> >              ovs.stream.Stream.open(remote), 10000)
> >  
> 

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to