HIPL

Code review comment for lp:~hipl-core/hipl/ecdsa-redhat

ecdsa-redhat
Merge into trunk

Revision history for this message

Miika Komu (miika-iki) wrote on 2011-11-03:

> review needs-fixing
>
> On Sun, Oct 30, 2011 at 07:49:28AM +0000, Miika Komu wrote:
> > Miika Komu has proposed merging lp:~hipl-core/hipl/ecdsa-redhat into
> lp:hipl.
> >
> > --- firewall/rule_management.c 2011-08-15 14:11:56 +0000
> > +++ firewall/rule_management.c 2011-10-30 07:48:24 +0000
> > @@ -81,8 +81,9 @@
> > /* filename needs to contain one of these to be valid HI file */
> > #define RSA_FILE "_rsa_"
> > #define DSA_FILE "_dsa_"
> > +#ifdef HAVE_EC_CRYPTO
> > #define ECDSA_FILE "_ecdsa_"
> > -
> > +#endif /* HAVE_EC_CRYPTO */
> > #define MAX_LINE_LENGTH 512
>
> unnecessary

Fixed.

> > @@ -444,6 +445,7 @@
> > return err;
> > }
> >
> > +#ifdef HAVE_EC_CRYPTO
> > /**
> > * Load an ECDSA public key from a file and convert it into a hip_host_id.
> > *
> > @@ -479,6 +481,8 @@
> > return err;
> > }
> >
> > +#endif /* HAVE_EC_CRYPTO */
> > +
> > /**
> > * load a public key from a file and convert it to a hip_host_id structure
> > *
>
> Drop the empty line before the #endif, same below

Done.

> > --- lib/core/builder.h 2011-08-15 14:11:56 +0000
> > +++ lib/core/builder.h 2011-10-30 07:48:24 +0000
> > @@ -26,18 +26,21 @@
> > #ifndef HIP_LIB_CORE_BUILDER_H
> > #define HIP_LIB_CORE_BUILDER_H
> >
> > +#include "config.h"
> > +
> > #include <stdint.h>
> > #include <netinet/in.h>
> > #include <openssl/rsa.h>
> > #include <openssl/dsa.h>
> > -#include <openssl/ec.h>
> >
> > -#include "config.h"
> > #include "certtools.h"
> > #include "debug.h"
> > #include "icomm.h"
> > #include "state.h"
> >
> > +#ifdef HAVE_EC_CRYPTO
> > +#include <openssl/ec.h>
> > +#endif /* HAVE_EC_CRYPTO */
>
> We have system headers before local headers for a reason.

Fixed.

> > --- lib/core/crypto.h 2011-07-18 13:10:26 +0000
> > +++ lib/core/crypto.h 2011-10-30 07:48:24 +0000
> > @@ -26,12 +26,16 @@
> > #ifndef HIP_LIB_CORE_CRYPTO_H
> > #define HIP_LIB_CORE_CRYPTO_H
> >
> > +#include "config.h"
> > +
> > #include <stdint.h>
> > #include <netinet/in.h>
> > #include <sys/types.h>
> > #include <openssl/dsa.h>
> > #include <openssl/rsa.h>
> > +#ifdef HAVE_EC_CRYPTO
> > #include <openssl/ec.h>
> > +#endif /* HAVE_EC_CRYPTO */
> > #include <openssl/dh.h>
> > #include <openssl/pem.h>
>
> .. like you did here ..
>
> > --- lib/core/hostid.c 2011-10-25 21:14:16 +0000
> > +++ lib/core/hostid.c 2011-10-30 07:48:24 +0000
> > @@ -28,6 +28,8 @@
> > * @brief Host identifier manipulation functions
> > */
> >
> > +#include "config.h"
> > +
> > #include <errno.h>
> > #include <stdint.h>
> > #include <stdlib.h>
> > @@ -40,7 +42,6 @@
> > #include <openssl/pem.h>
> > #include <openssl/rsa.h>
> >
> > -#include "config.h"
> > #include "lib/tool/pk.h"
> > #include "builder.h"
> > #include "crypto.h"
>
> unnecessary / unrelated

Removed.

> > @@ -689,6 +715,12 @@
> > struct endpoint_hip *endpoint_ecdsa_hip = NULL;
> > struct endpoint_hip *endpoint_ecdsa_pub_hip = NULL;
> >
> > + if (ecdsa_nid < 0) {
> > + err = -1;
> > + HIP_ERROR("NID for ECDSA is strange %d\n", ecdsa_nid);
> > + goto out_err;
> > + }
>
> ?

Does not compile otherwise when ECDSA is missing (gcc complains about missing variable). If you insist, I'll commit this separately to trunk or suggest a better fix.

> > @@ -1059,41 +1101,58 @@
> > if (rsa_filenamebase_pub != NULL) {
> > change_key_file_perms(rsa_filenamebase_pub);
> > }
> > - if (ecdsa_filenamebase_pub != NULL) {
> > - change_key_file_perms(ecdsa_filenamebase_pub);
> > - }
> > - if (ecdsa_filenamebase_pub != NULL) {
> > - change_key_file_perms(ecdsa_filenamebase_pub);
> > - }
> > +#ifdef HAVE_EC_CRYPTO
> > + if (ecdsa_filenamebase_pub != NULL) {
> > + change_key_file_perms(ecdsa_filenamebase_pub);
> > + }
> > + if (ecdsa_filenamebase_pub != NULL) {
> > + change_key_file_perms(ecdsa_filenamebase_pub);
> > + }
> > +#endif /* HAVE_EC_CRYPTO */
> >
> > free(dsa_host_id);
> > free(dsa_pub_host_id);
> > +#ifdef HAVE_EC_CRYPTO
> > free(ecdsa_host_id);
> > free(ecdsa_pub_host_id);
> > +#endif /* HAVE_EC_CRYPTO */
> > free(rsa_host_id);
> > free(rsa_pub_host_id);
> > DSA_free(dsa_key);
> > +#ifdef HAVE_EC_CRYPTO
> > EC_KEY_free(ecdsa_key);
> > +#endif /* HAVE_EC_CRYPTO */
> > RSA_free(rsa_key);
> > DSA_free(dsa_pub_key);
> > +#ifdef HAVE_EC_CRYPTO
> > EC_KEY_free(ecdsa_pub_key);
> > +#endif /* HAVE_EC_CRYPTO */
> > RSA_free(rsa_pub_key);
> > free(dsa_key_rr);
> > +#ifdef HAVE_EC_CRYPTO
> > free(ecdsa_key_rr);
> > +#endif /* HAVE_EC_CRYPTO */
> > free(rsa_key_rr);
> > free(dsa_pub_key_rr);
> > +#ifdef HAVE_EC_CRYPTO
> > free(ecdsa_pub_key_rr);
> > +#endif /* HAVE_EC_CRYPTO */
> > free(rsa_pub_key_rr);
> > free(endpoint_dsa_hip);
> > +#ifdef HAVE_EC_CRYPTO
> > free(endpoint_ecdsa_hip);
> > +#endif /* HAVE_EC_CRYPTO */
> > free(endpoint_rsa_hip);
> > free(endpoint_dsa_pub_hip);
> > +#ifdef HAVE_EC_CRYPTO
> > free(endpoint_ecdsa_pub_hip);
> > +#endif /* HAVE_EC_CRYPTO */
> > free(endpoint_rsa_pub_hip);
>
> ETOOMANYIFDEFS

Reduced to minimum.

> > --- lib/core/hostid.h 2011-07-18 13:10:26 +0000
> > +++ lib/core/hostid.h 2011-10-30 07:48:24 +0000
> > @@ -29,7 +29,9 @@
> > #include <netinet/in.h>
> > #include <openssl/dsa.h>
> > #include <openssl/rsa.h>
> > +#ifdef HAVE_EC_CRYPTO
> > #include <openssl/ec.h>
> > +#endif /* HAVE_EC_CRYPTO */
>
> This will not work without config.h.
>
> Did you test this on a system with ECC? Apparently not, this should at
> least lead to 'make checkheaders' failures.

I recall I did test. The checkheaders targets succeeds (at least now).

> > --- lib/tool/pk.h 2011-07-08 11:47:12 +0000
> > +++ lib/tool/pk.h 2011-10-30 07:48:24 +0000
> > @@ -9,8 +9,10 @@
> > int hip_dsa_sign(void *peer_pub, struct hip_common *msg);
> > int hip_rsa_verify(void *priv_key, struct hip_common *msg);
> > int hip_rsa_sign(void *peer_pub, struct hip_common *msg);
> > +#ifdef HAVE_EC_CRYPTO
> > int hip_ecdsa_verify(void *const peer_pub, struct hip_common *const msg);
> > int hip_ecdsa_sign(void *const peer_pub, struct hip_common *const msg);
> > +#endif /* HAVE_EC_CRYPTO */
> > int bn2bin_safe(const BIGNUM *a, unsigned char *to, int len);
>
> unnecessary

Removed.

> > === modified file 'test/lib/tool/pk.c'
> > --- test/lib/tool/pk.c 2011-07-18 13:10:10 +0000
> > +++ test/lib/tool/pk.c 2011-10-30 07:48:24 +0000
> > @@ -27,7 +27,9 @@
> > #include <stdlib.h>
> > #include <string.h>
> > #include <stdio.h>
> > +#ifdef HAVE_EC_CRYPTO
> > #include <openssl/ec.h>
> > +#endif /* HAVE_EC_CRYPTO */
> > #include <openssl/pem.h>
>
> see above

Did not get this.

>  review needs-fixing
> 
> On Sun, Oct 30, 2011 at 07:49:28AM +0000, Miika Komu wrote:
> > Miika Komu has proposed merging lp:~hipl-core/hipl/ecdsa-redhat into
> lp:hipl.
> >
> > --- firewall/rule_management.c        2011-08-15 14:11:56 +0000
> > +++ firewall/rule_management.c        2011-10-30 07:48:24 +0000
> > @@ -81,8 +81,9 @@
> >  /* filename needs to contain one of these to be valid HI file */
> >  #define RSA_FILE "_rsa_"
> >  #define DSA_FILE "_dsa_"
> > +#ifdef HAVE_EC_CRYPTO
> >  #define ECDSA_FILE "_ecdsa_"
> > -
> > +#endif /* HAVE_EC_CRYPTO */
> >  #define MAX_LINE_LENGTH 512
> 
> unnecessary

Fixed.

> > @@ -444,6 +445,7 @@
> >      return err;
> >  }
> >
> > +#ifdef HAVE_EC_CRYPTO
> >  /**
> >   * Load an ECDSA public key from a file and convert it into a hip_host_id.
> >   *
> > @@ -479,6 +481,8 @@
> >      return err;
> >  }
> >
> > +#endif /* HAVE_EC_CRYPTO */
> > +
> >  /**
> >   * load a public key from a file and convert it to a hip_host_id structure
> >   *
> 
> Drop the empty line before the #endif, same below

Done.

> > --- lib/core/builder.h        2011-08-15 14:11:56 +0000
> > +++ lib/core/builder.h        2011-10-30 07:48:24 +0000
> > @@ -26,18 +26,21 @@
> >  #ifndef HIP_LIB_CORE_BUILDER_H
> >  #define HIP_LIB_CORE_BUILDER_H
> >
> > +#include "config.h"
> > +
> >  #include <stdint.h>
> >  #include <netinet/in.h>
> >  #include <openssl/rsa.h>
> >  #include <openssl/dsa.h>
> > -#include <openssl/ec.h>
> >
> > -#include "config.h"
> >  #include "certtools.h"
> >  #include "debug.h"
> >  #include "icomm.h"
> >  #include "state.h"
> >
> > +#ifdef HAVE_EC_CRYPTO
> > +#include <openssl/ec.h>
> > +#endif /* HAVE_EC_CRYPTO */
> 
> We have system headers before local headers for a reason.

Fixed.

> > --- lib/core/crypto.h 2011-07-18 13:10:26 +0000
> > +++ lib/core/crypto.h 2011-10-30 07:48:24 +0000
> > @@ -26,12 +26,16 @@
> >  #ifndef HIP_LIB_CORE_CRYPTO_H
> >  #define HIP_LIB_CORE_CRYPTO_H
> >
> > +#include "config.h"
> > +
> >  #include <stdint.h>
> >  #include <netinet/in.h>
> >  #include <sys/types.h>
> >  #include <openssl/dsa.h>
> >  #include <openssl/rsa.h>
> > +#ifdef HAVE_EC_CRYPTO
> >  #include <openssl/ec.h>
> > +#endif /* HAVE_EC_CRYPTO */
> >  #include <openssl/dh.h>
> >  #include <openssl/pem.h>
> 
> .. like you did here ..
> 
> > --- lib/core/hostid.c 2011-10-25 21:14:16 +0000
> > +++ lib/core/hostid.c 2011-10-30 07:48:24 +0000
> > @@ -28,6 +28,8 @@
> >   * @brief Host identifier manipulation functions
> >   */
> >
> > +#include "config.h"
> > +
> >  #include <errno.h>
> >  #include <stdint.h>
> >  #include <stdlib.h>
> > @@ -40,7 +42,6 @@
> >  #include <openssl/pem.h>
> >  #include <openssl/rsa.h>
> >
> > -#include "config.h"
> >  #include "lib/tool/pk.h"
> >  #include "builder.h"
> >  #include "crypto.h"
> 
> unnecessary / unrelated

Removed.

> > @@ -689,6 +715,12 @@
> >      struct endpoint_hip     *endpoint_ecdsa_hip     = NULL;
> >      struct endpoint_hip     *endpoint_ecdsa_pub_hip = NULL;
> >
> > +    if (ecdsa_nid < 0) {
> > +        err = -1;
> > +        HIP_ERROR("NID for ECDSA is strange %d\n", ecdsa_nid);
> > +        goto out_err;
> > +    }
> 
> ?

Does not compile otherwise when ECDSA is missing (gcc complains about missing variable). If you insist, I'll commit this separately to trunk or suggest a better fix.

> > @@ -1059,41 +1101,58 @@
> >      if (rsa_filenamebase_pub != NULL) {
> >          change_key_file_perms(rsa_filenamebase_pub);
> >      }
> > -    if (ecdsa_filenamebase_pub != NULL) {
> > -        change_key_file_perms(ecdsa_filenamebase_pub);
> > -    }
> > -    if (ecdsa_filenamebase_pub != NULL) {
> > -        change_key_file_perms(ecdsa_filenamebase_pub);
> > -    }
> > +#ifdef HAVE_EC_CRYPTO
> > +    if (ecdsa_filenamebase_pub != NULL) {
> > +        change_key_file_perms(ecdsa_filenamebase_pub);
> > +    }
> > +    if (ecdsa_filenamebase_pub != NULL) {
> > +        change_key_file_perms(ecdsa_filenamebase_pub);
> > +    }
> > +#endif /* HAVE_EC_CRYPTO */
> >
> >      free(dsa_host_id);
> >      free(dsa_pub_host_id);
> > +#ifdef HAVE_EC_CRYPTO
> >      free(ecdsa_host_id);
> >      free(ecdsa_pub_host_id);
> > +#endif /* HAVE_EC_CRYPTO */
> >      free(rsa_host_id);
> >      free(rsa_pub_host_id);
> >      DSA_free(dsa_key);
> > +#ifdef HAVE_EC_CRYPTO
> >      EC_KEY_free(ecdsa_key);
> > +#endif /* HAVE_EC_CRYPTO */
> >      RSA_free(rsa_key);
> >      DSA_free(dsa_pub_key);
> > +#ifdef HAVE_EC_CRYPTO
> >      EC_KEY_free(ecdsa_pub_key);
> > +#endif /* HAVE_EC_CRYPTO */
> >      RSA_free(rsa_pub_key);
> >      free(dsa_key_rr);
> > +#ifdef HAVE_EC_CRYPTO
> >      free(ecdsa_key_rr);
> > +#endif /* HAVE_EC_CRYPTO */
> >      free(rsa_key_rr);
> >      free(dsa_pub_key_rr);
> > +#ifdef HAVE_EC_CRYPTO
> >      free(ecdsa_pub_key_rr);
> > +#endif /* HAVE_EC_CRYPTO */
> >      free(rsa_pub_key_rr);
> >      free(endpoint_dsa_hip);
> > +#ifdef HAVE_EC_CRYPTO
> >      free(endpoint_ecdsa_hip);
> > +#endif /* HAVE_EC_CRYPTO */
> >      free(endpoint_rsa_hip);
> >      free(endpoint_dsa_pub_hip);
> > +#ifdef HAVE_EC_CRYPTO
> >      free(endpoint_ecdsa_pub_hip);
> > +#endif /* HAVE_EC_CRYPTO */
> >      free(endpoint_rsa_pub_hip);
> 
> ETOOMANYIFDEFS

Reduced to minimum.

> > --- lib/core/hostid.h 2011-07-18 13:10:26 +0000
> > +++ lib/core/hostid.h 2011-10-30 07:48:24 +0000
> > @@ -29,7 +29,9 @@
> >  #include <netinet/in.h>
> >  #include <openssl/dsa.h>
> >  #include <openssl/rsa.h>
> > +#ifdef HAVE_EC_CRYPTO
> >  #include <openssl/ec.h>
> > +#endif /* HAVE_EC_CRYPTO */
> 
> This will not work without config.h.
> 
> Did you test this on a system with ECC?  Apparently not, this should at
> least lead to 'make checkheaders' failures.

I recall I did test. The checkheaders targets succeeds (at least now).

> > --- lib/tool/pk.h     2011-07-08 11:47:12 +0000
> > +++ lib/tool/pk.h     2011-10-30 07:48:24 +0000
> > @@ -9,8 +9,10 @@
> >  int hip_dsa_sign(void *peer_pub, struct hip_common *msg);
> >  int hip_rsa_verify(void *priv_key, struct hip_common *msg);
> >  int hip_rsa_sign(void *peer_pub, struct hip_common *msg);
> > +#ifdef HAVE_EC_CRYPTO
> >  int hip_ecdsa_verify(void *const peer_pub, struct hip_common *const msg);
> >  int hip_ecdsa_sign(void *const peer_pub, struct hip_common *const msg);
> > +#endif /* HAVE_EC_CRYPTO */
> >  int bn2bin_safe(const BIGNUM *a, unsigned char *to, int len);
> 
> unnecessary

Removed.

> > === modified file 'test/lib/tool/pk.c'
> > --- test/lib/tool/pk.c        2011-07-18 13:10:10 +0000
> > +++ test/lib/tool/pk.c        2011-10-30 07:48:24 +0000
> > @@ -27,7 +27,9 @@
> >  #include <stdlib.h>
> >  #include <string.h>
> >  #include <stdio.h>
> > +#ifdef HAVE_EC_CRYPTO
> >  #include <openssl/ec.h>
> > +#endif /* HAVE_EC_CRYPTO */
> >  #include <openssl/pem.h>
> 
> see above

Did not get this.

« Back to merge proposal