Nut upsdev - Jan 2011 - [nut-commits] svn commit r2804 - in branches/ssl-nss-port: clients server

Citeren Emilien Kia <emilienkia-guest op alioth.debian.org>:
> Modified: branches/ssl-nss-port/server/conf.c
>
=============================================================================>
--- branches/ssl-nss-port/server/conf.c	Wed Jan  5 21:12:03 2011	(r2803)
> +++ branches/ssl-nss-port/server/conf.c	Thu Jan  6 10:27:55 2011	(r2804)
> @@ -178,6 +178,22 @@
>  		return 1;
>  	}
>
> +	/* CERTREQUEST ("NO" | "REQUEST" |
"REQUIRE") */
> +	if (!strcmp(arg[0], "CERTREQUEST")) {
> +		if (strcasecmp(arg[1], "REQUEST") == 0) {
> +			certrequest = NETSSL_CERTREQ_REQUEST;
> +		} else if (strcasecmp(arg[1], "REQUIRE") == 0) {
> +			certrequest = NETSSL_CERTREQ_REQUIRE;
> +		} else if (strcasecmp(arg[1], "NO") == 0) {
> +			certrequest = NETSSL_CERTREQ_NO;
> +		} else {
> +			upslogx(LOG_WARNING, "CERTREQUEST in upsd.conf accept only values
"
> +				"\"REQUEST\", \"REQUIRE\" or
\"NO\", assuming \"NO\"");
> +			certrequest = NETSSL_CERTREQ_NO;
> +		}
> +		return 1;
> +	}
> +
 From a maintenance point of view, the validation of the CERTREQUEST  
parameter should be handled in 'netssl.c', not here. We really don't
want to mess with this here, to prevent having to change 'conf.c' too  
often when something changes in the NSS code.

Likewise, it would be useful if this would only be compiled in if the  
NSS library is actually used (same for CERTPATH and CERTIDENT). It  
would be better to complain about invalid parameters than to fail  
later on.

Best regards, Arjen
-- 
Please keep list traffic on the list (off-list replies will be rejected)

Hi Arjen, Hi all,

I agree with you to say that conf.c must do a minimal interpretation of SSL
configuration, and should not set ssl related variables directly (I will change
that soon), but I do understand the way you propose:

Do you mean that:
 - netssl.c provides a configuration function to call at the beginning of
conf.c:parse_upsd_conf_args() and it parse all the ssl config ?
 - netssl.c provides some configuration functions, one per directive, and in the
case of CERTREQUEST, parses and converts string argument into internal format
(IMHO better way) ?
 - netssl.c provides some configuration function, one per directive, CERTREQUEST
argument parsing is done in conf.c:parse_upsd_conf_args() and the dedicated nss
configuration function is called with the corresponding value (IMHO best way
because best decoupling between human readable configuration directive and
functional storage) ?

BR,
Emilien

-----Message d'origine-----
De : nut-upsdev-bounces+emilienkia=eaton.com at lists.alioth.debian.org
[mailto:nut-upsdev-bounces+emilienkia=eaton.com at lists.alioth.debian.org] De
la part de Arjen de Korte
Envoy? : vendredi 7 janvier 2011 21:10
? : nut-upsdev at lists.alioth.debian.org
Objet : Re: [Nut-upsdev] [nut-commits] svn commit r2804 - in
branches/ssl-nss-port: clients server

Citeren Emilien Kia <emilienkia-guest at alioth.debian.org>:
> Modified: branches/ssl-nss-port/server/conf.c
>
=============================================================================>
--- branches/ssl-nss-port/server/conf.c	Wed Jan  5 21:12:03 2011	(r2803)
> +++ branches/ssl-nss-port/server/conf.c	Thu Jan  6 10:27:55 2011	(r2804)
> @@ -178,6 +178,22 @@
>  		return 1;
>  	}
>
> +	/* CERTREQUEST ("NO" | "REQUEST" |
"REQUIRE") */
> +	if (!strcmp(arg[0], "CERTREQUEST")) {
> +		if (strcasecmp(arg[1], "REQUEST") == 0) {
> +			certrequest = NETSSL_CERTREQ_REQUEST;
> +		} else if (strcasecmp(arg[1], "REQUIRE") == 0) {
> +			certrequest = NETSSL_CERTREQ_REQUIRE;
> +		} else if (strcasecmp(arg[1], "NO") == 0) {
> +			certrequest = NETSSL_CERTREQ_NO;
> +		} else {
> +			upslogx(LOG_WARNING, "CERTREQUEST in upsd.conf accept only values
"
> +				"\"REQUEST\", \"REQUIRE\" or
\"NO\", assuming \"NO\"");
> +			certrequest = NETSSL_CERTREQ_NO;
> +		}
> +		return 1;
> +	}
> +
 From a maintenance point of view, the validation of the CERTREQUEST  
parameter should be handled in 'netssl.c', not here. We really don't
want to mess with this here, to prevent having to change 'conf.c' too  
often when something changes in the NSS code.

Likewise, it would be useful if this would only be compiled in if the  
NSS library is actually used (same for CERTPATH and CERTIDENT). It  
would be better to complain about invalid parameters than to fail  
later on.

Best regards, Arjen
-- 
Please keep list traffic on the list (off-list replies will be rejected)


_______________________________________________
Nut-upsdev mailing list
Nut-upsdev at lists.alioth.debian.org
http://lists.alioth.debian.org/mailman/listinfo/nut-upsdev

--------------------------------------------------------------------------

Citeren EmilienKia op Eaton.com:
>  - netssl.c provides some configuration function, one per directive,  
> CERTREQUEST argument parsing is done in  
> conf.c:parse_upsd_conf_args() and the dedicated nss configuration  
> function is called with the corresponding value (IMHO best way  
> because best decoupling between human readable configuration  
> directive and functional storage) ?
I see no benefit in using human readable directives over a numeric one  
here. We do the latter in many more locations and if chosen wisely,  
will offer more flexibility. So instead of checking for NO, REQUEST or  
REQUIRE, just call the levels 0, 1 and 2. It saves you from  
interpreting the string value in conf.c (what I really intended to say  
here, it should not validate input values).

Best regards, Arjen
-- 
Please keep list traffic on the list (off-list replies will be rejected)