[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libvir] PATCH: 2/4: Auth infrastructure & basic SASL support



"Daniel P. Berrange" <berrange redhat com> wrote:
> This patch hooks up the basic authentication RPC calls, and the specific
> SASL implementation. The SASL impl can be enabled/disable via the configurre
> script with --without-sasl / --with-sasl - it'll auto-enable it if it finds
> the headers & libs OK.

Nice!  I like the design.
I found some implementation nits:

...
> diff -r b5fe91c98e78 qemud/remote.c
> --- a/qemud/remote.c	Tue Oct 30 16:14:32 2007 -0400
> +++ b/qemud/remote.c	Thu Nov 01 16:54:13 2007 -0400
...
> +static int
> +remoteDispatchAuthList (struct qemud_client *client,
> +                        remote_message_header *req ATTRIBUTE_UNUSED,
> +                        void *args ATTRIBUTE_UNUSED,
> +                        remote_auth_list_ret *ret)
> +{
> +    ret->types.types_len = 1;
> +    ret->types.types_val = calloc (ret->types.types_len, sizeof (remote_auth_type));

Detect calloc failure:

    if (ret->types.types_val == NULL) {
        remoteDispatchError(client, req, "cannot allocate auth type list");
        return -1;
    }

> +    ret->types.types_val[0] = client->auth;
> +    return 0;
> +}
> +
> +
> +#if HAVE_SASL
> +static char *addrToString(struct qemud_client *client,
> +                          remote_message_header *req,
> +                          struct sockaddr_storage *sa, socklen_t salen) {
> +    char host[1024], port[20];
> +    char *addr;
> +
> +    if (getnameinfo((struct sockaddr *)sa, salen,
...
> +        remoteDispatchError(client, req,
> +                            "Cannot resolve address %d: %s", errno, strerror(errno));

There's enough shared code between this addrToString and the
identically-named function in qemud/remote.c, that it'd be nice to add a
warning/comment in each that they need to be kept in sync.  It's probably
not worth trying to merge them: with two uses of each, pulling the error-
reporting bits out would just unfactor/pollute into the callers.

Well, maybe it's worth factoring after all:
Both use errno rather than gai_strerror(the return value).
Also, it's better to test getnameinfo() != 0, since officially,
that's all that matters.  I.e.,

    if ((err = getnameinfo((struct sockaddr *)sa, salen,
                    host, sizeof(host),
                    port, sizeof(port),
                    NI_NUMERICHOST | NI_NUMERICSERV)) != 0) {
        __virRaiseError (NULL, NULL, NULL, VIR_FROM_REMOTE,
                         VIR_ERR_NO_MEMORY, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0,
                         "Cannot resolve address: %s", gai_strerror(err));
        return NULL;
    }


> +        remoteDispatchError(client, req,
> +                            "Cannot resolve address %d: %s", errno, strerror(errno));
> +        return NULL;
> +    }
> +
> +    addr = malloc(strlen(host) + 1 + strlen(port) + 1);
> +    if (!addr) {
> +        remoteDispatchError(client, req, "cannot allocate address");
> +        return NULL;
> +    }
> +
> +    strcpy(addr, host);
> +    strcat(addr, ";");
> +    strcat(addr, port);
> +    return addr;
> +}
> +
> +
> +/*
> + * Initializes the SASL session in prepare for authentication
> + * and gives the client a list of allowed mechansims to choose
> + *
> + * XXX callbacks for stuff like password verification ?
> + */
> +static int
> +remoteDispatchAuthSaslInit (struct qemud_client *client,
> +                            remote_message_header *req,
> +                            void *args ATTRIBUTE_UNUSED,
> +                            remote_auth_sasl_init_ret *ret)
> +{
...
> +    free(localAddr);
> +    free(remoteAddr);
> +    if (err != SASL_OK) {
> +        qemudLog(QEMUD_ERR, "sasl context setup failed %d (%s)",
> +                 err, sasl_errstring(err, NULL, NULL));
> +        remoteDispatchFailAuth(client, req);
> +        client->saslconn = NULL;
> +        return -2;
> +    }
> +
> +    err = sasl_listmech(client->saslconn,
> +                        NULL, /* Don't need to set user */
> +                        "", /* Prefix */
> +                        ",", /* Separator */
> +                        "", /* Suffix */
> +                        &mechlist,
> +                        NULL,
> +                        NULL);
> +    if (err != SASL_OK) {
> +        qemudLog(QEMUD_ERR, "cannot list SASL mechanisms %d (%s)",
> +                 err, sasl_errdetail(client->saslconn));
> +        remoteDispatchFailAuth(client, req);
> +        sasl_dispose(&client->saslconn);
> +        client->saslconn = NULL;
> +        return -2;
> +    }
> +    REMOTE_DEBUG("Available mechanisms for client: '%s'", mechlist);
> +    ret->mechlist = strdup(mechlist);
> +    if (!ret->mechlist) {

Maybe another qemudLog call here, for the sake of consistency?  e.g.,

           qemudLog(QEMUD_ERR, "mechlist allocation failure")


> +        remoteDispatchFailAuth(client, req);
> +        sasl_dispose(&client->saslconn);
> +        client->saslconn = NULL;
> +        return -2;
> +    }
> +
> +    return 0;
> +}
> +
> +
> +/*
> + * This starts the SASL authentication negotiation.
> + */
> +static int
> +remoteDispatchAuthSaslStart (struct qemud_client *client,
> +                             remote_message_header *req,
> +                             remote_auth_sasl_start_args *args,
> +                             remote_auth_sasl_start_ret *ret)
> +{
> +    const char *serverout;
> +    unsigned int serveroutlen;
> +    int err;
> +
> +    REMOTE_DEBUG("Start SASL auth %d", client->fd);
> +    if (client->auth != REMOTE_AUTH_SASL ||
> +        client->saslconn == NULL) {
> +        qemudLog(QEMUD_ERR, "client tried illegal SASL start request");

s/illegal/invalid/ (and two more, below)
"illegal" regards laws and the legal system

> +        remoteDispatchFailAuth(client, req);
> +        return -2;
...
> +static int
> +remoteDispatchAuthSaslStep (struct qemud_client *client,
> +                            remote_message_header *req,
> +                            remote_auth_sasl_step_args *args,
> +                            remote_auth_sasl_step_ret *ret)
...

The two preceding functions are so similar, that I took the time
to compare and factor them into one, to avoid the duplication.
In the process, I found one minor nit that might have caused confusion:
remoteDispatchAuthSasl*Step* can log an invalid *start* request,
when I think it means a "step" request:

  > +        qemudLog(QEMUD_ERR, "client tried illegal SASL start request");

Here's the factored version.  Yeah, it's a 70-line macro, and that's ugly,
but there's no other way.  IMHO, eliminating that much duplication makes
it worthwhile.

static inline sasl_server_start_or_step (int is_start,
					 sasl_conn_t *conn,
					 const char *mech,
					 const char *clientin,
					 unsigned *clientinlen,
					 const char **serverout,
					 unsigned *serveroutlen)
{
  if (is_start)
    sasl_server_start (conn, mech, clientin, clientinlen, serverout, serveroutlen);
  else
    sasl_server_step (conn, clientin, clientinlen, serverout, serveroutlen);
}

#define remoteDispatchAuthSasl_start_or_step(is_start,SS,ss)		\
static int								\
remoteDispatchAuthSasl##SS (struct qemud_client *client,		\
                            remote_message_header *req,			\
                            remote_auth_sasl_##ss##_args *args,		\
                            remote_auth_sasl_##ss##_ret *ret)		\
{									\
    const char *serverout;						\
    unsigned int serveroutlen;						\
    int err;								\
									\
    REMOTE_DEBUG(#SS " SASL auth %d", client->fd);			\
    if (client->auth != REMOTE_AUTH_SASL ||				\
        client->saslconn == NULL) {					\
        qemudLog(QEMUD_ERR, "client tried invalid SASL " #ss " request"); \
        remoteDispatchFailAuth(client, req);				\
        return -2;							\
    }									\
									\
    REMOTE_DEBUG("Using SASL (mechanism %s) Data %d bytes, nil: %d",	\
                 (is_start ? args->mech : "N.A."),			\
                 "N.A.", args->data.data_len, args->nil);		\
    err = sasl_server_start_or_step(is_start, client->saslconn,		\
				    args->mech,				\
				    code_if_start (is_start, args->mech) \
			  /* NB, distinction of NULL vs "" is *critical* in SASL */ \
				    args->nil ? NULL : args->data.data_val, \
				    args->data.data_len,		\
				    &serverout,				\
				    &serveroutlen);			\
    if (err != SASL_OK &&						\
        err != SASL_CONTINUE) {						\
        qemudLog(QEMUD_ERR, "sasl " #ss " failed %d (%s)",		\
                 err, sasl_errdetail(client->saslconn));		\
        sasl_dispose(&client->saslconn);				\
        client->saslconn = NULL;					\
        remoteDispatchFailAuth(client, req);				\
        return -2;							\
    }									\
    if (serveroutlen > REMOTE_AUTH_SASL_DATA_MAX) {			\
        qemudLog(QEMUD_ERR, "sasl " #ss " reply data too long %d", serveroutlen); \
        sasl_dispose(&client->saslconn);				\
        client->saslconn = NULL;					\
        remoteDispatchFailAuth(client, req);				\
        return -2;							\
    }									\
									\
    /* NB, distinction of NULL vs "" is *critical* in SASL */		\
    if (serverout) {							\
        ret->data.data_val = malloc(serveroutlen);			\
        if (!ret->data.data_val) {					\
            remoteDispatchError (client, req, "out of memory allocating array"); \
            return -2;							\
        }								\
        memcpy(ret->data.data_val, serverout, serveroutlen);		\
    } else {								\
        ret->data.data_val = NULL;					\
    }									\
    ret->nil = serverout ? 0 : 1;					\
    ret->data.data_len = serveroutlen;					\
									\
    REMOTE_DEBUG("SASL return data %d bytes, nil; %d", ret->data.data_len, ret->nil); \
    if (err == SASL_CONTINUE) {						\
        ret->complete = 0;						\
    } else {								\
        REMOTE_DEBUG("Authentication successful %d", client->fd);	\
        ret->complete = 1;						\
        client->auth = REMOTE_AUTH_NONE;				\
    }									\
									\
    return 0;								\
}
remoteDispatchAuthSasl_start_or_step(1, Start, start)
remoteDispatchAuthSasl_start_or_step(0, Step, step)
...
> +static int
> +remoteAuthSASL (virConnectPtr conn, struct private_data *priv, int in_open)
...
> +    if ((remoteAddr = addrToString(&sa, salen)) == NULL) {
> +        free(localAddr);
> +        return -1;
> +    }
> +    printf("'%s' '%s' '%s'\n", priv->hostname, localAddr, remoteAddr);

Is this printf for debugging?

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]