Re: svn commit: r30800 - in branches/dont-save-plaintext-passwords-by-default: . subversion/include subversion/libsvn_subr

On Sun, Apr 27, 2008 at 07:04:22AM -0700, stsp_at_tigris.org wrote:
> Author: stsp
> Date: Sun Apr 27 07:04:21 2008
> New Revision: 30800
>
> Log:
> On the dont-save-plaintext-passwords-by-default branch, fix a crash.
> Also, simplify the implementation of svn_auth_plaintext_prompt_func_t
> callbacks by moving user answer caching into the function calling the
> callback.
 
 
> * subversion/libsvn_subr/auth.c
> (svn_auth_save_credentials): Pass a pool to save_credentials that
> survives across RA sessions. This is what fixes the crash.

I'm not really happy with this, but I found no better way to do it.
This is a hack to deal with a rare corner case.

Pools are reused per RA session, and all the RA callbacks are called
per RA session. This means that when users pass the same URL twice,
like this:

        svn ls svn://example.com svn://example.com

svn_auth_save_credentials is called twice, because two RA sessions
are opened. Since this function may end up prompting the user about
storing plaintext passwords, we can either have the user answer the same
prompt twice (quite silly), or expand the life time of the pool passed
to svn_auth_save_credentials so we can reuse the answer given during the
first RA session for the second one. Not passing a pool with extended
lifetime and naively trying to use it across RA sessions anyway is what
was causing the crash mentioned in the log message.

Note that for different URLs, the user will be prompted twice, but
the prompt now indicates the authentication realm in question.
So this is not an issue.

I toyed with the idea of storing the answer in the provider parameters
hash, but it's clearly not meant to be used for that. I need to key
user answer by realmstring, so I cannot define an appropriate
SVN_AUTH_PARAM constant. I didn't want to start using the realmstring
as a key into the parameters hash, in case some day we may want to do
that for some other reason than caching plaintext password prompt answers.
Building some key based on the realmstring would require a pool with
sufficient life expectancy to store it in -- which brings us back to
square one :)

Another idea was a new member inside the auth_baton which already
contains a few other things that live across RA sessions. This would
have forced me to change yet another bit of public API, cause I would
have ended up having to pass the answer cache as an extra parameter to the
svn_auth_save_credentials callback. Since this is a really rare use case,
I really didn't want to end up revving public API just for this. Also,
I wanted a solution that is transparent to clients using any API
revision.

So I sneaked in this bit:

> Modified: branches/dont-save-plaintext-passwords-by-default/subversion/libsvn_subr/auth.c
> URL: http://svn.collab.net/viewvc/svn/branches/dont-save-plaintext-passwords-by-default/subversion/libsvn_subr/auth.c?pathrev=30800&r1=30799&r2=30800
> ==============================================================================
> --- branches/dont-save-plaintext-passwords-by-default/subversion/libsvn_subr/auth.c Sat Apr 26 14:25:28 2008 (r30799)
> +++ branches/dont-save-plaintext-passwords-by-default/subversion/libsvn_subr/auth.c Sun Apr 27 07:04:21 2008 (r30800)
> @@ -323,7 +323,10 @@ svn_auth_save_credentials(svn_auth_iters
> provider->provider_baton,
> auth_baton->parameters,
> state->realmstring,
> - pool));
> + /* The pool passed here
> + * must survive across
> + * RA sessions. */
> + auth_baton->pool));
> if (save_succeeded)
> return SVN_NO_ERROR;
>
> @@ -337,8 +340,11 @@ svn_auth_save_credentials(svn_auth_iters
> if (provider->vtable->save_credentials)
> SVN_ERR(provider->vtable->save_credentials
> (&save_succeeded, creds,
> - provider->provider_baton, auth_baton->parameters,
> - state->realmstring, pool));
> + provider->provider_baton,
> + auth_baton->parameters,
> + state->realmstring,
> + /* The pool passed here must survive across RA sessions. */
> + auth_baton->pool));
>
> if (save_succeeded)
> break;
>

I hope this is acceptable?

Thanks,

-- 
Stefan Sperling <stsp_at_elego.de>                    Software Monkey
 
German law requires the following banner :(
elego Software Solutions GmbH                            HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12        Tel:  +49 30 23 45 86 96 
13355 Berlin                              Fax:  +49 30 23 45 86 95
http://www.elego.de                               CEO: Olaf Wagner
 
Store password unencrypted (yes/no)? No