Hi Wietse,
On Fri, 2013-11-15 at 20:12 -0500, Wietse Venema wrote:
> 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.
Thanks for reviewing the patch.
Attached is a new patch against 2.11-20131114 with those fixes.
> 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.
> Wietse
diff -urN postfix-2.11-20131114.orig/man/man5/mysql_table.5 postfix-2.11-20131114/man/man5/mysql_table.5
--- postfix-2.11-20131114.orig/man/man5/mysql_table.5 2013-11-16 14:54:05.379045451 +1300
+++ postfix-2.11-20131114/man/man5/mysql_table.5 2013-11-16 15:00:55.751670027 +1300
@@ -256,6 +256,25 @@
temporary error if the limit is exceeded. Setting the
limit to 1 ensures that lookups do not return multiple
values.
+.IP "\fBtls_cert_file\fR"
+File containing client's X509 certificate.
+.IP "\fBtls_key_file\fR"
+File containing the private key corresponding to \fBtls_cert_file\fR.
+.IP "\fBtls_CAfile\fR"
+File containing certificates for all of the X509 Certificate
+Authorities the client will recognize. Takes precedence over
+\fBtls_CApath\fR.
+.IP "\fBtls_CApath\fR"
+Directory containing X509 Certificate Authority certificates
+in separate individual files.
+.IP "\fBtls_verify_cert (default: no)\fR"
+Verify that the server's name matches the common name in the
+certficate.
+.IP "\fBoption_file\fR"
+Read options from the given file instead of the default my.cnf
+location.
+.IP "\fBoption_group\fR"
+Read options from the given group.
.SH "OBSOLETE QUERY INTERFACE"
.na
.nf
diff -urN postfix-2.11-20131114.orig/src/global/dict_mysql.c postfix-2.11-20131114/src/global/dict_mysql.c
--- postfix-2.11-20131114.orig/src/global/dict_mysql.c 2013-11-16 14:54:05.371045594 +1300
+++ postfix-2.11-20131114/src/global/dict_mysql.c 2013-11-16 15:00:36.948014146 +1300
@@ -91,6 +91,23 @@
/* releases.
/* .IP hosts
/* List of hosts to connect to.
+/* .IP tls_cert_file
+/* File containing client's X509 certificate.
+/* .IP tls_key_file
+/* File containing the private key corresponding to \fItls_cert_file\fR.
+/* .IP tls_CAfile
+/* File containing certificates for all of the X509 Certificate
+/* Authorities the client will recognize. Takes precedence over
+/* \fItls_CApath\fR.
+/* .IP tls_CApath
+/* Directory containing X509 Certificate Authority certificates
+/* in separate individual files.
+/* .IP tls_verify_cert
+/* Verify that the server's name matches the common name of the certficate.
+/* .IP option_file
+/* Read options from the given file instead of the default my.cnf location.
+/* .IP option_group
+/* Read options from the given group.
/* .PP
/* For example, if you want the map to reference databases of
/* the name "your_db" and execute a query like this: select
@@ -217,6 +234,8 @@
CFG_PARSER *parser;
char *query;
char *result_format;
+ char *option_file;
+ char *option_group;
void *ctx;
int expansion_limit;
char *username;
@@ -226,6 +245,14 @@
PLMYSQL *pldb;
#if defined(MYSQL_VERSION_ID) && MYSQL_VERSION_ID >= 40000
HOST *active_host;
+ char *tls_cert_file;
+ char *tls_key_file;
+ char *tls_CAfile;
+ char *tls_CApath;
+ char *tls_ciphers;
+#if MYSQL_VERSION_ID >= 50023
+ int tls_verify_cert;
+#endif
#endif
} DICT_MYSQL;
@@ -242,12 +269,11 @@
/* internal function declarations */
static PLMYSQL *plmysql_init(ARGV *);
-static MYSQL_RES *plmysql_query(DICT_MYSQL *, const char *, VSTRING *, char *,
- char *, char *);
+static MYSQL_RES *plmysql_query(DICT_MYSQL *, const char *, VSTRING *);
static void plmysql_dealloc(PLMYSQL *);
static void plmysql_close_host(HOST *);
static void plmysql_down_host(HOST *);
-static void plmysql_connect_single(HOST *, char *, char *, char *);
+static void plmysql_connect_single(DICT_MYSQL *, HOST *);
static const char *dict_mysql_lookup(DICT *, const char *);
DICT *dict_mysql_open(const char *, int, int);
static void dict_mysql_close(DICT *);
@@ -349,10 +375,7 @@
return (0);
/* do the query - set dict->error & cleanup if there's an error */
- if ((query_res = plmysql_query(dict_mysql, name, query,
- dict_mysql->dbname,
- dict_mysql->username,
- dict_mysql->password)) == 0) {
+ if ((query_res = plmysql_query(dict_mysql, name, query)) == 0) {
dict->error = DICT_ERR_RETRY;
return (0);
}
@@ -428,10 +451,10 @@
/* dict_mysql_get_active - get an active connection */
-static HOST *dict_mysql_get_active(PLMYSQL *PLDB, char *dbname,
- char *username, char *password)
+static HOST *dict_mysql_get_active(DICT_MYSQL *dict_mysql)
{
const char *myname = "dict_mysql_get_active";
+ PLMYSQL *PLDB = dict_mysql->pldb;
HOST *host;
int count = RETRY_CONN_MAX;
@@ -457,7 +480,7 @@
if (msg_verbose)
msg_info("%s: attempting to connect to host %s", myname,
host->hostname);
- plmysql_connect_single(host, dbname, username, password);
+ plmysql_connect_single(dict_mysql, host);
if (host->stat == STATACTIVE)
return host;
}
@@ -485,17 +508,12 @@
static MYSQL_RES *plmysql_query(DICT_MYSQL *dict_mysql,
const char *name,
- VSTRING *query,
- char *dbname,
- char *username,
- char *password)
+ VSTRING *query)
{
- PLMYSQL *PLDB = dict_mysql->pldb;
HOST *host;
MYSQL_RES *res = 0;
- while ((host = dict_mysql_get_active(PLDB, dbname, username, password)) != NULL) {
-
+ while ((host = dict_mysql_get_active(dict_mysql)) != NULL) {
#if defined(MYSQL_VERSION_ID) && MYSQL_VERSION_ID >= 40000
/*
@@ -534,15 +552,30 @@
* used to reconnect to a single database when one is down or none is
* connected yet. Log all errors and set the stat field of host accordingly
*/
-static void plmysql_connect_single(HOST *host, char *dbname, char *username, char *password)
+static void plmysql_connect_single(DICT_MYSQL *dict_mysql, HOST *host)
{
if ((host->db = mysql_init(NULL)) == NULL)
msg_fatal("dict_mysql: insufficient memory");
+ mysql_options(host->db, MYSQL_READ_DEFAULT_FILE, dict_mysql->option_file);
+ mysql_options(host->db, MYSQL_READ_DEFAULT_GROUP, dict_mysql->option_group);
+#if defined(MYSQL_VERSION_ID) && MYSQL_VERSION_ID >= 40000
+ if (dict_mysql->tls_key_file || dict_mysql->tls_cert_file ||
+ dict_mysql->tls_CAfile || dict_mysql->tls_CApath || dict_mysql->tls_ciphers)
+ mysql_ssl_set(host->db,
+ dict_mysql->tls_key_file, dict_mysql->tls_cert_file,
+ dict_mysql->tls_CAfile, dict_mysql->tls_CApath,
+ dict_mysql->tls_ciphers);
+#if MYSQL_VERSION_ID >= 50023
+ if (dict_mysql->tls_verify_cert != -1)
+ mysql_options(host->db, MYSQL_OPT_SSL_VERIFY_SERVER_CERT,
+ &dict_mysql->tls_verify_cert);
+#endif
+#endif
if (mysql_real_connect(host->db,
(host->type == TYPEINET ? host->name : 0),
- username,
- password,
- dbname,
+ dict_mysql->username,
+ dict_mysql->password,
+ dict_mysql->dbname,
host->port,
(host->type == TYPEUNIX ? host->name : 0),
0)) {
@@ -582,7 +615,7 @@
static void mysql_parse_config(DICT_MYSQL *dict_mysql, const char *mysqlcf)
{
- const char *myname = "mysqlname_parse";
+ const char *myname = "mysql_parse_config";
CFG_PARSER *p = dict_mysql->parser;
VSTRING *buf;
char *hosts;
@@ -591,6 +624,18 @@
dict_mysql->password = cfg_get_str(p, "password", "", 0, 0);
dict_mysql->dbname = cfg_get_str(p, "dbname", "", 1, 0);
dict_mysql->result_format = cfg_get_str(p, "result_format", "%s", 1, 0);
+ 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);
+#if MYSQL_VERSION_ID >= 50023
+ dict_mysql->tls_verify_cert = cfg_get_bool(p, "tls_verify_cert", -1);
+#endif
+#endif
/*
* XXX: The default should be non-zero for safety, but that is not
@@ -759,6 +804,22 @@
myfree(dict_mysql->dbname);
myfree(dict_mysql->query);
myfree(dict_mysql->result_format);
+ if (dict_mysql->option_file)
+ myfree(dict_mysql->option_file);
+ if (dict_mysql->option_group)
+ myfree(dict_mysql->option_group);
+#if defined(MYSQL_VERSION_ID) && MYSQL_VERSION_ID >= 40000
+ if (dict_mysql->tls_key_file)
+ myfree(dict_mysql->tls_key_file);
+ if (dict_mysql->tls_cert_file)
+ myfree(dict_mysql->tls_cert_file);
+ if (dict_mysql->tls_CAfile)
+ myfree(dict_mysql->tls_CAfile);
+ if (dict_mysql->tls_CApath)
+ myfree(dict_mysql->tls_CApath);
+ if (dict_mysql->tls_ciphers)
+ myfree(dict_mysql->tls_ciphers);
+#endif
if (dict_mysql->hosts)
argv_free(dict_mysql->hosts);
if (dict_mysql->ctx)
