On 10/15/25 11:27 PM, Guru Shetty wrote:
> 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".
OK.
>
>> 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?
Initially I thought we'll need some changes in ovs-vswitchd as well, but
it's not the case.
>
>
>> 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.
Thanks!
>
>
>>>
>>> #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.
A separate test makes sense. I'll take a look at v3.
Best regards, Ilya Maximets.
>
> 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
