[Libvir] PATCH: 2/4: Auth infrastructure & basic SASL support
Jim Meyering
jim at meyering.net
Sun Nov 18 18:16:50 UTC 2007
"Daniel P. Berrange" <berrange at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: remoteDispatchAuthSasl_start_or_step.c
Type: text/x-csrc
Size: 3467 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20071118/ba379df8/attachment-0001.bin>
-------------- next part --------------
...
> +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?
More information about the libvir-list
mailing list