| Message ID | 1346503911-3781-1-git-send-email-mr.dash.four@googlemail.com |
|---|---|
| State | Not Applicable |
| Headers | show |
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,
Hi Eric, >> 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 ;) Hehe, that's why I use SSL engines instead - at least the password there is cached by the engine once entered (I have USB token which I use). >> '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 ? It is verified in two ways - via "verify-ca" and "verify-full". The sslroot contains the CA certificate chain. When ulogd attempts to connect, the server presents its certificate which is then verified whether it is signed by the CA present in sslroot. The whole CA chain is also then verified in that step. If verification fails, ulogd disconnects with an error. If not, and "sslmode=verify-ca" is specified, then the process stops there. If "sslmode=verify-full", then the process continues and host/IP address verification also takes place, which is then matched against the subject value (or alt subject if that value is present in the certificate). >> @@ -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 ? No, it relates to two things - the NULL pointer reference bug I found out, as well as the fact that the "public" schema is assumed by default as far as PostgreSQL server is concerned. In other words, if I specify a blank (""), that would be the equivalent to "public". In other words, "SELECT * FROM public.some_table" is the equivalent of "SELECT * FROM some_table" - there is no difference. > >> }, >> + { // 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. The "default" as far as PostgreSQL server is concerned (in other words if no sslmode is specified, i.e. "sslmode=" or nothing) is "prefer" - i.e. to attempt SSL connection first (with rudimentary SSL checks) and if that fails, downgrade the connection to non-SSL. See the description for "prefer" in my patch. So, if nothing is specified here for sslmode (i.e. "" or "sslmode="), then PostgreSQL server assumes a "prefer" mode. "", "sslmode=" and "sslmode=prefer" are therefore equivalents. > What is the difference with using "disable" as default ? "disable" explicitly asks for NO SSL connection to be attempted regardless and is not the default for PostgreSQL server - it has to be specified explicitly. >> /* 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 ? The length of the connection string is initially allocated to be 80 characters, excluding the user name and the database name (there is further increase of 20 characters if "port" is specified). That is more than enough to hold "sslmode=XXX" which is, at most, 20 characters (" sslmode=verify-full"). So, for this value no additional increase is required as the existing string length is more than enough to hold it. For the other parameters though, as these (potentially) specify file paths, increase is required. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Sep 01, 2012 at 01:51:51PM +0100, Mr Dash Four wrote: > 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. I won't take a patch that fixes a NULL pointer dereference and adds a new feature. Please, split this in two patches. First fix the bug, then add the new fature. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> I won't take a patch that fixes a NULL pointer dereference and adds a > new feature. > > Please, split this in two patches. First fix the bug, then add the new > fature. Sure. Are there any (other) objections to the patch I submitted? -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Le lundi 03 septembre 2012 à 02:38 +0100, Mr Dash Four a écrit : > > I won't take a patch that fixes a NULL pointer dereference and adds a > > new feature. > > > > Please, split this in two patches. First fix the bug, then add the new > > fature. > Sure. Are there any (other) objections to the patch I submitted? I've applied the current patch to give it a run. And it shows a lot of whitespace errors. You'll need to fix it too. BR,
>> Sure. Are there any (other) objections to the patch I submitted? >> > > I've applied the current patch to give it a run. And it shows a lot of > whitespace errors. You'll need to fix it too. > What do you mean by that? -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 03, 2012 at 02:38:35AM +0100, Mr Dash Four wrote: > > I won't take a patch that fixes a NULL pointer dereference and adds a > > new feature. > > > > Please, split this in two patches. First fix the bug, then add the new > > fature. > > Sure. Are there any (other) objections to the patch I submitted? Are you going to send the fix for the NULL pointer dereference in the PostgreSQL plugin. I'd like to release a new version of ulogd2 soon. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Sure. Are there any (other) objections to the patch I submitted? > > Are you going to send the fix for the NULL pointer dereference in the > PostgreSQL plugin. See above, and... >>> Sure. Are there any (other) objections to the patch I submitted? >>> >> >> I've applied the current patch to give it a run. And it shows a lot of >> whitespace errors. You'll need to fix it too. >> > What do you mean by that? -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Le lundi 03 septembre 2012 à 13:00 +0100, Mr Dash Four a écrit : > >> Sure. Are there any (other) objections to the patch I submitted? > >> > > > > I've applied the current patch to give it a run. And it shows a lot of > > whitespace errors. You'll need to fix it too. > > > What do you mean by that? There's a lot of spaces at end of lines or empty lines containing only spaces. Basically, s/ +$// to fix it. BR, -- Eric Leblond -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Eric Leblond wrote: > Hello, > > Le lundi 03 septembre 2012 à 13:00 +0100, Mr Dash Four a écrit : >>>> Sure. Are there any (other) objections to the patch I submitted? >>>> >>> I've applied the current patch to give it a run. And it shows a lot of >>> whitespace errors. You'll need to fix it too. >>> >> What do you mean by that? > > There's a lot of spaces at end of lines or empty lines containing only > spaces. Basically, s/ +$// to fix it. I see. The patch I submitted over 2 months ago was based on the (then) newest git tree. From that tree, the 2 files I modified were output/pgsql/ulogd_output_PGSQL.c and ulogd.conf.in. By looking at my patch, it is quite evident that the white space "errors", as you call them, originate mostly from the existing source in that git tree. In cases where I have introduced new "errors", these were caused by cut-and-paste from the original source after applying the modifications I needed done for the patch to fulfil its purpose. Even today, the present version of ulogd_output_PGSQL.c and ulogd.conf.in in the current ulogd2 git tree has "a lot of whitespace errors" in them, even without my patch being applied. A simple search for such "errors" in output/pgsql/ulogd_output_PGSQL.c alone produces 15 matches. I suspect if I run similar searches on the entire ulogd2 tree (or any other tree within netfilter.org for that matter), the results won't be any different. Since you and Pablo are the maintainers of that tree, I am assuming that these "errors" were good enough for you when you patched these files in the past. Why is that? What's changed? Am I supposed to run a clean sweep of the entire existing git tree before submitting any patches, so that they meet with your and Pablo's approval then? I won't even go into the 2 months it took for the above response to finally arrive. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/output/pgsql/ulogd_output_PGSQL.c b/output/pgsql/ulogd_output_PGSQL.c index cd793ca..c99a0d7 100644 --- a/output/pgsql/ulogd_output_PGSQL.c +++ b/output/pgsql/ulogd_output_PGSQL.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", }, + { // sslmode=disable|allow|prefer|require|requiressl|verify-ca|verify-full + .key = "sslmode", + .type = CONFIG_TYPE_STRING, + .options = CONFIG_OPT_NONE, + }, + { + .key = "sslcert", + .type = CONFIG_TYPE_STRING, + .options = CONFIG_OPT_NONE, + }, + { + .key = "sslkey", + .type = CONFIG_TYPE_STRING, + .options = CONFIG_OPT_NONE, + }, + { + .key = "sslroot", + .type = CONFIG_TYPE_STRING, + .options = CONFIG_OPT_NONE, + }, + { + .key = "sslcrl", + .type = CONFIG_TYPE_STRING, + .options = CONFIG_OPT_NONE, + }, }, }; #define db_ce(x) (x->ces[DB_CE_NUM+0]) @@ -80,6 +104,11 @@ static struct config_keyset pgsql_kset = { #define pass_ce(x) (x->ces[DB_CE_NUM+3]) #define port_ce(x) (x->ces[DB_CE_NUM+4]) #define schema_ce(x) (x->ces[DB_CE_NUM+5]) +#define sslmode_ce(x) (x->ces[DB_CE_NUM+6]) +#define sslcert_ce(x) (x->ces[DB_CE_NUM+7]) +#define sslkey_ce(x) (x->ces[DB_CE_NUM+8]) +#define sslroot_ce(x) (x->ces[DB_CE_NUM+9]) +#define sslcrl_ce(x) (x->ces[DB_CE_NUM+10]) #define PGSQL_HAVE_NAMESPACE_TEMPLATE \ "SELECT nspname FROM pg_namespace n WHERE n.nspname='%s'" @@ -127,9 +156,12 @@ static int pgsql_namespace(struct ulogd_pluginstance *upi) static int get_columns_pgsql(struct ulogd_pluginstance *upi) { struct pgsql_instance *pi = (struct pgsql_instance *) upi->private; + char *table = table_ce(upi->config_kset).u.string; + char *schema = schema_ce(upi->config_kset).u.string; + int schema_len = strlen(schema); char pgbuf[strlen(PGSQL_GETCOLUMN_TEMPLATE_SCHEMA) - + strlen(table_ce(upi->config_kset).u.string) - + strlen(pi->db_inst.schema) + 2]; + + strlen(table) + + schema_len + 2]; int i; if (!pi->dbh) { @@ -137,14 +169,14 @@ static int get_columns_pgsql(struct ulogd_pluginstance *upi) return 1; } - if (pi->db_inst.schema) { + if (schema_len > 0) { snprintf(pgbuf, sizeof(pgbuf)-1, PGSQL_GETCOLUMN_TEMPLATE_SCHEMA, - table_ce(upi->config_kset).u.string, - pi->db_inst.schema); + schema, + table); } else { snprintf(pgbuf, sizeof(pgbuf)-1, PGSQL_GETCOLUMN_TEMPLATE, - table_ce(upi->config_kset).u.string); + table); } 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); 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"); + } else { + strcat(connstr, " sslmode="); + strcat(connstr, sslmode); + } + + if (sslcert && strlen(sslcert) > 0) { + strcat(connstr, " sslcert="); + strcat(connstr, sslcert); + } + if (sslkey && strlen(sslkey)) { + strcat(connstr, " sslkey="); + strcat(connstr, sslkey); + } + if (sslroot && strlen(sslroot)) { + strcat(connstr, " sslrootcert="); + strcat(connstr, sslroot); + } + if (sslcrl && strlen(sslcrl)) { + strcat(connstr, " sslcrl="); + strcat(connstr, sslcrl); + } + } + pi->dbh = PQconnectdb(connstr); - if (PQstatus(pi->dbh) != CONNECTION_OK) { - ulogd_log(ULOGD_ERROR, "unable to connect to db (%s): %s\n", - connstr, PQerrorMessage(pi->dbh)); + status = PQstatus(pi->dbh); + if (status != CONNECTION_OK) { + ulogd_log(ULOGD_ERROR, "unable to connect to db (%s): %s; status=%u\n", + connstr, PQerrorMessage(pi->dbh),status); close_db_pgsql(upi); return -1; } diff --git a/ulogd.conf.in b/ulogd.conf.in index 3bd464b..81dabdc 100644 --- a/ulogd.conf.in +++ b/ulogd.conf.in @@ -240,6 +240,32 @@ table="nfacct" pass="changeme" procedure="INSERT_NFACCT" +[pgsql5] +db="nulog" +host="localhost" +user="nupik" +table="ulog2_ct" +pass="changeme" +procedure="INSERT_OR_REPLACE_CT" +sslmode="prefer" +sslcert="/etc/pki/tls/private/ct.crt" +sslkey="/etc/pki/tls/private/ct.key" +sslroot="/etc/pki/tls/private/root.crt" +sslcrl="/etc/pki/tls/private/root.crl" + +[pgsql6] +db="nulog" +host="localhost" +user="nupik" +table="nfacct" +pass="changeme" +procedure="INSERT_NFACCT" +sslmode="verify-full" +sslcert="/etc/pki/tls/private/nfacct.crt" +sslkey="/etc/pki/tls/private/nfacct.key" +sslroot="/etc/pki/tls/private/root.crt" +sslcrl="/etc/pki/tls/private/root.crl" + [dbi1] db="ulog2" dbtype="pgsql"
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: disable - only try a non-SSL connection allow - first try a non-SSL connection; if that fails, try SSL connection prefer - (default) first try an SSL connection; if that fails, try a non-SSL connection require - only try an SSL connection requiressl - equivalent to 'require', but for older PostgreSQL server versions verify-ca - only try an SSL connection, and verify that the server certificate is issued by a trusted CA verify-full - only try an SSL connection, verify that the server certificate is issued by a trusted CA and that the server host name matches that in the certificate In addition to the above parameter, 4 other connection options can be specified: 'sslcert' - This parameter specifies the file name of the client SSL certificate. This parameter is ignored if an SSL connection is not made. '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. '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. Example of use: ulogd.conf ~~~~~~~~~~ [pgsql3] db="ulogdb" host="10.1.1.17" port=5432 user="ulogd" pass="changeme" table="nfacct" procedure="INSERT_NFACCT" sslmode="verify-full" sslcert="/etc/pki/tls/private/postgresql.crt" sslkey="/etc/pki/tls/private/postgresql.key" sslroot="/etc/pki/tls/private/root.crt" sslcrl="/etc/pki/tls/private/root.crl" Signed-off-by: Mr Dash Four <mr.dash.four@googlemail.com> --- output/pgsql/ulogd_output_PGSQL.c | 99 ++++++++++++++++++++++++++++++++----- ulogd.conf.in | 26 ++++++++++ 2 files changed, 113 insertions(+), 12 deletions(-)