Re: [PATCH] pgsql: add SSL connection implementation to PGSQL plugin

[Eric Leblond <eric@regit.org>]

[-- Attachment #1: Type: text/plain, Size: 4963 bytes --]

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 #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]