PATCH: Add support in dict_mysql.c for enabling SSL and reading from my.cnf files

Discussion:

Gareth Palmer

2013-11-13 04:36:09 UTC

Hello,

The following patch adds support for using SSL when connecting to a
MySQL server and support for reading my.cnf files which can set other
connection related options.

New configuration parameters for SSL connections are -

tls_cert_file - File containing the client's public key.
tls_key_file - File containing the client's private key.
tls_CAfile - File containing the server's public key.
tls_CApath - Directory containing the server's public key.
tls_ciphers - A list of permissible ciphers to use for encryption.
tls_verify_cert - Verify that the name of the server matches the
common name in the certificate.

New configuration parameters for reading my.cnf files are -

option_file - Name of the file to read from.
option_group - Name of the group to read from.

Both are empty by default so no option file will be read from.

Wietse Venema

2013-11-16 01:12:47 UTC

Permalink

Post by Gareth Palmer
Hello,
The following patch adds support for using SSL when connecting to a
MySQL server and support for reading my.cnf files which can set other
connection related options.
New configuration parameters for SSL connections are -
tls_cert_file - File containing the client's public key.
tls_key_file - File containing the client's private key.
tls_CAfile - File containing the server's public key.
tls_CApath - Directory containing the server's public key.
tls_ciphers - A list of permissible ciphers to use for encryption.
tls_verify_cert - Verify that the name of the server matches the
common name in the certificate.
New configuration parameters for reading my.cnf files are -
option_file - Name of the file to read from.
option_group - Name of the group to read from.
Both are empty by default so no option file will be read from.

Thanks for cleaning up some of the internal APIs.

However, please confirm that your code works with configuration
files that do not set the new parameters.

In the following code, cfg_get_str() returns a null character pointer
when a parameter is not specified in the Postfix configuration file:

+ dict_mysql->option_file = cfg_get_str(p, "option_file", NULL, 0, 0);
+ dict_mysql->option_group = cfg_get_str(p, "option_group", NULL, 0, 0);
+#if defined(MYSQL_VERSION_ID) && MYSQL_VERSION_ID >= 40000
+ dict_mysql->tls_key_file = cfg_get_str(p, "tls_key_file", NULL, 0, 0);
+ dict_mysql->tls_cert_file = cfg_get_str(p, "tls_cert_file", NULL, 0, 0);
+ dict_mysql->tls_CAfile = cfg_get_str(p, "tls_CAfile", NULL, 0, 0);
+ dict_mysql->tls_CApath = cfg_get_str(p, "tls_CApath", NULL, 0, 0);
+ dict_mysql->tls_ciphers = cfg_get_str(p, "tls_ciphers", NULL, 0, 0);

When the database is closed, the dict_mysql_close() function passes
these null pointers to myfree().

+ myfree(dict_mysql->option_file);
+ myfree(dict_mysql->option_group);
+ #if defined(MYSQL_VERSION_ID) && MYSQL_VERSION_ID >= 40000
+ myfree(dict_mysql->tls_key_file);
+ myfree(dict_mysql->tls_cert_file);
+ myfree(dict_mysql->tls_CAfile);
+ myfree(dict_mysql->tls_CApath);
+ myfree(dict_mysql->tls_ciphers);
+ #endif

However. myfree() does not accept null pointer arguments: It will
abort the program instead.

You can test the dict_mysql_close() routine with the "mail_dict"
test program:

$ cd src/global
$ make mail_dict
$ ./mail_dict mysql:/path/to/file read
get foo
^D

The simplest fix is to replace

myfree(dict_mysql->whatever);

with
if (dict_mysql->whatever)
myfree(dict_mysql->whatever);

I can make that change if you like.

However it would be good if you can confirm that the code
works even when none of the new parameters is specified.

Wietse

Gareth Palmer

2013-11-16 02:35:48 UTC

Permalink

Hi Wietse,

Post by Wietse Venema

Thanks for cleaning up some of the internal APIs.
However, please confirm that your code works with configuration
files that do not set the new parameters.
In the following code, cfg_get_str() returns a null character pointer
+ dict_mysql->option_file = cfg_get_str(p, "option_file", NULL, 0, 0);
+ dict_mysql->option_group = cfg_get_str(p, "option_group", NULL, 0, 0);
+#if defined(MYSQL_VERSION_ID) && MYSQL_VERSION_ID >= 40000
+ dict_mysql->tls_key_file = cfg_get_str(p, "tls_key_file", NULL, 0, 0);
+ dict_mysql->tls_cert_file = cfg_get_str(p, "tls_cert_file", NULL, 0, 0);
+ dict_mysql->tls_CAfile = cfg_get_str(p, "tls_CAfile", NULL, 0, 0);
+ dict_mysql->tls_CApath = cfg_get_str(p, "tls_CApath", NULL, 0, 0);
+ dict_mysql->tls_ciphers = cfg_get_str(p, "tls_ciphers", NULL, 0, 0);
When the database is closed, the dict_mysql_close() function passes
these null pointers to myfree().
+ myfree(dict_mysql->option_file);
+ myfree(dict_mysql->option_group);
+ #if defined(MYSQL_VERSION_ID) && MYSQL_VERSION_ID >= 40000
+ myfree(dict_mysql->tls_key_file);
+ myfree(dict_mysql->tls_cert_file);
+ myfree(dict_mysql->tls_CAfile);
+ myfree(dict_mysql->tls_CApath);
+ myfree(dict_mysql->tls_ciphers);
+ #endif
However. myfree() does not accept null pointer arguments: It will
abort the program instead.
You can test the dict_mysql_close() routine with the "mail_dict"
$ cd src/global
$ make mail_dict
$ ./mail_dict mysql:/path/to/file read
get foo
^D
The simplest fix is to replace
myfree(dict_mysql->whatever);
with
if (dict_mysql->whatever)
myfree(dict_mysql->whatever);
I can make that change if you like.

Thanks for reviewing the patch.

Attached is a new patch against 2.11-20131114 with those fixes.

Post by Wietse Venema
However it would be good if you can confirm that the code
works even when none of the new parameters is specified.

I tested using mail_dict with none, some and all of the new parameters,
it doesn't segfault now.

Testing also revealed another bug in the patch when giving a NULL arg to
mysql_options(). Fixed that too.

Post by Wietse Venema
Wietse

Gareth Palmer

2013-11-16 03:04:21 UTC

Permalink

Oops, supplied an older version of the patch without the mysql_option()
fix.

Proper version attached.

Post by Wietse Venema

Thanks for cleaning up some of the internal APIs.
However, please confirm that your code works with configuration
files that do not set the new parameters.
In the following code, cfg_get_str() returns a null character pointer
+ dict_mysql->option_file = cfg_get_str(p, "option_file", NULL, 0, 0);
+ dict_mysql->option_group = cfg_get_str(p, "option_group", NULL, 0, 0);
+#if defined(MYSQL_VERSION_ID) && MYSQL_VERSION_ID >= 40000
+ dict_mysql->tls_key_file = cfg_get_str(p, "tls_key_file", NULL, 0, 0);
+ dict_mysql->tls_cert_file = cfg_get_str(p, "tls_cert_file", NULL, 0, 0);
+ dict_mysql->tls_CAfile = cfg_get_str(p, "tls_CAfile", NULL, 0, 0);
+ dict_mysql->tls_CApath = cfg_get_str(p, "tls_CApath", NULL, 0, 0);
+ dict_mysql->tls_ciphers = cfg_get_str(p, "tls_ciphers", NULL, 0, 0);
When the database is closed, the dict_mysql_close() function passes
these null pointers to myfree().
+ myfree(dict_mysql->option_file);
+ myfree(dict_mysql->option_group);
+ #if defined(MYSQL_VERSION_ID) && MYSQL_VERSION_ID >= 40000
+ myfree(dict_mysql->tls_key_file);
+ myfree(dict_mysql->tls_cert_file);
+ myfree(dict_mysql->tls_CAfile);
+ myfree(dict_mysql->tls_CApath);
+ myfree(dict_mysql->tls_ciphers);
+ #endif
However. myfree() does not accept null pointer arguments: It will
abort the program instead.
You can test the dict_mysql_close() routine with the "mail_dict"
$ cd src/global
$ make mail_dict
$ ./mail_dict mysql:/path/to/file read
get foo
^D
The simplest fix is to replace
myfree(dict_mysql->whatever);
with
if (dict_mysql->whatever)
myfree(dict_mysql->whatever);
I can make that change if you like.
However it would be good if you can confirm that the code
works even when none of the new parameters is specified.
Wietse

Wietse Venema

2013-11-17 14:12:19 UTC

Permalink

Post by Gareth Palmer
Oops, supplied an older version of the patch without the mysql_option()
fix.
Proper version attached.

This looks good. For sites that need this in Postfix 2.10 and earlier,
it can be found on the source code mirrors as

.../experimental/feature-patches/20131116-mysql-options-tls-patch

Wietse