[ovs-dev,1/2] ovn-ctl: Allow passing ssl certs when starting OVN DBs in ssl mode.

Message ID 1537460151-22894-1-git-send-email-aginwala@ebay.com
State Superseded
Headers show
Series
  • [ovs-dev,1/2] ovn-ctl: Allow passing ssl certs when starting OVN DBs in ssl mode.
Related show

Commit Message

aginwala aginwala Sept. 20, 2018, 4:15 p.m.
For OVN DBs to work with SSL in HA, we need to have capability to
 pass ssl certs when starting OVN DBs. Say when starting OVN DBs in active
 passive mode, in order for the standby DBs to sync from master node, it
 cannot sync because the required ssl certs are not passed when standby DBs
 are initialized. Hence, we need to have this option.

e.g. start nb db with ssl certs as below:
/usr/share/openvswitch/scripts/ovn-ctl --ovn-nb-db-ssl-key=/etc/openvswitch/ovnnb-privkey.pem \
--ovn-nb-db-ssl-cert=/etc/openvswitch/ovnnb-cert.pem \
--ovn-nb-db-ssl-ca-cert=/etc/openvswitch/cacert.pem \
--db-nb-create-insecure-remote=no start_nb_ovsdb

Certs can be generated based on ovs ssl docs:
http://docs.openvswitch.org/en/latest/howto/ssl/

Signed-off-by: aginwala <aginwala@ebay.com>
---
 ovn/utilities/ovn-ctl | 50 +++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 7 deletions(-)

Comments

Han Zhou Oct. 4, 2018, 4:57 p.m. | #1
Thanks Ali, please see my comments below

On Fri, Sep 21, 2018 at 5:34 PM <amginwal@gmail.com> wrote:
>
>  For OVN DBs to work with SSL in HA, we need to have capability to
>  pass ssl certs when starting OVN DBs. Say when starting OVN DBs in active
>  passive mode, in order for the standby DBs to sync from master node, it
>  cannot sync because the required ssl certs are not passed when standby
DBs
>  are initialized. Hence, we need to have this option.
>
> e.g. start nb db with ssl certs as below:
> /usr/share/openvswitch/scripts/ovn-ctl
--ovn-nb-db-ssl-key=/etc/openvswitch/ovnnb-privkey.pem \
> --ovn-nb-db-ssl-cert=/etc/openvswitch/ovnnb-cert.pem \
> --ovn-nb-db-ssl-ca-cert=/etc/openvswitch/cacert.pem \
> --db-nb-create-insecure-remote=no start_nb_ovsdb
>
> Certs can be generated based on ovs ssl docs:
> http://docs.openvswitch.org/en/latest/howto/ssl/
>
> Signed-off-by: aginwala <aginwala@ebay.com>
> ---
>  ovn/utilities/ovn-ctl | 50
+++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 43 insertions(+), 7 deletions(-)
>
> diff --git a/ovn/utilities/ovn-ctl b/ovn/utilities/ovn-ctl
> index 3ff0df6..4f45f3d 100755
> --- a/ovn/utilities/ovn-ctl
> +++ b/ovn/utilities/ovn-ctl
> @@ -116,6 +116,9 @@ start_ovsdb__() {
>      local addr
>      local active_conf_file
>      local use_remote_in_db
> +    local ovn_db_ssl_key
> +    local ovn_db_ssl_cert
> +    local ovn_db_ssl_cacert
>      eval pid=\$DB_${DB}_PID
>      eval cluster_local_addr=\$DB_${DB}_CLUSTER_LOCAL_ADDR
>      eval cluster_local_port=\$DB_${DB}_CLUSTER_LOCAL_PORT
> @@ -137,6 +140,9 @@ start_ovsdb__() {
>      eval addr=\$DB_${DB}_ADDR
>      eval active_conf_file=\$ovn${db}_active_conf_file
>      eval use_remote_in_db=\$DB_${DB}_USE_REMOTE_IN_DB
> +    eval ovn_db_ssl_key=\$OVN_${DB}_DB_SSL_KEY
> +    eval ovn_db_ssl_cert=\$OVN_${DB}_DB_SSL_CERT
> +    eval ovn_db_ssl_cacert=\$OVN_${DB}_DB_SSL_CA_CERT
>
>      # Check and eventually start ovsdb-server for DB
>      if pidfile_is_running $pid; then
> @@ -182,17 +188,32 @@ $cluster_remote_port
>
>      if test X"$use_remote_in_db" != Xno; then
>          set "$@" --remote=db:$schema_name,$table_name,connections
> +        if test X"$create_insecure_remote" = Xno; then
> +            set "$@" --remote=pssl:$port:$addr
> +        elif test X"$create_insecure_remote" = Xyes; then
> +            set "$@" --remote=ptcp:$port:$addr
> +        fi
Why moving the logic here? This if block only says if the connection
settings in DB table should be used. Whether insecure mode is allowed was
supposed to be independent with this condition. Could you explain the
reason behind the change?

>      fi
> -    set "$@" --private-key=db:$schema_name,SSL,private_key
> -    set "$@" --certificate=db:$schema_name,SSL,certificate
> -    set "$@" --ca-cert=db:$schema_name,SSL,ca_cert
> -    set "$@" --ssl-protocols=db:$schema_name,SSL,ssl_protocols
> -    set "$@" --ssl-ciphers=db:$schema_name,SSL,ssl_ciphers

So it will not use the settings in DB any more? It seems this change
removed support for "--use-remote-in-db=yes", which is the default behavior
that should be kept. The DB settings should not be used only if
"--use-remote-in-db=no"

>
> -    if test X"$create_insecure_remote" = Xyes; then
> -        set "$@" --remote=ptcp:$port:$addr
> +    if test X"$ovn_db_ssl_key" != X; then
> +        set "$@" --private-key=$ovn_db_ssl_key
> +    else
> +        set "$@" --private-key=db:$schema_name,SSL,private_key
> +    fi
> +    if test X"$ovn_db_ssl_cert" != X; then
> +        set "$@" --certificate=$ovn_db_ssl_cert
> +    else
> +        set "$@" --certificate=db:$schema_name,SSL,certificate
> +    fi
> +    if test X"$ovn_db_ssl_cacert" != X; then
> +        set "$@" --ca-cert=$ovn_db_ssl_cacert
> +    else
> +        set "$@" --ca-cert=db:$schema_name,SSL,ca_cert
>      fi
>
> +    set "$@" --ssl-protocols=db:$schema_name,SSL,ssl_protocols
> +    set "$@" --ssl-ciphers=db:$schema_name,SSL,ssl_ciphers
> +
>      if test $mode = active_passive; then
>          set "$@" --sync-from=`cat $active_conf_file`
>      fi
> @@ -481,6 +502,15 @@ set_defaults () {
>      OVN_NORTHD_SB_DB="unix:$DB_SB_SOCK"
>      DB_NB_USE_REMOTE_IN_DB="yes"
>      DB_SB_USE_REMOTE_IN_DB="yes"
> +
> +    OVN_NB_DB_SSL_KEY=""
> +    OVN_NB_DB_SSL_CERT=""
> +    OVN_NB_DB_SSL_CA_CERT=""
> +
> +    OVN_SB_DB_SSL_KEY=""
> +    OVN_SB_DB_SSL_CERT=""
> +    OVN_SB_DB_SSL_CA_CERT=""
> +
>  }
>
>  set_option () {
> @@ -536,6 +566,12 @@ Options:
>    --ovn-controller-ssl-cert=CERT OVN Southbound SSL certificate file
>    --ovn-controller-ssl-ca-cert=CERT OVN Southbound SSL CA certificate
file
>    --ovn-controller-ssl-bootstrap-ca-cert=CERT Bootstrapped OVN
Southbound SSL CA certificate file
> +  --ovn-nb-db-ssl-key=KEY OVN Northbound DB SSL private key file
> +  --ovn-nb-db-ssl-cert=CERT OVN Northbound DB SSL certificate file
> +  --ovn-nb-db-ssl-ca-cert=CERT OVN Northbound DB SSL CA certificate file
> +  --ovn-sb-db-ssl-key=KEY OVN Southbound DB SSL private key file
> +  --ovn-sb-db-ssl-cert=CERT OVN Southbound DB SSL certificate file
> +  --ovn-sb-db-ssl-ca-cert=CERT OVN Southbound DB SSL CA certificate file
>    --ovn-manage-ovsdb=yes|no        Whether or not the OVN databases
should be
>                                     automatically started and stopped
along
>                                     with ovn-northd. The default is
"yes". If
> --
> 1.9.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
aginwala aginwala Oct. 6, 2018, 1:48 a.m. | #2
Thanks for the review Han. Please find the comments inline below:
On Thu, Oct 4, 2018 at 9:58 AM Han Zhou <zhouhan@gmail.com> wrote:

> Thanks Ali, please see my comments below
>
> On Fri, Sep 21, 2018 at 5:34 PM <amginwal@gmail.com> wrote:
> >
> >  For OVN DBs to work with SSL in HA, we need to have capability to
> >  pass ssl certs when starting OVN DBs. Say when starting OVN DBs in
> active
> >  passive mode, in order for the standby DBs to sync from master node, it
> >  cannot sync because the required ssl certs are not passed when standby
> DBs
> >  are initialized. Hence, we need to have this option.
> >
> > e.g. start nb db with ssl certs as below:
> > /usr/share/openvswitch/scripts/ovn-ctl
> --ovn-nb-db-ssl-key=/etc/openvswitch/ovnnb-privkey.pem \
> > --ovn-nb-db-ssl-cert=/etc/openvswitch/ovnnb-cert.pem \
> > --ovn-nb-db-ssl-ca-cert=/etc/openvswitch/cacert.pem \
> > --db-nb-create-insecure-remote=no start_nb_ovsdb
> >
> > Certs can be generated based on ovs ssl docs:
> > http://docs.openvswitch.org/en/latest/howto/ssl/
> >
> > Signed-off-by: aginwala <aginwala@ebay.com>
> > ---
> >  ovn/utilities/ovn-ctl | 50
> +++++++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 43 insertions(+), 7 deletions(-)
> >
> > diff --git a/ovn/utilities/ovn-ctl b/ovn/utilities/ovn-ctl
> > index 3ff0df6..4f45f3d 100755
> > --- a/ovn/utilities/ovn-ctl
> > +++ b/ovn/utilities/ovn-ctl
> > @@ -116,6 +116,9 @@ start_ovsdb__() {
> >      local addr
> >      local active_conf_file
> >      local use_remote_in_db
> > +    local ovn_db_ssl_key
> > +    local ovn_db_ssl_cert
> > +    local ovn_db_ssl_cacert
> >      eval pid=\$DB_${DB}_PID
> >      eval cluster_local_addr=\$DB_${DB}_CLUSTER_LOCAL_ADDR
> >      eval cluster_local_port=\$DB_${DB}_CLUSTER_LOCAL_PORT
> > @@ -137,6 +140,9 @@ start_ovsdb__() {
> >      eval addr=\$DB_${DB}_ADDR
> >      eval active_conf_file=\$ovn${db}_active_conf_file
> >      eval use_remote_in_db=\$DB_${DB}_USE_REMOTE_IN_DB
> > +    eval ovn_db_ssl_key=\$OVN_${DB}_DB_SSL_KEY
> > +    eval ovn_db_ssl_cert=\$OVN_${DB}_DB_SSL_CERT
> > +    eval ovn_db_ssl_cacert=\$OVN_${DB}_DB_SSL_CA_CERT
> >
> >      # Check and eventually start ovsdb-server for DB
> >      if pidfile_is_running $pid; then
> > @@ -182,17 +188,32 @@ $cluster_remote_port
> >
> >      if test X"$use_remote_in_db" != Xno; then
> >          set "$@" --remote=db:$schema_name,$table_name,connections
> > +        if test X"$create_insecure_remote" = Xno; then
> > +            set "$@" --remote=pssl:$port:$addr
> > +        elif test X"$create_insecure_remote" = Xyes; then
> > +            set "$@" --remote=ptcp:$port:$addr
> > +        fi
> Why moving the logic here? This if block only says if the connection
> settings in DB table should be used. Whether insecure mode is allowed was
> supposed to be independent with this condition. Could you explain the
> reason behind the change?
> >> I moved it because remote=db is needed if ovsdb is running as a
> standalone node or an active ovsdb server node. For standby nodes in case
> of active_passive mode, remote=db will not be there because it uses
> --sync-from. Hope its clear.
>


> >      fi
> > -    set "$@" --private-key=db:$schema_name,SSL,private_key
> > -    set "$@" --certificate=db:$schema_name,SSL,certificate
> > -    set "$@" --ca-cert=db:$schema_name,SSL,ca_cert
> > -    set "$@" --ssl-protocols=db:$schema_name,SSL,ssl_protocols
> > -    set "$@" --ssl-ciphers=db:$schema_name,SSL,ssl_ciphers
>
> So it will not use the settings in DB any more? It seems this change
> removed support for "--use-remote-in-db=yes", which is the default behavior
> that should be kept. The DB settings should not be used only if
> "--use-remote-in-db=no"
>
>>  As discussed, this is similar support that we have for ovn-controller
with ssl. If the key is passed from cli, it will use the key else fall back
to default setters for ssl.

> >
> > -    if test X"$create_insecure_remote" = Xyes; then
> > -        set "$@" --remote=ptcp:$port:$addr
> > +    if test X"$ovn_db_ssl_key" != X; then
> > +        set "$@" --private-key=$ovn_db_ssl_key
> > +    else
> > +        set "$@" --private-key=db:$schema_name,SSL,private_key
> > +    fi
> > +    if test X"$ovn_db_ssl_cert" != X; then
> > +        set "$@" --certificate=$ovn_db_ssl_cert
> > +    else
> > +        set "$@" --certificate=db:$schema_name,SSL,certificate
> > +    fi
> > +    if test X"$ovn_db_ssl_cacert" != X; then
> > +        set "$@" --ca-cert=$ovn_db_ssl_cacert
> > +    else
> > +        set "$@" --ca-cert=db:$schema_name,SSL,ca_cert
> >      fi
> >
> > +    set "$@" --ssl-protocols=db:$schema_name,SSL,ssl_protocols
> > +    set "$@" --ssl-ciphers=db:$schema_name,SSL,ssl_ciphers
> > +
> >      if test $mode = active_passive; then
> >          set "$@" --sync-from=`cat $active_conf_file`
> >      fi
> > @@ -481,6 +502,15 @@ set_defaults () {
> >      OVN_NORTHD_SB_DB="unix:$DB_SB_SOCK"
> >      DB_NB_USE_REMOTE_IN_DB="yes"
> >      DB_SB_USE_REMOTE_IN_DB="yes"
> > +
> > +    OVN_NB_DB_SSL_KEY=""
> > +    OVN_NB_DB_SSL_CERT=""
> > +    OVN_NB_DB_SSL_CA_CERT=""
> > +
> > +    OVN_SB_DB_SSL_KEY=""
> > +    OVN_SB_DB_SSL_CERT=""
> > +    OVN_SB_DB_SSL_CA_CERT=""
> > +
> >  }
> >
> >  set_option () {
> > @@ -536,6 +566,12 @@ Options:
> >    --ovn-controller-ssl-cert=CERT OVN Southbound SSL certificate file
> >    --ovn-controller-ssl-ca-cert=CERT OVN Southbound SSL CA certificate
> file
> >    --ovn-controller-ssl-bootstrap-ca-cert=CERT Bootstrapped OVN
> Southbound SSL CA certificate file
> > +  --ovn-nb-db-ssl-key=KEY OVN Northbound DB SSL private key file
> > +  --ovn-nb-db-ssl-cert=CERT OVN Northbound DB SSL certificate file
> > +  --ovn-nb-db-ssl-ca-cert=CERT OVN Northbound DB SSL CA certificate file
> > +  --ovn-sb-db-ssl-key=KEY OVN Southbound DB SSL private key file
> > +  --ovn-sb-db-ssl-cert=CERT OVN Southbound DB SSL certificate file
> > +  --ovn-sb-db-ssl-ca-cert=CERT OVN Southbound DB SSL CA certificate file
> >    --ovn-manage-ovsdb=yes|no        Whether or not the OVN databases
> should be
> >                                     automatically started and stopped
> along
> >                                     with ovn-northd. The default is
> "yes". If
> > --
> > 1.9.1
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Han Zhou Oct. 8, 2018, 5:46 p.m. | #3
On Fri, Oct 5, 2018 at 6:48 PM aginwala aginwala <amginwal@gmail.com> wrote:
>
> Thanks for the review Han. Please find the comments inline below:
> On Thu, Oct 4, 2018 at 9:58 AM Han Zhou <zhouhan@gmail.com> wrote:
>>
>> Thanks Ali, please see my comments below
>>
>> On Fri, Sep 21, 2018 at 5:34 PM <amginwal@gmail.com> wrote:
>> >
>> >  For OVN DBs to work with SSL in HA, we need to have capability to
>> >  pass ssl certs when starting OVN DBs. Say when starting OVN DBs in
active
>> >  passive mode, in order for the standby DBs to sync from master node,
it
>> >  cannot sync because the required ssl certs are not passed when
standby DBs
>> >  are initialized. Hence, we need to have this option.
>> >
>> > e.g. start nb db with ssl certs as below:
>> > /usr/share/openvswitch/scripts/ovn-ctl
--ovn-nb-db-ssl-key=/etc/openvswitch/ovnnb-privkey.pem \
>> > --ovn-nb-db-ssl-cert=/etc/openvswitch/ovnnb-cert.pem \
>> > --ovn-nb-db-ssl-ca-cert=/etc/openvswitch/cacert.pem \
>> > --db-nb-create-insecure-remote=no start_nb_ovsdb
>> >
>> > Certs can be generated based on ovs ssl docs:
>> > http://docs.openvswitch.org/en/latest/howto/ssl/
>> >
>> > Signed-off-by: aginwala <aginwala@ebay.com>
>> > ---
>> >  ovn/utilities/ovn-ctl | 50
+++++++++++++++++++++++++++++++++++++++++++-------
>> >  1 file changed, 43 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/ovn/utilities/ovn-ctl b/ovn/utilities/ovn-ctl
>> > index 3ff0df6..4f45f3d 100755
>> > --- a/ovn/utilities/ovn-ctl
>> > +++ b/ovn/utilities/ovn-ctl
>> > @@ -116,6 +116,9 @@ start_ovsdb__() {
>> >      local addr
>> >      local active_conf_file
>> >      local use_remote_in_db
>> > +    local ovn_db_ssl_key
>> > +    local ovn_db_ssl_cert
>> > +    local ovn_db_ssl_cacert
>> >      eval pid=\$DB_${DB}_PID
>> >      eval cluster_local_addr=\$DB_${DB}_CLUSTER_LOCAL_ADDR
>> >      eval cluster_local_port=\$DB_${DB}_CLUSTER_LOCAL_PORT
>> > @@ -137,6 +140,9 @@ start_ovsdb__() {
>> >      eval addr=\$DB_${DB}_ADDR
>> >      eval active_conf_file=\$ovn${db}_active_conf_file
>> >      eval use_remote_in_db=\$DB_${DB}_USE_REMOTE_IN_DB
>> > +    eval ovn_db_ssl_key=\$OVN_${DB}_DB_SSL_KEY
>> > +    eval ovn_db_ssl_cert=\$OVN_${DB}_DB_SSL_CERT
>> > +    eval ovn_db_ssl_cacert=\$OVN_${DB}_DB_SSL_CA_CERT
>> >
>> >      # Check and eventually start ovsdb-server for DB
>> >      if pidfile_is_running $pid; then
>> > @@ -182,17 +188,32 @@ $cluster_remote_port
>> >
>> >      if test X"$use_remote_in_db" != Xno; then
>> >          set "$@" --remote=db:$schema_name,$table_name,connections
>> > +        if test X"$create_insecure_remote" = Xno; then
>> > +            set "$@" --remote=pssl:$port:$addr
>> > +        elif test X"$create_insecure_remote" = Xyes; then
>> > +            set "$@" --remote=ptcp:$port:$addr
>> > +        fi
>> Why moving the logic here? This if block only says if the connection
settings in DB table should be used. Whether insecure mode is allowed was
supposed to be independent with this condition. Could you explain the
reason behind the change?
>> >> I moved it because remote=db is needed if ovsdb is running as a
standalone node or an active ovsdb server node. For standby nodes in case
of active_passive mode, remote=db will not be there because it uses
--sync-from. Hope its clear.

As discussed, $use_remote_in_db and $create_insecure_remote were
independent. Moving $create_insecure_remote logic here make it useful only
when $use_remote_in_db is "yes", which is not how it was supposed to be.

I understand that for standby node, we will set $use_remote_in_db as "no".
Is this a problem?

--sync-from has nothing to do with "remote".
>
>
>>
>> >      fi
>> > -    set "$@" --private-key=db:$schema_name,SSL,private_key
>> > -    set "$@" --certificate=db:$schema_name,SSL,certificate
>> > -    set "$@" --ca-cert=db:$schema_name,SSL,ca_cert
>> > -    set "$@" --ssl-protocols=db:$schema_name,SSL,ssl_protocols
>> > -    set "$@" --ssl-ciphers=db:$schema_name,SSL,ssl_ciphers
>>
>> So it will not use the settings in DB any more? It seems this change
removed support for "--use-remote-in-db=yes", which is the default behavior
that should be kept. The DB settings should not be used only if
"--use-remote-in-db=no"
>
> >>  As discussed, this is similar support that we have for ovn-controller
with ssl. If the key is passed from cli, it will use the key else fall back
to default setters for ssl.
>>
I missed the "else" below. So it does fallback to use DB config when
command line option is not specified. It looks good for me, but it would be
better to clarify the behavior in help message of this tool that command
line options for these SSL related parameters overrides any DB setting.

>> >
>> > -    if test X"$create_insecure_remote" = Xyes; then
>> > -        set "$@" --remote=ptcp:$port:$addr
>> > +    if test X"$ovn_db_ssl_key" != X; then
>> > +        set "$@" --private-key=$ovn_db_ssl_key
>> > +    else
>> > +        set "$@" --private-key=db:$schema_name,SSL,private_key
>> > +    fi
>> > +    if test X"$ovn_db_ssl_cert" != X; then
>> > +        set "$@" --certificate=$ovn_db_ssl_cert
>> > +    else
>> > +        set "$@" --certificate=db:$schema_name,SSL,certificate
>> > +    fi
>> > +    if test X"$ovn_db_ssl_cacert" != X; then
>> > +        set "$@" --ca-cert=$ovn_db_ssl_cacert
>> > +    else
>> > +        set "$@" --ca-cert=db:$schema_name,SSL,ca_cert
>> >      fi
>> >
>> > +    set "$@" --ssl-protocols=db:$schema_name,SSL,ssl_protocols
>> > +    set "$@" --ssl-ciphers=db:$schema_name,SSL,ssl_ciphers
>> > +
>> >      if test $mode = active_passive; then
>> >          set "$@" --sync-from=`cat $active_conf_file`
>> >      fi
>> > @@ -481,6 +502,15 @@ set_defaults () {
>> >      OVN_NORTHD_SB_DB="unix:$DB_SB_SOCK"
>> >      DB_NB_USE_REMOTE_IN_DB="yes"
>> >      DB_SB_USE_REMOTE_IN_DB="yes"
>> > +
>> > +    OVN_NB_DB_SSL_KEY=""
>> > +    OVN_NB_DB_SSL_CERT=""
>> > +    OVN_NB_DB_SSL_CA_CERT=""
>> > +
>> > +    OVN_SB_DB_SSL_KEY=""
>> > +    OVN_SB_DB_SSL_CERT=""
>> > +    OVN_SB_DB_SSL_CA_CERT=""
>> > +
>> >  }
>> >
>> >  set_option () {
>> > @@ -536,6 +566,12 @@ Options:
>> >    --ovn-controller-ssl-cert=CERT OVN Southbound SSL certificate file
>> >    --ovn-controller-ssl-ca-cert=CERT OVN Southbound SSL CA certificate
file
>> >    --ovn-controller-ssl-bootstrap-ca-cert=CERT Bootstrapped OVN
Southbound SSL CA certificate file
>> > +  --ovn-nb-db-ssl-key=KEY OVN Northbound DB SSL private key file
>> > +  --ovn-nb-db-ssl-cert=CERT OVN Northbound DB SSL certificate file
>> > +  --ovn-nb-db-ssl-ca-cert=CERT OVN Northbound DB SSL CA certificate
file
>> > +  --ovn-sb-db-ssl-key=KEY OVN Southbound DB SSL private key file
>> > +  --ovn-sb-db-ssl-cert=CERT OVN Southbound DB SSL certificate file
>> > +  --ovn-sb-db-ssl-ca-cert=CERT OVN Southbound DB SSL CA certificate
file
>> >    --ovn-manage-ovsdb=yes|no        Whether or not the OVN databases
should be
>> >                                     automatically started and stopped
along
>> >                                     with ovn-northd. The default is
"yes". If
>> > --
>> > 1.9.1
>> >
>> > _______________________________________________
>> > dev mailing list
>> > dev@openvswitch.org
>> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
aginwala aginwala Oct. 8, 2018, 11:55 p.m. | #4
On Mon, Oct 8, 2018 at 10:47 AM Han Zhou <zhouhan@gmail.com> wrote:

>
>
> On Fri, Oct 5, 2018 at 6:48 PM aginwala aginwala <amginwal@gmail.com>
> wrote:
> >
> > Thanks for the review Han. Please find the comments inline below:
> > On Thu, Oct 4, 2018 at 9:58 AM Han Zhou <zhouhan@gmail.com> wrote:
> >>
> >> Thanks Ali, please see my comments below
> >>
> >> On Fri, Sep 21, 2018 at 5:34 PM <amginwal@gmail.com> wrote:
> >> >
> >> >  For OVN DBs to work with SSL in HA, we need to have capability to
> >> >  pass ssl certs when starting OVN DBs. Say when starting OVN DBs in
> active
> >> >  passive mode, in order for the standby DBs to sync from master node,
> it
> >> >  cannot sync because the required ssl certs are not passed when
> standby DBs
> >> >  are initialized. Hence, we need to have this option.
> >> >
> >> > e.g. start nb db with ssl certs as below:
> >> > /usr/share/openvswitch/scripts/ovn-ctl
> --ovn-nb-db-ssl-key=/etc/openvswitch/ovnnb-privkey.pem \
> >> > --ovn-nb-db-ssl-cert=/etc/openvswitch/ovnnb-cert.pem \
> >> > --ovn-nb-db-ssl-ca-cert=/etc/openvswitch/cacert.pem \
> >> > --db-nb-create-insecure-remote=no start_nb_ovsdb
> >> >
> >> > Certs can be generated based on ovs ssl docs:
> >> > http://docs.openvswitch.org/en/latest/howto/ssl/
> >> >
> >> > Signed-off-by: aginwala <aginwala@ebay.com>
> >> > ---
> >> >  ovn/utilities/ovn-ctl | 50
> +++++++++++++++++++++++++++++++++++++++++++-------
> >> >  1 file changed, 43 insertions(+), 7 deletions(-)
> >> >
> >> > diff --git a/ovn/utilities/ovn-ctl b/ovn/utilities/ovn-ctl
> >> > index 3ff0df6..4f45f3d 100755
> >> > --- a/ovn/utilities/ovn-ctl
> >> > +++ b/ovn/utilities/ovn-ctl
> >> > @@ -116,6 +116,9 @@ start_ovsdb__() {
> >> >      local addr
> >> >      local active_conf_file
> >> >      local use_remote_in_db
> >> > +    local ovn_db_ssl_key
> >> > +    local ovn_db_ssl_cert
> >> > +    local ovn_db_ssl_cacert
> >> >      eval pid=\$DB_${DB}_PID
> >> >      eval cluster_local_addr=\$DB_${DB}_CLUSTER_LOCAL_ADDR
> >> >      eval cluster_local_port=\$DB_${DB}_CLUSTER_LOCAL_PORT
> >> > @@ -137,6 +140,9 @@ start_ovsdb__() {
> >> >      eval addr=\$DB_${DB}_ADDR
> >> >      eval active_conf_file=\$ovn${db}_active_conf_file
> >> >      eval use_remote_in_db=\$DB_${DB}_USE_REMOTE_IN_DB
> >> > +    eval ovn_db_ssl_key=\$OVN_${DB}_DB_SSL_KEY
> >> > +    eval ovn_db_ssl_cert=\$OVN_${DB}_DB_SSL_CERT
> >> > +    eval ovn_db_ssl_cacert=\$OVN_${DB}_DB_SSL_CA_CERT
> >> >
> >> >      # Check and eventually start ovsdb-server for DB
> >> >      if pidfile_is_running $pid; then
> >> > @@ -182,17 +188,32 @@ $cluster_remote_port
> >> >
> >> >      if test X"$use_remote_in_db" != Xno; then
> >> >          set "$@" --remote=db:$schema_name,$table_name,connections
> >> > +        if test X"$create_insecure_remote" = Xno; then
> >> > +            set "$@" --remote=pssl:$port:$addr
> >> > +        elif test X"$create_insecure_remote" = Xyes; then
> >> > +            set "$@" --remote=ptcp:$port:$addr
> >> > +        fi
> >> Why moving the logic here? This if block only says if the connection
> settings in DB table should be used. Whether insecure mode is allowed was
> supposed to be independent with this condition. Could you explain the
> reason behind the change?
> >> >> I moved it because remote=db is needed if ovsdb is running as a
> standalone node or an active ovsdb server node. For standby nodes in case
> of active_passive mode, remote=db will not be there because it uses
> --sync-from. Hope its clear.
>
> As discussed, $use_remote_in_db and $create_insecure_remote were
> independent. Moving $create_insecure_remote logic here make it useful only
> when $use_remote_in_db is "yes", which is not how it was supposed to be.
>
> I understand that for standby node, we will set $use_remote_in_db as "no".
> Is this a problem?
>
> >> Just verified and have kept the logic intact as even for ssl, we have
connection table set for 0.0.0.0 for LB use case and it works fine. Hence,
have updated it in v2. Please take a look and let me know.

> --sync-from has nothing to do with "remote".
> >
> >
> >>
> >> >      fi
> >> > -    set "$@" --private-key=db:$schema_name,SSL,private_key
> >> > -    set "$@" --certificate=db:$schema_name,SSL,certificate
> >> > -    set "$@" --ca-cert=db:$schema_name,SSL,ca_cert
> >> > -    set "$@" --ssl-protocols=db:$schema_name,SSL,ssl_protocols
> >> > -    set "$@" --ssl-ciphers=db:$schema_name,SSL,ssl_ciphers
> >>
> >> So it will not use the settings in DB any more? It seems this change
> removed support for "--use-remote-in-db=yes", which is the default behavior
> that should be kept. The DB settings should not be used only if
> "--use-remote-in-db=no"
> >
> > >>  As discussed, this is similar support that we have for
> ovn-controller with ssl. If the key is passed from cli, it will use the key
> else fall back to default setters for ssl.
> >>
> I missed the "else" below. So it does fallback to use DB config when
> command line option is not specified. It looks good for me, but it would be
> better to clarify the behavior in help message of this tool that command
> line options for these SSL related parameters overrides any DB setting.
>
>>> Cool. So it does shows ssl key in  options section when you type help
similar to ovn-controller ssl support and hope that suffices.  Please ack
if all ok in v2 patch that I sent.

> >> >
> >> > -    if test X"$create_insecure_remote" = Xyes; then
> >> > -        set "$@" --remote=ptcp:$port:$addr
> >> > +    if test X"$ovn_db_ssl_key" != X; then
> >> > +        set "$@" --private-key=$ovn_db_ssl_key
> >> > +    else
> >> > +        set "$@" --private-key=db:$schema_name,SSL,private_key
> >> > +    fi
> >> > +    if test X"$ovn_db_ssl_cert" != X; then
> >> > +        set "$@" --certificate=$ovn_db_ssl_cert
> >> > +    else
> >> > +        set "$@" --certificate=db:$schema_name,SSL,certificate
> >> > +    fi
> >> > +    if test X"$ovn_db_ssl_cacert" != X; then
> >> > +        set "$@" --ca-cert=$ovn_db_ssl_cacert
> >> > +    else
> >> > +        set "$@" --ca-cert=db:$schema_name,SSL,ca_cert
> >> >      fi
> >> >
> >> > +    set "$@" --ssl-protocols=db:$schema_name,SSL,ssl_protocols
> >> > +    set "$@" --ssl-ciphers=db:$schema_name,SSL,ssl_ciphers
> >> > +
> >> >      if test $mode = active_passive; then
> >> >          set "$@" --sync-from=`cat $active_conf_file`
> >> >      fi
> >> > @@ -481,6 +502,15 @@ set_defaults () {
> >> >      OVN_NORTHD_SB_DB="unix:$DB_SB_SOCK"
> >> >      DB_NB_USE_REMOTE_IN_DB="yes"
> >> >      DB_SB_USE_REMOTE_IN_DB="yes"
> >> > +
> >> > +    OVN_NB_DB_SSL_KEY=""
> >> > +    OVN_NB_DB_SSL_CERT=""
> >> > +    OVN_NB_DB_SSL_CA_CERT=""
> >> > +
> >> > +    OVN_SB_DB_SSL_KEY=""
> >> > +    OVN_SB_DB_SSL_CERT=""
> >> > +    OVN_SB_DB_SSL_CA_CERT=""
> >> > +
> >> >  }
> >> >
> >> >  set_option () {
> >> > @@ -536,6 +566,12 @@ Options:
> >> >    --ovn-controller-ssl-cert=CERT OVN Southbound SSL certificate file
> >> >    --ovn-controller-ssl-ca-cert=CERT OVN Southbound SSL CA
> certificate file
> >> >    --ovn-controller-ssl-bootstrap-ca-cert=CERT Bootstrapped OVN
> Southbound SSL CA certificate file
> >> > +  --ovn-nb-db-ssl-key=KEY OVN Northbound DB SSL private key file
> >> > +  --ovn-nb-db-ssl-cert=CERT OVN Northbound DB SSL certificate file
> >> > +  --ovn-nb-db-ssl-ca-cert=CERT OVN Northbound DB SSL CA certificate
> file
> >> > +  --ovn-sb-db-ssl-key=KEY OVN Southbound DB SSL private key file
> >> > +  --ovn-sb-db-ssl-cert=CERT OVN Southbound DB SSL certificate file
> >> > +  --ovn-sb-db-ssl-ca-cert=CERT OVN Southbound DB SSL CA certificate
> file
> >> >    --ovn-manage-ovsdb=yes|no        Whether or not the OVN databases
> should be
> >> >                                     automatically started and stopped
> along
> >> >                                     with ovn-northd. The default is
> "yes". If
> >> > --
> >> > 1.9.1
> >> >
> >> > _______________________________________________
> >> > dev mailing list
> >> > dev@openvswitch.org
> >> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Han Zhou Oct. 9, 2018, 12:32 a.m. | #5
On Mon, Oct 8, 2018 at 4:55 PM aginwala aginwala <amginwal@gmail.com> wrote:
>
>
>
> On Mon, Oct 8, 2018 at 10:47 AM Han Zhou <zhouhan@gmail.com> wrote:
>>
>>
>>
>> On Fri, Oct 5, 2018 at 6:48 PM aginwala aginwala <amginwal@gmail.com>
wrote:
>> >
>> > Thanks for the review Han. Please find the comments inline below:
>> > On Thu, Oct 4, 2018 at 9:58 AM Han Zhou <zhouhan@gmail.com> wrote:
>> >>
>> >> Thanks Ali, please see my comments below
>> >>
>> >> On Fri, Sep 21, 2018 at 5:34 PM <amginwal@gmail.com> wrote:
>> >> >
>> >> >  For OVN DBs to work with SSL in HA, we need to have capability to
>> >> >  pass ssl certs when starting OVN DBs. Say when starting OVN DBs in
active
>> >> >  passive mode, in order for the standby DBs to sync from master
node, it
>> >> >  cannot sync because the required ssl certs are not passed when
standby DBs
>> >> >  are initialized. Hence, we need to have this option.
>> >> >
>> >> > e.g. start nb db with ssl certs as below:
>> >> > /usr/share/openvswitch/scripts/ovn-ctl
--ovn-nb-db-ssl-key=/etc/openvswitch/ovnnb-privkey.pem \
>> >> > --ovn-nb-db-ssl-cert=/etc/openvswitch/ovnnb-cert.pem \
>> >> > --ovn-nb-db-ssl-ca-cert=/etc/openvswitch/cacert.pem \
>> >> > --db-nb-create-insecure-remote=no start_nb_ovsdb
>> >> >
>> >> > Certs can be generated based on ovs ssl docs:
>> >> > http://docs.openvswitch.org/en/latest/howto/ssl/
>> >> >
>> >> > Signed-off-by: aginwala <aginwala@ebay.com>
>> >> > ---
>> >> >  ovn/utilities/ovn-ctl | 50
+++++++++++++++++++++++++++++++++++++++++++-------
>> >> >  1 file changed, 43 insertions(+), 7 deletions(-)
>> >> >
>> >> > diff --git a/ovn/utilities/ovn-ctl b/ovn/utilities/ovn-ctl
>> >> > index 3ff0df6..4f45f3d 100755
>> >> > --- a/ovn/utilities/ovn-ctl
>> >> > +++ b/ovn/utilities/ovn-ctl
>> >> > @@ -116,6 +116,9 @@ start_ovsdb__() {
>> >> >      local addr
>> >> >      local active_conf_file
>> >> >      local use_remote_in_db
>> >> > +    local ovn_db_ssl_key
>> >> > +    local ovn_db_ssl_cert
>> >> > +    local ovn_db_ssl_cacert
>> >> >      eval pid=\$DB_${DB}_PID
>> >> >      eval cluster_local_addr=\$DB_${DB}_CLUSTER_LOCAL_ADDR
>> >> >      eval cluster_local_port=\$DB_${DB}_CLUSTER_LOCAL_PORT
>> >> > @@ -137,6 +140,9 @@ start_ovsdb__() {
>> >> >      eval addr=\$DB_${DB}_ADDR
>> >> >      eval active_conf_file=\$ovn${db}_active_conf_file
>> >> >      eval use_remote_in_db=\$DB_${DB}_USE_REMOTE_IN_DB
>> >> > +    eval ovn_db_ssl_key=\$OVN_${DB}_DB_SSL_KEY
>> >> > +    eval ovn_db_ssl_cert=\$OVN_${DB}_DB_SSL_CERT
>> >> > +    eval ovn_db_ssl_cacert=\$OVN_${DB}_DB_SSL_CA_CERT
>> >> >
>> >> >      # Check and eventually start ovsdb-server for DB
>> >> >      if pidfile_is_running $pid; then
>> >> > @@ -182,17 +188,32 @@ $cluster_remote_port
>> >> >
>> >> >      if test X"$use_remote_in_db" != Xno; then
>> >> >          set "$@" --remote=db:$schema_name,$table_name,connections
>> >> > +        if test X"$create_insecure_remote" = Xno; then
>> >> > +            set "$@" --remote=pssl:$port:$addr
>> >> > +        elif test X"$create_insecure_remote" = Xyes; then
>> >> > +            set "$@" --remote=ptcp:$port:$addr
>> >> > +        fi
>> >> Why moving the logic here? This if block only says if the connection
settings in DB table should be used. Whether insecure mode is allowed was
supposed to be independent with this condition. Could you explain the
reason behind the change?
>> >> >> I moved it because remote=db is needed if ovsdb is running as a
standalone node or an active ovsdb server node. For standby nodes in case
of active_passive mode, remote=db will not be there because it uses
--sync-from. Hope its clear.
>>
>> As discussed, $use_remote_in_db and $create_insecure_remote were
independent. Moving $create_insecure_remote logic here make it useful only
when $use_remote_in_db is "yes", which is not how it was supposed to be.
>>
>> I understand that for standby node, we will set $use_remote_in_db as
"no". Is this a problem?
>>
> >> Just verified and have kept the logic intact as even for ssl, we have
connection table set for 0.0.0.0 for LB use case and it works fine. Hence,
have updated it in v2. Please take a look and let me know.
>>
>> --sync-from has nothing to do with "remote".
>> >
>> >
>> >>
>> >> >      fi
>> >> > -    set "$@" --private-key=db:$schema_name,SSL,private_key
>> >> > -    set "$@" --certificate=db:$schema_name,SSL,certificate
>> >> > -    set "$@" --ca-cert=db:$schema_name,SSL,ca_cert
>> >> > -    set "$@" --ssl-protocols=db:$schema_name,SSL,ssl_protocols
>> >> > -    set "$@" --ssl-ciphers=db:$schema_name,SSL,ssl_ciphers
>> >>
>> >> So it will not use the settings in DB any more? It seems this change
removed support for "--use-remote-in-db=yes", which is the default behavior
that should be kept. The DB settings should not be used only if
"--use-remote-in-db=no"
>> >
>> > >>  As discussed, this is similar support that we have for
ovn-controller with ssl. If the key is passed from cli, it will use the key
else fall back to default setters for ssl.
>> >>
>> I missed the "else" below. So it does fallback to use DB config when
command line option is not specified. It looks good for me, but it would be
better to clarify the behavior in help message of this tool that command
line options for these SSL related parameters overrides any DB setting.
>
> >>> Cool. So it does shows ssl key in  options section when you type help
similar to ovn-controller ssl support and hope that suffices.  Please ack
if all ok in v2 patch that I sent.
>>
I meant documenting the behavior that command line option always override
DB settings if both exist.
aginwala aginwala Oct. 9, 2018, 1:49 a.m. | #6
Sure. I will add ssl usage example with some brief in the ovn-ctl.8.xml and
send v3 for this patch . Does that sound good?

On Mon, Oct 8, 2018 at 5:32 PM Han Zhou <zhouhan@gmail.com> wrote:

>
>
> On Mon, Oct 8, 2018 at 4:55 PM aginwala aginwala <amginwal@gmail.com>
> wrote:
> >
> >
> >
> > On Mon, Oct 8, 2018 at 10:47 AM Han Zhou <zhouhan@gmail.com> wrote:
> >>
> >>
> >>
> >> On Fri, Oct 5, 2018 at 6:48 PM aginwala aginwala <amginwal@gmail.com>
> wrote:
> >> >
> >> > Thanks for the review Han. Please find the comments inline below:
> >> > On Thu, Oct 4, 2018 at 9:58 AM Han Zhou <zhouhan@gmail.com> wrote:
> >> >>
> >> >> Thanks Ali, please see my comments below
> >> >>
> >> >> On Fri, Sep 21, 2018 at 5:34 PM <amginwal@gmail.com> wrote:
> >> >> >
> >> >> >  For OVN DBs to work with SSL in HA, we need to have capability to
> >> >> >  pass ssl certs when starting OVN DBs. Say when starting OVN DBs
> in active
> >> >> >  passive mode, in order for the standby DBs to sync from master
> node, it
> >> >> >  cannot sync because the required ssl certs are not passed when
> standby DBs
> >> >> >  are initialized. Hence, we need to have this option.
> >> >> >
> >> >> > e.g. start nb db with ssl certs as below:
> >> >> > /usr/share/openvswitch/scripts/ovn-ctl
> --ovn-nb-db-ssl-key=/etc/openvswitch/ovnnb-privkey.pem \
> >> >> > --ovn-nb-db-ssl-cert=/etc/openvswitch/ovnnb-cert.pem \
> >> >> > --ovn-nb-db-ssl-ca-cert=/etc/openvswitch/cacert.pem \
> >> >> > --db-nb-create-insecure-remote=no start_nb_ovsdb
> >> >> >
> >> >> > Certs can be generated based on ovs ssl docs:
> >> >> > http://docs.openvswitch.org/en/latest/howto/ssl/
> >> >> >
> >> >> > Signed-off-by: aginwala <aginwala@ebay.com>
> >> >> > ---
> >> >> >  ovn/utilities/ovn-ctl | 50
> +++++++++++++++++++++++++++++++++++++++++++-------
> >> >> >  1 file changed, 43 insertions(+), 7 deletions(-)
> >> >> >
> >> >> > diff --git a/ovn/utilities/ovn-ctl b/ovn/utilities/ovn-ctl
> >> >> > index 3ff0df6..4f45f3d 100755
> >> >> > --- a/ovn/utilities/ovn-ctl
> >> >> > +++ b/ovn/utilities/ovn-ctl
> >> >> > @@ -116,6 +116,9 @@ start_ovsdb__() {
> >> >> >      local addr
> >> >> >      local active_conf_file
> >> >> >      local use_remote_in_db
> >> >> > +    local ovn_db_ssl_key
> >> >> > +    local ovn_db_ssl_cert
> >> >> > +    local ovn_db_ssl_cacert
> >> >> >      eval pid=\$DB_${DB}_PID
> >> >> >      eval cluster_local_addr=\$DB_${DB}_CLUSTER_LOCAL_ADDR
> >> >> >      eval cluster_local_port=\$DB_${DB}_CLUSTER_LOCAL_PORT
> >> >> > @@ -137,6 +140,9 @@ start_ovsdb__() {
> >> >> >      eval addr=\$DB_${DB}_ADDR
> >> >> >      eval active_conf_file=\$ovn${db}_active_conf_file
> >> >> >      eval use_remote_in_db=\$DB_${DB}_USE_REMOTE_IN_DB
> >> >> > +    eval ovn_db_ssl_key=\$OVN_${DB}_DB_SSL_KEY
> >> >> > +    eval ovn_db_ssl_cert=\$OVN_${DB}_DB_SSL_CERT
> >> >> > +    eval ovn_db_ssl_cacert=\$OVN_${DB}_DB_SSL_CA_CERT
> >> >> >
> >> >> >      # Check and eventually start ovsdb-server for DB
> >> >> >      if pidfile_is_running $pid; then
> >> >> > @@ -182,17 +188,32 @@ $cluster_remote_port
> >> >> >
> >> >> >      if test X"$use_remote_in_db" != Xno; then
> >> >> >          set "$@" --remote=db:$schema_name,$table_name,connections
> >> >> > +        if test X"$create_insecure_remote" = Xno; then
> >> >> > +            set "$@" --remote=pssl:$port:$addr
> >> >> > +        elif test X"$create_insecure_remote" = Xyes; then
> >> >> > +            set "$@" --remote=ptcp:$port:$addr
> >> >> > +        fi
> >> >> Why moving the logic here? This if block only says if the connection
> settings in DB table should be used. Whether insecure mode is allowed was
> supposed to be independent with this condition. Could you explain the
> reason behind the change?
> >> >> >> I moved it because remote=db is needed if ovsdb is running as a
> standalone node or an active ovsdb server node. For standby nodes in case
> of active_passive mode, remote=db will not be there because it uses
> --sync-from. Hope its clear.
> >>
> >> As discussed, $use_remote_in_db and $create_insecure_remote were
> independent. Moving $create_insecure_remote logic here make it useful only
> when $use_remote_in_db is "yes", which is not how it was supposed to be.
> >>
> >> I understand that for standby node, we will set $use_remote_in_db as
> "no". Is this a problem?
> >>
> > >> Just verified and have kept the logic intact as even for ssl, we have
> connection table set for 0.0.0.0 for LB use case and it works fine. Hence,
> have updated it in v2. Please take a look and let me know.
> >>
> >> --sync-from has nothing to do with "remote".
> >> >
> >> >
> >> >>
> >> >> >      fi
> >> >> > -    set "$@" --private-key=db:$schema_name,SSL,private_key
> >> >> > -    set "$@" --certificate=db:$schema_name,SSL,certificate
> >> >> > -    set "$@" --ca-cert=db:$schema_name,SSL,ca_cert
> >> >> > -    set "$@" --ssl-protocols=db:$schema_name,SSL,ssl_protocols
> >> >> > -    set "$@" --ssl-ciphers=db:$schema_name,SSL,ssl_ciphers
> >> >>
> >> >> So it will not use the settings in DB any more? It seems this change
> removed support for "--use-remote-in-db=yes", which is the default behavior
> that should be kept. The DB settings should not be used only if
> "--use-remote-in-db=no"
> >> >
> >> > >>  As discussed, this is similar support that we have for
> ovn-controller with ssl. If the key is passed from cli, it will use the key
> else fall back to default setters for ssl.
> >> >>
> >> I missed the "else" below. So it does fallback to use DB config when
> command line option is not specified. It looks good for me, but it would be
> better to clarify the behavior in help message of this tool that command
> line options for these SSL related parameters overrides any DB setting.
> >
> > >>> Cool. So it does shows ssl key in  options section when you type
> help similar to ovn-controller ssl support and hope that suffices.  Please
> ack if all ok in v2 patch that I sent.
> >>
> I meant documenting the behavior that command line option always override
> DB settings if both exist.
>
aginwala aginwala Oct. 10, 2018, 12:52 a.m. | #7
Hi Han:

Have added the man section for ssl in v3. PTAL.

Thanks,

On Mon, Oct 8, 2018 at 6:49 PM aginwala aginwala <amginwal@gmail.com> wrote:

> Sure. I will add ssl usage example with some brief in the ovn-ctl.8.xml
> and send v3 for this patch . Does that sound good?
>
> On Mon, Oct 8, 2018 at 5:32 PM Han Zhou <zhouhan@gmail.com> wrote:
>
>>
>>
>> On Mon, Oct 8, 2018 at 4:55 PM aginwala aginwala <amginwal@gmail.com>
>> wrote:
>> >
>> >
>> >
>> > On Mon, Oct 8, 2018 at 10:47 AM Han Zhou <zhouhan@gmail.com> wrote:
>> >>
>> >>
>> >>
>> >> On Fri, Oct 5, 2018 at 6:48 PM aginwala aginwala <amginwal@gmail.com>
>> wrote:
>> >> >
>> >> > Thanks for the review Han. Please find the comments inline below:
>> >> > On Thu, Oct 4, 2018 at 9:58 AM Han Zhou <zhouhan@gmail.com> wrote:
>> >> >>
>> >> >> Thanks Ali, please see my comments below
>> >> >>
>> >> >> On Fri, Sep 21, 2018 at 5:34 PM <amginwal@gmail.com> wrote:
>> >> >> >
>> >> >> >  For OVN DBs to work with SSL in HA, we need to have capability to
>> >> >> >  pass ssl certs when starting OVN DBs. Say when starting OVN DBs
>> in active
>> >> >> >  passive mode, in order for the standby DBs to sync from master
>> node, it
>> >> >> >  cannot sync because the required ssl certs are not passed when
>> standby DBs
>> >> >> >  are initialized. Hence, we need to have this option.
>> >> >> >
>> >> >> > e.g. start nb db with ssl certs as below:
>> >> >> > /usr/share/openvswitch/scripts/ovn-ctl
>> --ovn-nb-db-ssl-key=/etc/openvswitch/ovnnb-privkey.pem \
>> >> >> > --ovn-nb-db-ssl-cert=/etc/openvswitch/ovnnb-cert.pem \
>> >> >> > --ovn-nb-db-ssl-ca-cert=/etc/openvswitch/cacert.pem \
>> >> >> > --db-nb-create-insecure-remote=no start_nb_ovsdb
>> >> >> >
>> >> >> > Certs can be generated based on ovs ssl docs:
>> >> >> > http://docs.openvswitch.org/en/latest/howto/ssl/
>> >> >> >
>> >> >> > Signed-off-by: aginwala <aginwala@ebay.com>
>> >> >> > ---
>> >> >> >  ovn/utilities/ovn-ctl | 50
>> +++++++++++++++++++++++++++++++++++++++++++-------
>> >> >> >  1 file changed, 43 insertions(+), 7 deletions(-)
>> >> >> >
>> >> >> > diff --git a/ovn/utilities/ovn-ctl b/ovn/utilities/ovn-ctl
>> >> >> > index 3ff0df6..4f45f3d 100755
>> >> >> > --- a/ovn/utilities/ovn-ctl
>> >> >> > +++ b/ovn/utilities/ovn-ctl
>> >> >> > @@ -116,6 +116,9 @@ start_ovsdb__() {
>> >> >> >      local addr
>> >> >> >      local active_conf_file
>> >> >> >      local use_remote_in_db
>> >> >> > +    local ovn_db_ssl_key
>> >> >> > +    local ovn_db_ssl_cert
>> >> >> > +    local ovn_db_ssl_cacert
>> >> >> >      eval pid=\$DB_${DB}_PID
>> >> >> >      eval cluster_local_addr=\$DB_${DB}_CLUSTER_LOCAL_ADDR
>> >> >> >      eval cluster_local_port=\$DB_${DB}_CLUSTER_LOCAL_PORT
>> >> >> > @@ -137,6 +140,9 @@ start_ovsdb__() {
>> >> >> >      eval addr=\$DB_${DB}_ADDR
>> >> >> >      eval active_conf_file=\$ovn${db}_active_conf_file
>> >> >> >      eval use_remote_in_db=\$DB_${DB}_USE_REMOTE_IN_DB
>> >> >> > +    eval ovn_db_ssl_key=\$OVN_${DB}_DB_SSL_KEY
>> >> >> > +    eval ovn_db_ssl_cert=\$OVN_${DB}_DB_SSL_CERT
>> >> >> > +    eval ovn_db_ssl_cacert=\$OVN_${DB}_DB_SSL_CA_CERT
>> >> >> >
>> >> >> >      # Check and eventually start ovsdb-server for DB
>> >> >> >      if pidfile_is_running $pid; then
>> >> >> > @@ -182,17 +188,32 @@ $cluster_remote_port
>> >> >> >
>> >> >> >      if test X"$use_remote_in_db" != Xno; then
>> >> >> >          set "$@" --remote=db:$schema_name,$table_name,connections
>> >> >> > +        if test X"$create_insecure_remote" = Xno; then
>> >> >> > +            set "$@" --remote=pssl:$port:$addr
>> >> >> > +        elif test X"$create_insecure_remote" = Xyes; then
>> >> >> > +            set "$@" --remote=ptcp:$port:$addr
>> >> >> > +        fi
>> >> >> Why moving the logic here? This if block only says if the
>> connection settings in DB table should be used. Whether insecure mode is
>> allowed was supposed to be independent with this condition. Could you
>> explain the reason behind the change?
>> >> >> >> I moved it because remote=db is needed if ovsdb is running as a
>> standalone node or an active ovsdb server node. For standby nodes in case
>> of active_passive mode, remote=db will not be there because it uses
>> --sync-from. Hope its clear.
>> >>
>> >> As discussed, $use_remote_in_db and $create_insecure_remote were
>> independent. Moving $create_insecure_remote logic here make it useful only
>> when $use_remote_in_db is "yes", which is not how it was supposed to be.
>> >>
>> >> I understand that for standby node, we will set $use_remote_in_db as
>> "no". Is this a problem?
>> >>
>> > >> Just verified and have kept the logic intact as even for ssl, we
>> have connection table set for 0.0.0.0 for LB use case and it works fine.
>> Hence, have updated it in v2. Please take a look and let me know.
>> >>
>> >> --sync-from has nothing to do with "remote".
>> >> >
>> >> >
>> >> >>
>> >> >> >      fi
>> >> >> > -    set "$@" --private-key=db:$schema_name,SSL,private_key
>> >> >> > -    set "$@" --certificate=db:$schema_name,SSL,certificate
>> >> >> > -    set "$@" --ca-cert=db:$schema_name,SSL,ca_cert
>> >> >> > -    set "$@" --ssl-protocols=db:$schema_name,SSL,ssl_protocols
>> >> >> > -    set "$@" --ssl-ciphers=db:$schema_name,SSL,ssl_ciphers
>> >> >>
>> >> >> So it will not use the settings in DB any more? It seems this
>> change removed support for "--use-remote-in-db=yes", which is the default
>> behavior that should be kept. The DB settings should not be used only if
>> "--use-remote-in-db=no"
>> >> >
>> >> > >>  As discussed, this is similar support that we have for
>> ovn-controller with ssl. If the key is passed from cli, it will use the key
>> else fall back to default setters for ssl.
>> >> >>
>> >> I missed the "else" below. So it does fallback to use DB config when
>> command line option is not specified. It looks good for me, but it would be
>> better to clarify the behavior in help message of this tool that command
>> line options for these SSL related parameters overrides any DB setting.
>> >
>> > >>> Cool. So it does shows ssl key in  options section when you type
>> help similar to ovn-controller ssl support and hope that suffices.  Please
>> ack if all ok in v2 patch that I sent.
>> >>
>> I meant documenting the behavior that command line option always override
>> DB settings if both exist.
>>
>

Patch

diff --git a/ovn/utilities/ovn-ctl b/ovn/utilities/ovn-ctl
index 3ff0df6..4f45f3d 100755
--- a/ovn/utilities/ovn-ctl
+++ b/ovn/utilities/ovn-ctl
@@ -116,6 +116,9 @@  start_ovsdb__() {
     local addr
     local active_conf_file
     local use_remote_in_db
+    local ovn_db_ssl_key
+    local ovn_db_ssl_cert
+    local ovn_db_ssl_cacert
     eval pid=\$DB_${DB}_PID
     eval cluster_local_addr=\$DB_${DB}_CLUSTER_LOCAL_ADDR
     eval cluster_local_port=\$DB_${DB}_CLUSTER_LOCAL_PORT
@@ -137,6 +140,9 @@  start_ovsdb__() {
     eval addr=\$DB_${DB}_ADDR
     eval active_conf_file=\$ovn${db}_active_conf_file
     eval use_remote_in_db=\$DB_${DB}_USE_REMOTE_IN_DB
+    eval ovn_db_ssl_key=\$OVN_${DB}_DB_SSL_KEY
+    eval ovn_db_ssl_cert=\$OVN_${DB}_DB_SSL_CERT
+    eval ovn_db_ssl_cacert=\$OVN_${DB}_DB_SSL_CA_CERT
 
     # Check and eventually start ovsdb-server for DB
     if pidfile_is_running $pid; then
@@ -182,17 +188,32 @@  $cluster_remote_port
 
     if test X"$use_remote_in_db" != Xno; then
         set "$@" --remote=db:$schema_name,$table_name,connections
+        if test X"$create_insecure_remote" = Xno; then
+            set "$@" --remote=pssl:$port:$addr
+        elif test X"$create_insecure_remote" = Xyes; then
+            set "$@" --remote=ptcp:$port:$addr
+        fi
     fi
-    set "$@" --private-key=db:$schema_name,SSL,private_key
-    set "$@" --certificate=db:$schema_name,SSL,certificate
-    set "$@" --ca-cert=db:$schema_name,SSL,ca_cert
-    set "$@" --ssl-protocols=db:$schema_name,SSL,ssl_protocols
-    set "$@" --ssl-ciphers=db:$schema_name,SSL,ssl_ciphers
 
-    if test X"$create_insecure_remote" = Xyes; then
-        set "$@" --remote=ptcp:$port:$addr
+    if test X"$ovn_db_ssl_key" != X; then
+        set "$@" --private-key=$ovn_db_ssl_key
+    else
+        set "$@" --private-key=db:$schema_name,SSL,private_key
+    fi
+    if test X"$ovn_db_ssl_cert" != X; then
+        set "$@" --certificate=$ovn_db_ssl_cert
+    else
+        set "$@" --certificate=db:$schema_name,SSL,certificate
+    fi
+    if test X"$ovn_db_ssl_cacert" != X; then
+        set "$@" --ca-cert=$ovn_db_ssl_cacert
+    else
+        set "$@" --ca-cert=db:$schema_name,SSL,ca_cert
     fi
 
+    set "$@" --ssl-protocols=db:$schema_name,SSL,ssl_protocols
+    set "$@" --ssl-ciphers=db:$schema_name,SSL,ssl_ciphers
+
     if test $mode = active_passive; then
         set "$@" --sync-from=`cat $active_conf_file`
     fi
@@ -481,6 +502,15 @@  set_defaults () {
     OVN_NORTHD_SB_DB="unix:$DB_SB_SOCK"
     DB_NB_USE_REMOTE_IN_DB="yes"
     DB_SB_USE_REMOTE_IN_DB="yes"
+
+    OVN_NB_DB_SSL_KEY=""
+    OVN_NB_DB_SSL_CERT=""
+    OVN_NB_DB_SSL_CA_CERT=""
+
+    OVN_SB_DB_SSL_KEY=""
+    OVN_SB_DB_SSL_CERT=""
+    OVN_SB_DB_SSL_CA_CERT=""
+
 }
 
 set_option () {
@@ -536,6 +566,12 @@  Options:
   --ovn-controller-ssl-cert=CERT OVN Southbound SSL certificate file
   --ovn-controller-ssl-ca-cert=CERT OVN Southbound SSL CA certificate file
   --ovn-controller-ssl-bootstrap-ca-cert=CERT Bootstrapped OVN Southbound SSL CA certificate file
+  --ovn-nb-db-ssl-key=KEY OVN Northbound DB SSL private key file
+  --ovn-nb-db-ssl-cert=CERT OVN Northbound DB SSL certificate file
+  --ovn-nb-db-ssl-ca-cert=CERT OVN Northbound DB SSL CA certificate file
+  --ovn-sb-db-ssl-key=KEY OVN Southbound DB SSL private key file
+  --ovn-sb-db-ssl-cert=CERT OVN Southbound DB SSL certificate file
+  --ovn-sb-db-ssl-ca-cert=CERT OVN Southbound DB SSL CA certificate file
   --ovn-manage-ovsdb=yes|no        Whether or not the OVN databases should be
                                    automatically started and stopped along
                                    with ovn-northd. The default is "yes". If