[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] gpg-agent support

From: Philip Martin <philip.martin_at_wandisco.com>
Date: Thu, 26 Nov 2009 09:17:25 +0000

Dan Engel <dan_at_sourceharvest.com> 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 <dev_at_subversion.apache.org> mailing list.
To subscribe to the new list, send an empty e-mail to <dev-subscribe_at_subversion.apache.org>.
Received on 2009-11-26 10:17:49 CET

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.