Hi Ales: Ping. Following up for the test coverage bits. I would like to keep it slick for the bug fix/feature gap first. Please confirm so I can amend accordingly.
Ali On Fri, Jan 12, 2024 at 8:11 AM aginwala <[email protected]> wrote: > Thanks for review. > > On Fri, Jan 12, 2024 at 2:08 AM Ales Musil <[email protected]> wrote: > >> On Thu, Jan 11, 2024 at 2:30 PM <[email protected]> wrote: >> >> > From: Aliasgar Ginwala <[email protected]> >> > >> > Signed-off-by: Aliasgar Ginwala <[email protected]> >> > --- >> > >> >> Hi Aliasgar, >> >> thank you for the series. Those two patches should be squashed to one and >> I >> have a couple of small comments down below. >> > Can it be done while applying. I realized I didnt add tests so added an > add on commit for the same. Would be neat to keep it that way too. Let me > know I can still proceed to squash in one commit if not acceptable. > >> >> >> > tests/ovn-controller.at | 26 ++++++ >> > tests/ovn.at | 182 ++++++++++++++++++++++++++++++++++++++++ >> > 2 files changed, 208 insertions(+) >> > >> > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at >> > index 9d2a37c72..df5662527 100644 >> > --- a/tests/ovn-controller.at >> > +++ b/tests/ovn-controller.at >> > @@ -2712,3 +2712,29 @@ AT_CHECK([as hv1 ovs-ofctl dump-flows br-int >> > table=40 | grep -q controller], [1] >> > OVN_CLEANUP([hv1]) >> > AT_CLEANUP >> > ]) >> > + >> > + >> > +AT_SETUP([ovn-controller - ssl ciphers using command line options]) >> > +AT_KEYWORDS([ovn]) >> > +AT_SKIP_IF([test "$HAVE_OPENSSL" = no]) >> > +PKIDIR="$(cd $abs_top_builddir/tests && pwd)" >> > +AT_SKIP_IF([expr "$PKIDIR" : ".*[[ '\" >> > +\\]]"]) >> > >> >> This check is not needed, as we don't operate with the PKI files directly. >> >> I would like to keep it to cover at-least one case for ssl-cipher arg for > ovn-controller. Its needed as it didnt execute ovs-vsctl del-ssl command so > ovn will use the pki bits from ovsdb and -ssl-ciphers will be passed > explicitly > >> >> > +ovn_start >> > + >> > +net_add n1 >> > +sim_add hv1 >> > +ovs-vsctl add-br br-phys >> > +ovn_attach n1 br-phys 192.168.0.20 >> > + >> > +# Set cipher and and it should connect >> > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) >> > +start_daemon ovn-controller --ssl-ciphers='HIGH:!aNULL:!MD5:@SECLEVEL >> =1' >> > --ssl-protocols='TLSv1,TLSv1.1,TLSv1.2' >> > + >> > +OVS_WAIT_FOR_OUTPUT([ovn-appctl -t ovn-controller connection-status], >> > [0], [connected >> > +]) >> > + >> > +cat hv1/ovn-controller.log >> > + >> > +OVN_CLEANUP([hv1]) >> > +AT_CLEANUP >> > diff --git a/tests/ovn.at b/tests/ovn.at >> > index c3644ac78..34f277ef9 100644 >> > --- a/tests/ovn.at >> > +++ b/tests/ovn.at >> > @@ -37588,3 +37588,185 @@ OVN_CLEANUP([hv1]) >> > >> > AT_CLEANUP >> > ]) >> > + >> > +AT_SETUP([read-only sb db:pssl access with ssl-ciphers and >> ssl-protocols]) >> > +AT_SKIP_IF([test "$HAVE_OPENSSL" = no]) >> > +PKIDIR="$(cd $abs_top_builddir/tests && pwd)" >> > +AT_SKIP_IF([expr "$PKIDIR" : ".*[[ '\" >> > +\\]]"]) >> > + >> > +: > .$1.db.~lock~ >> > +ovsdb-tool create ovn-sb.db "$abs_top_srcdir"/ovn-sb.ovsschema >> > + >> > +# Add read-only remote to sb ovsdb-server >> > +AT_CHECK( >> > + [ovsdb-tool transact ovn-sb.db \ >> > + ['["OVN_Southbound", >> > + {"op": "insert", >> > + "table": "SB_Global", >> > + "row": { >> > + "connections": ["set", [["named-uuid", "xyz"]]]}}, >> > + {"op": "insert", >> > + "table": "Connection", >> > + "uuid-name": "xyz", >> > + "row": {"target": "pssl:0:127.0.0.1", >> > + "read_only": true}}]']], [0], [ignore], [ignore]) >> > + >> > +start_daemon ovsdb-server --remote=punix:ovn-sb.sock \ >> > + >> > --remote=db:OVN_Southbound,SB_Global,connections \ >> > + >> > --private-key="$PKIDIR/testpki-test2-privkey.pem" \ >> > + >> --certificate="$PKIDIR/testpki-test2-cert.pem" \ >> > + --ca-cert="$PKIDIR/testpki-cacert.pem" \ >> > + --ssl-ciphers='HIGH:!aNULL:!MD5:@SECLEVEL=1' >> \ >> > + --ssl-protocols='TLSv1,TLSv1.1,TLSv1.2' \ >> > + ovn-sb.db >> > + >> > +PARSE_LISTENING_PORT([ovsdb-server.log], [TCP_PORT]) >> > + >> > +# read-only accesses should succeed >> > +AT_CHECK([ovn-sbctl --db=ssl:127.0.0.1:$TCP_PORT \ >> > + --private-key=$PKIDIR/testpki-test-privkey.pem \ >> > + --certificate=$PKIDIR/testpki-test-cert.pem \ >> > + --ca-cert=$PKIDIR/testpki-cacert.pem \ >> > + --ssl-ciphers='HIGH:!aNULL:!MD5:@SECLEVEL=1' \ >> > + --ssl-protocols='TLSv1,TLSv1.1,TLSv1.2' \ >> > + list SB_Global], [0], [stdout], [ignore]) >> > +AT_CHECK([ovn-sbctl --db=ssl:127.0.0.1:$TCP_PORT \ >> > + --private-key=$PKIDIR/testpki-test-privkey.pem \ >> > + --certificate=$PKIDIR/testpki-test-cert.pem \ >> > + --ca-cert=$PKIDIR/testpki-cacert.pem \ >> > + --ssl-ciphers='HIGH:!aNULL:!MD5:@SECLEVEL=1' \ >> > + --ssl-protocols='TLSv1,TLSv1.1,TLSv1.2' \ >> > + list Connection], [0], [stdout], [ignore]) >> > + >> > +# write access should fail >> > +AT_CHECK([ovn-sbctl --db=ssl:127.0.0.1:$TCP_PORT \ >> > + --private-key=$PKIDIR/testpki-test-privkey.pem \ >> > + --certificate=$PKIDIR/testpki-test-cert.pem \ >> > + --ca-cert=$PKIDIR/testpki-cacert.pem \ >> > + --ssl-ciphers='HIGH:!aNULL:!MD5:@SECLEVEL=1' \ >> > + --ssl-protocols='TLSv1,TLSv1.1,TLSv1.2' \ >> > + chassis-add ch vxlan 1.2.4.8], [1], [ignore], >> > +[ovn-sbctl: transaction error: {"details":"insert operation not allowed >> > when database server is in read only mode","error":"not allowed"} >> > +]) >> > + >> > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >> > +AT_CLEANUP >> > >> >> I don't think that this test has anything to do with allowed >> ciphers/protocols. We actually have a read only test so we can most likely >> drop this one. >> >> I kept it to cover both read and write part of ssl as we dont have much > coverage in the repo so kept it. Doesnt harm. > >> >> > + >> > +AT_SETUP([nb connection/ssl commands with ssl-ciphers and >> ssl-protocols]) >> > +AT_SKIP_IF([test "$HAVE_OPENSSL" = no]) >> > +PKIDIR="$(cd $abs_top_builddir/tests && pwd)" >> > +AT_SKIP_IF([expr "$PKIDIR" : ".*[[ '\" >> > +\\]]"]) >> > + >> > +: > .$1.db.~lock~ >> > +ovsdb-tool create ovn-nb.db "$abs_top_srcdir"/ovn-nb.ovsschema >> > >> >> nit: To slightly simplify the test you could use something like the below: >> >> m4_define([SSL_PARAMS], [--ssl-ciphers='HIGH:!aNULL:!MD5:@SECLEVEL=1' \ >> --ssl-protocols='TLSv1,TLSv1.1,TLSv1.2']) >> m4_define([PKI], [--private-key=$PKIDIR/testpki-test-privkey.pem \ >> --certificate=$PKIDIR/testpki-test-cert.pem \ >> --ca-cert=$PKIDIR/testpki-cacert.pem]) >> >> + >> > +# Start nb db server using db connection/ssl entries (unpopulated >> > initially) >> > +start_daemon ovsdb-server --remote=punix:ovnnb_db.sock \ >> > + >> > --remote=db:OVN_Northbound,NB_Global,connections \ >> > + >> --private-key=db:OVN_Northbound,SSL,private_key >> > \ >> > + >> --certificate=db:OVN_Northbound,SSL,certificate >> > \ >> > + --ca-cert=db:OVN_Northbound,SSL,ca_cert \ >> > + --ssl-ciphers='HIGH:!aNULL:!MD5:@SECLEVEL=1' >> \ >> > + --ssl-protocols='TLSv1,TLSv1.1,TLSv1.2' \ >> > + ovn-nb.db >> > + >> > +# Populate SSL configuration entries in nb db >> > +AT_CHECK( >> > + [ovn-nbctl set-ssl $PKIDIR/testpki-test-privkey.pem \ >> > + $PKIDIR/testpki-test-cert.pem \ >> > + $PKIDIR/testpki-cacert.pem], [0], [stdout], >> > [ignore]) >> > + >> > +# Populate a passive SSL connection in nb db >> > +AT_CHECK([ovn-nbctl set-connection pssl:0:127.0.0.1], [0], [stdout], >> > [ignore]) >> > + >> > +PARSE_LISTENING_PORT([ovsdb-server.log], [TCP_PORT]) >> > + >> > +# Verify SSL connetivity to nb db server >> > +AT_CHECK([ovn-nbctl --db=ssl:127.0.0.1:$TCP_PORT \ >> > + --private-key=$PKIDIR/testpki-test-privkey.pem \ >> > + --certificate=$PKIDIR/testpki-test-cert.pem \ >> > + --ca-cert=$PKIDIR/testpki-cacert.pem \ >> > + --ssl-ciphers='HIGH:!aNULL:!MD5:@SECLEVEL=1' \ >> > + --ssl-protocols='TLSv1,TLSv1.1,TLSv1.2' \ >> > + list NB_Global], >> > >> >> With the m4_define above this could be reduced to "ovn-nbctl >> --db=ssl:127.0.0.1:$TCP_PORT PKI SSL_PARAMS list NB_Global". >> This is applicable to all the ovn-nbctl and ovn-sbctl commands. >> >> >> I think its better to optimize with the follow up patch as its already in > multiple places e.g > https://github.com/ovn-org/ovn/blob/main/tests/ovn.at#L10342 .That is why > I retained original way. > >> > + [0], [stdout], [ignore]) >> > +AT_CHECK([ovn-nbctl --db=ssl:127.0.0.1:$TCP_PORT \ >> > + --private-key=$PKIDIR/testpki-test-privkey.pem \ >> > + --certificate=$PKIDIR/testpki-test-cert.pem \ >> > + --ca-cert=$PKIDIR/testpki-cacert.pem \ >> > + --ssl-ciphers='HIGH:!aNULL:!MD5:@SECLEVEL=1' \ >> > + --ssl-protocols='TLSv1,TLSv1.1,TLSv1.2' \ >> > + list Connection], >> > + [0], [stdout], [ignore]) >> > +AT_CHECK([ovn-nbctl --db=ssl:127.0.0.1:$TCP_PORT \ >> > + --private-key=$PKIDIR/testpki-test-privkey.pem \ >> > + --certificate=$PKIDIR/testpki-test-cert.pem \ >> > + --ca-cert=$PKIDIR/testpki-cacert.pem \ >> > + --ssl-ciphers='HIGH:!aNULL:!MD5:@SECLEVEL=1' \ >> > + --ssl-protocols='TLSv1,TLSv1.1,TLSv1.2' \ >> > + get-connection], >> > + [0], [stdout], [ignore]) >> > + >> > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >> > +AT_CLEANUP >> > + >> > +AT_SETUP([sb connection/ssl commands with ssl-ciphers and >> ssl-protocols]) >> > +AT_SKIP_IF([test "$HAVE_OPENSSL" = no]) >> > +PKIDIR="$(cd $abs_top_builddir/tests && pwd)" >> > +AT_SKIP_IF([expr "$PKIDIR" : ".*[[ '\" >> > +\\]]"]) >> > + >> > +: > .$1.db.~lock~ >> > +ovsdb-tool create ovn-sb.db "$abs_top_srcdir"/ovn-sb.ovsschema >> > + >> > +# Start sb db server using db connection/ssl entries (unpopulated >> > initially) >> > +start_daemon ovsdb-server --remote=punix:ovnsb_db.sock \ >> > + >> > --remote=db:OVN_Southbound,SB_Global,connections \ >> > + >> --private-key=db:OVN_Southbound,SSL,private_key >> > \ >> > + >> --certificate=db:OVN_Southbound,SSL,certificate >> > \ >> > + --ca-cert=db:OVN_Southbound,SSL,ca_cert \ >> > + --ssl-ciphers='HIGH:!aNULL:!MD5:@SECLEVEL=1' >> \ >> > + --ssl-protocols='TLSv1,TLSv1.1,TLSv1.2' \ >> > + ovn-sb.db >> > + >> > +# Populate SSL configuration entries in sb db >> > +AT_CHECK( >> > + [ovn-sbctl set-ssl $PKIDIR/testpki-test-privkey.pem \ >> > + $PKIDIR/testpki-test-cert.pem \ >> > + $PKIDIR/testpki-cacert.pem], [0], [stdout], >> > [ignore]) >> > + >> > +# Populate a passive SSL connection in sb db >> > +AT_CHECK([ovn-sbctl set-connection pssl:0:127.0.0.1], [0], [stdout], >> > [ignore]) >> > + >> > +PARSE_LISTENING_PORT([ovsdb-server.log], [TCP_PORT]) >> > + >> > +# Verify SSL connetivity to sb db server >> > +AT_CHECK([ovn-sbctl --db=ssl:127.0.0.1:$TCP_PORT \ >> > + --private-key=$PKIDIR/testpki-test-privkey.pem \ >> > + --certificate=$PKIDIR/testpki-test-cert.pem \ >> > + --ca-cert=$PKIDIR/testpki-cacert.pem \ >> > + --ssl-ciphers='HIGH:!aNULL:!MD5:@SECLEVEL=1' \ >> > + --ssl-protocols='TLSv1,TLSv1.1,TLSv1.2' \ >> > + list SB_Global], >> > + [0], [stdout], [ignore]) >> > +AT_CHECK([ovn-sbctl --db=ssl:127.0.0.1:$TCP_PORT \ >> > + --private-key=$PKIDIR/testpki-test-privkey.pem \ >> > + --certificate=$PKIDIR/testpki-test-cert.pem \ >> > + --ca-cert=$PKIDIR/testpki-cacert.pem \ >> > + --ssl-ciphers='HIGH:!aNULL:!MD5:@SECLEVEL=1' \ >> > + --ssl-protocols='TLSv1,TLSv1.1,TLSv1.2' \ >> > + list Connection], >> > + [0], [stdout], [ignore]) >> > +AT_CHECK([ovn-sbctl --db=ssl:127.0.0.1:$TCP_PORT \ >> > + --private-key=$PKIDIR/testpki-test-privkey.pem \ >> > + --certificate=$PKIDIR/testpki-test-cert.pem \ >> > + --ca-cert=$PKIDIR/testpki-cacert.pem \ >> > + --ssl-ciphers='HIGH:!aNULL:!MD5:@SECLEVEL=1' \ >> > + --ssl-protocols='TLSv1,TLSv1.1,TLSv1.2' \ >> > + get-connection], >> > + [0], [stdout], [ignore]) >> > + >> > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >> > +AT_CLEANUP >> > -- >> > 2.39.3 (Apple Git-145) >> > >> > _______________________________________________ >> > dev mailing list >> > [email protected] >> > >> https://urldefense.com/v3/__https://mail.openvswitch.org/mailman/listinfo/ovs-dev__;!!IKRxdwAv5BmarQ!b9pUg4X974A51WZFKZfkdEpZfonrVQK6Py8Vn8OwxGmUPEMOPQt5bxmdT-lSbLg4SRGxmwmpDKmTpQ$ >> > >> > >> Thanks, >> Ales >> >> -- >> >> Ales Musil >> >> Senior Software Engineer - OVN Core >> >> Red Hat EMEA < >> https://urldefense.com/v3/__https://www.redhat.com__;!!IKRxdwAv5BmarQ!b9pUg4X974A51WZFKZfkdEpZfonrVQK6Py8Vn8OwxGmUPEMOPQt5bxmdT-lSbLg4SRGxmwncGrVSrg$ >> > >> >> [email protected] >> < >> https://urldefense.com/v3/__https://red.ht/sig__;!!IKRxdwAv5BmarQ!b9pUg4X974A51WZFKZfkdEpZfonrVQK6Py8Vn8OwxGmUPEMOPQt5bxmdT-lSbLg4SRGxmwlOrm0WvQ$ >> > >> _______________________________________________ >> dev mailing list >> [email protected] >> >> https://urldefense.com/v3/__https://mail.openvswitch.org/mailman/listinfo/ovs-dev__;!!IKRxdwAv5BmarQ!b9pUg4X974A51WZFKZfkdEpZfonrVQK6Py8Vn8OwxGmUPEMOPQt5bxmdT-lSbLg4SRGxmwmpDKmTpQ$ >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
