[hipl-dev] Re: [Merge] lp:~hipl-core/hipl/certificate-exchange into lp:hipl
- From: Diego Biurrun <diego@xxxxxxxxxx>
- To: mp+95634@xxxxxxxxxxxxxxxxxx
- Date: Tue, 13 Mar 2012 17:31:34 +0100
On Mon, Mar 12, 2012 at 09:29:29PM +0100, Diego Biurrun wrote:
> On Thu, Mar 08, 2012 at 06:41:47PM +0100, Diego Biurrun wrote:
> > 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
> >
> > The functions in this file are HUUUUGE - IMO this needs some refactoring.
>
> Any volunteers? The original authors maybe? I still don't really
> understand the "big picture" of this branch.
Left for never^wlater.
> > > --- lib/core/cert.c 1970-01-01 00:00:00 +0000
> > > +++ lib/core/cert.c 2012-03-02 18:07:40 +0000
> > > @@ -0,0 +1,292 @@
> > > +/**
> > > + * 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)
> > > +{
> > > + FILE *fp = NULL;
> > > + X509 *cert = NULL;
> > > +
> > > + if (!file) {
> > > + HIP_ERROR("Cannot read certificate from NULL-filename.\n");
> > > + return NULL;
> > > + }
> > > +
> > > + fp = fopen(file, "rb");
> > > + if (!fp) {
> > > + HIP_ERROR("Could not open file for reading: %s\n", file);
> > > + return NULL;
> > > + }
> > > +
> > > + if (fmt == ENCODING_FORMAT_PEM) {
> > > + cert = PEM_read_X509(fp, NULL, NULL, NULL);
> > > + } else if (fmt == ENCODING_FORMAT_DER) {
> > > + cert = d2i_X509_fp(fp, NULL);
> > > + } else {
> > > + HIP_ERROR("Invalid encoding format %i \n", fmt);
> > > + return NULL;
> > > + }
> >
> > switch/case? Might more formats get added here?
>
> Anybody?
Converted to switch/case, looks nicer at least.
> > > + if (fclose(fp)) {
> > > + HIP_ERROR("Error closing file: %s\n", file);
> > > + X509_free(cert);
> > > + return NULL;
> > > + } else if (!cert) {
> > > + HIP_ERROR("Could not decode certificate from file.\n");
> > > + return NULL;
> > > + }
> > > +
> > > + return cert;
> > > +}
> >
> > If I read this correctly, fp will not get closed on encountering an
> > invalid encoding format.
>
> Is my assessment correct?
It is correct, function logic fixed on branch.
> > > +/**
> > > + * Search for hip_cert parameter in @ msg and try to decode the data in
> > > + * the first certificate parameter of a X509 certificate.
> > > + *
> > > + * @param msg the message to extract the certificate from
> > > + *
> > > + * @return the first X509 certificate found in the message on
> > > success,
> > > + * NULL on error and if no certificates were found
> > > + */
> > > +X509 *cert_get_X509_from_msg(const struct hip_common *const msg)
> > > +{
> > > + const struct hip_cert *param_cert = NULL;
> > > +
> > > + if (!(param_cert = hip_get_param(msg, HIP_PARAM_CERT))) {
> > > + HIP_ERROR("Message contains no certificate.\n");
> > > + return NULL;
> > > + }
> > > +
> > > + /* The contents of the certificate begin after the header of the
> > > + * hip_cert parameter. */
> > > + return cert_DER_to_X509((const unsigned char *) (param_cert + 1),
> > > + ntohs(param_cert->length) -
> > > + sizeof(struct hip_cert) +
> > > + sizeof(struct hip_tlv_common));
> > > +}
> >
> > What is "param_cert + 1" supposed to be? Are you trying to move the
> > pointer past the struct? This is extremely brittle, see the cast
> > and the length calculation that are necessary.
>
> I'm still mystified about this one.
Christoph confirms the intention. Repairing this likely entails quite
a bit of fiddling with structures.
> > > --- lib/core/conf.c 2012-01-13 16:25:15 +0000
> > > +++ lib/core/conf.c 2012-03-02 18:07:40 +0000
> > > @@ -2424,6 +2435,122 @@
> > >
> > > /**
> > > + * Utility function which compactly checks whether a string represents
> > > + * a positive natural number, since scanf() is too lenient.
> > > + *
> > > + * @param string NULL-terminated string to be checked
> > > + *
> > > + * @return 1 if the only characters in the string are digits,
> > > + * optionally with one leading '+' sign; 0 otherwise
> > > + */
> > > +static int is_pos_natural_number(const char *string)
> > > +{
> > > + if (string) {
> > > + size_t len;
> > > +
> > > + // allow one leading '+'
> > > + if (string[0] == '+') {
> > > + string++;
> > > + }
> > > +
> > > + if ((len = strlen(string)) > 0) {
> > > + for (size_t i = 0; i < len; i++) {
> > > + if (string[i] < '0' || string[i] > '9') {
> > > + return 0;
> > > + }
> > > + }
> > > + return 1;
> > > + }
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > +static int conf_handle_certificate(struct hip_common *msg,
> > > + int action,
> > > + const char *opt[],
> > > + int optc,
> > > + UNUSED int send_only)
> > > +{
> > > + hip_hit_t subject_hit;
> > > + uint32_t valid_until_h = 0, valid_until_n =
> > > 0;
> > > + int scanf_res = 0, err = 0;
> > > + X509 *cert = NULL;
> > > + const struct hip_cert_x509_resp *p = NULL;
> > > +
> > > +
> > > + /* convert the parameter strings input by the user into the
> > > + * appropriate binary formats for further processing */
> > > + const int pton_res = inet_pton(AF_INET6, opt[0], &subject_hit);
> > > +
> > > + if (optc == 2 && is_pos_natural_number(opt[1])) {
> >
> > OK, what exactly is the problem here? Why do you need the utility function?
>
> Can the original author shed some light on this?
Christoph said scanf did not work for him. I guess we'll trust his word.
Diego
Other related posts:
