Hello,
Please find my review inline.
Le samedi 01 septembre 2012 à 13:51 +0100, Mr Dash Four a écrit :
> This patch fixes a NULL pointer reference bug which existed in the
> PGSQL output plugin, as well as enables SSL connections to be made
> to PostgreSQL server by the ulog daemon. Parameters introduced are:
>
> 'sslmode' - one of:
>
...
> 'sslkey' - This parameter specifies the location for the secret key used
> for the client certificate. It can either specify a file name that
> will be used or it can specify a key obtained from an external
> “engine” (engines are OpenSSL loadable modules). An external
> engine specification should consist of a colon-separated engine
> name and an engine-specific key identifier. This parameter is
> ignored if SSL connection is not made. If this key is protected
> with a password, this will be asked when the connection is made.
> It is asked every time an attempt for a connection is made.
Entering key for each new connection, you've find a new business for low
profile admin ;)
> 'sslroot' - This parameter specifies the name of a file containing SSL
> certificate authority (CA) certificate(s). If the file exists,
> the server's certificate will be verified to be signed by one of
> these authorities.
> 'sslcrl' - This parameter specifies the file name of the SSL certificate
> revocation list (CRL). Certificates listed in this file, if it
> exists, will be rejected while attempting to authenticate the
> server's certificate.
I don't see here the 'sslca' parameter: how ulogd does to verify
database certificate if it does not know which CA certs to use ?
> Example of use:
...
> SQL.c
> @@ -38,7 +38,7 @@ struct pgsql_instance {
>
> /* our configuration directives */
> static struct config_keyset pgsql_kset = {
> - .num_ces = DB_CE_NUM + 6,
> + .num_ces = DB_CE_NUM + 11,
> .ces = {
> DB_CES,
> {
> @@ -70,8 +70,32 @@ static struct config_keyset pgsql_kset = {
> .key = "schema",
> .type = CONFIG_TYPE_STRING,
> .options = CONFIG_OPT_NONE,
> - .u.string = "public",
I don't see why this default value has been removed. Is this linked with
current feature ?
> },
> + { // sslmode=disable|allow|prefer|require|requiressl|verify-ca|verify-full
> + .key = "sslmode",
> + .type = CONFIG_TYPE_STRING,
> + .options = CONFIG_OPT_NONE,
> + },
No default value here. From code below, I understand that we will not
pass any SSL-related parameter in PGSQL connection chain if there is no
value.
What is the difference with using "disable" as default ?
> + {
> + .key = "sslcert",
> + .type = CONFIG_TYPE_STRING,
> + .options = CONFIG_OPT_NONE,
> + },
> + {
...
> ulogd_log(ULOGD_DEBUG, "%s\n", pgbuf);
> @@ -217,23 +249,39 @@ static int open_db_pgsql(struct ulogd_pluginstance *upi)
> {
> struct pgsql_instance *pi = (struct pgsql_instance *) upi->private;
> int len;
> + int status;
> char *connstr;
> char *server = host_ce(upi->config_kset).u.string;
> unsigned int port = port_ce(upi->config_kset).u.value;
> char *user = user_ce(upi->config_kset).u.string;
> char *pass = pass_ce(upi->config_kset).u.string;
> char *db = db_ce(upi->config_kset).u.string;
> + char *sslmode = sslmode_ce(upi->config_kset).u.string;
> + char *sslcert = sslcert_ce(upi->config_kset).u.string;
> + char *sslkey = sslkey_ce(upi->config_kset).u.string;
> + char *sslroot = sslroot_ce(upi->config_kset).u.string;
> + char *sslcrl = sslcrl_ce(upi->config_kset).u.string;
>
> /* 80 is more than what we need for the fixed parts below */
> len = 80 + strlen(user) + strlen(db);
>
> - /* hostname and and password are the only optionals */
> + /* hostname and password are not the only optional parameters */
> if (server)
> len += strlen(server);
> if (pass)
> len += strlen(pass);
> if (port)
> len += 20;
> + if (sslmode)
> + len += strlen(sslmode);
> + if (sslcert)
> + len += strlen(sslcert);
> + if (sslkey)
> + len += strlen(sslkey);
> + if (sslroot)
> + len += strlen(sslroot);
> + if (sslcrl)
> + len += strlen(sslcrl);
OK, we need to increase the length of the connection string and thus
this code is needed.
But, I don't see the length for the prefix use in the connection string.
For example we have below:
strcat(connstr, " sslmode=");
strcat(connstr, sslmode);
Am I missing something ?
> connstr = (char *) malloc(len);
> if (!connstr)
> @@ -261,10 +309,37 @@ static int open_db_pgsql(struct ulogd_pluginstance *upi)
> strcat(connstr, pass);
> }
>
> + if (sslmode && strlen(sslmode) > 0) {
> + if (strncmp(sslmode, "requiressl", 10) == 0) {
> + strcat(connstr, " requiressl=1");
BR,
--
Eric Leblond
Blog: http://home.regit.org/ - Portfolio: http://regit.500px.com/
Attachment:
signature.asc
Description: This is a digitally signed message part
