On Wed, Jan 17, 2024 at 6:22 AM aginwala <[email protected]> wrote: > 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. >
Sorry for the late response, please see reply inline. > > > 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 >> > There is probably some misunderstanding, the test case is fine and should stay. By not needed I meant only the following part: PKIDIR="$(cd $abs_top_builddir/tests && pwd)" AT_SKIP_IF([expr "$PKIDIR" : ".*[[ '\" \\]]"]) > >>> > +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. >> > It doesn't harm, but arguably it has still nothing to do with the ciphers/protocols change. However I won't block the patch on this. > >>> > + >>> > +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. >> > Followup patch is also fine to adjust all of those tests. > > + [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$ >>> >> Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
