"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 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?