Attempting to call SSL_set_session() on every trip through the SSL
connection state machine seems like it could cause the session to be
re-set to the cached one even after the server has told us which session
is actually to be used.
In testing, this change didn't make any difference, but it seems seems like
the right thing to do.
Bug #4448.
A cached SSL session may only be used for new connections after the initial
connection has shut down. As far as I can tell, nothing in the OpenSSL
documentation actually comes out and says this, but it is implied by
various examples found around the web and doing it this way makes caching
work much more reliably in my testing.
Bug #4448.
In the finest OpenSSL tradition of putting important documentation only in
code comments, ssl/ssl_sess.c in the OpenSSL tree has the following comment
inside ssl_get_prev_session():
/* We can't be sure if this session is being used out of
* context, which is especially important for SSL_VERIFY_PEER.
* The application should have used SSL[_CTX]_set_session_id_context.
*
* For this error case, we generate an error instead of treating
* the event like a cache miss (otherwise it would be easy for
* applications to effectively disable the session cache by
* accident without anyone noticing).
*/
This meant that ovs-controller couldn't effectively cache SSL server
sessions and we got a weird error whenever ovs-vswitchd tried.
Bug #4448.
CC: David Tsai <dtsai@nicira.com>
CC: Jeremy Stribling <strib@nicira.com>
Until now, the collection of coverage counters supported by a given OVS
program was not specific to that program. That means that, for example,
even though ovs-dpctl does not have anything to do with mac_learning, it
still has a coverage counter for it. This is confusing, at best.
This commit fixes the problem on some systems, in particular on ones that
use GCC and the GNU linker. It uses the feature of the GNU linker
described in its manual as:
If an orphaned section's name is representable as a C identifier then
the linker will automatically see PROVIDE two symbols: __start_SECNAME
and __end_SECNAME, where SECNAME is the name of the section. These
indicate the start address and end address of the orphaned section
respectively.
Systems that don't support these features retain the earlier behavior.
This commit also fixes the annoyance that files that include coverage
counters must be listed on COVERAGE_FILES in lib/automake.mk.
This commit also fixes the annoyance that modifying any source file that
includes a coverage counter caused all programs that link against
libopenvswitch.a to relink, even programs that the source file was not
linked into. For example, modifying ofproto/ofproto.c (which includes
coverage counters) caused tests/test-aes128 to relink, even though
test-aes128 does not link again ofproto.o.
All of these changes avoid using the same name for two local variables
within a same function. None of them are actual bugs as far as I can tell,
but any of them could be confusing to the casual reader.
The one in lib/ovsdb-idl.c is particularly brilliant: inner and outer
loops both using (different) variables named 'i'.
Found with GCC -Wshadow.
OpenSSL is picky about the order in which keys and certificates are
changed: you have to change the certificate first, then the key. It
doesn't document this, but deep in the source code, in a function that sets
a new certificate, it has this comment:
/* don't fail for a cert/key mismatch, just free
* current private key (when switching to a different
* cert & key, first this function should be used,
* then ssl_set_pkey */
Brilliant, guys, thanks a lot.
Bug #2921.
Adding a macro to define the vlog module in use adds a level of
indirection, which makes it easier to change how the vlog module must be
defined. A followup commit needs to do that, so getting these widespread
changes out of the way first should make that commit easier to review.
Sometimes seeing a little bit of SSL protocol information can be valuable
in debugging connection problems. With this commit, setting the stream_ssl
logging module to DBG level will cause basic SSL handshake information to
be logged for new connections.
The OpenSSL manpage for SSL_get_error() says this:
In addition to ssl and ret, SSL_get_error() inspects the current
thread's OpenSSL error queue. Thus, SSL_get_error() must be used in
the same thread that performed the TLS/SSL I/O operation, and no other
OpenSSL function calls should appear in between. The current thread's
error queue must be empty before the TLS/SSL I/O operation is
attempted, or SSL_get_error() will not work reliably.
We weren't taking this advice literally enough, which meant that this
would happen:
1. Call SSL_shutdown() on one connection.
2. Call SSL_read() on another connection, returning 0 bytes. (This is
normal. It just means that no more data has arrived yet.)
3. Call SSL_get_error() for that second connection to check whether
the 0-byte return value was a real error. (This should return
SSL_ERROR_WANT_READ to indicate that more data is needed.)
4. Actually get some other error indicating that the SSL_shutdown()
call returned an error.
This commit fixes the problem by flushing the OpenSSL error queue after
calling SSL_shutdown().
Without this commit, starting an ovsdb-server with two active SSL remotes,
running two ovsdb-clients listening for connections from the ovsdb-server
remotes, then killing one of the ovsdb-clients (with e.g. Control+C), will
cause ovsdb-server to drop the other ovsdb-client connnection the next time
that SSL_read() is called on it. With this commit, this scenario works
correctly (e.g. ovsdb-server keeps the remaining connection up).
CC: Jeremy Stribling <strib@nicira.com>
Sometimes, when a user asks me to help debug a problem, it turns out that
an SSL connection was being made on a TCP port, or vice versa, or that an
OpenFlow connection was being made on a JSON-RPC port, or vice versa, and
so on. This commit adds log messages that diagnose this kind of problem,
e.g. "tcp:127.0.0.1:6633: received JSON-RPC data on OpenFlow channel".
Commit b84f503d "stream-ssl: Read existing CA certificate more eagerly
during bootstrap" inadvertently introduced an access-after-free error:
do_ca_cert_bootstrap() calls
stream_ssl_set_ca_cert_file(ca_cert.file_name, true), which calls
update_ssl_config(&ca_cert, file_name), which calls
free(ca_cert.file_name) then xstrdup(ca_cert.file_name).
Fix the problem.
Reported-by: Cedric Hobbs <cedric@nicira.com>
Reported-by: Peter Balland <peter@nicira.com>
In Citrix XenServer, the hosts have SSL private keys and certificates, but
those certificates are not signed by any certificate authority. So we
must provide a way to avoid checking certificates against a CA if we want
other OVS tools to be able to talk to XenServer hosts over SSL. This
commit makes that possible.
When do_ca_cert_bootstrap() attempts to bootstrap a CA certificate from a
remote host, it gives up if the CA certificate file already exists. It
knows that this file did not exist some time earlier (because it checked),
so it logged a warning and just returns. The next time that
stream_ssl_set_ca_cert_file() gets called, it will read the new CA
certificate file and all will be well.
That works OK in ovsdb-server, which calls stream_ssl_set_ca_cert_file()
every time through its main loop. It does not work well for ovs-vswitchd,
which only calls that function when it needs to reconfigure. But it
should work fine to call it directly from do_ca_cert_bootstrap(), so this
commit changes it to do that.
Bug #2635.
Commit 415f6c0b1 "stream-ssl: Make no-op reconfiguration cheap" caused
ovsdb-server to re-read its certificates and keys every 60 seconds just
in case they changed. However, doing this causes OpenSSL to drop its
connections. This commit solves the problem by making stream-ssl re-read
certificates and keys only if the files changed.
Bug #2535.
Reported-by: Ram Jothikumar <rjothikumar@nicira.com>
Until now, the stream_ssl functions for configuring private keys,
certificates, and CA certificates have always called into OpenSSL to read
a file. This commit instead makes them do that only if the file name
changed (or it has been 60 seconds since we last tried, in case someone
installed the file behind our backs).
This allows us to factor some code out of vswitchd. In an upcoming commit
we will want to do essentially the same thing from ovsdb-server, so this
avoid code redundancy.
If two processes were both configured to bootstrap the CA certificate, then
one of them would succeed in writing it to a file and use it, and the other
one would fail to use it because the file was created behind its back.
This commit fixes the problem by making the bootstrap code accept a CA
certificate file that exists at the time that bootstrapping tries to create
it.
The names of passive SSL and TCP streams were being poorly reported: TCP
always simply reported "ptcp", and SSL reported whatever was passed in.
This commit makes them report the addresses that were actually bound by
the TCP/IP stack, which is more useful for testing, debugging, and logging.
