Code review comment for lp:~fahad-aizaz/hipl/hip-cert-conf

René Hummen (rene-hummen) wrote :

On 25.01.2012, at 16:14, Fahad Aizaz wrote:
> Fahad Aizaz has proposed merging lp:~fahad-aizaz/hipl/hip-cert-conf into lp:hipl.
>
> Requested reviews:
> HIPL core team (hipl-core)
>
> For more details, see:
> https://code.launchpad.net/~fahad-aizaz/hipl/hip-cert-conf/+merge/90130
>
> Removing runtime generation of configuration file hip_cert.conf. With additional support for its installation along with HIPL package for OpenWRT, Debian and RPM (package based distributions)
>
> Note:
> While doing so, I observed that the configuration file though referenced in the code for reading, but I didn't find any trigger to actually read the contents of the file. That is, the function that reads the configuration file section by section, is not being called though its definition exist.
>
> Please correct me about this if I am wrong.

Certificates can currently only be used and exchanged between the test/certteststub executable and hipd. hipd does not load certificates and their configuration file, but instead receives the certificate from certteststub and verifies the received message (signature + certificate). That's why you cannot find the code for reading the config file in the hipd.

@all: As I mentioned in https://code.launchpad.net/~henrik-ziegeldorf/hipl/pisa-merge/+merge/85871, I suggest to replace the current certificate code with the by far more complete certificate functionality provided by the pisa branch.

Further comments inline.

> === added file 'hipd/hip_cert.conf'
> --- hipd/hip_cert.conf 1970-01-01 00:00:00 +0000
> +++ hipd/hip_cert.conf 2012-01-25 15:13:34 +0000
> @@ -0,0 +1,34 @@
> +# Section containing SPKI related information
> +#
> +# issuerhit = what hit is to be used when signing
> +# days = how long is this key valid
> +
> +# Place issuer hit and expiry days as suggested here
> +# [ hip_spki ]
> +# issuerhit = 2001:0016:b2d1:5bcf:f107:84c4:1e1d:bede

Diego suggested to drop the issuer HIT from the configuration file and use the default HIT in the code instead. As certteststub is only a testing application, I agree that this configuration option is not required in the config file.

> === modified file 'hipd/init.c'
> --- hipd/init.c 2011-12-13 13:50:53 +0000
> +++ hipd/init.c 2012-01-25 15:13:34 +0000

> @@ -465,81 +435,6 @@
> return err;
> }
>
> -/* Needed if the configuration file for certs did not exist */
> -#define HIP_CERT_INIT_DAYS 10

... and once we are on it, we can also use this define instead of the parameter in the configuration file. This change would make hip_cert.conf obsolete.

Can we agree on my suggestion?

--
Dipl.-Inform. Rene Hummen, Ph.D. Student
Chair of Communication and Distributed Systems
RWTH Aachen University, Germany
tel: +49 241 80 20772
web: http://www.comsys.rwth-aachen.de/team/rene-hummen/

« Back to merge proposal