On Wed, 15 Oct 2025 at 04:52, Ilya Maximets <[email protected]> wrote:
> On 10/13/25 11:07 PM, Gurucharan Shetty wrote: > > This commit introduces Server Name Indication (SNI) support for all OVS > > SSL/TLS clients, enabling clients to specify which hostname they are > > attempting to reach during the SSL handshake. This is essential for > > connecting through proxies, load balancers, and service meshes where > > the connection endpoint differs from the intended server name. > > > > An example use case is trying to connect to a ovsdb-server through > > a istio proxy gateway in kubernetes environments > > > > Signed-off-by: Gurucharan Shetty <[email protected]> > > --- > > lib/ssl.man | 7 ++++ > > lib/ssl.xml | 9 +++++ > > lib/stream-nossl.c | 7 ++++ > > lib/stream-ssl.c | 20 +++++++++- > > lib/stream-ssl.h | 14 +++++-- > > tests/ovsdb-server.at | 86 ++++++++++++++++++++++++++++++++++++++++++- > > 6 files changed, 138 insertions(+), 5 deletions(-) > > Hi. Thanks for the patch! The change is definitely a nice addition. > > See a few comments below. > > > > > diff --git a/lib/ssl.man b/lib/ssl.man > > index 9bec3a786..d67c91a8f 100644 > > --- a/lib/ssl.man > > +++ b/lib/ssl.man > > @@ -24,3 +24,10 @@ be a different one, depending on the PKI design in > use.) > > Disables verification of certificates presented by SSL/TLS peers. This > > introduces a security risk, because it means that certificates cannot > > be verified to be those of known trusted hosts. > > +. > > +.IP "\fB\-\-ssl\-server\-name=\fIservername\fR" > > +Specifies the server name to use for SSL/TLS Server Name Indication > (SNI). > > nit: SNI is not supported by SSL, only TLS, so we may just say TLS here. > We keep SSL/TLS in many places for things that can be in both even though > we no longer support SSL, but it may be good to not mention it for TLS-only > features and try to slowly move away from using SSL at least in the docs. > Done > > > +By default, the hostname from the connection string is used for SNI. > > +This option allows overriding the SNI hostname, which is useful when > > +connecting through proxies or service meshes where the connection > endpoint > > +differs from the intended server name. > > diff --git a/lib/ssl.xml b/lib/ssl.xml > > index bd2502898..c7c0bddd4 100644 > > --- a/lib/ssl.xml > > +++ b/lib/ssl.xml > > @@ -33,4 +33,13 @@ > > introduces a security risk, because it means that certificates > cannot > > be verified to be those of known trusted hosts. > > </dd> > > + > > + <dt><code>--ssl-server-name=</code><var>servername</var></dt> > > + <dd> > > + Specifies the server name to use for SSL/TLS Server Name Indication > (SNI). > > nit: TLS. > Done > > > + By default, the hostname from the connection string is used for SNI. > > + This option allows overriding the SNI hostname, which is useful when > > + connecting through proxies or service meshes where the connection > endpoint > > + differs from the intended server name. > > + </dd> > > </dl> > > We need a change in the ssl-syn.man as well. > Done > > I'm also not sure if this should be in "ssl" or "ssl-connect". It can > likely > be argued both ways, so it's fine to keep the new options in "ssl". Okay. I am keeping it in "ssl". > Current man > pages are definitely a mess, as not every binary that has "ssl-connect" > options > includes the "ssl-connect" man page shard. At the same time most binaries > include ssl.man in their man pages, but not all of them implement > --ssl-server-name > option with this patch. For exmaple, ovsdb-server includes ssl.man, but > this > change doesn't add support for the the server-name to it. ovsdb-server > dosn't use > the STREAM_SSL_OPTION_HANDLERS macro, because it allows getting the > configuration > directly from a database, so we need to do manual changes while adding new > options. > > Commit 4d09d6b48ef5 ("stream-ssl: Add explicit support for configuring > TLSv1.3.") > has an exmaple of what needs to be changed in ovsdb-server. > Thanks. I made changes to ovsdb-server too by looking at the commit. Did you have other binaries too in mind that need special handling? > And we'll need an update for the stream_usage() function in lib/stream.c. > Done > > And a small NEWS entry would also be nice to have. > Done > > > diff --git a/lib/stream-nossl.c b/lib/stream-nossl.c > > index 105ac377a..665f65b51 100644 > > --- a/lib/stream-nossl.c > > +++ b/lib/stream-nossl.c > > @@ -96,3 +96,10 @@ stream_ssl_set_ciphersuites(const char *arg > OVS_UNUSED) > > /* Ignore this option since it seems harmless to set TLS > ciphersuites if > > * SSL/TLS won't be used. */ > > } > > + > > +void > > +stream_ssl_set_server_name(const char *server_name OVS_UNUSED) > > +{ > > + /* Ignore this option since it seems harmless to set SSL/TLS server > name if > > nit: TLS. > Done > > > + * SSL/TLS won't be used. */ > > +} > > diff --git a/lib/stream-ssl.c b/lib/stream-ssl.c > > index 99c82b6af..f97bfd739 100644 > > --- a/lib/stream-ssl.c > > +++ b/lib/stream-ssl.c > > @@ -168,6 +168,11 @@ static char *ssl_protocols = "TLSv1.2+"; > > static char *ssl_ciphers = "DEFAULT:@SECLEVEL=2"; > > static char *ssl_ciphersuites = ""; /* Using default ones, unless > specified. */ > > > > +/* Server name override for SNI (Server Name Indication). > > + * If set, this name will be used for SNI instead of the hostname > > + * extracted from the connection string. */ > > +static char *ssl_server_name_override; > > + > > /* Ordinarily, the SSL client and server verify each other's > certificates using > > * a CA certificate. Setting this to false disables this behavior. > (This is a > > * security risk.) */ > > @@ -372,7 +377,10 @@ ssl_open(const char *name, char *suffix, struct > stream **streamp, uint8_t dscp) > > dscp); > > if (fd >= 0) { > > int state = error ? STATE_TCP_CONNECTING : STATE_SSL_CONNECTING; > > - return new_ssl_stream(xstrdup(name), get_server_name(suffix), > > + char *server_name = ssl_server_name_override > > + ? xstrdup(ssl_server_name_override) > > + : get_server_name(suffix); > > nit: Since this block is getting larger, it may be good to have an empty > line > here between the definitions and the return. > Done > > > + return new_ssl_stream(xstrdup(name), server_name, > > fd, CLIENT, state, streamp); > > } else { > > VLOG_ERR("%s: connect: %s", name, ovs_strerror(error)); > > @@ -1251,6 +1259,16 @@ stream_ssl_set_ciphersuites(const char *arg) > > ssl_ciphersuites = xstrdup(arg); > > } > > > > +/* Sets the server name override for SNI (Server Name Indication). > > + * If 'server_name' is NULL, clears any existing override and SNI will > > + * use the hostname from the connection string. */ > > +void > > +stream_ssl_set_server_name(const char *server_name) > > +{ > > + free(ssl_server_name_override); > > + ssl_server_name_override = server_name ? xstrdup(server_name) : > NULL; > > nit: nullable_xstrdup(server_name); > Done > > > +} > > + > > /* Set SSL/TLS protocols based on the string input. Aborts with an error > > * message if 'arg' is invalid. */ > > void > > diff --git a/lib/stream-ssl.h b/lib/stream-ssl.h > > index 7036b176d..6127d6fb1 100644 > > --- a/lib/stream-ssl.h > > +++ b/lib/stream-ssl.h > > @@ -28,11 +28,13 @@ void stream_ssl_set_key_and_cert(const char > *private_key_file, > > void stream_ssl_set_protocols(const char *arg); > > void stream_ssl_set_ciphers(const char *arg); > > void stream_ssl_set_ciphersuites(const char *arg); > > +void stream_ssl_set_server_name(const char *server_name); > > > > #define SSL_OPTION_ENUMS \ > > OPT_SSL_PROTOCOLS, \ > > OPT_SSL_CIPHERS, \ > > - OPT_SSL_CIPHERSUITES > > + OPT_SSL_CIPHERSUITES, \ > > + OPT_SSL_SERVER_NAME > > > > #define STREAM_SSL_LONG_OPTIONS \ > > {"private-key", required_argument, NULL, 'p'}, \ > > @@ -40,7 +42,8 @@ void stream_ssl_set_ciphersuites(const char *arg); > > {"ca-cert", required_argument, NULL, 'C'}, \ > > {"ssl-protocols", required_argument, NULL, OPT_SSL_PROTOCOLS}, \ > > {"ssl-ciphers", required_argument, NULL, OPT_SSL_CIPHERS}, \ > > - {"ssl-ciphersuites", required_argument, NULL, > OPT_SSL_CIPHERSUITES} > > + {"ssl-ciphersuites", required_argument, NULL, > OPT_SSL_CIPHERSUITES}, \ > > + {"ssl-server-name", required_argument, NULL, > OPT_SSL_SERVER_NAME} > > > > #define STREAM_SSL_OPTION_HANDLERS \ > > case 'p': \ > > @@ -65,9 +68,14 @@ void stream_ssl_set_ciphersuites(const char *arg); > > \ > > case OPT_SSL_CIPHERSUITES: \ > > stream_ssl_set_ciphersuites(optarg); \ > > + break; \ > > + \ > > + case OPT_SSL_SERVER_NAME: \ > > + stream_ssl_set_server_name(optarg); \ > > break; > > > > #define STREAM_SSL_CASES \ > > - case 'p': case 'c': case 'C': case OPT_SSL_PROTOCOLS: case > OPT_SSL_CIPHERS: > > + case 'p': case 'c': case 'C': case OPT_SSL_PROTOCOLS: \ > > + case OPT_SSL_CIPHERS: case OPT_SSL_CIPHERSUITES: case > OPT_SSL_SERVER_NAME: > > Hmm. Could you split the addition of OPT_SSL_CIPHERSUITES into a separate > patch, so we can backport this fix? > > Done. I made this into a 2 patch series. > > > > #endif /* stream-ssl.h */ > > diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at > > index ca6e931be..245ef01ca 100644 > > --- a/tests/ovsdb-server.at > > +++ b/tests/ovsdb-server.at > > @@ -1043,7 +1043,91 @@ OVSDB_SERVER_SHUTDOWN([" > > /Protocol error/d > > "]) > > AT_CLEANUP > > - > > + > > Keep the form feed. > > > +AT_SETUP([SSL/TLS db: SNI (Server Name Indication)]) > > We have a similar test in ovs-vsctl.at, maybe we should extend it instead > of creating a new one here? Either way, we're not really testing the > server here, but the client. It seems reasonable to use ovs-vsctl for the > test instead of ovsdb-client, as database control tools are more likely > to be used in practice. > For v3, I decided to add a new test in ovs-vsctl.at instead. It felt like a new test would be cleaner as it specifically tests SNI override. I can do a v4 if you want to merge into a single test. The new test includes all your other comments from below. > > > +AT_KEYWORDS([ovsdb server positive ssl tls sni]) > > +AT_SKIP_IF([test "$HAVE_OPENSSL" = no]) > > +# For this test, we pass PKIDIR through a ovsdb-tool transact and > > Doesn't look like we do. > > > +# msys on Windows does not convert the path style automatically. > > +# So, do that forcefully with a 'pwd -W' (called through pwd() > function). > > +PKIDIR="$(cd $abs_top_builddir/tests && pwd)" > > +AT_SKIP_IF([expr "$PKIDIR" : ".*[[ '\" > > +\\]]"]) > > + > > +# Create additional certificates for SNI testing in the current > directory > > nit: Period at the end of the sentence. Same for other comments. > > > +OVS_PKI="sh $abs_top_srcdir/utilities/ovs-pki.in --dir=./sni-pki > --log=./sni-pki.log" > > I see other tests do this, but it's not clear to me why. We should be > able to > use ovs-pki script inside the tests directly without the 'sh' magic. > > > +# Remove any existing PKI directory to ensure clean state for test > > +rm -rf ./sni-pki > > Tests are always executed from a clean state, this should not be needed. > > > +AT_CHECK([$OVS_PKI init], [0], [ignore], [ignore]) > > + > > +# Create a certificate for a hostname that differs from the connection > IP > > +AT_CHECK([$OVS_PKI req+sign backend-service.test switch], [0], > [ignore], [ignore]) > > Also, do we need to generate new certificates here? Can we use the > existing > ones from $abs_top_builddir/tests ? > > > + > > +AT_DATA([schema], > > + [[{"name": "mydb", > > + "tables": { > > + "SSL": { > > + "columns": { > > + "private_key": {"type": "string"}, > > + "certificate": {"type": "string"}, > > + "ca_cert": {"type": "string"} > > + }}}} > > +]]) > > +AT_CHECK([ovsdb-tool create db schema], [0], [stdout], [ignore]) > > + > > +# Use direct file paths for SSL configuration > > +on_exit 'kill `cat *.pid`' > > +AT_CHECK( > > + [ovsdb-server --log-file -vstream_ssl:dbg --detach --no-chdir > --pidfile \ > > nit: -vstream_ssl:file:dbg to avoid accidentally enabling other types of > logging. > > > + --private-key=backend-service.test-privkey.pem \ > > + --certificate=backend-service.test-cert.pem \ > > + --ca-cert=sni-pki/switchca/cacert.pem \ > > + --remote=pssl:0:127.0.0.1 db], > > + [0], [ignore], [ignore]) > > +PARSE_LISTENING_PORT([ovsdb-server.log], [SSL_PORT]) > > + > > +# SSL_OVSDB_CLIENT_SNI(SNI_HOSTNAME) > > +m4_define([SSL_OVSDB_CLIENT_SNI], [dnl > > + ovsdb-client -vconsole:stream_ssl:dbg \ > > Looks like there is a bug in other tests, the logging module should go > before the target, i.e. -vstream_ssl:console:dbg. > > > + --private-key=backend-service.test-privkey.pem \ > > + --certificate=backend-service.test-cert.pem \ > > + --ca-cert=sni-pki/switchca/cacert.pem \ > > + --ssl-server-name=[$1] \ > > + list-dbs ssl:127.0.0.1:$SSL_PORT]) > > + > > +# SSL_OVSDB_CLIENT_NO_SNI() - test default behavior without SNI override > > +m4_define([SSL_OVSDB_CLIENT_NO_SNI], [dnl > > + ovsdb-client -vconsole:stream_ssl:dbg \ > > + --private-key=backend-service.test-privkey.pem \ > > + --certificate=backend-service.test-cert.pem \ > > + --ca-cert=sni-pki/switchca/cacert.pem \ > > + list-dbs ssl:127.0.0.1:$SSL_PORT]) > > There is no need for two macros, we could check if the arguemnt provided > or not and add the argument conditionally, i.e.: > > m4_if([$1], [], [], [--ssl-server-name=$1]) \ > > > + > > +# SNI Test Flow: > > +# 1. Client sends SNI extension with --ssl-server-name=HOSTNAME during > SSL handshake > > +# 2. Server receives SNI extension and logs "connection indicated > server name HOSTNAME" > > +# 3. We verify SNI transmission by checking server log for the expected > message > > +# This validates that the client-side SNI implementation correctly > sends SNI to the server > > + > > +# Test 1: SNI matches server certificate - should succeed > > +AT_CHECK(SSL_OVSDB_CLIENT_SNI([backend-service.test]), [0], [stdout], > [stderr]) > > +AT_CHECK([grep -q "connection indicated server name > backend-service.test" ovsdb-server.log]) > > May need to be OVS_WAIT_UNTIL. > > > + > > +# Save the current log size to check only new messages in Test 2 > > +LOG_SIZE=$(wc -l < ovsdb-server.log) > > + > > +# Test 2: Default behavior without SNI override - should work > > +# and NOT show SNI server name in debug log (since no --ssl-server-name > was used) > > +AT_CHECK(SSL_OVSDB_CLIENT_NO_SNI(), [0], [stdout], [stderr]) > > +# Check only new log entries after Test 1 - should have no SNI messages > > +AT_CHECK([tail -n +$(($LOG_SIZE + 1)) ovsdb-server.log | grep -q > "connection indicated server name"], [1]) > > It's hard to wait for something that shouldn't happen, so we may need to > do this > check after the server shutdown to be sure that the logs are flushed. > > Also, the line is a little too long, can be split with a '\' in the middle. > > > + > > +OVSDB_SERVER_SHUTDOWN([" > > + /stream_ssl|WARN/d > > + /Protocol error/d > > Do we need these? I haven't run the test, but it shouldn't cause any > warnings, IIUC. > > > +"]) > > +AT_CLEANUP > > + > > OVS_START_SHELL_HELPERS > > # ovsdb_check_online_compaction MODEL > > # > > Best regards, Ilya Maximets. > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
