On Wed, Oct 06, 2010 at 02:41:35PM -0000, stylesen_at_apache.org wrote:
> Author: stylesen
> Date: Wed Oct 6 14:41:35 2010
> New Revision: 1005065
>
> URL: http://svn.apache.org/viewvc?rev=1005065&view=rev
> Log:
> On the 'gpg-agent-password-store' branch:
>
> Add GPG-AGENT support for Subversion password caching.
Hi Senthil,
some review inline below.
> Added: subversion/branches/gpg-agent-password-store/subversion/libsvn_auth_gpg_agent/gpg_agent.c
> URL: http://svn.apache.org/viewvc/subversion/branches/gpg-agent-password-store/subversion/libsvn_auth_gpg_agent/gpg_agent.c?rev=1005065&view=auto
> ==============================================================================
> --- subversion/branches/gpg-agent-password-store/subversion/libsvn_auth_gpg_agent/gpg_agent.c (added)
> +++ subversion/branches/gpg-agent-password-store/subversion/libsvn_auth_gpg_agent/gpg_agent.c Wed Oct 6 14:41:35 2010
> @@ -0,0 +1,248 @@
> +/*
> + * gpg_agent.c: GPG Agent provider for SVN_AUTH_CRED_*
> + *
> + * ====================================================================
> + * Licensed to the Apache Software Foundation (ASF) under one
> + * or more contributor license agreements. See the NOTICE file
> + * distributed with this work for additional information
> + * regarding copyright ownership. The ASF licenses this file
> + * to you under the Apache License, Version 2.0 (the
> + * "License"); you may not use this file except in compliance
> + * with the License. You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing,
> + * software distributed under the License is distributed on an
> + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
> + * KIND, either express or implied. See the License for the
> + * specific language governing permissions and limitations
> + * under the License.
> + * ====================================================================
> + */
> +
> +/* ==================================================================== */
> +
> +
> +
> +/*** Includes. ***/
> +
> +#include <unistd.h>
> +
> +#include <sys/socket.h>
> +#include <sys/un.h>
> +
> +#include <apr_pools.h>
> +#include "svn_auth.h"
> +#include "svn_config.h"
> +#include "svn_error.h"
> +#include "svn_pools.h"
> +#include "svn_cmdline.h"
> +#include "svn_checksum.h"
> +#include "svn_string.h"
> +
> +#include "private/svn_auth_private.h"
> +
> +#include "svn_private_config.h"
> +
> +static const int buffer_size = 1024;
Please write the constant in upper case: BUFFER_SIZE
And maybe use a #define instead?
> +
> +/* Implementation of password_get_t that retrieves the password
> + from gpg-agent */
> +static svn_boolean_t
> +password_get_gpg_agent(const char **password,
> + apr_hash_t *creds,
> + const char *realmstring,
> + const char *username,
> + apr_hash_t *parameters,
> + svn_boolean_t non_interactive,
> + apr_pool_t *pool)
> +{
> + int sd;
> + char *gpg_agent_info = NULL;
> + char *value;
> + char *p = NULL;
> + char *ep = NULL;
> + char *buffer;
> +
> + apr_array_header_t *socket_details;
> + char *request = NULL;
> + const char *cache_id = NULL;
> + struct sockaddr_un addr;
> + int recvd;
> + char *tty_name;
> + char *tty_type;
> + const char *socket_name = NULL;
> + svn_checksum_t *digest = NULL;
> +
> + value = getenv( "GPG_AGENT_INFO");
> +
> + if (value != NULL)
> + {
> + gpg_agent_info = apr_pstrmemdup(pool, value, strlen(value));
> + socket_details = svn_cstring_split(gpg_agent_info, ":", TRUE, pool);
> + socket_name = APR_ARRAY_IDX(socket_details, 0, const char *);
> + }
> + else
> + return FALSE;
> +
> + value = getenv("GPG_TTY");
> + if (value != NULL)
> + tty_name = apr_pstrmemdup(pool, value, strlen(value));
> + else
> + return FALSE;
> +
> + value = getenv("TERM");
> + if (value != NULL)
> + tty_type = apr_pstrmemdup(pool, value, strlen(value));
> + else
> + return FALSE;
> +
> + if (socket_name != NULL)
> + {
> + addr.sun_family = AF_UNIX;
> + strncpy(addr.sun_path, socket_name, 108);
Using 108 is dangerous, because the buffer isn't guaranteed to be 109 bytes
long. E.g. on OpenBSD it's 104. In this case, we could overrun the buffer.
On Linux it's 108, and because strncpy() does not nul-terminate the
string if the nul doesn't fit, the above risks a non-terminated string.
So please use (sizeof(addr.sun_path) - 1) instead (- 1 to account for '\0').
This seems to be common practice, and aparrently sun_path is always
defined as an array rather than a pointer.
See also http://mail-index.netbsd.org/tech-kern/1997/02/25/0014.html
which explains historical reasons for the different buffer sizes between
systems.
Also, since strncpy() does not guarantee to nul-terminate the buffer,
please add:
addr.sun_path[sizeof(addr.sun_path) - 1] = '\0';
> + sd = socket(AF_UNIX, SOCK_STREAM, 0);
> + if (sd == -1)
> + return FALSE;
> +
> + if (connect(sd, (struct sockaddr *)&addr, sizeof(addr)) == -1)
> + {
> + close(sd);
> + return FALSE;
> + }
> + }
> + else
> + return FALSE;
> +
> + /* Receive the connection status from the gpg-agent daemon. */
> + buffer = apr_palloc(pool, buffer_size);
> + recvd = recv(sd, buffer, buffer_size-1, 0);
Spaces around binary operators:
recvd = recv(sd, buffer, buffer_size - 1, 0);
> + buffer[recvd] = '\0';
> +
> + if (strncmp(buffer, "OK", 2) != 0)
> + return FALSE;
> +
> + /* Send TTY_NAME to the gpg-agent daemon. */
> + request = apr_psprintf(pool, "OPTION ttyname=%s\n", tty_name);
> + send(sd, request, strlen(request), 0);
> + recvd = recv(sd, buffer, buffer_size - 1, 0);
> + buffer[recvd] = '\0';
> +
> + if (strncmp(buffer, "OK", 2) != 0)
> + return FALSE;
> +
> + /* Send TTY_TYPE to the gpg-agent daemon. */
> + request = apr_psprintf(pool, "OPTION ttytype=%s\n", tty_type);
> + send(sd, request, strlen(request), 0);
> + recvd = recv(sd, buffer, buffer_size - 1, 0);
> + buffer[recvd] = '\0';
> +
> + if (strncmp(buffer, "OK", 2) != 0)
> + return FALSE;
> +
> + /* Create the CACHE_ID which will be generated based on REALMSTRING similar
> + to other password caching mechanisms. */
> + digest = svn_checksum_create(svn_checksum_md5, pool);
> + svn_checksum(&digest, svn_checksum_md5, realmstring, strlen(realmstring),
> + pool);
Maybe use SHA1 instead?
The rest looks fine.
Thanks,
Stefan
Received on 2010-10-06 17:28:31 CEST