[hipl-dev] Re: [Merge] lp:~hipl-core/hipl/certificate-exchange into lp:hipl
- From: Diego Biurrun <diego@xxxxxxxxxx>
- To: mp+95634@xxxxxxxxxxxxxxxxxx
- Date: Fri, 09 Mar 2012 11:46:54 +0100
On Thu, Mar 08, 2012 at 06:41:47PM +0100, Diego Biurrun wrote:
> review needs-fixing
It still needs fixing, I'm just commenting on the issues I addressed.
> On Fri, Mar 02, 2012 at 06:08:20PM +0000, Diego Biurrun wrote:
> > Diego Biurrun has proposed merging lp:~hipl-core/hipl/certificate-exchange
> > into lp:hipl.
> >
> > --- hipd/cert.c 2012-03-01 14:06:24 +0000
> > +++ hipd/cert.c 2012-03-02 18:07:40 +0000
> > @@ -682,89 +688,109 @@
> > +
> > + /* Were we sent a timestamp which indicates a requested cert validity?
> > */
> > + validity_param = hip_get_param(msg, HIP_PARAM_UINT);
> > +
> > + if (validity_param) {
> > + const uint32_t valid_until_n = *(const uint32_t *)
> > hip_get_param_contents_direct(validity_param);
> > + const uint32_t valid_until_h = ntohl(valid_until_n);
>
> Gah, pointer type punning.
Fixed.
> > @@ -797,33 +821,67 @@
> > +
> > + X509_get_notBefore(cert)->type = V_ASN1_GENERALIZEDTIME;
> > + X509_get_notAfter(cert)->type = V_ASN1_GENERALIZEDTIME;
> > +
> > + {
> > + const time_t now = time(NULL);
> > + time_t starttime = 0;
> > + time_t endtime = 0;
> > + time_t *starttime_p = NULL;
> > + time_t *endtime_p = NULL;
>
> The block braces are unnecessary, we allow mixed declarations and
> statements.
Fixed.
Apparently Christoph's intention here was to reduce the scope of the
variables and possibly split the function into pieces along this line.
So if somebody wants to step forward and do that.. :)
> > switch (algo) {
> > case HIP_HI_RSA:
> > - HIP_IFEL(!EVP_PKEY_assign_RSA(pkey, rsa), -1,
> > + HIP_IFEL(!EVP_PKEY_set1_RSA(pkey, (RSA *) ha->peer_pub_key), -1,
> > "Failed to convert RSA to EVP_PKEY\n");
> > HIP_IFEL(X509_set_pubkey(cert, pkey) != 1, -1,
> > "Failed to set public key of the certificate\n");
> > break;
> > case HIP_HI_DSA:
> > - HIP_IFEL(!EVP_PKEY_assign_DSA(pkey, dsa), -1,
> > + HIP_IFEL(!EVP_PKEY_set1_DSA(pkey, (DSA *) ha->peer_pub_key), -1,
> > "Failed to convert DSA to EVP_PKEY\n");
>
> peer_pub_key is a void*, the casts are unnecessary.
Fixed.
> > @@ -926,7 +984,25 @@
> >
> > + switch (host_id->rdata.algorithm) {
> > + case HIP_HI_RSA:
> > + HIP_IFEL(!EVP_PKEY_set1_RSA(sig_key, (RSA *) key), -1,
> > + "Failed to convert RSA to EVP_PKEY\n");
> > + break;
> > + case HIP_HI_DSA:
> > + HIP_IFEL(!EVP_PKEY_set1_DSA(sig_key, (DSA *) key), -1,
> > + "Failed to convert DSA to EVP_PKEY\n");
> > + break;
>
> same here
Fixed.
> > --- hipfw/pisa.c 2011-11-25 13:52:20 +0000
> > +++ hipfw/cert.c 2012-03-02 18:07:40 +0000
> > @@ -25,551 +25,192 @@
> > +#include "rule_management.h"
> > +#include "cert.h"
> > +
> > +// runtime configuration
> > +int use_cert = false;
>
> This could be static.
Fixed.
> > +const char *root_certificate_file = NULL;
>
> This is unused.
Fixed.
> > --- lib/core/builder.c 2012-03-01 14:06:24 +0000
> > +++ lib/core/builder.c 2012-03-02 18:07:40 +0000
> > @@ -3388,16 +3395,21 @@
> > int hip_build_param_cert_x509_ver(struct hip_common *msg, char *der, int
> > len)
> > {
> > + int err = -1;
> > + struct hip_cert_x509_resp subj = { 0 };
> > +
> > + if (len > 0 && (unsigned int) len <= sizeof(subj.der)) {
> > + hip_set_param_type((struct hip_tlv_common *) &subj,
> > HIP_PARAM_CERT_X509_REQ);
> > + hip_calc_param_len((struct hip_tlv_common *) &subj,
> > + sizeof(struct hip_cert_x509_resp)
> > + - sizeof(struct hip_tlv_common));
> > + memcpy(&subj.der, der, len);
> > +
> > + const uint32_t der_len_h = len;
> > + subj.der_len = htonl(der_len_h);
> > + err = hip_build_param(msg, &subj);
> > + }
> > + return err;
> > }
>
> len should just be made unsigned instead of casting and checking.
Fixed.
> > @@ -3414,16 +3426,22 @@
> > int hip_build_param_cert_x509_resp(struct hip_common *msg, char *der, int
> > len)
> > {
> > + int err = -1;
> > + struct hip_cert_x509_resp local = { 0 };
> > +
> > + if (len > 0 && (unsigned int) len <= sizeof(local.der)) {
> > + hip_set_param_type((struct hip_tlv_common *) &local,
> > + HIP_PARAM_CERT_X509_RESP);
> > + hip_calc_param_len((struct hip_tlv_common *) &local,
> > + sizeof(struct hip_cert_x509_resp)
> > + - sizeof(struct hip_tlv_common));
> > + memcpy(&local.der, der, len);
> > +
> > + const uint32_t der_len_h = len;
> > + local.der_len = htonl(der_len_h);
> > + err = hip_build_param(msg, &local);
> > + }
> > + return err;
> > }
>
> Same - oh wait, this is code duplication at its worst ...
Fixed.
> > --- lib/core/cert.c 1970-01-01 00:00:00 +0000
> > +++ lib/core/cert.c 2012-03-02 18:07:40 +0000
> > +X509 *cert_DER_to_X509(const unsigned char *const buf, const int len)
> > +{
> > + const unsigned char *p;
> > +
> > + if (!buf) {
> > + HIP_ERROR("Cannot decode from NULL-buffer\n");
> > + return NULL;
> > + }
> > + if (len <= 0) {
> > + HIP_ERROR("Cannot decode certificate of length <= 0\n");
> > + return NULL;
> > + }
> > + p = buf;
> > + return d2i_X509(NULL, &p, len);
> > +}
>
> Is there a point in the variable indirection through p? It seems to be
> to work around a const-related warning, but if d2i_x509 does not expect
> a const pointer, don't pass one to it through pointer aliasing. This
> takes away the compiler's ability to check correctness!
Fixed.
> > +/**
> > + * Load a X509 certificate from @ file. If @ file contains more
> > + * than one certificate, the certificate at the top of the file is
> > + * returned.
> > + *
> > + * @param file the file to load the certficate from
> > + * @param fmt the input format of the certificate
> > + *
> > + * @return a X509 certificate on success, NULL on error
> > + */
> > +X509 *cert_load_x509_certificate(const char *const file,
> > + enum encoding_format fmt)
>
> What's the idea behind the "@ file"? This is not correct Doxygen syntax.
> I'm assuming @p was meant here to refer to the function parameter and
> show a marked-up version of the parameter name in the generated HTML.
Fixed.
> > +/**
> > + * Compare a given public key @ pkey with the public key
> > + * contained in @ cert.
> > + *
> > + * @param cert the X509 certificate
> > + * @param pkey the public key to match
> > + *
> > + * @return 1 if match, 0 otherwise
> > + */
> > +int cert_match_public_key(X509 *cert, const EVP_PKEY *pkey)
>
> same comment wrt the @.
Fixed.
> > +{
> > + EVP_PKEY *pkey2 = NULL;
> > +
> > + if (!cert) {
> > + return 0;
> > + }
> > + if (!pkey) {
> > + return 0;
> > + }
>
> These two could be merged.
Fixed.
Diego
Other related posts:
