[Spice-devel] [PATCH 8/9] common: add ssl_verify.c common code

Sun May 1 04:37:52 PDT 2011

On Sun, May 01, 2011 at 07:08:58AM -0400, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
> > On Tue, Jan 25, 2011 at 07:17:27PM +0100, Marc-André Lureau wrote:
> > 
> > Missing #ifdef HAVE_CONFIG_H
> > #include <config.h>
> > #endif
> > 
> 
> It didn't use any config.h define, and the files in common/ don't include config.h. But I would prefer if they do. So, I don't mind having it.

I added #include <config.h> to all these files recently in one of the patch
series on this mailing list recently, hence my comment. And I've seen weird
bugs occurring due to inconsistent use of #include <config.h> so I prefer
to have it even if it seems not to be used (basically, one of the libc data
structures on x86 has a different layout depending on whether large file
support is enabled or not, and this support is enabled through one or two
defines in config.h which are then picked by system headers).

> 
> > > +
> > > +#include "mem.h"
> > > +#include "ssl_verify.h"
> > > +
> > > +#include <alloca.h>
> > > +#include <sys/socket.h>
> > > +#include <netinet/in.h>
> > > +#include <arpa/inet.h>
> > > +
> > > +
> > > +#ifndef SPICE_DEBUG
> > > +# define SPICE_DEBUG(format, ...) printf("%s: " format "\n",
> > > __FUNCTION__, ## __VA_ARGS__)
> > > +#endif
> > 
> > Use the macro from spice_common.h
> 
> But it's not there (at least today):
> 
> http://cgit.freedesktop.org/spice/spice/tree/common/spice_common.h

Of course it's not, it's ASSERT and friends which is there.
/me hides
Forget this comment :)

> 
> > > +
> > > +static int verify_pubkey(X509* cert, const char *key, size_t
> > > key_size)
> > 
> > the corresponding code in red_peer.c returns a bool, I thought we were
> > using C99 and that this was allowed? Most of the boolean in the
> > earlier
> > code were changed to int.
> 
> I changed it to return an int, that way negative values can indicate errors, so we don't loose informations from EVP_PKEY_cmp():
> 
> "-1 if the key types are different and -2 if the operation is not supported"
> 
> It's also more future proof. Also, we use C99, but it doesn't mean we follow strictly C99, imho that wouldn't make sense.

There are a few places in this patch where a bool makes things clearer as
to what the intent of the variable is, even if it's not the case for this
specific one. I can go over v2 of this patch to readd bool where I feel
that's appropriate and submit the diff for review.

> 
> > > +X509_NAME* subject_to_x509_name(const char *subject, int *nentries)
> > > +{
> > > + X509_NAME* in_subject;
> > 
> > What does the in_ prefix stands for here?
> 
> I can't remember. We could rename it xsubject if you prefer. Other ideas?

No need to change, I was just curious.
> 
> > 
> > > + const char *p;
> > > + char *key, *val, *k, *v = NULL;
> > > + int escape;
> > > + enum {
> > > + KEY,
> > > + VALUE
> > > + } state;
> > > +
> > > + key = (char*)alloca(strlen(subject));
> > > + val = (char*)alloca(strlen(subject));
> > 
> > Any idea if this implies potential portability issues on windows or
> > macosx?
> > glib provides g_alloca so I assume it's available on the platforms
> > we're
> > interested in?
> > Reading
> > http://www.gnu.org/s/hello/manual/autoconf/Particular-Functions.html
> > we may
> > need something more complicated for win32
> 
> Correct, I used this in spice-gtk, it could be included in the patch series.
> 
> https://gitorious.org/spice-gtk/spice-gtk/commit/46be1ca0485fd2e6dcb1b43fb43ffa37e7f3a3f7
> 
> Or we could use the snippet from the autoconf manual, which looks good as well.

I'd favour the one from the autoconf manual which doesn't have the unneeded
(in spice/) GLIB_HAVE_ALLOCA_H bit. Unless you need this bit in spice-gtk.

> > > + if (*p == '\\') {
> > > + ++p;
> > > + if (*p != '\\' && *p != ',') {
> > > + SPICE_DEBUG("Invalid character after \\");
> > > + goto fail;
> > > + }
> > > + escape = 1;
> > > + }
> > > +
> > > + switch (state) {
> > > + case KEY:
> > > + if (*p == 0) {
> > > + if (k == key) /* empty key */
> > > + goto success;
> > > + goto fail;
> > > + } else if (*p == '=' && !escape) {
> > > + state = VALUE;
> > > + *k = 0;
> > > + v = val;
> > > + } else
> > > + *k++ = *p;
> > > + break;
> > 
> > This code should check for ',' somewhere if we want to get the code to
> > behave as Application::set_host_cert_subject: when parsing
> > "foo,bar=foobar", this code will parse (foo,bar ; foobar) while the
> > old code would have failed with "host_subject bad format: assignment
> > for
> > foo is missing"
> 
> I didn't test it, but With "foo,", v == val and it will go to fail.

As I understand the parser, when in the KEY state, we'll be consuming chars
until we find the '=', in which case we'll switch to the VALUE state. So at
this point, we'll have k="foo,bar" and we'll start reading the value until
the end of the string. Here we'll have k="foo,bar" and v="foobar" and we'll
add an entry for this, so I don't think we'll get v == val and go to fail.

> 
> > It will also silently go to the VALUE state when the key is empty (
> > "=foo"), and the VALUE parsing isn't checking for tha either,I don't
> > know
> > if that's desirable
> 
> It rely on X509_NAME_add_entry_by_txt() to do the right thing when key is empty instead.
> 
> I don't know if an empty VALUE is correct either, but I guess it's fine.

Ok, I'll trust you on this one :)

> > Similarly to the "KEY" state, here we will parse foo=bar=foobar as
> > (foo ;
> > bar=foobar) unless X509_NAME_add_entry_by_txt complains about this,
> > but the
> > old code behaved the same as far as I can tell. Reading
> > apps/apps.c:parse_name in openssl source seems to imply that anything
> > after
> > \\ is to be taken verbatim, ie the only invalid use of \\ is when it's
> > at
> > the end of a string. It also seems that '+' is a magic value. But the
> > parsed string has a slightly different format (values separated by '/'
> > instead of ','), so maybe we can't compare the 2 implementations.
> > This code also lost the fix from 12f99d3 which skips spaces between
> > ',' and
> > keys, but I can't blame you for that since this patch predates this
> > commit
> > :)
> > 
> 
> Why do you think that foo=bar=foobar -> (foo ; bar=foobar) should be an error?

I have no idea whether this should be an error or not. This is a corner
case whose handling might have been forgotten, so I'm mentioning it. I'm
totaly fine with " '=' are allowed in values " so the parser does the right
thing. I don't know if it's valid or not :)

> > > + /* Note: this check is redundant with the pre-condition in
> > > X509_NAME_cmp */
> > 
> > This was already the case in openssl 0.9.8, I'd just drop this test
> 
> But this way we get an error message that we can actually debug easily.

Ok, I don't feel strongly either way so let's keep it.

> 
> > Since the verification operation are mutually exclusive, another
> > possibility is to have 3 or 4 different constructors, and to drop the
> > SPICE_SSL_VERIFY_OP enum from the public API.
> 
> Why do you think they are mutually exclusive?

Because the old code only allow one at a time as you mentioned in your
introductory email :) When providing a subject string, I'm not sure it
makes much sense to also check the host name since the subject string will
be more accurate, but that's just the feeling of someone absolutely not
familiar with x509 :(

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20110501/9a4885da/attachment.pgp>