maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Vladislav!

Again, that's quite ok.
I don't like a new lock around sslaccept(), but I couldn't find any
other solution for it :(

See few comments below.

On Dec 03, Vladislav Vaintroub wrote:
> revision-id: af6a8ea0f79d628384a61986482dec7d3ff751dd (mariadb-10.4.0-55-gaf6a8ea0f79)
> parent(s): 757530b83ca1cc550127311dc6edb47152ba0c6a
> author: Vladislav Vaintroub <wlad@xxxxxxxxxxx>
> committer: Vladislav Vaintroub <wlad@xxxxxxxxxxx>
> timestamp: 2018-12-02 01:25:04 +0100
> message:
> 
> MDEV-16266 : New command FLUSH SSL ro reload SSL acceptor context.
> 
> Reloads certificate, key, CA, CRL.
> 
> diff --git a/mysql-test/main/flush_ssl.test b/mysql-test/main/flush_ssl.test
> new file mode 100644
> index 00000000000..24dbb4719f3
> --- /dev/null
> +++ b/mysql-test/main/flush_ssl.test
> @@ -0,0 +1,24 @@
> +source include/have_ssl_communication.inc;
> +
> +connect  ssl_con,localhost,root,,,,,SSL;
> +SELECT VARIABLE_VALUE != '0' FROM INFORMATION_SCHEMA.GLOBAL_STATUS WHERE VARIABLE_NAME='SSL_ACCEPTS';
> +FLUSH SSL;
> +
> +# Check if SSL_ACCEPTS was flushed (SSL_ACCEPTS = 0)
> +SELECT VARIABLE_VALUE FROM INFORMATION_SCHEMA.GLOBAL_STATUS WHERE VARIABLE_NAME='SSL_ACCEPTS';

How about the test that a certificate was actually reloaded?
That's the main functionality, right? Nobody wanted a new command
just to reset SSL_ACCEPTS

Note, that if you want to generate a new certificate for mysql-test,
don't forget to extend mysql-test/lib/generate-ssl-certs.sh

> +
> +CREATE USER u;
> +connect  ssl_con2,localhost,u,,,,,SSL;
> +
> +# Check SSL_ACCEPTS increased to 1
> +SELECT VARIABLE_VALUE FROM INFORMATION_SCHEMA.GLOBAL_STATUS WHERE VARIABLE_NAME='SSL_ACCEPTS';
> +
> +# Check that FLUSH SSL does not work for unprivileged user
> +error ER_SPECIFIC_ACCESS_DENIED_ERROR;
> +FLUSH SSL;
> +
> +disconnect ssl_con2;
> +disconnect ssl_con;
> +connection default;
> +DROP USER u;
> +
> diff --git a/sql/mysqld.cc b/sql/mysqld.cc
> index 9ff47dc1ff1..13aad0a0197 100644
> --- a/sql/mysqld.cc
> +++ b/sql/mysqld.cc
> @@ -4766,6 +4771,56 @@ static void openssl_lock(int mode, openssl_lock_t *lock, const char *file,
>  }
>  #endif /* HAVE_OPENSSL10 */
>  
> +
> +struct SSL_ACCEPTOR_STATS
> +{
> +  long accept;
> +  long accept_good;
> +  long cache_size;
> +  long verify_mode;
> +  long verify_depth;
> +  const char *session_cache_mode;
> +  long zero;
> +  SSL_ACCEPTOR_STATS()
> +  {
> +    session_cache_mode = "NONE";

are you relying on everything else being zero'ed by default because
SSL_ACCEPTOR_STATS is instantiated statically? May be, still, add a
bzero?

> +  }
> +  void init()
> +  {
> +    DBUG_ASSERT(ssl_acceptor_fd);
> +    accept = 0;
> +    accept_good = 0;
> +    verify_mode = SSL_CTX_get_verify_mode(ssl_acceptor_fd->ssl_context);
> +    verify_depth = SSL_CTX_get_verify_depth(ssl_acceptor_fd->ssl_context);
> +    cache_size = SSL_CTX_sess_get_cache_size(ssl_acceptor_fd->ssl_context);
> +    switch (SSL_CTX_get_session_cache_mode(ssl_acceptor_fd->ssl_context))
> +    {
> +    case SSL_SESS_CACHE_OFF:
> +      session_cache_mode = "OFF"; break;
> +    case SSL_SESS_CACHE_CLIENT:
> +      session_cache_mode = "CLIENT"; break;
> +    case SSL_SESS_CACHE_SERVER:
> +      session_cache_mode = "SERVER"; break;
> +    case SSL_SESS_CACHE_BOTH:
> +      session_cache_mode = "BOTH"; break;
> +    case SSL_SESS_CACHE_NO_AUTO_CLEAR:
> +      session_cache_mode = "NO_AUTO_CLEAR"; break;
> +    case SSL_SESS_CACHE_NO_INTERNAL_LOOKUP:
> +      session_cache_mode = "NO_INTERNAL_LOOKUP"; break;
> +    default:
> +      session_cache_mode = "Unknown"; break;
> +    }
> +  }
> +};
> +
> +static SSL_ACCEPTOR_STATS ssl_acceptor_stats;
> +void ssl_acceptor_stats_update(int sslaccept_ret)
> +{
> +  ssl_acceptor_stats.accept++;
> +  if (!sslaccept_ret)
> +    ssl_acceptor_stats.accept_good++;

use statistic_increment here

> +}
> +
>  static void init_ssl()
>  {
>  #if defined(HAVE_OPENSSL) && !defined(EMBEDDED_LIBRARY)
> @@ -4804,6 +4862,38 @@ static void init_ssl()
>  #endif /* HAVE_OPENSSL && ! EMBEDDED_LIBRARY */
>  }
>  
> +/* Reinitialize SSL (FLUSH SSL) */
> +int reinit_ssl()
> +{
> +#if defined(HAVE_OPENSSL) && !defined(EMBEDDED_LIBRARY)
> +  if (!opt_use_ssl)
> +    return 0;
> +#ifndef DBUG_OFF
> +  char *old_ssl_cert;

it's generally prefered to use __attribute__((unused)) instead of #ifdef

> +#endif
> +
> +  DBUG_EXECUTE_IF("simulate_bad_ssl_cert", old_ssl_cert= opt_ssl_cert; opt_ssl_cert=const_cast<char *>(""););
> +
> +  enum enum_ssl_init_error error = SSL_INITERR_NOERROR;
> +  st_VioSSLFd *new_fd = new_VioSSLAcceptorFd(opt_ssl_key, opt_ssl_cert,
> +    opt_ssl_ca, opt_ssl_capath, opt_ssl_cipher, &error, opt_ssl_crl, opt_ssl_crlpath);
> +
> +  DBUG_EXECUTE_IF("simulate_bad_ssl_cert", opt_ssl_cert= old_ssl_cert;);
> +
> +  if (!new_fd)
> +  {
> +    my_printf_error(ER_UNKNOWN_ERROR, "Failed to refresh SSL, error: %s", MYF(0),
> +      sslGetErrString(error));
> +    return 1;
> +  }
> +  mysql_rwlock_wrlock(&LOCK_ssl_refresh);
> +  free_vio_ssl_acceptor_fd(ssl_acceptor_fd);
> +  ssl_acceptor_fd= new_fd;
> +  ssl_acceptor_stats.init();
> +  mysql_rwlock_unlock(&LOCK_ssl_refresh);
> +  return 0;
> +#endif
> +}
>  
Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx