Merge lp:~fahad-aizaz/hipl/hip-cert-conf into lp:hipl

[Fahad Aizaz]

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.

[Stefan Götz (stefan.goetz-deactivatedaccount)]

Hi Fahad!

 review abstain

The code 'changes' look good to me, but I cannot comment on the
packaging stuff.

> 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.

Which one is the function for reading the config file?
hip_cert_x509v3_handle_request_to_sign? Isn't that one called in
hipd/user.c?

Stefan

review: Abstain

[Diego Biurrun (diego-biurrun)]

 review needs-info

On Wed, Jan 25, 2012 at 03:14:31PM +0000, Fahad Aizaz wrote:
>
> 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.

What function are you talking about? Please provide details, do not
let us guess.

The patch itself looks OK, but this is an issue that we will of course
have to sort out.

> --- hipd/init.c 2011-12-13 13:50:53 +0000
> +++ hipd/init.c 2012-01-25 15:13:34 +0000
> @@ -924,7 +819,7 @@
> int hipd_init(const uint64_t flags)
> {
> - int err = 0, certerr = 0, i, j;
> + int err = 0, i, j;
> int killold = (flags & HIPD_START_KILL_OLD) > 0;
> @@ -1065,12 +960,6 @@
>
> - certerr = 0;
> - certerr = init_certs();
> - if (certerr < 0) {
> - HIP_DEBUG("Initializing cert configuration file returned error\n");
> - }

So certerr used to get initialized a full *three* times? This is, ummm,
suboptimal even by HIPL standards ...

Diego

review: Needs Information

[Samuel Richter (samuel-richter)]

Hi,
please add the file to 'define Package/hipl-daemon/conffiles' in the openwrt makefile. Otherwise opkg will overwrite the file on each package update/reinstall, possibly deleting user changes.
The debian packager automatically marks all files in /etc as config-files. For rpm, the %config(noreplace)-directive achieves this.
Greetings
Samuel

review: Needs Fixing

[René Hummen (rene-hummen)]

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: [phone]
web: http://www.comsys.rwth-aachen.de/team/rene-hummen/

[Diego Biurrun (diego-biurrun)]

On Sat, Jan 28, 2012 at 07:09:32PM +0100, René 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.
> >
> @all: As I mentioned in
> https://code.launchpad.net/~henrik-ziegeldorf/hipl/pisa-merge/+merge/8
> 5871, I suggest to replace the current certificate code with the by
> far more complete certificate functionality provided by the pisa
> branch.
>
> Can we agree on my suggestion?

It's not clear to me how your suggestion applies to this branch.
This merge proposal is small and close to merging, pisa-merge is
huge and still in need of considerable work.

> > --- hipd/init.c 2011-12-13 13:50:53 +0000
> > +++ hipd/init.c 2012-01-25 15:13:34 +0000
> > @@ -465,81 +435,6 @@
> >
> > -/* 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.

How can build-time configuration make run-time configuration obsolete?

Diego

[René Hummen (rene-hummen)]

On 30.01.2012, at 12:41, Diego Biurrun wrote:

> On Sat, Jan 28, 2012 at 07:09:32PM +0100, René 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.
>>>
>>> --- hipd/init.c 2011-12-13 13:50:53 +0000
>>> +++ hipd/init.c 2012-01-25 15:13:34 +0000
>>> @@ -465,81 +435,6 @@
>>>
>>> -/* 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.
>
> How can build-time configuration make run-time configuration obsolete?

The generated configuration file is only used by the test application certteststub. My thought: Is it really necessary to offer run-time configuration support by means of a config file for a test application? I especially ask this question seeing that the current certificate code neither allows signaling of certificates between two instances of the hipd nor verification of certificates by the hipfw. In my opinion, having a config file for this specific test stub is overkill and the removal of the code responsible for reading and parsing the config file would enable us to further shrink the HIPL codebase.

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

[Diego Biurrun (diego-biurrun)]

On Mon, Jan 30, 2012 at 08:30:37PM +0100, René Hummen wrote:
>
> On 30.01.2012, at 12:41, Diego Biurrun wrote:
>
> > On Sat, Jan 28, 2012 at 07:09:32PM +0100, René 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.
> >>>
> >>> --- hipd/init.c 2011-12-13 13:50:53 +0000
> >>> +++ hipd/init.c 2012-01-25 15:13:34 +0000
> >>> @@ -465,81 +435,6 @@
> >>>
> >>> -/* 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.
> >
> > How can build-time configuration make run-time configuration obsolete?
>
> The generated configuration file is only used by the test application
> certteststub. My thought: Is it really necessary to offer run-time
> configuration support by means of a config file for a test
> application? I especially ask this question seeing that the current
> certificate code neither allows signaling of certificates between two
> instances of the hipd nor verification of certificates by the hipfw.
> In my opinion, having a config file for this specific test stub is
> overkill and the removal of the code responsible for reading and
> parsing the config file would enable us to further shrink the HIPL
> codebase.

OK, I understand what you mean now and agree in full - let's remove this
config file and its reading code. Note that there will still be three
other ways of dealing with config files left afterwards...

Diego

[Diego Biurrun (diego-biurrun)]

On Tue, Jan 31, 2012 at 12:40:11AM +0100, Diego Biurrun wrote:
> On Mon, Jan 30, 2012 at 08:30:37PM +0100, René Hummen wrote:
> > On 30.01.2012, at 12:41, Diego Biurrun wrote:
> > > On Sat, Jan 28, 2012 at 07:09:32PM +0100, René 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.
> > >>>
> > >>> --- hipd/init.c 2011-12-13 13:50:53 +0000
> > >>> +++ hipd/init.c 2012-01-25 15:13:34 +0000
> > >>> @@ -465,81 +435,6 @@
> > >>>
> > >>> -/* 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.
> > >
> > > How can build-time configuration make run-time configuration obsolete?
> >
> > The generated configuration file is only used by the test application
> > certteststub. My thought: Is it really necessary to offer run-time
> > configuration support by means of a config file for a test
> > application? I especially ask this question seeing that the current
> > certificate code neither allows signaling of certificates between two
> > instances of the hipd nor verification of certificates by the hipfw.
> > In my opinion, having a config file for this specific test stub is
> > overkill and the removal of the code responsible for reading and
> > parsing the config file would enable us to further shrink the HIPL
> > codebase.
>
> OK, I understand what you mean now and agree in full - let's remove this
> config file and its reading code. Note that there will still be three
> other ways of dealing with config files left afterwards...

Fahad can you tackle this?

Diego

Unmerged revisions

6261. By Fahad Aizaz on 2012-01-25

Commiting all recent changes from the trunk into the branch.

6260. By Fahad Aizaz on 2012-01-25

The function is no longer required.

The function retrives the hit of the machine and places it as
issuer hit. It is no longer requried since the config file in not
generated at runtime anymore.

6259. By Fahad Aizaz on 2012-01-25

build: moving the configuration file to the correct path.

Placing the hip_cert.conf file at its correct path while HIP in installed
and configured on following distributions:
1. Debian
2. OpenWRT
3. Fedora

Note: the path is determined by the 'configure' script otherwise it depends
upon the distribution default path for keeping configuration files.
e.g. /etc/<package name>

6258. By Fahad Aizaz on 2012-01-25

Comenting out of the name, value pairs in the configuration file.

The configuration file contains 'issuer hit' and 'days' of expiry.
The issuer hit which is the HIT of the machine assigned to it when
HIP daemon is executed, was placed in the configuration file at runtime.

Since the HIT will be different for different machines, the configuration
file contains commented out entries. The user must place the correct
entries to this file, or those he/she want to use for certificate
generation.

6257. By Fahad Aizaz on 2012-01-25

Adding default 'hip_cert.conf' file to HIP package.

6256. By Fahad Aizaz on 2012-01-25

Removing code generating 'hip_cert.conf' at runtime

The function 'init_certs()' is removed so that the configuration file
is not created and populated at runtime.

Instead a default hip_cert.conf file is packaged with HIP package.

=== modified file 'Makefile.am'
--- Makefile.am	2012-01-25 10:44:48 +0000
+++ Makefile.am	2012-01-25 15:13:34 +0000
@@ -137,7 +137,8 @@
                     modules/midauth/hipd/midauth.c      \
                     hipd/main.c
 
-dist_sysconf_DATA = hipd/hipd.conf                \
+dist_sysconf_DATA = hipd/hip_cert.conf            \
+                    hipd/hipd.conf                \
                     hipd/hosts                    \
                     hipd/relay.conf               \
                     hipfw/esp_prot.conf           \

=== modified file 'debian/hipl-daemon.install'
--- debian/hipl-daemon.install	2012-01-25 10:44:48 +0000
+++ debian/hipl-daemon.install	2012-01-25 15:13:34 +0000
@@ -2,6 +2,7 @@
 debian/tmp/usr/sbin/hipconf usr/sbin/
 debian/tmp/usr/sbin/pisacert usr/sbin/
 debian/tmp/usr/sbin/nsupdate usr/sbin/
+debian/tmp/etc/hip/hip_cert.conf etc/hip/
 debian/tmp/etc/hip/hipd.conf etc/hip/
 debian/tmp/etc/hip/hosts etc/hip/
 debian/tmp/etc/hip/nsupdate.conf etc/hip/

=== 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
+# days = 10
+
+# Section containing HIP 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_x509v3 ]
+# issuerhit = 2001:0016:b2d1:5bcf:f107:84c4:1e1d:bede
+# days = 10
+
+#Section containing the name section for the x509v3 issuer name
+
+# Place issuer hit and expiry days as suggested here
+# [ hip_x509v3_name ]
+# issuerhit = 2001:0016:b2d1:5bcf:f107:84c4:1e1d:bede
+
+# Uncomment this section to add x509 extensions
+# to the certificate
+#
+# DO NOT use subjectAltName, issuerAltName or
+# basicConstraints implementation uses them already
+# All other extensions are allowed
+
+# [ hip_x509v3_extensions ]

=== 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
@@ -373,36 +373,6 @@
 }
 
 /**
- * find the first RSA-based host id
- *
- * @return the host id or NULL if none found
- */
-static struct local_host_id *return_first_rsa(void)
-{
-    LHASH_NODE           *curr, *iter;
-    struct local_host_id *tmp = NULL;
-    int                   c;
-    uint16_t              algo = 0;
-
-    list_for_each_safe(curr, iter, hip_local_hostid_db, c) {
-        tmp = list_entry(curr);
-        HIP_DEBUG_HIT("Found HIT", &tmp->hit);
-        algo = hip_get_host_id_algo(&tmp->host_id);
-        HIP_DEBUG("hits algo %d HIP_HI_RSA = %d\n",
-                  algo, HIP_HI_RSA);
-        if (algo == HIP_HI_RSA) {
-            goto out_err;
-        }
-    }
-
-out_err:
-    if (algo == HIP_HI_RSA) {
-        return tmp;
-    }
-    return NULL;
-}
-
-/**
  * Initialize local host IDs.
  *
  * @return zero on success or negative on failure
@@ -465,81 +435,6 @@
     return err;
 }
 
-/* Needed if the configuration file for certs did not exist  */
-#define HIP_CERT_INIT_DAYS 10
-
-/**
- * Initialize certificates for the local host
- *
- * @return zero on success or negative on failure
- */
-static int init_certs(void)
-{
-    int                   err = 0;
-    char                  hit[41];
-    FILE                 *conf_file;
-    struct local_host_id *entry;
-    char                  hostname[HIP_HOST_ID_HOSTNAME_LEN_MAX];
-
-    HIP_IFEL(gethostname(hostname, sizeof(hostname)), -1,
-             "gethostname failed\n");
-
-    conf_file = fopen(HIP_CERT_CONF_PATH, "r");
-    if (!conf_file) {
-        HIP_DEBUG("Configuration file did NOT exist creating it and "
-                  "filling it with default information\n");
-        /* Fetch the first RSA HIT */
-        entry = return_first_rsa();
-        if (entry == NULL) {
-            HIP_DEBUG("Failed to get the first RSA HI");
-            goto out_err;
-        }
-        hip_in6_ntop(&entry->hit, hit);
-        conf_file = fopen(HIP_CERT_CONF_PATH, "w+");
-        fprintf(conf_file,
-                "# Section containing SPKI related information\n"
-                "#\n"
-                "# issuerhit = what hit is to be used when signing\n"
-                "# days = how long is this key valid\n"
-                "\n"
-                "[ hip_spki ]\n"
-                "issuerhit = %s\n"
-                "days = %d\n"
-                "\n"
-                "# Section containing HIP related information\n"
-                "#\n"
-                "# issuerhit = what hit is to be used when signing\n"
-                "# days = how long is this key valid\n"
-                "\n"
-                "[ hip_x509v3 ]\n"
-                "issuerhit = %s\n"
-                "days = %d\n"
-                "\n"
-                "#Section containing the name section for the x509v3 issuer name"
-                "\n"
-                "[ hip_x509v3_name ]\n"
-                "issuerhit = %s\n"
-                "\n"
-                "# Uncomment this section to add x509 extensions\n"
-                "# to the certificate\n"
-                "#\n"
-                "# DO NOT use subjectAltName, issuerAltName or\n"
-                "# basicConstraints implementation uses them already\n"
-                "# All other extensions are allowed\n"
-                "\n"
-                "# [ hip_x509v3_extensions ]\n",
-                hit, HIP_CERT_INIT_DAYS,
-                hit, HIP_CERT_INIT_DAYS,
-                hit /* TODO SAMU: removed because not used:*/  /*, hostname*/);
-    } else {
-        HIP_DEBUG("Configuration file existed exiting init_certs\n");
-    }
-    fclose(conf_file);
-
-out_err:
-    return err;
-}
-
 static void init_packet_types(void)
 {
     lmod_register_packet_type(HIP_I1,        "HIP_I1");
@@ -924,7 +819,7 @@
  */
 int hipd_init(const uint64_t flags)
 {
-    int                 err     = 0, certerr = 0, i, j;
+    int                 err     = 0, i, j;
     int                 killold = (flags & HIPD_START_KILL_OLD) > 0;
     unsigned int        mtu_val = HIP_HIT_DEV_MTU;
     char                str[64];
@@ -1065,12 +960,6 @@
         return -1;
     }
 
-    certerr = 0;
-    certerr = init_certs();
-    if (certerr < 0) {
-        HIP_DEBUG("Initializing cert configuration file returned error\n");
-    }
-
     /* Service initialization. */
     hip_init_services();
 

=== modified file 'packaging/hipl.spec'
--- packaging/hipl.spec	2012-01-25 10:44:48 +0000
+++ packaging/hipl.spec	2012-01-25 15:13:34 +0000
@@ -154,6 +154,7 @@
 %{_sbindir}/pisacert
 %{_sbindir}/nsupdate
 %{_initddir}/hipd
+%config(noreplace) %{_sysconfdir}/hip/hip_cert.conf
 %config(noreplace) %{_sysconfdir}/hip/hipd.conf
 %config(noreplace) %{_sysconfdir}/hip/hosts
 %config(noreplace) %{_sysconfdir}/hip/nsupdate.conf

=== modified file 'packaging/openwrt/hipl/Makefile.in'
--- packaging/openwrt/hipl/Makefile.in	2012-01-20 15:34:21 +0000
+++ packaging/openwrt/hipl/Makefile.in	2012-01-25 15:13:34 +0000
@@ -59,6 +59,7 @@
 	$(INSTALL_BIN) $(PKG_INSTALL_DIR)/usr/sbin/hipd $(1)/usr/sbin/
 	$(INSTALL_BIN) $(PKG_INSTALL_DIR)/usr/sbin/hipconf $(1)/usr/sbin/
 	$(INSTALL_BIN) $(PKG_INSTALL_DIR)/usr/sbin/hipdnsproxy $(1)/usr/sbin/
+	$(INSTALL_DATA) $(PKG_INSTALL_DIR)/etc/hip/hip_cert.conf $(1)/etc/hip/
 	$(INSTALL_DATA) $(PKG_INSTALL_DIR)/etc/hip/hipd.conf $(1)/etc/hip/
 	$(INSTALL_DATA) $(PKG_INSTALL_DIR)/etc/hip/hosts $(1)/etc/hip/
 	$(INSTALL_DATA) $(PKG_INSTALL_DIR)/etc/hip/nsupdate.conf $(1)/etc/hip/