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
