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

Re: Subversion sometimes needlessly asks for confirmation to store already stored plaintext passwords

From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Fri, 18 Jul 2008 19:11:08 +0300 (Jerusalem Daylight Time)

Stefan Sperling wrote on Fri, 18 Jul 2008 at 17:37 +0200:
> On Fri, Jul 18, 2008 at 05:20:59PM +0300, Daniel Shahaf wrote:
> > Stefan Sperling wrote on Fri, 18 Jul 2008 at 15:20 +0200:
> > > [[[
> > > Fix unnecessary plaintext password saving prompt when the username
> > > is supplied on the command line and the password is already cached.
> > >
> > > * subversion/libsvn_subr/simple_providers.c
> > > (svn_auth__simple_first_creds_helper): Start out with may_save = FALSE.
> > > The old code did set creds->may_save to TRUE whenever a username
> > > was supplied on the command line, regardless of whether a password
> > > was already cached or not. This unconditionally triggered the whole
> > > auth caching logic (including the 'may I save your password in
> > > plaintext?' prompt) to be run again, even if a password was already
> > > cached.
> > >
> > > Patch by: stylesen
> > > Found by: arfrever
> > > Approved by: stsp
> > > ]]]
> > >
> > > Please commit if you agree with my log message.
> > >
> >
> > After this patch, svn_auth__simple_first_creds_helper() will never set
> > 'may_save' to TRUE except when the auth cache exists and is of a older
> > format that contains a password but not a passtype:
> >
> > /* If the auth data didn't contain a password type,
> > force a write to upgrade the format of the auth
> > data file. */
> > if (password && passtype && !have_passtype)
> > may_save = TRUE;
> >
> > Don't we need to set may_save to 'TRUE' in some cases as well?
> >
> > Daniel
> > (who didn't reply in the morning, because he thinks he's missing
> > something obvious, again)
>
> No, we don't need to set may_save to TRUE in any other case.
>
> All that svn_auth__simple_first_creds_helper() (part of the
> "simple file" provider) does is look whether there are some credentials
> cached already. If there are any, there's not much point in caching
> them again, right? So creds->may_save can be as FALSE as it wants to be.
>
> If the creds don't authenticate the next provider will be tried
> anyway, because the simple file provider has no 'next_credentials'
> method (see the definition of svn_auth_provider_t in svn_auth.h
> if you don't know the difference between 'first_credentials' and
> 'next_credentials').
>
> If svn_auth__simple_first_creds_helper() cannot find any creds in the
> auth cache, it says "I have no credentials" (by setting *credentials
> to NULL) and, again, the next provider is tried.
>
> Among the next providers is likely to be the 'prompt provider', which
> prompts the user interactively and creates its own creds object.
>
> There are two invocations of the prompt provider in simple_providers.c,
> both of which use '! no_auth_cache' to set the 'may_save' parameter
> while prompting the user for a username and password. Both calls,
> if successful, create an entirely new credentials object:
>
> One is in simple_prompt_first_creds:
> SVN_ERR(prompt_for_simple_creds((svn_auth_cred_simple_t **) credentials_p,
> pb, parameters, realmstring, TRUE,
> ! no_auth_cache, pool));
>
> The other one is in simple_prompt_next_creds:
> SVN_ERR(prompt_for_simple_creds((svn_auth_cred_simple_t **) credentials_p,
> pb, parameters, realmstring, FALSE,
> ! no_auth_cache, pool));
>
> Here's the signature of prompt_for_simple_creds for reference:
>
> static svn_error_t *
> prompt_for_simple_creds(svn_auth_cred_simple_t **cred_p,
> simple_prompt_provider_baton_t *pb,
> apr_hash_t *parameters,
> const char *realmstring,
> svn_boolean_t first_time,
> svn_boolean_t may_save,
> apr_pool_t *pool)
>
> The may_save parameter is copied straight into the returned credentials
> objects.

Not always:

  if (def_username && def_password)
    {
      *cred_p = apr_palloc(pool, sizeof(**cred_p));
      (*cred_p)->username = apr_pstrdup(pool, def_username);
      (*cred_p)->password = apr_pstrdup(pool, def_password);
      (*cred_p)->may_save = TRUE;
    }

ignores the 'may_save' parameter. (Here, def_{username,password}
are the values of --username and --password.)

> So unless the user passes --no-auth-cache, the credentials
> returned from these calls have their may_save member set to TRUE,
> and they will be cached in the usual way.
>
> Did this relieve your concerns?
>

Yes, and thank you for the long explanation.

Daniel

> Stefan
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-07-18 18:11:35 CEST

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.