Re: [PATCH] gpg-agent support

Dan Engel <[email protected]> writes:

Looks interesting.

> --- ./configure.ac	2009-11-25 18:50:03.000000000 -0500
> +++ ../subversion-gpg-agent/configure.ac	2009-11-25 18:13:14.000000000 -0500
> @@ -516,6 +516,37 @@
>    fi
>  fi
>  
> +dnl GPG Agent -------------------
> +
> +AC_ARG_WITH(gpg_agent,
> +  AS_HELP_STRING([--with-gpg-agent], 
> +                 [Enable use of GPG AGENT for auth credentials]),
> +                 [with_gpg_agent="$withval"],
> +                 [with_gpg_agent=no])
> +
> +AC_MSG_CHECKING([whether to look for GPG AGENT])
> +if test "$with_gpg_agent" != "no"; then
> +  AC_MSG_RESULT([yes])
> +  if test "$enable_shared" = "yes"; then
> +    if test "$APR_HAS_DSO" = "yes"; then
> +            if test "$with_gpg_agent" = "yes"; then
> +              AC_MSG_RESULT([yes])
> +              AC_DEFINE([SVN_HAVE_GPG_AGENT], [1], 
> +                        [Is GPG Agent support enabled?])
> +	      SVN_GPG_AGENT_LIBS="-lapr-1 -laprutil-1"

Nothing else uses -lapr-1 explicitly.

> --- ./subversion/libsvn_auth_gpg_agent/gpg_agent.c	1969-12-31 19:00:00.000000000 -0500
> +++ ../subversion-gpg-agent/subversion/libsvn_auth_gpg_agent/gpg_agent.c	2009-11-24 17:03:40.000000000 -0500
> @@ -0,0 +1,388 @@
> +/*
> + * gpg_agent.c: GPG Agent provider for SVN_AUTH_CRED_*
> + *
> + * ====================================================================
> + * Copyright (c) 2008-2009 CollabNet.  All rights reserved.

Should be the ASF notice these days.

> + *
> + * This software is licensed as described in the file COPYING, which
> + * you should have received as part of this distribution.  The terms
> + * are also available at http://subversion.tigris.org/license-1.html.
> + * If newer versions of this license are posted there, you may use a
> + * newer version instead, at your option.
> + *
> + * This software consists of voluntary contributions made by many
> + * individuals.  For exact contribution history, see the revision
> + * history and logs, available at http://subversion.tigris.org/.
> + * ====================================================================
> + */
> +
> +/* ==================================================================== */
> +
> +
> +
> +/*** Includes. ***/
> +//#include <stdio.h>

Don't use C99/C++ style comments.

> +
> +#include <stdlib.h>
> +#include <string.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 "private/svn_auth_private.h"
> +
> +#include "svn_private_config.h"
> +
> +static const int buffer_size = 1024;
> +
> +/* 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 s;
> +	char *gpg_agent_info = NULL;
> +	char *value;
> +	char *p = NULL;
> +	char *ep = NULL;
> +	char *buffer;
> +	int i;
> +
> +	char *realm_str = NULL;
> +	char *user_str = NULL;
> +
> +  char *request = NULL;
> +  const char *cache_id = NULL;
> +  svn_checksum_t *digest;
> +  int request_len = 0;
> +  struct sockaddr_un addr;
> +  int recvd;
> +  int tot_recvd;
> +  	char *ttyname;
> +	char *ttytype;

Indentation is inconsistent.

> +
> +	//fprintf( stderr, "doing gpg-agent stuff\n");

Try to avoid commented-out code.

> +	value = getenv( "GPG_AGENT_INFO");
> +
> +	if ( value != NULL)

We don't put that much whitespace around parentheses.

> +	{
> +		gpg_agent_info = apr_pstrmemdup( pool, value, strlen( value));
> +
> +		p = strrchr( gpg_agent_info, ':');
> +
> +		if ( p != NULL)
> +		{
> +			*p = '\0';
> +			p = strrchr( gpg_agent_info, ':');
> +		}
> +	}
> +	else
> +	{
> +		return FALSE;
> +	}
> +
> +	if ( p == NULL)
> +	{
> +		return FALSE;
> +	}
> +
> +	*p = '\0';
> +
> +	value = getenv( "GPG_TTY");
> +	if ( value == NULL)
> +	{
> +		return FALSE;
> +	}
> +
> +	ttyname = apr_pstrmemdup( pool, value, strlen( value));
> +
> +	value = getenv( "TERM");
> +	if ( value == NULL)
> +	{
> +		return FALSE;
> +	}
> +
> +	ttytype = apr_pstrmemdup( pool, value, strlen( value));
> +
> +	if ( ( ttyname == NULL) || ( ttytype == NULL))
> +	{
> +		return FALSE;
> +	}
> +
> +	addr.sun_family = AF_UNIX;
> +	strncpy( &addr.sun_path[0], gpg_agent_info, 108);
> +
> +	s = socket( AF_UNIX, SOCK_STREAM, 0);
> +	if ( s == -1)
> +	{
> +		return FALSE;
> +	}
> +
> +	//fprintf( stderr, "opening socket\n");
> +	if ( connect( s, (struct sockaddr *)&addr, sizeof( addr)) != 0)
> +	{
> +		close( s);
> +		return FALSE;
> +	}
> +
> +	//fprintf( stderr, "opened socket\n");
> +
> +	realm_str = apr_pstrmemdup( pool, realmstring, strlen( realmstring));
> +	user_str = apr_pstrmemdup( pool, username, strlen( username));
> +
> +	cache_id = apr_pstrmemdup( pool, realm_str, strlen( realmstring));
> +	cache_id = apr_pstrcat( pool, cache_id, user_str, NULL);
> +
> +	//fprintf( stderr, "fixing strings\n");
> +

Why do strings need to be fixed?  Something to do with the GPG
protocol?

> +	p = realm_str;
> +	while ( *p)
> +	{
> +		if ( *p == ' ')
> +		{
> +			*p = '+';
> +		}
> +		p++;
> +	}
> +
> +	//fprintf( stderr, "fixing strings\n");
> +	p = user_str;
> +	while ( *p)
> +	{
> +		if ( *p == ' ')
> +		{
> +			*p = '+';
> +		}
> +		p++;
> +	}
> +
> +	buffer = apr_palloc( pool, buffer_size);
> +
> +	//fprintf( stderr, "receiving from socket\n");
> +	recvd = recv( s, buffer, buffer_size-1, 0);
> +	buffer[recvd] = '\0';
> +	//fprintf( stderr, "received '%s'\n", &buffer[0]);
> +	if ( strncmp( buffer, "OK", 2) != 0)
> +	{
> +		return FALSE;
> +	}
> +
> +	request_len = snprintf( request, 0, "OPTION ttyname=%s\n", ttyname);
> +	request = apr_palloc( pool, request_len + 1);
> +	snprintf( request, request_len + 1, "OPTION ttyname=%s\n", ttyname);
> +	request[request_len] = '\0';

Use one of the apr functions, apr_psprintf perhaps.

> +
> +	//fprintf( stderr, "sending '%s', len = %d\n", request, request_len);
> +	send( s, request, request_len, 0);
> +
> +	recvd = recv( s, buffer, buffer_size - 1, 0);
> +	buffer[recvd] = '\0';
> +
> +	if ( strncmp( &buffer[0], "OK", 2) != 0)
> +	{
> +		return FALSE;
> +	}
> +
> +	request_len = snprintf( request, 0, "OPTION ttytype=%s\n", ttytype);
> +	request = apr_palloc( pool, request_len + 1);
> +	snprintf( request, request_len + 1, "OPTION ttytype=%s\n", ttytype);
> +	request[request_len] = '\0';
> +
> +	//fprintf( stderr, "sending '%s', len = %d\n", request, request_len);
> +	send( s, request, request_len, 0);
> +
> +	recvd = recv( s, buffer, buffer_size - 1, 0);
> +	buffer[recvd] = '\0';
> +
> +	if ( strncmp( &buffer[0], "OK", 2) != 0)
> +	{
> +		return FALSE;
> +	}
> +
> +	digest = svn_checksum_create( svn_checksum_md5, pool);
> +
> +	svn_checksum( &digest, svn_checksum_md5, cache_id, strlen(cache_id), pool);
> +	cache_id = svn_checksum_to_cstring( digest, pool);
> +
> +	if ( non_interactive)
> +	{
> +		request_len = snprintf( request, 0, "GET_PASSPHRASE --data --no-ask %s X Password: Subversion+needs+a+password+for+user+%s+to+access+repository+%s\n", cache_id, user_str, realm_str);

Please split long lines.

> +		if ( request_len > 0)
> +		{
> +			request = (char *)apr_palloc( pool, request_len + 5);
> +		}
> +		request_len = snprintf( request, request_len + 5, "GET_PASSPHRASE --data --no-ask %s X Password: Subversion+needs+a+password+for+user+%s+to+access+repository+%s\n", cache_id, user_str, realm_str);
> +	}
> +	else
> +	{
> +		request_len = snprintf( request, 0, "GET_PASSPHRASE --data %s X Password: Subversion+needs+a+password+for+user+%s+to+access+repository+%s\n", cache_id, user_str, realm_str);
> +		if ( request_len > 0)
> +		{
> +			request = (char *)apr_palloc( pool, request_len + 5);
> +		}
> +		request_len = snprintf( request, request_len + 5, "GET_PASSPHRASE --data %s X Password: Subversion+needs+a+password+for+user+%s+to+access+repository+%s\n", cache_id, user_str, realm_str);
> +	}
> +
> +
> +	send( s, request, request_len, 0);
> +
> +	recvd = recv( s, buffer, buffer_size - 1, 0);
> +	buffer[recvd] = '\0';
> +
> +	tot_recvd = 0;
> +	while ( recvd > 0)
> +	{
> +		// if it's the beginning of the response
> +		if ( tot_recvd == 0)
> +		{
> +			if ( strncmp( buffer, "ERR", 3) == 0)
> +			{
> +				return FALSE;
> +			}
> +
> +			if ( strncmp( buffer, "D", 1) == 0)
> +			{
> +				p = &buffer[2];
> +			}
> +		}
> +		else
> +		{
> +			p = &buffer[0];
> +		}
> +
> +
> +		ep = strchr( p, '\n');
> +		if ( ep != NULL)
> +		{
> +			*ep = '\0';
> +		}
> +		
> +		if ( tot_recvd == 0)
> +		{
> +			*password = apr_pstrmemdup( pool, p, recvd);
> +		}
> +		else
> +		{
> +			*password = apr_pstrcat( pool, *password, p, NULL);
> +		}
> +
> +		tot_recvd += recvd;
> +
> +		// keep going until we find the newline
> +		if ( ep == NULL)
> +		{
> +			recvd = recv( s, buffer, buffer_size - 1, 0);
> +			buffer[recvd] = '\0';
> +		}
> +		else
> +		{
> +			recvd = 0;
> +		}
> +	}
> +
> +	return TRUE;
> +}
> +
> +/* Implementation of password_set_t that stores the password in
> +   GPG Agent. */
> +static svn_boolean_t
> +password_set_gpg_agent(apr_hash_t *creds,
> +                           const char *realmstring,
> +                           const char *username,
> +                           const char *password,
> +                           apr_hash_t *parameters,
> +                           svn_boolean_t non_interactive,
> +                           apr_pool_t *pool)
> +{
> +	// just return true
> +	return TRUE;

Avoid comments that merely duplicate the code.  Your remark in the
email about not knowing whether it was neccessary would be better.

> +}
> +
> +
> +

-- 
Philip

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2424554

Please start new threads on the <[email protected]> mailing list.
To subscribe to the new list, send an empty e-mail to <[email protected]>.