"Daniel P. Berrange" <berrange@xxxxxxxxxx> 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?
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list