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

Proposed by Fahad Aizaz on 2012-01-25
Status: Needs review
Proposed branch: lp:~fahad-aizaz/hipl/hip-cert-conf
Merge into: lp:hipl
Diff against target: 233 lines (+40/-113)
6 files modified
Makefile.am (+2/-1)
debian/hipl-daemon.install (+1/-0)
hipd/hip_cert.conf (+34/-0)
hipd/init.c (+1/-112)
packaging/hipl.spec (+1/-0)
packaging/openwrt/hipl/Makefile.in (+1/-0)
To merge this branch: bzr merge lp:~fahad-aizaz/hipl/hip-cert-conf
Reviewer Review Type Date Requested Status
Samuel Richter Needs Fixing on 2012-01-25
Diego Biurrun Needs Information on 2012-01-25
Stefan Götz (community) 2012-01-25 Abstain on 2012-01-25
Review via email: mp+90130@code.launchpad.net

Description of the change

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.

To post a comment you must log in.

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) wrote :

 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) wrote :

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) 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/

Diego Biurrun (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.
> >
> @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) 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.

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

Diego Biurrun (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...

Diego

Diego Biurrun (diego-biurrun) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'Makefile.am'
2--- Makefile.am 2012-01-25 10:44:48 +0000
3+++ Makefile.am 2012-01-25 15:13:34 +0000
4@@ -137,7 +137,8 @@
5 modules/midauth/hipd/midauth.c \
6 hipd/main.c
7
8-dist_sysconf_DATA = hipd/hipd.conf \
9+dist_sysconf_DATA = hipd/hip_cert.conf \
10+ hipd/hipd.conf \
11 hipd/hosts \
12 hipd/relay.conf \
13 hipfw/esp_prot.conf \
14
15=== modified file 'debian/hipl-daemon.install'
16--- debian/hipl-daemon.install 2012-01-25 10:44:48 +0000
17+++ debian/hipl-daemon.install 2012-01-25 15:13:34 +0000
18@@ -2,6 +2,7 @@
19 debian/tmp/usr/sbin/hipconf usr/sbin/
20 debian/tmp/usr/sbin/pisacert usr/sbin/
21 debian/tmp/usr/sbin/nsupdate usr/sbin/
22+debian/tmp/etc/hip/hip_cert.conf etc/hip/
23 debian/tmp/etc/hip/hipd.conf etc/hip/
24 debian/tmp/etc/hip/hosts etc/hip/
25 debian/tmp/etc/hip/nsupdate.conf etc/hip/
26
27=== added file 'hipd/hip_cert.conf'
28--- hipd/hip_cert.conf 1970-01-01 00:00:00 +0000
29+++ hipd/hip_cert.conf 2012-01-25 15:13:34 +0000
30@@ -0,0 +1,34 @@
31+# Section containing SPKI related information
32+#
33+# issuerhit = what hit is to be used when signing
34+# days = how long is this key valid
35+
36+# Place issuer hit and expiry days as suggested here
37+# [ hip_spki ]
38+# issuerhit = 2001:0016:b2d1:5bcf:f107:84c4:1e1d:bede
39+# days = 10
40+
41+# Section containing HIP related information
42+#
43+# issuerhit = what hit is to be used when signing
44+# days = how long is this key valid
45+
46+# Place issuer hit and expiry days as suggested here
47+# [ hip_x509v3 ]
48+# issuerhit = 2001:0016:b2d1:5bcf:f107:84c4:1e1d:bede
49+# days = 10
50+
51+#Section containing the name section for the x509v3 issuer name
52+
53+# Place issuer hit and expiry days as suggested here
54+# [ hip_x509v3_name ]
55+# issuerhit = 2001:0016:b2d1:5bcf:f107:84c4:1e1d:bede
56+
57+# Uncomment this section to add x509 extensions
58+# to the certificate
59+#
60+# DO NOT use subjectAltName, issuerAltName or
61+# basicConstraints implementation uses them already
62+# All other extensions are allowed
63+
64+# [ hip_x509v3_extensions ]
65
66=== modified file 'hipd/init.c'
67--- hipd/init.c 2011-12-13 13:50:53 +0000
68+++ hipd/init.c 2012-01-25 15:13:34 +0000
69@@ -373,36 +373,6 @@
70 }
71
72 /**
73- * find the first RSA-based host id
74- *
75- * @return the host id or NULL if none found
76- */
77-static struct local_host_id *return_first_rsa(void)
78-{
79- LHASH_NODE *curr, *iter;
80- struct local_host_id *tmp = NULL;
81- int c;
82- uint16_t algo = 0;
83-
84- list_for_each_safe(curr, iter, hip_local_hostid_db, c) {
85- tmp = list_entry(curr);
86- HIP_DEBUG_HIT("Found HIT", &tmp->hit);
87- algo = hip_get_host_id_algo(&tmp->host_id);
88- HIP_DEBUG("hits algo %d HIP_HI_RSA = %d\n",
89- algo, HIP_HI_RSA);
90- if (algo == HIP_HI_RSA) {
91- goto out_err;
92- }
93- }
94-
95-out_err:
96- if (algo == HIP_HI_RSA) {
97- return tmp;
98- }
99- return NULL;
100-}
101-
102-/**
103 * Initialize local host IDs.
104 *
105 * @return zero on success or negative on failure
106@@ -465,81 +435,6 @@
107 return err;
108 }
109
110-/* Needed if the configuration file for certs did not exist */
111-#define HIP_CERT_INIT_DAYS 10
112-
113-/**
114- * Initialize certificates for the local host
115- *
116- * @return zero on success or negative on failure
117- */
118-static int init_certs(void)
119-{
120- int err = 0;
121- char hit[41];
122- FILE *conf_file;
123- struct local_host_id *entry;
124- char hostname[HIP_HOST_ID_HOSTNAME_LEN_MAX];
125-
126- HIP_IFEL(gethostname(hostname, sizeof(hostname)), -1,
127- "gethostname failed\n");
128-
129- conf_file = fopen(HIP_CERT_CONF_PATH, "r");
130- if (!conf_file) {
131- HIP_DEBUG("Configuration file did NOT exist creating it and "
132- "filling it with default information\n");
133- /* Fetch the first RSA HIT */
134- entry = return_first_rsa();
135- if (entry == NULL) {
136- HIP_DEBUG("Failed to get the first RSA HI");
137- goto out_err;
138- }
139- hip_in6_ntop(&entry->hit, hit);
140- conf_file = fopen(HIP_CERT_CONF_PATH, "w+");
141- fprintf(conf_file,
142- "# Section containing SPKI related information\n"
143- "#\n"
144- "# issuerhit = what hit is to be used when signing\n"
145- "# days = how long is this key valid\n"
146- "\n"
147- "[ hip_spki ]\n"
148- "issuerhit = %s\n"
149- "days = %d\n"
150- "\n"
151- "# Section containing HIP related information\n"
152- "#\n"
153- "# issuerhit = what hit is to be used when signing\n"
154- "# days = how long is this key valid\n"
155- "\n"
156- "[ hip_x509v3 ]\n"
157- "issuerhit = %s\n"
158- "days = %d\n"
159- "\n"
160- "#Section containing the name section for the x509v3 issuer name"
161- "\n"
162- "[ hip_x509v3_name ]\n"
163- "issuerhit = %s\n"
164- "\n"
165- "# Uncomment this section to add x509 extensions\n"
166- "# to the certificate\n"
167- "#\n"
168- "# DO NOT use subjectAltName, issuerAltName or\n"
169- "# basicConstraints implementation uses them already\n"
170- "# All other extensions are allowed\n"
171- "\n"
172- "# [ hip_x509v3_extensions ]\n",
173- hit, HIP_CERT_INIT_DAYS,
174- hit, HIP_CERT_INIT_DAYS,
175- hit /* TODO SAMU: removed because not used:*/ /*, hostname*/);
176- } else {
177- HIP_DEBUG("Configuration file existed exiting init_certs\n");
178- }
179- fclose(conf_file);
180-
181-out_err:
182- return err;
183-}
184-
185 static void init_packet_types(void)
186 {
187 lmod_register_packet_type(HIP_I1, "HIP_I1");
188@@ -924,7 +819,7 @@
189 */
190 int hipd_init(const uint64_t flags)
191 {
192- int err = 0, certerr = 0, i, j;
193+ int err = 0, i, j;
194 int killold = (flags & HIPD_START_KILL_OLD) > 0;
195 unsigned int mtu_val = HIP_HIT_DEV_MTU;
196 char str[64];
197@@ -1065,12 +960,6 @@
198 return -1;
199 }
200
201- certerr = 0;
202- certerr = init_certs();
203- if (certerr < 0) {
204- HIP_DEBUG("Initializing cert configuration file returned error\n");
205- }
206-
207 /* Service initialization. */
208 hip_init_services();
209
210
211=== modified file 'packaging/hipl.spec'
212--- packaging/hipl.spec 2012-01-25 10:44:48 +0000
213+++ packaging/hipl.spec 2012-01-25 15:13:34 +0000
214@@ -154,6 +154,7 @@
215 %{_sbindir}/pisacert
216 %{_sbindir}/nsupdate
217 %{_initddir}/hipd
218+%config(noreplace) %{_sysconfdir}/hip/hip_cert.conf
219 %config(noreplace) %{_sysconfdir}/hip/hipd.conf
220 %config(noreplace) %{_sysconfdir}/hip/hosts
221 %config(noreplace) %{_sysconfdir}/hip/nsupdate.conf
222
223=== modified file 'packaging/openwrt/hipl/Makefile.in'
224--- packaging/openwrt/hipl/Makefile.in 2012-01-20 15:34:21 +0000
225+++ packaging/openwrt/hipl/Makefile.in 2012-01-25 15:13:34 +0000
226@@ -59,6 +59,7 @@
227 $(INSTALL_BIN) $(PKG_INSTALL_DIR)/usr/sbin/hipd $(1)/usr/sbin/
228 $(INSTALL_BIN) $(PKG_INSTALL_DIR)/usr/sbin/hipconf $(1)/usr/sbin/
229 $(INSTALL_BIN) $(PKG_INSTALL_DIR)/usr/sbin/hipdnsproxy $(1)/usr/sbin/
230+ $(INSTALL_DATA) $(PKG_INSTALL_DIR)/etc/hip/hip_cert.conf $(1)/etc/hip/
231 $(INSTALL_DATA) $(PKG_INSTALL_DIR)/etc/hip/hipd.conf $(1)/etc/hip/
232 $(INSTALL_DATA) $(PKG_INSTALL_DIR)/etc/hip/hosts $(1)/etc/hip/
233 $(INSTALL_DATA) $(PKG_INSTALL_DIR)/etc/hip/nsupdate.conf $(1)/etc/hip/

Subscribers

People subscribed via source and target branches

to all changes: