You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Karl Fogel <kf...@red-bean.com> on 2008/08/05 16:27:48 UTC
Re: Subversion sometimes needlessly asks for confirmation to store already stored plaintext passwords
Senthil Kumaran S <se...@collab.net> writes:
> I am attaching an updated patch along with this email which passes all the
> tests and also satisfies use cases 1), 3)a & 3)b as explained here
> http://svn.haxx.se/dev/archive-2008-07/0533.shtml
>
> Use case 2 is tested in basic_tests.py test number 46 ie., "basic auth test",
> which is the reverse of what I ve explained in the above thread.
>
> [[[
> Fix unnecessary plaintext password saving prompt when the username
> is supplied on the command line and the password is already cached.
What happens if all of the following are true:
- the user passes '--username=my_user_name'
- there is a cached password for my_user_name and this repository
- the cached password is incorrect (according to the repository)
- the user's ~/.subversion configuration says may_save = FALSE
? The correct behavior then would be to prompt the user for a new
username/password (after discovering that the old password doesn't work,
the client prompts for both username and password, I assume), use the
new creds for this operation, but *not* save them. I think. Does that
make sense to you?
Is that what would happen?
Some code comments below.
> * subversion/libsvn_subr/simple_providers.c
> (simple_username_get): New function to get username for simple provider.
> (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. Here we check for different combinations
> of username and password either supplied in the command line or
> already cached in the auth area, based on that we toggle creds->may_save.
>
> Patch by: stylesen
> Found by: arfrever
> ]]]
>
> --- subversion/libsvn_subr/simple_providers.c (revision 32316)
> +++ subversion/libsvn_subr/simple_providers.c (working copy)
> @@ -94,6 +94,23 @@
> return TRUE;
> }
>
> +/* Retrieves the username from CREDS. */
> +static svn_boolean_t
> +simple_username_get(const char **username,
> + apr_hash_t *creds,
> + const char *realmstring,
> + svn_boolean_t non_interactive)
Doc string doesn't mention the other parameters, but also doesn't say
this is an implementation of some pre-existing function type. Should do
one or the other...
> +{
> + svn_string_t *str;
> + str = apr_hash_get(creds, AUTHN_USERNAME_KEY, APR_HASH_KEY_STRING);
> + if (str && str->data)
> + {
> + *username = str->data;
> + return TRUE;
> + }
> + return FALSE;
> +}
Is there any need to mention lifetime issues with the returned data, or
are there conventions already in place about the 'creds' hash and data
retrieved from it that would be obvious to anyone reading this code in
context?
(As I look through simple_providers.c, I'm unclear on this point...)
> /* Common implementation for simple_first_creds and
> windows_simple_first_creds. Uses PARAMETERS, REALMSTRING and the
> simple auth provider's username and password cache to fill a set of
> @@ -122,45 +139,85 @@
> svn_boolean_t non_interactive = apr_hash_get(parameters,
> SVN_AUTH_PARAM_NON_INTERACTIVE,
> APR_HASH_KEY_STRING) != NULL;
> -
> - svn_boolean_t may_save = username || password;
> + const char *def_username = NULL;
> + const char *def_password = NULL;
> + svn_boolean_t may_save = FALSE;
> + apr_hash_t *creds_hash = NULL;
> svn_error_t *err;
> + svn_string_t *str;
> + svn_boolean_t have_passtype = FALSE;
>
> - /* If we don't have a username and a password yet, we try the auth cache */
> - if (! (username && password))
> + /* Try to load credentials from a file on disk, based on the
> + realmstring. Don't throw an error, though: if something went
> + wrong reading the file, no big deal. What really matters is that
> + we failed to get the creds, so allow the auth system to try the
> + next provider. */
> + err = svn_config_read_auth_data(&creds_hash, SVN_AUTH_CRED_SIMPLE,
> + realmstring, config_dir, pool);
> + svn_error_clear(err);
> +
> + /* We have something in the auth cache for this realm. */
> + if (! err && creds_hash)
> {
> - apr_hash_t *creds_hash = NULL;
> + /* The password type in the auth data must match the
> + mangler's type, otherwise the password must be
> + interpreted by another provider. */
> + str = apr_hash_get(creds_hash, AUTHN_PASSTYPE_KEY, APR_HASH_KEY_STRING);
> + if (str && str->data)
> + if (passtype && (0 == strcmp(str->data, passtype)))
> + have_passtype = TRUE;
>
> - /* Try to load credentials from a file on disk, based on the
> - realmstring. Don't throw an error, though: if something went
> - wrong reading the file, no big deal. What really matters is that
> - we failed to get the creds, so allow the auth system to try the
> - next provider. */
> - err = svn_config_read_auth_data(&creds_hash, SVN_AUTH_CRED_SIMPLE,
> - realmstring, config_dir, pool);
> - svn_error_clear(err);
> - if (! err && creds_hash)
> + /* See if we need to save this username if it is not present in
> + auth cache. */
> + if (username)
> {
> - svn_string_t *str;
> - if (! username)
> + if (!simple_username_get(&def_username, creds_hash, realmstring,
> + non_interactive))
> {
> - str = apr_hash_get(creds_hash, AUTHN_USERNAME_KEY,
> - APR_HASH_KEY_STRING);
> - if (str && str->data)
> - username = str->data;
> + may_save = TRUE;
> }
> + else
> + {
> + if (0 == strcmp(def_username, username))
> + may_save = FALSE;
> + else
> + may_save = TRUE;
> + }
> + }
I don't understand the logic of these different settings of 'may_save'.
Is the 'may_save' variable supposed to represent what the configuration
says about the question of whether or not we can save creds? Or does it
represent the question of whether or not we got different data from what
was in the cache -- in other words, it's really "need_to_save"?
If the variable only represents one thing, then maybe it's just that its
name needs to be changed. But if it represents several different
questions, it might be good to have several different variables, so the
readers can understand what questions are being considered...
> + /* See if we need to save this password if it is not present in
> + auth cache. */
This comment indicates it's about "need_to_save" not "may_save"...
The rest of the change I found hard to understand, but I think my
confusion all stems from one basic source: the difference between "may
save" and "need to save (if saving is allowed)". One is about what the
user has given Subvesrion permission to do; the other is about what
Subversion *should* do, assuming it has the necessary permissions from
the user at all.
-Karl
> + if (password)
> + {
> + if (have_passtype)
> + {
> + if (!password_get(&def_password, creds_hash, realmstring,
> + username, non_interactive, pool))
> + {
> + may_save = TRUE;
> + }
> + else
> + {
> + if (0 == strcmp(def_password, password))
> + may_save = FALSE;
> + else
> + may_save = TRUE;
> + }
> + }
> + }
> +
> + /* If we don't have a username and a password yet, we try the
> + auth cache */
> + if (! (username && password))
> + {
> + if (! username)
> + if (!simple_username_get(&username, creds_hash, realmstring,
> + non_interactive))
> + username = NULL;
> +
> if (username && ! password)
> {
> - svn_boolean_t have_passtype;
> - /* The password type in the auth data must match the
> - mangler's type, otherwise the password must be
> - interpreted by another provider. */
> - str = apr_hash_get(creds_hash, AUTHN_PASSTYPE_KEY,
> - APR_HASH_KEY_STRING);
> - have_passtype = (str && str->data);
> - if (have_passtype && passtype
> - && 0 != strcmp(str->data, passtype))
> + if (! have_passtype)
> password = NULL;
> else
> {
> @@ -171,12 +228,18 @@
> /* 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)
> + if (password && ! have_passtype)
> may_save = TRUE;
> }
> }
> }
> }
> + else
> + {
> + /* Nothing was present in the auth cache, so save the credentials if
> + we are asked to do so. */
> + may_save = TRUE;
> + }
>
> /* Ask the OS for the username if we have a password but no
> username. */
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Subversion sometimes needlessly asks for confirmation to store
already stored plaintext passwords
Posted by Senthil Kumaran S <se...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi,
I would like to get some review on this patch, so that it will get into trunk
(this does not have an issue in our issue tracker, fyi).
Thank You.
Senthil Kumaran S wrote:
> Hi Karl,
>
> Karl Fogel wrote:
>> What happens if all of the following are true:
>
>> - the user passes '--username=my_user_name'
>> - there is a cached password for my_user_name and this repository
>> - the cached password is incorrect (according to the repository)
>> - the user's ~/.subversion configuration says may_save = FALSE
>
>> ? The correct behavior then would be to prompt the user for a new
>> username/password (after discovering that the old password doesn't work,
>> the client prompts for both username and password, I assume), use the
>> new creds for this operation, but *not* save them. I think. Does that
>> make sense to you?
>
> If there is a change in credentials then the changed creds will be cached in
> the auth area afresh. This is what happens now. This was the behavior
> previously in older versions, so I didn't want to break it, because it breaks
> tests.
>
>>> +/* Retrieves the username from CREDS. */
>>> +static svn_boolean_t
>>> +simple_username_get(const char **username,
>>> + apr_hash_t *creds,
>>> + const char *realmstring,
>>> + svn_boolean_t non_interactive)
>> Doc string doesn't mention the other parameters, but also doesn't say
>> this is an implementation of some pre-existing function type. Should do
>> one or the other...
>
> Done in the new patch.
>
>>> +{
>>> + svn_string_t *str;
>>> + str = apr_hash_get(creds, AUTHN_USERNAME_KEY, APR_HASH_KEY_STRING);
>>> + if (str && str->data)
>>> + {
>>> + *username = str->data;
>>> + return TRUE;
>>> + }
>>> + return FALSE;
>>> +}
>> Is there any need to mention lifetime issues with the returned data, or
>> are there conventions already in place about the 'creds' hash and data
>> retrieved from it that would be obvious to anyone reading this code in
>> context?
>
> Since the values from creds_hash is used temporarily there is no lifetime
> issues to mention here, IMHO. We use these values in order to set
> "svn_auth_cred_simple_t *creds" which must be persistent and it _is_ throughout
> the code.
>
>> I don't understand the logic of these different settings of 'may_save'.
>
>> Is the 'may_save' variable supposed to represent what the configuration
>> says about the question of whether or not we can save creds? Or does it
>> represent the question of whether or not we got different data from what
>> was in the cache -- in other words, it's really "need_to_save"?
>
> - From svn_auth_cred_simple_t doc string:
>
> <snip>
> /** Indicates if the credentials may be saved (to disk). For example, a
> * GUI prompt implementation with a remember password checkbox shall set
> * @a may_save to TRUE if the checkbox is checked.
> */
> svn_boolean_t may_save;
> </snip>
>
>> If the variable only represents one thing, then maybe it's just that its
>> name needs to be changed. But if it represents several different
>> questions, it might be good to have several different variables, so the
>> readers can understand what questions are being considered...
>
> This variable just addresses, whether we need to save the credentials supplied.
> So I am changing the variable name in the updated patch to need_to_save.
>
>>> + /* See if we need to save this password if it is not present in
>>> + auth cache. */
>> This comment indicates it's about "need_to_save" not "may_save"...
>
> In all the places this variable is used to predict whether we need to save the
> creds, so changing its name.
>
>> The rest of the change I found hard to understand, but I think my
>> confusion all stems from one basic source: the difference between "may
>> save" and "need to save (if saving is allowed)". One is about what the
>> user has given Subvesrion permission to do; the other is about what
>> Subversion *should* do, assuming it has the necessary permissions from
>> the user at all.
>
> I will try to summarize the change as follows:
>
> 1) Get the username and password supplied by the user in command line.
> 2) We check whether there is any creds cached in the auth area for this REALM.
> 3) If 2) is TRUE, then get the def_username from the auth cache, else we need
> to save whatever username we get from the following steps to the auth cache, if
> the user indicated us to save auth creds ie., by "store-auth-creds" in the
> servers file (but the storing part check is done in save_credentials function).
> What we really are bothered here is to toggle the creds->may_save, in order to
> make a decision whether we need to save these credentials or not.
> 4) If 3) gives a def_username, then compare this username with whatever
> username we have (if any) from 1). If they are same then we need not change the
> username creds in the auth cache, else we need to save the username creds in
> the auth cache.
> 5) If 2) is TRUE, then get the def_password from the auth cache, else we need
> to save whatever password we get from the following steps to the auth cache.
> 6) If 5) gives a def_password, then compare this password with whatever
> password we have (if any) from 1). If they are same then we need not change the
> password creds in the auth cache, else we need to save the password creds in
> the auth cache.
>
> Steps 1) to 6) above are checks made in order to see whether the creds supplied
> in command line matches with the stored creds in the auth cache, if not we will
> make the change in the auth cache.
>
> 7) Get the username and password from the auth cache, if it is not supplied in
> the command line. Here there is one check to need_to_save, if the passtype is
> not cached for these credentials.
> 8) If 2) fails then, there was nothing in the auth cache for this REALM, so we
> set need_to_save as TRUE, which will be handled by save_credentials related
> functions based on "store-auth-creds" value in servers file.
>
> The remaining part of the code ie., the following is obvious:
> <snip>
> /* Ask the OS for the username if we have a password but no
> username. */
> if (password && ! username)
> username = svn_user_get_name(pool);
>
> if (username && password)
> {
> svn_auth_cred_simple_t *creds = apr_pcalloc(pool, sizeof(*creds));
> creds->username = username;
> creds->password = password;
> creds->may_save = need_to_save;
> *credentials = creds;
> }
> else
> *credentials = NULL;
> </snip>
>
> [[[
> 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
> (simple_username_get): New function to get username for simple provider.
> (svn_auth__simple_first_creds_helper): Start out with need_to_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. Here we check for different combinations
> of username and password either supplied in the command line or
> already cached in the auth area, based on that we toggle creds->may_save
> through need_to_save.
>
> Patch by: stylesen
> Found by: arfrever
> ]]]
>
> Thank You.
- ------------------------------------------------------------------------
Index: subversion/libsvn_subr/simple_providers.c
===================================================================
- --- subversion/libsvn_subr/simple_providers.c (revision 32393)
+++ subversion/libsvn_subr/simple_providers.c (working copy)
@@ -94,6 +94,24 @@
return TRUE;
}
+/* Set **USERNAME to the username retrieved from CREDS; ignore
+ other parameters. */
+static svn_boolean_t
+simple_username_get(const char **username,
+ apr_hash_t *creds,
+ const char *realmstring,
+ svn_boolean_t non_interactive)
+{
+ svn_string_t *str;
+ str = apr_hash_get(creds, AUTHN_USERNAME_KEY, APR_HASH_KEY_STRING);
+ if (str && str->data)
+ {
+ *username = str->data;
+ return TRUE;
+ }
+ return FALSE;
+}
+
/* Common implementation for simple_first_creds and
windows_simple_first_creds. Uses PARAMETERS, REALMSTRING and the
simple auth provider's username and password cache to fill a set of
@@ -122,45 +140,85 @@
svn_boolean_t non_interactive = apr_hash_get(parameters,
SVN_AUTH_PARAM_NON_INTERACTIVE,
APR_HASH_KEY_STRING) != NULL;
- -
- - svn_boolean_t may_save = username || password;
+ const char *def_username = NULL;
+ const char *def_password = NULL;
+ svn_boolean_t need_to_save = FALSE;
+ apr_hash_t *creds_hash = NULL;
svn_error_t *err;
+ svn_string_t *str;
+ svn_boolean_t have_passtype = FALSE;
- - /* If we don't have a username and a password yet, we try the auth cache */
- - if (! (username && password))
+ /* Try to load credentials from a file on disk, based on the
+ realmstring. Don't throw an error, though: if something went
+ wrong reading the file, no big deal. What really matters is that
+ we failed to get the creds, so allow the auth system to try the
+ next provider. */
+ err = svn_config_read_auth_data(&creds_hash, SVN_AUTH_CRED_SIMPLE,
+ realmstring, config_dir, pool);
+ svn_error_clear(err);
+
+ /* We have something in the auth cache for this realm. */
+ if (! err && creds_hash)
{
- - apr_hash_t *creds_hash = NULL;
+ /* The password type in the auth data must match the
+ mangler's type, otherwise the password must be
+ interpreted by another provider. */
+ str = apr_hash_get(creds_hash, AUTHN_PASSTYPE_KEY, APR_HASH_KEY_STRING);
+ if (str && str->data)
+ if (passtype && (0 == strcmp(str->data, passtype)))
+ have_passtype = TRUE;
- - /* Try to load credentials from a file on disk, based on the
- - realmstring. Don't throw an error, though: if something went
- - wrong reading the file, no big deal. What really matters is that
- - we failed to get the creds, so allow the auth system to try the
- - next provider. */
- - err = svn_config_read_auth_data(&creds_hash, SVN_AUTH_CRED_SIMPLE,
- - realmstring, config_dir, pool);
- - svn_error_clear(err);
- - if (! err && creds_hash)
+ /* See if we need to save this username if it is not present in
+ auth cache. */
+ if (username)
{
- - svn_string_t *str;
- - if (! username)
+ if (!simple_username_get(&def_username, creds_hash, realmstring,
+ non_interactive))
{
- - str = apr_hash_get(creds_hash, AUTHN_USERNAME_KEY,
- - APR_HASH_KEY_STRING);
- - if (str && str->data)
- - username = str->data;
+ need_to_save = TRUE;
}
+ else
+ {
+ if (0 == strcmp(def_username, username))
+ need_to_save = FALSE;
+ else
+ need_to_save = TRUE;
+ }
+ }
+ /* See if we need to save this password if it is not present in
+ auth cache. */
+ if (password)
+ {
+ if (have_passtype)
+ {
+ if (!password_get(&def_password, creds_hash, realmstring,
+ username, non_interactive, pool))
+ {
+ need_to_save = TRUE;
+ }
+ else
+ {
+ if (0 == strcmp(def_password, password))
+ need_to_save = FALSE;
+ else
+ need_to_save = TRUE;
+ }
+ }
+ }
+
+ /* If we don't have a username and a password yet, we try the
+ auth cache */
+ if (! (username && password))
+ {
+ if (! username)
+ if (!simple_username_get(&username, creds_hash, realmstring,
+ non_interactive))
+ username = NULL;
+
if (username && ! password)
{
- - svn_boolean_t have_passtype;
- - /* The password type in the auth data must match the
- - mangler's type, otherwise the password must be
- - interpreted by another provider. */
- - str = apr_hash_get(creds_hash, AUTHN_PASSTYPE_KEY,
- - APR_HASH_KEY_STRING);
- - have_passtype = (str && str->data);
- - if (have_passtype && passtype
- - && 0 != strcmp(str->data, passtype))
+ if (! have_passtype)
password = NULL;
else
{
@@ -171,12 +229,18 @@
/* 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;
+ if (password && ! have_passtype)
+ need_to_save = TRUE;
}
}
}
}
+ else
+ {
+ /* Nothing was present in the auth cache, so save the credentials if
+ we are asked to do so. */
+ need_to_save = TRUE;
+ }
/* Ask the OS for the username if we have a password but no
username. */
@@ -188,7 +252,7 @@
svn_auth_cred_simple_t *creds = apr_pcalloc(pool, sizeof(*creds));
creds->username = username;
creds->password = password;
- - creds->may_save = may_save;
+ creds->may_save = need_to_save;
*credentials = creds;
}
else
- ------------------------------------------------------------------------
- ---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
- --
Senthil Kumaran S
http://www.stylesen.org/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
iD8DBQFIrqNz9o1G+2zNQDgRAjHQAJ9PtiXuLCzUV1CXyA14+I1k0eUwzACgoJqj
0Kk11uyEm5F6n+X8D/wEsx0=
=9zj2
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Subversion sometimes needlessly asks for confirmation to store
already stored plaintext passwords
Posted by Senthil Kumaran S <se...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Karl Fogel wrote:
> I like the log message -- it explains the core meaning of the change
> very clearly.
The credits goes to stsp :) (http://svn.haxx.se/dev/archive-2008-07/0541.shtml).
> This conditional structure is a bit odd, because you check 'err' (the
> pointer) after clearing 'err' (the value).
>
> Usually we set errors to NULL after clearing them, so that we're not
> keeping a pointer that points to garbage. Thus, it would be better to
> do something like this:
>
> err = svn_config_read_auth_data(&creds_hash, SVN_AUTH_CRED_SIMPLE,
> realmstring, config_dir, pool);
> if (err)
> {
> svn_error_clear(err);
> err = NULL;
> }
> else if (creds_hash)
> {
> /* We have something in the auth cache for this realm. */
> [...]
> }
>
> Rest looks good to me!
I ve made the above modification and committed in r32874.
Thank You.
- --
Senthil Kumaran S
http://www.stylesen.org/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
iD8DBQFIvuPM9o1G+2zNQDgRAuBAAKC+j7+RzgdTj83E+mBXwU0jU/M+MwCgp5pl
ubG8ULTPGqKL+/nwCRJvaic=
=qFTR
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Subversion sometimes needlessly asks for confirmation to store already stored plaintext passwords
Posted by Karl Fogel <kf...@red-bean.com>.
Senthil Kumaran S <se...@collab.net> writes:
> The updated patch has more clarification in the doc string for
> 'need_to_save' variable. Please let me know if the patch is good so
> that I can commit it to trunk.
>
> [[[
> 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
> (simple_username_get): New function to get username for simple provider.
> (svn_auth__simple_first_creds_helper): Start out with need_to_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. Here we check for different combinations
> of username and password either supplied in the command line or
> already cached in the auth area, based on that we toggle creds->may_save
> through need_to_save.
>
> Found by: arfrever
> Review by: kfogel
> ]]]
I like the log message -- it explains the core meaning of the change
very clearly.
> --- subversion/libsvn_subr/simple_providers.c (revision 32779)
> +++ subversion/libsvn_subr/simple_providers.c (working copy)
> @@ -127,45 +145,88 @@
> svn_boolean_t non_interactive = apr_hash_get(parameters,
> SVN_AUTH_PARAM_NON_INTERACTIVE,
> APR_HASH_KEY_STRING) != NULL;
> + const char *default_username = NULL; /* Default username from cache. */
> + const char *default_password = NULL; /* Default password from cache. */
>
> - svn_boolean_t may_save = username || password;
> + /* This checks if we should save the CREDS, iff saving the credentials is
> + allowed by the run-time configuration. */
> + svn_boolean_t need_to_save = FALSE;
> + apr_hash_t *creds_hash = NULL;
> svn_error_t *err;
> + svn_string_t *str;
> + svn_boolean_t have_passtype = FALSE;
>
> - /* If we don't have a username and a password yet, we try the auth cache */
> - if (! (username && password))
> + /* Try to load credentials from a file on disk, based on the
> + realmstring. Don't throw an error, though: if something went
> + wrong reading the file, no big deal. What really matters is that
> + we failed to get the creds, so allow the auth system to try the
> + next provider. */
> + err = svn_config_read_auth_data(&creds_hash, SVN_AUTH_CRED_SIMPLE,
> + realmstring, config_dir, pool);
> + svn_error_clear(err);
> +
> + /* We have something in the auth cache for this realm. */
> + if (! err && creds_hash)
> {
This conditional structure is a bit odd, because you check 'err' (the
pointer) after clearing 'err' (the value).
Usually we set errors to NULL after clearing them, so that we're not
keeping a pointer that points to garbage. Thus, it would be better to
do something like this:
err = svn_config_read_auth_data(&creds_hash, SVN_AUTH_CRED_SIMPLE,
realmstring, config_dir, pool);
if (err)
{
svn_error_clear(err);
err = NULL;
}
else if (creds_hash)
{
/* We have something in the auth cache for this realm. */
[...]
}
Rest looks good to me!
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Subversion sometimes needlessly asks for confirmation to store
already stored plaintext passwords
Posted by Senthil Kumaran S <se...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi Karl,
Karl Fogel wrote:
> + /* Nothing was present in the auth cache, so indicate that these
> + credentials should be saved if saving is allowed by the run-time
> + configuration. */
>
> I'm just saying that it would be a bit better if that kind of
> explanation were collected at the variable's declaration, so the reader
> could understand the full semantics from the start -- instead of having
> to collect the information bit by bit from reading through the code.
> It's not a big deal, though; I think it's pretty clear as-is.
The updated patch has more clarification in the doc string for 'need_to_save'
variable. Please let me know if the patch is good so that I can commit it to trunk.
[[[
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
(simple_username_get): New function to get username for simple provider.
(svn_auth__simple_first_creds_helper): Start out with need_to_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. Here we check for different combinations
of username and password either supplied in the command line or
already cached in the auth area, based on that we toggle creds->may_save
through need_to_save.
Found by: arfrever
Review by: kfogel
]]]
Thank You.
- --
Senthil Kumaran S
http://www.stylesen.org/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
iD8DBQFIu9qR9o1G+2zNQDgRAoxtAJ9mhrTUocyqufD6x602OJ+CvjZ4ZACfcQQK
JD+BxwiW6Wcmp2PBSDGa1+I=
=qc48
-----END PGP SIGNATURE-----
Re: Subversion sometimes needlessly asks for confirmation to store already stored plaintext passwords
Posted by Karl Fogel <kf...@red-bean.com>.
Senthil Kumaran S <se...@collab.net> writes:
> + svn_boolean_t need_to_save = FALSE; /* Check if we need to save CREDS. */
Well, this just repeats what the name says :-). When I said "document
the semantics", what I really meant was a general explanation of when
the variable is true and when it's false -- in other words, what the
variable means in practice.
You actually do this later, in a comment near one assignment of the
variable:
+ /* Nothing was present in the auth cache, so indicate that these
+ credentials should be saved if saving is allowed by the run-time
+ configuration. */
I'm just saying that it would be a bit better if that kind of
explanation were collected at the variable's declaration, so the reader
could understand the full semantics from the start -- instead of having
to collect the information bit by bit from reading through the code.
It's not a big deal, though; I think it's pretty clear as-is.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Subversion sometimes needlessly asks for confirmation to store
already stored plaintext passwords
Posted by Senthil Kumaran S <se...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi Karl,
Karl Fogel wrote:
> Oops, sorry Senthil. I owed a response to your response to my response
> to your patch :-).
:) My comments inline.
>> If there is a change in credentials then the changed creds will be cached in
>> the auth area afresh. This is what happens now. This was the behavior
>> previously in older versions, so I didn't want to break it, because it breaks
>> tests.
>
> Hmm. That seems counterintuitive to me, but if it's current behavior,
> let's keep it for now, and raise this as a separate issue (unrelated to
> your patch).
Ok.
> Well, the current caller uses it temporarily, but simple_username_get()
> is providing an interface, and we don't know who will use it in the
> future. It might be good to state explicitly that:
>
> "*USERNAME will have the same lifetime as CREDS."
Documented the above in "simple_username_get()".
>> - svn_boolean_t may_save = username || password;
>> + const char *def_username = NULL;
>> + const char *def_password = NULL;
>> + svn_boolean_t need_to_save = FALSE;
>> + apr_hash_t *creds_hash = NULL;
>> svn_error_t *err;
>> + svn_string_t *str;
>> + svn_boolean_t have_passtype = FALSE;
>
> I think the subsequent code would become much more comprehensible if you
> document some of these variables here, in particular need_to_save. (For
> example, at the end when you assign "creds->may_save = need_to_save;",
> the semantic transfer from one to the other will be confusing unless the
> reader totally understands what need_to_save's semantics are.)
Added comments for the above variables.
> With def_username and def_password, does "def" mean "default"? If so,
> just write it out -- that way their meaning is instantly clear.
Done this in new patch.
Another instance of the above in "prompt_for_simple_creds()" was corrected in
r32717.
>> + else
>> + {
>> + /* Nothing was present in the auth cache, so save the credentials if
>> + we are asked to do so. */
>> + need_to_save = TRUE;
>> + }
>
> Where's the check for "if we are asked to do so", though? This new
> 'else' is the counter-condition to this 'if':
>
> /* We have something in the auth cache for this realm. */
> if (! err && creds_hash)
> {
> [...]
> }
>
> Or do you mean "... so indicate that these credentials should be saved
> if saving is allowed by the run-time configuration"? (Since the
> run-time config is checked in svn_auth__simple_save_creds_helper(), I
> guess.)
Yes this is checked in svn_auth__simple_save_creds_helper() before saving, so I
changed the comment in the attached patch according to the above (as you have
specified).
[[[
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
(simple_username_get): New function to get username for simple provider.
(svn_auth__simple_first_creds_helper): Start out with need_to_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. Here we check for different combinations
of username and password either supplied in the command line or
already cached in the auth area, based on that we toggle creds->may_save
through need_to_save.
Patch by: stylesen
Found by: arfrever
Review by: kfogel
]]]
Thank You.
- --
Senthil Kumaran S
http://www.stylesen.org/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
iD8DBQFItAQE9o1G+2zNQDgRAt8XAJ4gO2mfjZjtvAikLPxA8rXKTnEXSwCghM/L
xiBs5/tMDZlwMDW7x7UOujc=
=a9mO
-----END PGP SIGNATURE-----
Re: Subversion sometimes needlessly asks for confirmation to store already stored plaintext passwords
Posted by Karl Fogel <kf...@red-bean.com>.
Senthil Kumaran S <se...@collab.net> writes:
> I would like to get some review on this patch, so that it will get
> into trunk (this does not have an issue in our issue tracker, fyi).
Oops, sorry Senthil. I owed a response to your response to my response
to your patch :-).
> Karl Fogel wrote:
>> What happens if all of the following are true:
>>
>> - the user passes '--username=my_user_name'
>> - there is a cached password for my_user_name and this repository
>> - the cached password is incorrect (according to the repository)
>> - the user's ~/.subversion configuration says may_save = FALSE
>>
>> ? The correct behavior then would be to prompt the user for a new
>> username/password (after discovering that the old password doesn't work,
>> the client prompts for both username and password, I assume), use the
>> new creds for this operation, but *not* save them. I think. Does that
>> make sense to you?
>
> If there is a change in credentials then the changed creds will be cached in
> the auth area afresh. This is what happens now. This was the behavior
> previously in older versions, so I didn't want to break it, because it breaks
> tests.
Hmm. That seems counterintuitive to me, but if it's current behavior,
let's keep it for now, and raise this as a separate issue (unrelated to
your patch).
I think that 'may_save=FALSE' should mean that no new passwords get
cached -- whether or not a password is *currently* cached for a given
username. But we can deal with that outside this change.
>>> +/* Retrieves the username from CREDS. */
>>> +static svn_boolean_t
>>> +simple_username_get(const char **username,
>>> + apr_hash_t *creds,
>>> + const char *realmstring,
>>> + svn_boolean_t non_interactive)
>>
>> Doc string doesn't mention the other parameters, but also doesn't say
>> this is an implementation of some pre-existing function type. Should do
>> one or the other...
>
> Done in the new patch.
Thanks.
>>> +{
>>> + svn_string_t *str;
>>> + str = apr_hash_get(creds, AUTHN_USERNAME_KEY, APR_HASH_KEY_STRING);
>>> + if (str && str->data)
>>> + {
>>> + *username = str->data;
>>> + return TRUE;
>>> + }
>>> + return FALSE;
>>> +}
>>
>> Is there any need to mention lifetime issues with the returned data, or
>> are there conventions already in place about the 'creds' hash and data
>> retrieved from it that would be obvious to anyone reading this code in
>> context?
>
> Since the values from creds_hash is used temporarily there is no lifetime
> issues to mention here, IMHO. We use these values in order to set
> "svn_auth_cred_simple_t *creds" which must be persistent and it _is_
> throughout the code.
Well, the current caller uses it temporarily, but simple_username_get()
is providing an interface, and we don't know who will use it in the
future. It might be good to state explicitly that:
"*USERNAME will have the same lifetime as CREDS."
>> I don't understand the logic of these different settings of 'may_save'.
>>
>> Is the 'may_save' variable supposed to represent what the configuration
>> says about the question of whether or not we can save creds? Or does it
>> represent the question of whether or not we got different data from what
>> was in the cache -- in other words, it's really "need_to_save"?
>
> From svn_auth_cred_simple_t doc string:
>
> <snip>
> /** Indicates if the credentials may be saved (to disk). For example, a
> * GUI prompt implementation with a remember password checkbox shall set
> * @a may_save to TRUE if the checkbox is checked.
> */
> svn_boolean_t may_save;
> </snip>
Well, sure, but that's a field in a struct. Just because it has the
same name as a local variable used elsewhere doesn't mean they have the
same semantics (at least, no reader could assume that).
>> If the variable only represents one thing, then maybe it's just that its
>> name needs to be changed. But if it represents several different
>> questions, it might be good to have several different variables, so the
>> readers can understand what questions are being considered...
>
> This variable just addresses, whether we need to save the credentials
> supplied.
> So I am changing the variable name in the updated patch to need_to_save.
Sounds good to me.
> [[[
> 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
> (simple_username_get): New function to get username for simple provider.
> (svn_auth__simple_first_creds_helper): Start out with need_to_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. Here we check for different combinations
> of username and password either supplied in the command line or
> already cached in the auth area, based on that we toggle creds->may_save
> through need_to_save.
>
> Patch by: stylesen
> Found by: arfrever
> ]]]
>
> Thank You.
> --
> Senthil Kumaran S
> http://www.stylesen.org/
>
> Index: subversion/libsvn_subr/simple_providers.c
> ===================================================================
> --- subversion/libsvn_subr/simple_providers.c (revision 32393)
> +++ subversion/libsvn_subr/simple_providers.c (working copy)
> @@ -94,6 +94,24 @@
> return TRUE;
> }
>
> +/* Set **USERNAME to the username retrieved from CREDS; ignore
> + other parameters. */
> +static svn_boolean_t
> +simple_username_get(const char **username,
> + apr_hash_t *creds,
> + const char *realmstring,
> + svn_boolean_t non_interactive)
> +{
> + svn_string_t *str;
> + str = apr_hash_get(creds, AUTHN_USERNAME_KEY, APR_HASH_KEY_STRING);
> + if (str && str->data)
> + {
> + *username = str->data;
> + return TRUE;
> + }
> + return FALSE;
> +}
> +
> /* Common implementation for simple_first_creds and
> windows_simple_first_creds. Uses PARAMETERS, REALMSTRING and the
> simple auth provider's username and password cache to fill a set of
> @@ -122,45 +140,85 @@
> svn_boolean_t non_interactive = apr_hash_get(parameters,
> SVN_AUTH_PARAM_NON_INTERACTIVE,
> APR_HASH_KEY_STRING) != NULL;
> -
> - svn_boolean_t may_save = username || password;
> + const char *def_username = NULL;
> + const char *def_password = NULL;
> + svn_boolean_t need_to_save = FALSE;
> + apr_hash_t *creds_hash = NULL;
> svn_error_t *err;
> + svn_string_t *str;
> + svn_boolean_t have_passtype = FALSE;
I think the subsequent code would become much more comprehensible if you
document some of these variables here, in particular need_to_save. (For
example, at the end when you assign "creds->may_save = need_to_save;",
the semantic transfer from one to the other will be confusing unless the
reader totally understands what need_to_save's semantics are.)
With def_username and def_password, does "def" mean "default"? If so,
just write it out -- that way their meaning is instantly clear.
> - /* If we don't have a username and a password yet, we try the auth cache */
> - if (! (username && password))
> + /* Try to load credentials from a file on disk, based on the
> + realmstring. Don't throw an error, though: if something went
> + wrong reading the file, no big deal. What really matters is that
> + we failed to get the creds, so allow the auth system to try the
> + next provider. */
> + err = svn_config_read_auth_data(&creds_hash, SVN_AUTH_CRED_SIMPLE,
> + realmstring, config_dir, pool);
> + svn_error_clear(err);
> +
> + /* We have something in the auth cache for this realm. */
> + if (! err && creds_hash)
> {
> - apr_hash_t *creds_hash = NULL;
> + /* The password type in the auth data must match the
> + mangler's type, otherwise the password must be
> + interpreted by another provider. */
> + str = apr_hash_get(creds_hash, AUTHN_PASSTYPE_KEY, APR_HASH_KEY_STRING);
> + if (str && str->data)
> + if (passtype && (0 == strcmp(str->data, passtype)))
> + have_passtype = TRUE;
>
> - /* Try to load credentials from a file on disk, based on the
> - realmstring. Don't throw an error, though: if something went
> - wrong reading the file, no big deal. What really matters is that
> - we failed to get the creds, so allow the auth system to try the
> - next provider. */
> - err = svn_config_read_auth_data(&creds_hash, SVN_AUTH_CRED_SIMPLE,
> - realmstring, config_dir, pool);
> - svn_error_clear(err);
> - if (! err && creds_hash)
> + /* See if we need to save this username if it is not present in
> + auth cache. */
> + if (username)
> {
> - svn_string_t *str;
> - if (! username)
> + if (!simple_username_get(&def_username, creds_hash, realmstring,
> + non_interactive))
> {
> - str = apr_hash_get(creds_hash, AUTHN_USERNAME_KEY,
> - APR_HASH_KEY_STRING);
> - if (str && str->data)
> - username = str->data;
> + need_to_save = TRUE;
> }
> + else
> + {
> + if (0 == strcmp(def_username, username))
> + need_to_save = FALSE;
> + else
> + need_to_save = TRUE;
> + }
> + }
>
> + /* See if we need to save this password if it is not present in
> + auth cache. */
> + if (password)
> + {
> + if (have_passtype)
> + {
> + if (!password_get(&def_password, creds_hash, realmstring,
> + username, non_interactive, pool))
> + {
> + need_to_save = TRUE;
> + }
> + else
> + {
> + if (0 == strcmp(def_password, password))
> + need_to_save = FALSE;
> + else
> + need_to_save = TRUE;
> + }
> + }
> + }
> +
> + /* If we don't have a username and a password yet, we try the
> + auth cache */
> + if (! (username && password))
> + {
> + if (! username)
> + if (!simple_username_get(&username, creds_hash, realmstring,
> + non_interactive))
> + username = NULL;
> +
> if (username && ! password)
> {
> - svn_boolean_t have_passtype;
> - /* The password type in the auth data must match the
> - mangler's type, otherwise the password must be
> - interpreted by another provider. */
> - str = apr_hash_get(creds_hash, AUTHN_PASSTYPE_KEY,
> - APR_HASH_KEY_STRING);
> - have_passtype = (str && str->data);
> - if (have_passtype && passtype
> - && 0 != strcmp(str->data, passtype))
> + if (! have_passtype)
> password = NULL;
> else
> {
> @@ -171,12 +229,18 @@
> /* 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;
> + if (password && ! have_passtype)
> + need_to_save = TRUE;
> }
> }
> }
> }
> + else
> + {
> + /* Nothing was present in the auth cache, so save the credentials if
> + we are asked to do so. */
> + need_to_save = TRUE;
> + }
Where's the check for "if we are asked to do so", though? This new
'else' is the counter-condition to this 'if':
/* We have something in the auth cache for this realm. */
if (! err && creds_hash)
{
[...]
}
Or do you mean "... so indicate that these credentials should be saved
if saving is allowed by the run-time configuration"? (Since the
run-time config is checked in svn_auth__simple_save_creds_helper(), I
guess.)
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Subversion sometimes needlessly asks for confirmation to store
already stored plaintext passwords
Posted by Senthil Kumaran S <se...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi Karl,
Karl Fogel wrote:
> What happens if all of the following are true:
>
> - the user passes '--username=my_user_name'
> - there is a cached password for my_user_name and this repository
> - the cached password is incorrect (according to the repository)
> - the user's ~/.subversion configuration says may_save = FALSE
>
> ? The correct behavior then would be to prompt the user for a new
> username/password (after discovering that the old password doesn't work,
> the client prompts for both username and password, I assume), use the
> new creds for this operation, but *not* save them. I think. Does that
> make sense to you?
If there is a change in credentials then the changed creds will be cached in
the auth area afresh. This is what happens now. This was the behavior
previously in older versions, so I didn't want to break it, because it breaks
tests.
>> +/* Retrieves the username from CREDS. */
>> +static svn_boolean_t
>> +simple_username_get(const char **username,
>> + apr_hash_t *creds,
>> + const char *realmstring,
>> + svn_boolean_t non_interactive)
>
> Doc string doesn't mention the other parameters, but also doesn't say
> this is an implementation of some pre-existing function type. Should do
> one or the other...
Done in the new patch.
>> +{
>> + svn_string_t *str;
>> + str = apr_hash_get(creds, AUTHN_USERNAME_KEY, APR_HASH_KEY_STRING);
>> + if (str && str->data)
>> + {
>> + *username = str->data;
>> + return TRUE;
>> + }
>> + return FALSE;
>> +}
>
> Is there any need to mention lifetime issues with the returned data, or
> are there conventions already in place about the 'creds' hash and data
> retrieved from it that would be obvious to anyone reading this code in
> context?
Since the values from creds_hash is used temporarily there is no lifetime
issues to mention here, IMHO. We use these values in order to set
"svn_auth_cred_simple_t *creds" which must be persistent and it _is_ throughout
the code.
> I don't understand the logic of these different settings of 'may_save'.
>
> Is the 'may_save' variable supposed to represent what the configuration
> says about the question of whether or not we can save creds? Or does it
> represent the question of whether or not we got different data from what
> was in the cache -- in other words, it's really "need_to_save"?
- From svn_auth_cred_simple_t doc string:
<snip>
/** Indicates if the credentials may be saved (to disk). For example, a
* GUI prompt implementation with a remember password checkbox shall set
* @a may_save to TRUE if the checkbox is checked.
*/
svn_boolean_t may_save;
</snip>
> If the variable only represents one thing, then maybe it's just that its
> name needs to be changed. But if it represents several different
> questions, it might be good to have several different variables, so the
> readers can understand what questions are being considered...
This variable just addresses, whether we need to save the credentials supplied.
So I am changing the variable name in the updated patch to need_to_save.
>> + /* See if we need to save this password if it is not present in
>> + auth cache. */
>
> This comment indicates it's about "need_to_save" not "may_save"...
In all the places this variable is used to predict whether we need to save the
creds, so changing its name.
> The rest of the change I found hard to understand, but I think my
> confusion all stems from one basic source: the difference between "may
> save" and "need to save (if saving is allowed)". One is about what the
> user has given Subvesrion permission to do; the other is about what
> Subversion *should* do, assuming it has the necessary permissions from
> the user at all.
I will try to summarize the change as follows:
1) Get the username and password supplied by the user in command line.
2) We check whether there is any creds cached in the auth area for this REALM.
3) If 2) is TRUE, then get the def_username from the auth cache, else we need
to save whatever username we get from the following steps to the auth cache, if
the user indicated us to save auth creds ie., by "store-auth-creds" in the
servers file (but the storing part check is done in save_credentials function).
What we really are bothered here is to toggle the creds->may_save, in order to
make a decision whether we need to save these credentials or not.
4) If 3) gives a def_username, then compare this username with whatever
username we have (if any) from 1). If they are same then we need not change the
username creds in the auth cache, else we need to save the username creds in
the auth cache.
5) If 2) is TRUE, then get the def_password from the auth cache, else we need
to save whatever password we get from the following steps to the auth cache.
6) If 5) gives a def_password, then compare this password with whatever
password we have (if any) from 1). If they are same then we need not change the
password creds in the auth cache, else we need to save the password creds in
the auth cache.
Steps 1) to 6) above are checks made in order to see whether the creds supplied
in command line matches with the stored creds in the auth cache, if not we will
make the change in the auth cache.
7) Get the username and password from the auth cache, if it is not supplied in
the command line. Here there is one check to need_to_save, if the passtype is
not cached for these credentials.
8) If 2) fails then, there was nothing in the auth cache for this REALM, so we
set need_to_save as TRUE, which will be handled by save_credentials related
functions based on "store-auth-creds" value in servers file.
The remaining part of the code ie., the following is obvious:
<snip>
/* Ask the OS for the username if we have a password but no
username. */
if (password && ! username)
username = svn_user_get_name(pool);
if (username && password)
{
svn_auth_cred_simple_t *creds = apr_pcalloc(pool, sizeof(*creds));
creds->username = username;
creds->password = password;
creds->may_save = need_to_save;
*credentials = creds;
}
else
*credentials = NULL;
</snip>
[[[
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
(simple_username_get): New function to get username for simple provider.
(svn_auth__simple_first_creds_helper): Start out with need_to_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. Here we check for different combinations
of username and password either supplied in the command line or
already cached in the auth area, based on that we toggle creds->may_save
through need_to_save.
Patch by: stylesen
Found by: arfrever
]]]
Thank You.
- --
Senthil Kumaran S
http://www.stylesen.org/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
iD8DBQFImu109o1G+2zNQDgRAsF2AJ9nNVWkcaWuSuOAjQjouCuOjFfXHgCdEkAN
3594GGOd7bq8cFczqFXSiZg=
=zcAC
-----END PGP SIGNATURE-----