| Message ID | 20210513224614.1878220-1-hzhou@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,1/5] ovn-controller: Support ssl cert rotation when command line options are used. | expand |
Hi Han, My comments below apply equally to the other patches in this series since they are generally similar. I think each patch could use a simple accompanying test case. The test could ensure that updates to the files are picked up by the OVN component. It could also potentially ensure that nothing awful happens if, say, you delete one or more of the files. See below for more: On 5/13/21 6:46 PM, Han Zhou wrote: > When SSL configurations are set in Open_vSwitch SSL table, > ovn-controller handles file update properly by re-applying the settings > in the main loop. However, it is also valid to set the options in > command line of ovn-controller without using the SSL table. In this > case, the options are set onetime only and it never reapplies when the > file content changes. This patch fixes this by allowing reapplying the > command line options in the main loop, if they are set. SSL table > settings still takes precedence if both exist. > > Signed-off-by: Han Zhou <hzhou@ovn.org> > --- > controller/ovn-controller.c | 24 +++++++++++++++++++++++- > 1 file changed, 23 insertions(+), 1 deletion(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 67c51a86f..5a755276b 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -97,6 +97,11 @@ static unixctl_cb_func debug_delay_nb_cfg_report; > static char *parse_options(int argc, char *argv[]); > OVS_NO_RETURN static void usage(void); > > +/* SSL options */ > +static const char *ssl_private_key_file; > +static const char *ssl_certificate_file; > +static const char *ssl_ca_cert_file; > + > /* By default don't set an upper bound for the lflow cache. */ > #define DEFAULT_LFLOW_CACHE_MAX_ENTRIES UINT32_MAX > #define DEFAULT_LFLOW_CACHE_MAX_MEM_KB (UINT64_MAX / 1024) > @@ -441,6 +446,11 @@ update_ssl_config(const struct ovsrec_ssl_table *ssl_table) > if (ssl) { > stream_ssl_set_key_and_cert(ssl->private_key, ssl->certificate); > stream_ssl_set_ca_cert_file(ssl->ca_cert, ssl->bootstrap_ca_cert); > + } else if (ssl_private_key_file && ssl_certificate_file && > + ssl_ca_cert_file) { Why must all three of these be non-NULL to call the stream_ssl functions below? Since the command line options used to result in individual calls to stream_ssl functions while parsing options, this represents a potential behavior change. For instance, if a user had for some reason only specified the -C option when starting ovn-controller, we would have called stream_ssl_set_ca_cert_file(). But now if they only specify -C, then we will not call stream_ssl_set_ca_cert_file() since they did not also set the -c and -p options. > + stream_ssl_set_key_and_cert(ssl_private_key_file, > + ssl_certificate_file); > + stream_ssl_set_ca_cert_file(ssl_ca_cert_file, false); > } > } > > @@ -3320,7 +3330,19 @@ parse_options(int argc, char *argv[]) > > VLOG_OPTION_HANDLERS > OVN_DAEMON_OPTION_HANDLERS > - STREAM_SSL_OPTION_HANDLERS > + > + case 'p': > + ssl_private_key_file = optarg; > + break; > + > + case 'c': > + ssl_certificate_file = optarg; > + break; > + > + case 'C': > + ssl_ca_cert_file = optarg; > + break; > + Is there any downside to not calling the stream_ssl_set_*() functions at this point and waiting until the main loop to do it? > > case OPT_PEER_CA_CERT: > stream_ssl_set_peer_ca_cert_file(optarg); >
On Mon, May 17, 2021 at 6:08 PM Mark Michelson <mmichels@redhat.com> wrote: > > Hi Han, > > My comments below apply equally to the other patches in this series > since they are generally similar. > > I think each patch could use a simple accompanying test case. The test > could ensure that updates to the files are picked up by the OVN > component. It could also potentially ensure that nothing awful happens > if, say, you delete one or more of the files. > Thanks Mark for the review. Ok, I was manually testing by playing around with the expiration time of the certs. Let me see if I can work out a way that is feasible for a test case. > See below for more: > > On 5/13/21 6:46 PM, Han Zhou wrote: > > When SSL configurations are set in Open_vSwitch SSL table, > > ovn-controller handles file update properly by re-applying the settings > > in the main loop. However, it is also valid to set the options in > > command line of ovn-controller without using the SSL table. In this > > case, the options are set onetime only and it never reapplies when the > > file content changes. This patch fixes this by allowing reapplying the > > command line options in the main loop, if they are set. SSL table > > settings still takes precedence if both exist. > > > > Signed-off-by: Han Zhou <hzhou@ovn.org> > > --- > > controller/ovn-controller.c | 24 +++++++++++++++++++++++- > > 1 file changed, 23 insertions(+), 1 deletion(-) > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 67c51a86f..5a755276b 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -97,6 +97,11 @@ static unixctl_cb_func debug_delay_nb_cfg_report; > > static char *parse_options(int argc, char *argv[]); > > OVS_NO_RETURN static void usage(void); > > > > +/* SSL options */ > > +static const char *ssl_private_key_file; > > +static const char *ssl_certificate_file; > > +static const char *ssl_ca_cert_file; > > + > > /* By default don't set an upper bound for the lflow cache. */ > > #define DEFAULT_LFLOW_CACHE_MAX_ENTRIES UINT32_MAX > > #define DEFAULT_LFLOW_CACHE_MAX_MEM_KB (UINT64_MAX / 1024) > > @@ -441,6 +446,11 @@ update_ssl_config(const struct ovsrec_ssl_table *ssl_table) > > if (ssl) { > > stream_ssl_set_key_and_cert(ssl->private_key, ssl->certificate); > > stream_ssl_set_ca_cert_file(ssl->ca_cert, ssl->bootstrap_ca_cert); > > + } else if (ssl_private_key_file && ssl_certificate_file && > > + ssl_ca_cert_file) { > > Why must all three of these be non-NULL to call the stream_ssl functions > below? Since the command line options used to result in individual calls > to stream_ssl functions while parsing options, this represents a > potential behavior change. For instance, if a user had for some reason > only specified the -C option when starting ovn-controller, we would have > called stream_ssl_set_ca_cert_file(). But now if they only specify -C, > then we will not call stream_ssl_set_ca_cert_file() since they did not > also set the -c and -p options. > IIUC, the three files must be supplied for the SSL to work, so I think it makes no sense to proceed if any of them is NULL. However, you are right that this is a behavior change. In the old implementation, there will be error logs complaining the missing file, but now it would be complaining about all the three files. Since the stream_ssl_xxx() interfaces already checks NULL, I will skip the NULL check here and call the functions with whatever is available in v2. > > + stream_ssl_set_key_and_cert(ssl_private_key_file, > > + ssl_certificate_file); > > + stream_ssl_set_ca_cert_file(ssl_ca_cert_file, false); > > } > > } > > > > @@ -3320,7 +3330,19 @@ parse_options(int argc, char *argv[]) > > > > VLOG_OPTION_HANDLERS > > OVN_DAEMON_OPTION_HANDLERS > > - STREAM_SSL_OPTION_HANDLERS > > + > > + case 'p': > > + ssl_private_key_file = optarg; > > + break; > > + > > + case 'c': > > + ssl_certificate_file = optarg; > > + break; > > + > > + case 'C': > > + ssl_ca_cert_file = optarg; > > + break; > > + > > Is there any downside to not calling the stream_ssl_set_*() functions at > this point and waiting until the main loop to do it? > There shouldn't be any difference because in ovn-controller and other daemons the first attempt to connect to SB is after calling update_ssl_config() in main loop. For nbctl daemon mode it is different, so the update_ssl_config() is called directly in the apply_options_direct(). Thanks, Han > > > > case OPT_PEER_CA_CERT: > > stream_ssl_set_peer_ca_cert_file(optarg); > > >
On Mon, May 17, 2021 at 7:05 PM Han Zhou <hzhou@ovn.org> wrote: > > > > On Mon, May 17, 2021 at 6:08 PM Mark Michelson <mmichels@redhat.com> wrote: > > > > Hi Han, > > > > My comments below apply equally to the other patches in this series > > since they are generally similar. > > > > I think each patch could use a simple accompanying test case. The test > > could ensure that updates to the files are picked up by the OVN > > component. It could also potentially ensure that nothing awful happens > > if, say, you delete one or more of the files. > > > Thanks Mark for the review. > Ok, I was manually testing by playing around with the expiration time of the certs. Let me see if I can work out a way that is feasible for a test case. > > > See below for more: > > > > On 5/13/21 6:46 PM, Han Zhou wrote: > > > When SSL configurations are set in Open_vSwitch SSL table, > > > ovn-controller handles file update properly by re-applying the settings > > > in the main loop. However, it is also valid to set the options in > > > command line of ovn-controller without using the SSL table. In this > > > case, the options are set onetime only and it never reapplies when the > > > file content changes. This patch fixes this by allowing reapplying the > > > command line options in the main loop, if they are set. SSL table > > > settings still takes precedence if both exist. > > > > > > Signed-off-by: Han Zhou <hzhou@ovn.org> > > > --- > > > controller/ovn-controller.c | 24 +++++++++++++++++++++++- > > > 1 file changed, 23 insertions(+), 1 deletion(-) > > > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > > index 67c51a86f..5a755276b 100644 > > > --- a/controller/ovn-controller.c > > > +++ b/controller/ovn-controller.c > > > @@ -97,6 +97,11 @@ static unixctl_cb_func debug_delay_nb_cfg_report; > > > static char *parse_options(int argc, char *argv[]); > > > OVS_NO_RETURN static void usage(void); > > > > > > +/* SSL options */ > > > +static const char *ssl_private_key_file; > > > +static const char *ssl_certificate_file; > > > +static const char *ssl_ca_cert_file; > > > + > > > /* By default don't set an upper bound for the lflow cache. */ > > > #define DEFAULT_LFLOW_CACHE_MAX_ENTRIES UINT32_MAX > > > #define DEFAULT_LFLOW_CACHE_MAX_MEM_KB (UINT64_MAX / 1024) > > > @@ -441,6 +446,11 @@ update_ssl_config(const struct ovsrec_ssl_table *ssl_table) > > > if (ssl) { > > > stream_ssl_set_key_and_cert(ssl->private_key, ssl->certificate); > > > stream_ssl_set_ca_cert_file(ssl->ca_cert, ssl->bootstrap_ca_cert); > > > + } else if (ssl_private_key_file && ssl_certificate_file && > > > + ssl_ca_cert_file) { > > > > Why must all three of these be non-NULL to call the stream_ssl functions > > below? Since the command line options used to result in individual calls > > to stream_ssl functions while parsing options, this represents a > > potential behavior change. For instance, if a user had for some reason > > only specified the -C option when starting ovn-controller, we would have > > called stream_ssl_set_ca_cert_file(). But now if they only specify -C, > > then we will not call stream_ssl_set_ca_cert_file() since they did not > > also set the -c and -p options. > > > IIUC, the three files must be supplied for the SSL to work, so I think it makes no sense to proceed if any of them is NULL. > However, you are right that this is a behavior change. In the old implementation, there will be error logs complaining the missing file, but now it would be complaining about all the three files. Since the stream_ssl_xxx() interfaces already checks NULL, I will skip the NULL check here and call the functions with whatever is available in v2. > Hi Mark, I think it is still necessary to check NULL pointers, so in v2 I just change the if condition to check key+cert and cacert separately before calling the respective ssl_set_xxx functions. Please take a look at v2: https://patchwork.ozlabs.org/project/ovn/list/?series=244813 Thanks, Han
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 67c51a86f..5a755276b 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -97,6 +97,11 @@ static unixctl_cb_func debug_delay_nb_cfg_report; static char *parse_options(int argc, char *argv[]); OVS_NO_RETURN static void usage(void); +/* SSL options */ +static const char *ssl_private_key_file; +static const char *ssl_certificate_file; +static const char *ssl_ca_cert_file; + /* By default don't set an upper bound for the lflow cache. */ #define DEFAULT_LFLOW_CACHE_MAX_ENTRIES UINT32_MAX #define DEFAULT_LFLOW_CACHE_MAX_MEM_KB (UINT64_MAX / 1024) @@ -441,6 +446,11 @@ update_ssl_config(const struct ovsrec_ssl_table *ssl_table) if (ssl) { stream_ssl_set_key_and_cert(ssl->private_key, ssl->certificate); stream_ssl_set_ca_cert_file(ssl->ca_cert, ssl->bootstrap_ca_cert); + } else if (ssl_private_key_file && ssl_certificate_file && + ssl_ca_cert_file) { + stream_ssl_set_key_and_cert(ssl_private_key_file, + ssl_certificate_file); + stream_ssl_set_ca_cert_file(ssl_ca_cert_file, false); } } @@ -3320,7 +3330,19 @@ parse_options(int argc, char *argv[]) VLOG_OPTION_HANDLERS OVN_DAEMON_OPTION_HANDLERS - STREAM_SSL_OPTION_HANDLERS + + case 'p': + ssl_private_key_file = optarg; + break; + + case 'c': + ssl_certificate_file = optarg; + break; + + case 'C': + ssl_ca_cert_file = optarg; + break; + case OPT_PEER_CA_CERT: stream_ssl_set_peer_ca_cert_file(optarg);
When SSL configurations are set in Open_vSwitch SSL table, ovn-controller handles file update properly by re-applying the settings in the main loop. However, it is also valid to set the options in command line of ovn-controller without using the SSL table. In this case, the options are set onetime only and it never reapplies when the file content changes. This patch fixes this by allowing reapplying the command line options in the main loop, if they are set. SSL table settings still takes precedence if both exist. Signed-off-by: Han Zhou <hzhou@ovn.org> --- controller/ovn-controller.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)