Since 1.8 we've supported using gpg-agent to store passwords in memory.
http://subversion.apache.org/docs/release-notes/1.8.html#gpg-agent
Today I was getting asked about some odd behavior that a customer was seeing so
I took some time to investigate.
The behavior of this setup is very wonky and not what a user would expect.
When gpg-agent is setup the first time you connect to a realm you'll be
prompted by the Subversion client for the password. It'll then store in our
auth cache that the password is stored in gpg-agent. However, the agent
doesn't get this password from this action. The next time the user connects to
this realm they are prompted by the pinentry program that is configured with
gpg-agent to get the password. After that there is a confirmation prompt
requiring they re-enter the password. Subsequent connections to the realm then
use the password if it is still cached in gpg-agent. If the agent's cache has
been cleared (timeout or other actions that would clear it) then the pinentry
prompt (confirmation entry included) is repeated.
This seems like a very poor implementation from an end user perspective. None
of the above behavior is documented in our release notes. So the average user
is probably going to believe that things are not working properly. There are
of course some very good reasons for this behavior.
gpg-agent and Subversion's auth system has somewhat contradictory designs.
gpg-agent takes on the responsibility of not just storing the password but also
of the user interface for the user to input the password if it is not stored.
Subversion on the other hand presumes that the cache systems are just dumb
storage and that it has to handle the UI. Subversion independently stores that
it has a password cached in a given cache system (in a file under
~/.subversion/auth/svn.simple with the file being the MD5 hash of the realm).
It only contacts that system for the cached password if the appropriate
svn.simple entry is present.
With every other Subversion password caching mechanism Subversion would prompt
you for your password the first time you connected to the realm and then if the
password is correct it would store it in the appropriate cache and create the
svn.simple entry. However, with gpg-agent it does not store the password with
that first use, but does store the svn.simple entry causing subsequent requests
to go to gpg-agent. Since gpg-agent does not have the password cached it then
prompts the user.
Subversion currently tells gpg-agent when prompting the user for a password to
confirm the password by asking the user to re-enter it. This choice appears to
have been made because gpg-agent will cache a password without knowing for sure
that the password is correct. If an incorrect password were to be cached then
the user would have to take steps to clear the gpg-agent cache on their own
(kill -HUP, gpg-connect-agent CLEAR_PASSPHARSE). Until they did so they would
be prompted for the password by Subversion (since the cached password failed)
on every single connection. Since the cached password in gpg-agent would be
tried on every connection, the user may run into systems that lock them out as
well. The choice of a confirmation entry doesn't really seem very good in my
opinion. First, it only protects against typos, a user entering the wrong
password (though entering the same password) twice will still experience the
problem described above. Second, the default timeout for passwords in
gpg-agent is 10 minutes. So now a user using this setup will have to enter
their password twice every 10 minutes. The caching feature is a convenience
feature, but having to enter the password twice seems not very convenient.
Fortunately there are some thing we can do to resolve these issues. First
let's talk about the double prompt from pinentry. This is entirely
unnecessary. Subversion's API provides two functions to retrieve credentials
from the cache. A first credential function and a next credential function.
The purpose being to allow multiple credentials to be stored for a realm and
then walked until you find the ones that work. In practice we've never used
this for passwords because there's no point in storing more than one credential
for a password since only one will work. However, we can use this to allow us
to drive gpg-agent as it intends. We do a GET_PASSPHRASE in first and then if
we receive a next call we do a CLEAR_PASSPHRASE followed by a GET_PASSPHRASE
with an error text telling the user the authentication failed (if we're being
run in interactive mode). I've attached a patch that implements this bit. But
I haven't committed it because I'm not sure this is necessarily the best
solution. I'm also abusing the parameters hash to avoid duplicating the
svn_auth__simple_creds_cache_get() function.
The failure to cache on the first connection to the realm issue is a little bit
harder to solve. There is actually a PRESET_PASSPHARSE call in gpg-agent's
API. But it only works when gpg-agent is started with
--allow-preset-passphrase. I think we should make the save function of the
gpg-agent provider implement the PRESET_PASSPHRASE call. We can document to
users they will have a better experience if they provide the
--allow-preset-passphrase option to gpg-agent when they start it. We can
ignore errors if it doesn't.
There is another option and that is to use gpg-agent as a dumb store like we do
other caches by combining PRESET_PASSPHRASE in the save function and
GET_PASSPHARSE --no-ask in the first function. This would allow us to retain
the behavior that the svn client asks for the password and thus not have to
jump through hoops to support the cache system being responsible for the UI to
request the password. Doing this of course would rquire that
--allow-preset-passphrase be passed. So I think it'd probably be best to have
a setting in our Subversion config that enables this mode but that then fails
if --allow-preset-passphrase is not enabled on gpg-agent. This behavior would
give the best experience to our users, but since it's intrusive on gpg-agent's
configuration I don't think it should be default.
Because of these things I believe the patch should be applied and the later two
things should be iterated on top of that.
Thoughts?
Commit message for the patch:
[[[
Make the gpg-agent pinentry not ask for confirmation of password entries and
make it re-prompt if the password is incorrect.
* subversion/libsvn_subr/gpg_agent.c:
(ATTEMPT_PARAMETER): New macro.
(send_options, get_cache_id): New functions with code taken out of
password_get_gpg_agent() so it can be reused.
(password_get_gpg_agent): Use send_options() and get_cache_id(),
retrieve the attempt from the parameters and use it to determine
if we should set an error message that will be displayed in pinentry.
(simple_gpg_agent_first_creds): Set a iter_baton so we can limit the
retries, put the iter_baton in the parameters so password_get_gpg_agent()
can access it.
(simple_gpg_agent_next_creds): New function, removes the cached password
and prompts the user again.
(gpg_agent_simple_provider): Add simple_gpg_agent_next_creds callback.
]]]
Index: subversion/libsvn_subr/gpg_agent.c
===================================================================
--- subversion/libsvn_subr/gpg_agent.c (revision 1599379)
+++ subversion/libsvn_subr/gpg_agent.c (working copy)
@@ -72,6 +72,7 @@
#include "svn_cmdline.h"
#include "svn_checksum.h"
#include "svn_string.h"
+#include "svn_hash.h"
#include "auth.h"
#include "private/svn_auth_private.h"
@@ -81,6 +82,7 @@
#ifdef SVN_HAVE_GPG_AGENT
#define BUFFER_SIZE 1024
+#define ATTEMPT_PARAMETER "svn.simple.gpg_agent.attempt"
/* Modify STR in-place such that blanks are escaped as required by the
* gpg-agent protocol. Return a pointer to STR. */
@@ -99,6 +101,24 @@ escape_blanks(char *str)
return str;
}
+/* Generate the string CACHE_ID_P based on the REALMSTRING allocated in
+ * RESULT_POOL using SCRATCH_POOL for temporary allocations. This is similar
+ * to other password caching mechanisms. */
+static svn_error_t *
+get_cache_id(const char **cache_id_p, const char *realmstring,
+ apr_pool_t *scratch_pool, apr_pool_t *result_pool)
+{
+ const char *cache_id = NULL;
+ svn_checksum_t *digest = NULL;
+
+ SVN_ERR(svn_checksum(&digest, svn_checksum_md5, realmstring,
+ strlen(realmstring), scratch_pool));
+ cache_id = svn_checksum_to_cstring(digest, result_pool);
+ *cache_id_p = cache_id;
+
+ return SVN_NO_ERROR;
+}
+
/* Attempt to read a gpg-agent response message from the socket SD into
* buffer BUF. Buf is assumed to be N bytes large. Return TRUE if a response
* message could be read that fits into the buffer. Else return FALSE.
@@ -266,49 +286,20 @@ find_running_gpg_agent(int *new_sd, apr_pool_t *po
return SVN_NO_ERROR;
}
-/* Implementation of svn_auth__password_get_t that retrieves the password
- from gpg-agent */
-static svn_error_t *
-password_get_gpg_agent(svn_boolean_t *done,
- 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)
+static svn_boolean_t
+send_options(int sd, char *buf, size_t n, apr_pool_t *scratch_pool)
{
- int sd;
- const char *p = NULL;
- char *ep = NULL;
- char *buffer;
- const char *request = NULL;
- const char *cache_id = NULL;
const char *tty_name;
const char *tty_type;
const char *lc_ctype;
const char *display;
- svn_checksum_t *digest = NULL;
- char *password_prompt;
- char *realm_prompt;
- *done = FALSE;
-
- SVN_ERR(find_running_gpg_agent(&sd, pool));
- if (sd == -1)
- return SVN_NO_ERROR;
-
- buffer = apr_palloc(pool, BUFFER_SIZE);
-
/* Send TTY_NAME to the gpg-agent daemon. */
tty_name = getenv("GPG_TTY");
if (tty_name != NULL)
{
- if (!send_option(sd, buffer, BUFFER_SIZE, "ttyname", tty_name, pool))
- {
- close(sd);
- return SVN_NO_ERROR;
- }
+ if (!send_option(sd, buf, n, "ttyname", tty_name, scratch_pool))
+ return FALSE;
}
/* Send TTY_TYPE to the gpg-agent daemon. */
@@ -315,11 +306,8 @@ find_running_gpg_agent(int *new_sd, apr_pool_t *po
tty_type = getenv("TERM");
if (tty_type != NULL)
{
- if (!send_option(sd, buffer, BUFFER_SIZE, "ttytype", tty_type, pool))
- {
- close(sd);
- return SVN_NO_ERROR;
- }
+ if (!send_option(sd, buf, n, "ttytype", tty_type, scratch_pool))
+ return FALSE;
}
/* Compute LC_CTYPE. */
@@ -332,11 +320,8 @@ find_running_gpg_agent(int *new_sd, apr_pool_t *po
/* Send LC_CTYPE to the gpg-agent daemon. */
if (lc_ctype != NULL)
{
- if (!send_option(sd, buffer, BUFFER_SIZE, "lc-ctype", lc_ctype, pool))
- {
- close(sd);
- return SVN_NO_ERROR;
- }
+ if (!send_option(sd, buf, n, "lc-ctype", lc_ctype, scratch_pool))
+ return FALSE;
}
/* Send DISPLAY to the gpg-agent daemon. */
@@ -343,27 +328,69 @@ find_running_gpg_agent(int *new_sd, apr_pool_t *po
display = getenv("DISPLAY");
if (display != NULL)
{
- if (!send_option(sd, buffer, BUFFER_SIZE, "display", display, pool))
- {
- close(sd);
- return SVN_NO_ERROR;
- }
+ if (!send_option(sd, buf, n, "display", display, scratch_pool))
+ return FALSE;
}
- /* Create the CACHE_ID which will be generated based on REALMSTRING similar
- to other password caching mechanisms. */
- SVN_ERR(svn_checksum(&digest, svn_checksum_md5, realmstring,
- strlen(realmstring), pool));
- cache_id = svn_checksum_to_cstring(digest, pool);
+ return TRUE;
+}
+/* Implementation of svn_auth__password_get_t that retrieves the password
+ from gpg-agent */
+static svn_error_t *
+password_get_gpg_agent(svn_boolean_t *done,
+ 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;
+ const char *p = NULL;
+ char *ep = NULL;
+ char *buffer;
+ const char *request = NULL;
+ const char *cache_id = NULL;
+ char *password_prompt;
+ char *realm_prompt;
+ char *error_prompt;
+ int *attempt;
+
+ *done = FALSE;
+
+ attempt = svn_hash_gets(parameters, ATTEMPT_PARAMETER);
+
+ SVN_ERR(find_running_gpg_agent(&sd, pool));
+ if (sd == -1)
+ return SVN_NO_ERROR;
+
+ buffer = apr_palloc(pool, BUFFER_SIZE);
+
+ if (!send_options(sd, buffer, BUFFER_SIZE, pool))
+ {
+ close(sd);
+ return SVN_NO_ERROR;
+ }
+
+ SVN_ERR(get_cache_id(&cache_id, realmstring, pool, pool));
+
password_prompt = apr_psprintf(pool, _("Password for '%s': "), username);
realm_prompt = apr_psprintf(pool, _("Enter your Subversion password for %s"),
realmstring);
+ if (*attempt == 1)
+ /* X means no error to the gpg-agent protocol */
+ error_prompt = apr_pstrdup(pool, "X");
+ else
+ error_prompt = apr_pstrdup(pool, _("Authentication failed"));
+
request = apr_psprintf(pool,
- "GET_PASSPHRASE --data %s--repeat=1 "
- "%s X %s %s\n",
+ "GET_PASSPHRASE --data %s"
+ "%s %s %s %s\n",
non_interactive ? "--no-ask " : "",
cache_id,
+ escape_blanks(error_prompt),
escape_blanks(password_prompt),
escape_blanks(realm_prompt));
@@ -441,14 +468,111 @@ simple_gpg_agent_first_creds(void **credentials,
const char *realmstring,
apr_pool_t *pool)
{
- return svn_auth__simple_creds_cache_get(credentials, iter_baton,
- provider_baton, parameters,
- realmstring, password_get_gpg_agent,
- SVN_AUTH__GPG_AGENT_PASSWORD_TYPE,
- pool);
+ svn_error_t *err;
+ int *attempt = apr_palloc(pool, sizeof(*attempt));
+
+ *attempt = 1;
+ svn_hash_sets(parameters, ATTEMPT_PARAMETER, attempt);
+ err = svn_auth__simple_creds_cache_get(credentials, iter_baton,
+ provider_baton, parameters,
+ realmstring, password_get_gpg_agent,
+ SVN_AUTH__GPG_AGENT_PASSWORD_TYPE,
+ pool);
+ *iter_baton = attempt;
+
+ return err;
}
+/* An implementation of svn_auth_provider_t::next_credentials() */
+static svn_error_t *
+simple_gpg_agent_next_creds(void **credentials,
+ void *iter_baton,
+ void *provider_baton,
+ apr_hash_t *parameters,
+ const char *realmstring,
+ apr_pool_t *pool)
+{
+ int *attempt = (int *)iter_baton;
+ int sd;
+ char *buffer;
+ const char *cache_id = NULL;
+ const char *request = NULL;
+ *credentials = NULL;
+
+ /* The users previous credentials failed so first remove the cached entry,
+ * before trying to retrieve them again. Because gpg-agent stores cached
+ * credentials immediately upon retrieving them, this gives us the
+ * opportunity to prompt remove the invalid credentials and prompt the
+ * user again. While it's possible that server side issues could trigger
+ * this, this cache is emphermial so at worst we're just speeding up
+ * when the user would need to reenter their password. */
+
+ if (svn_hash_gets(parameters, SVN_AUTH_PARAM_NON_INTERACTIVE))
+ {
+ /* In this case since we're running non-interactively we do not
+ * want to clear the cache since the user was never prompted by
+ * gpg-agent to set a password. */
+ return SVN_NO_ERROR;
+ }
+
+ *attempt = *attempt + 1;
+
+ SVN_ERR(find_running_gpg_agent(&sd, pool));
+ if (sd == -1)
+ return SVN_NO_ERROR;
+
+ buffer = apr_palloc(pool, BUFFER_SIZE);
+
+ if (!send_options(sd, buffer, BUFFER_SIZE, pool))
+ {
+ close(sd);
+ return SVN_NO_ERROR;
+ }
+
+ SVN_ERR(get_cache_id(&cache_id, realmstring, pool, pool));
+
+ request = apr_psprintf(pool, "CLEAR_PASSPHRASE %s\n", cache_id);
+
+ if (write(sd, request, strlen(request)) == -1)
+ {
+ close(sd);
+ return SVN_NO_ERROR;
+ }
+
+ if (!receive_from_gpg_agent(sd, buffer, BUFFER_SIZE))
+ {
+ close(sd);
+ return SVN_NO_ERROR;
+ }
+
+ if (strncmp(buffer, "OK\n", 3) != 0)
+ {
+ close(sd);
+ return SVN_NO_ERROR;
+ }
+
+ /* TODO: This attempt limit hard codes it at 3 attempts (or 2 retries)
+ * which matches that svn command line client's retry_limit as set in
+ * svn_cmdline_create_auth_baton(). It would be nice to have that
+ * limit reflected here but that violates the boundry between the
+ * prompt provider and the cache provider. gpg-agent is acting as
+ * both here due to the peculiarties of their design so we'll have to
+ * live with this for now. Note that when these failures get exceeded
+ * it'll eventually fall back on the retry limits of whatever prompt
+ * provider is in effect, so this effectively doubles the limit. */
+ if (*attempt < 4)
+ return svn_auth__simple_creds_cache_get(credentials, &iter_baton,
+ provider_baton, parameters,
+ realmstring,
+ password_get_gpg_agent,
+ SVN_AUTH__GPG_AGENT_PASSWORD_TYPE,
+ pool);
+
+ return SVN_NO_ERROR;
+}
+
+
/* An implementation of svn_auth_provider_t::save_credentials() */
static svn_error_t *
simple_gpg_agent_save_creds(svn_boolean_t *saved,
@@ -469,7 +593,7 @@ simple_gpg_agent_save_creds(svn_boolean_t *saved,
static const svn_auth_provider_t gpg_agent_simple_provider = {
SVN_AUTH_CRED_SIMPLE,
simple_gpg_agent_first_creds,
- NULL,
+ simple_gpg_agent_next_creds,
simple_gpg_agent_save_creds
};
