12:16 p.m.
"Daniel P. Berrange" <berrange(a)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?