You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.co.il> on 2008/04/29 17:29:46 UTC
Review of dont-* branch
Good morning Stefan,
Reviewing (at your request):
Stefan Sperling wrote:
> $ svn diff https://svn.collab.net/repos/svn/trunk@30801 \
> https://svn.collab.net/repos/svn/branches/dont-save-plaintext-passwords-by-default
> Index: subversion/include/svn_cmdline.h
> ===================================================================
> --- subversion/include/svn_cmdline.h (.../trunk) (revision 30801)
> +++ subversion/include/svn_cmdline.h (.../branches/dont-save-plaintext-passwords-by-default) (revision 30836)
> @@ -140,10 +140,23 @@ int svn_cmdline_handle_exit_error(svn_error_t *err
> apr_pool_t *pool,
> const char *prefix);
>
> +/** A cancellation function/baton pair, and the path to the configuration
> + * directory. To be passed as the baton argument to the
> + * @c svn_cmdline_*_prompt functions.
> + *
> + * @since New in 1.6.
> + */
> +typedef struct svn_cmdline_prompt_baton2_t {
> + svn_cancel_func_t cancel_func;
> + void *cancel_baton;
> + const char *config_dir;
> +} svn_cmdline_prompt_baton2_t;
> +
> /** A cancellation function/baton pair to be passed as the baton argument
> * to the @c svn_cmdline_*_prompt functions.
> *
> * @since New in 1.4.
> + * @deprecated Provided for backward compatibility with the 1.5 API.
> */
> typedef struct svn_cmdline_prompt_baton_t {
Should svn_cmdline_prompt_baton_t's docstring point to svn_cmdline_prompt_baton2_t?
> @@ -253,6 +266,18 @@ svn_cmdline_auth_ssl_client_cert_pw_prompt
> svn_boolean_t may_save,
> apr_pool_t *pool);
>
> +/** An implementation of @c svn_auth_plaintext_prompt_func_t that
> + * prompts the user whether storing unencypted passwords to disk
> + * is OK on the command line.
^^^^^^^^^^^^^^^^^^^
"Storing unencrypted passwords ... on the command line"? As opposed to
storing them (the passwords) on the kitchen table? ;)
> + *
> + * @since New in 1.6.
> + */
> +svn_error_t *
> +svn_cmdline_auth_plaintext_prompt(svn_boolean_t *may_save_plaintext,
> Index: subversion/include/svn_auth.h
> ===================================================================
> --- subversion/include/svn_auth.h (.../trunk) (revision 30801)
> +++ subversion/include/svn_auth.h (.../branches/dont-save-plaintext-passwords-by-default) (revision 30836)
> @@ -524,6 +559,12 @@ const void * svn_auth_get_parameter(svn_auth_baton
> #define SVN_AUTH_PARAM_DONT_STORE_PASSWORDS SVN_AUTH_PARAM_PREFIX \
> "dont-store-passwords"
>
> +/** @brief Indicates whether providers may save passwords to disk in
> + * plaintext. Property value can be either SVN_CONFIG_TRUE,
> + * SVN_CONFIG_FALSE, or SVN_CONFIG_PROMPT. */
^^^^^^
s/PROMPT/ASK/
> @@ -587,9 +627,12 @@ svn_error_t * svn_auth_next_credentials(void **cre
> /** Save a set of credentials.
> *
> * Ask @a state to store the most recently returned credentials,
> - * presumably because they successfully authenticated. Use @a pool
> - * for temporary allocation. If no credentials were ever returned, do
> - * nothing.
> + * presumably because they successfully authenticated.
> + * All allocations should be done in @a pool, which can be
^^^^^^
Since it's a function we implement, not a callback, might as well
s/can be/is/.
If we do not rev this API, we need to make sure it continues to work for
pre-1.6 callers as well. In this case, what happens if the pool does
*not* survive across RA sessions? Does it only not cache the user's
answers to the prompt, or could it segfault? (I think the former is
fine.)
(The same considerations apply to save_credentials in
svn_auth_provider_t, whose docstring was similarly updated.)
> + * assumed to survive across RA sessions; auth providers that store
> + * passwords in plaintext rely on this.
> + *
> + * If no credentials were ever returned, do nothing.
> */
> svn_error_t * svn_auth_save_credentials(svn_auth_iterstate_t *state,
> apr_pool_t *pool);
> Index: subversion/libsvn_subr/config_file.c
> ===================================================================
> --- subversion/libsvn_subr/config_file.c (.../trunk) (revision 30801)
> +++ subversion/libsvn_subr/config_file.c (.../branches/dont-save-plaintext-passwords-by-default) (revision 30836)
> @@ -897,10 +900,19 @@ svn_config_ensure(const char *config_dir, apr_pool
> + "[auth]" NL
> "### Section for authentication and authorization customizations." NL
> - "[auth]" NL
Was this change involuntary?
> Index: subversion/libsvn_subr/cmdline.c
> ===================================================================
> --- subversion/libsvn_subr/cmdline.c (.../trunk) (revision 30801)
> +++ subversion/libsvn_subr/cmdline.c (.../branches/dont-save-plaintext-passwords-by-default) (revision 30836)
> @@ -459,21 +472,24 @@ svn_cmdline_setup_auth_baton(svn_auth_baton_t **ab
> svn_auth_set_parameter(*ab, SVN_AUTH_PARAM_CONFIG_DIR,
> config_dir);
>
> + /* Determine whether storing passwords in any form is allowed.
> + * This is the deprecated location for this option, the new
> + * location is SVN_CONFIG_CATEGORY_SERVERS. */
> SVN_ERR(svn_config_get_bool(cfg, &store_password_val,
> SVN_CONFIG_SECTION_AUTH,
> SVN_CONFIG_OPTION_STORE_PASSWORDS,
> - TRUE));
> + SVN_CONFIG_DEFAULT_OPTION_STORE_PASSWORDS));
>
> if (! store_password_val)
> svn_auth_set_parameter(*ab, SVN_AUTH_PARAM_DONT_STORE_PASSWORDS, "");
>
> - /* There are two different ways the user can disable disk caching
> - of credentials: either via --no-auth-cache, or in the config
> - file ('store-auth-creds = no'). */
> + /* Determine whether we are allowed to write to the auth/ area.
> + * This is the deprecated location for this option, the new
> + * location is SVN_CONFIG_CATEGORY_SERVERS. */
So, if I understand correctly, the setting in ~/.subversion/config is
passed as an SVN_AUTH_PARAM_* parameter, and the setting(s) in
~/.subversion/servers are read and used in svn_ra_open3()?
It is not entirely obvious, when reading this function in isolation or
reading svn_ra_open3() in isolation, where the other config file's
options are handled. Maybe add cross-reference comments between them?
> Index: subversion/libsvn_subr/prompt.c
> ===================================================================
> --- subversion/libsvn_subr/prompt.c (.../trunk) (revision 30801)
> +++ subversion/libsvn_subr/prompt.c (.../branches/dont-save-plaintext-passwords-by-default) (revision 30836)
> @@ -376,7 +376,67 @@ svn_cmdline_auth_ssl_client_cert_pw_prompt
> +/* This implements 'svn_auth_plaintext_prompt_func_t'. */
> +svn_error_t *
> +svn_cmdline_auth_plaintext_prompt(svn_boolean_t *may_save_plaintext,
> + const char *realmstring,
> + void *baton,
> + apr_pool_t *pool)
> +{
> + do
> + {
> + svn_error_t *err = prompt(&answer, prompt_string, FALSE, pb, pool);
> + if (err)
> + {
> + if (err->apr_err == SVN_ERR_CANCELLED)
> + {
> + svn_error_clear(err);
> + *may_save_plaintext = FALSE;
> + return SVN_NO_ERROR;
> + }
> + else
> + return err;
> + }
> + if (svn_cstring_casecmp(answer, _("yes")) == 0)
svn_cstring_casecmp() is:
/**
* Compare two strings @a atr1 and @a atr2, treating case-equivalent
* unaccented Latin (ASCII subset) letters as equal.
*/
int svn_cstring_casecmp(const char *str1, const char *str2);
Do we want to also consider non-ASCII characters as equal regardless of case?
>
> @@ -386,10 +446,12 @@ svn_cmdline_prompt_user2(const char **result,
> svn_cmdline_prompt_baton_t *baton,
> apr_pool_t *pool)
> {
> - return prompt(result, prompt_str, FALSE /* don't hide input */, baton, pool);
> + /* XXX: We know prompt doesn't use the new members
> + * of svn_cmdline_prompt_baton2_t. */
> + return prompt(result, prompt_str, FALSE /* don't hide input */,
> + (svn_cmdline_prompt_baton2_t *)baton, pool);
Add a reverse pointer, from prompt()'s docstring to this comment? If
prompt() started using the new members, this function will need to be
adjusted.
> Index: subversion/libsvn_subr/simple_providers.c
> ===================================================================
> --- subversion/libsvn_subr/simple_providers.c (.../trunk) (revision 30801)
> +++ subversion/libsvn_subr/simple_providers.c (.../branches/dont-save-plaintext-passwords-by-default) (revision 30836)
> @@ -258,46 +272,117 @@ simple_save_creds_helper(svn_boolean_t *saved,
> + /* Don't store passwords in any form if the user has told
> + * us not to do so. */
> + if (! dont_store_passwords)
> {
> + svn_boolean_t may_save_password = FALSE;
>
> + /* If the password is going to be stored encrypted, go right
> + * ahead and store it to disk. Else determine whether saving
> + * in plaintext is OK. */
> + if (strcmp(passtype, SVN_AUTH__WINCRYPT_PASSWORD_TYPE) == 0
> + || strcmp(passtype, SVN_AUTH__KEYCHAIN_PASSWORD_TYPE) == 0)
Do we want to hardcode which password types are encrypted?
> Index: TODO.branch
> ===================================================================
> --- TODO.branch (.../trunk) (revision 0)
> +++ TODO.branch (.../branches/dont-save-plaintext-passwords-by-default) (revision 30836)
(included for completeness) File should be removed before merging to trunk.
> Property changes on: .
> ___________________________________________________________________
> Modified: svn:mergeinfo
> Merged /trunk:r30654-30800
>
>
Thanks Stefan for your work on this branch; it looks good to me.
Daniel
Re: Review of dont-* branch
Posted by Stefan Sperling <st...@elego.de>.
On Thu, May 01, 2008 at 08:49:23PM +0300, Daniel Shahaf wrote:
> Stefan Sperling wrote on Thu, 1 May 2008 at 19:39 +0200:
> > I'm not sure how exactly you want me to document this.
> > Can you provide a patch?
> >
> >
>
> Index: simple_providers.c
> ===================================================================
> --- simple_providers.c (revision 30887)
> +++ simple_providers.c (working copy)
> @@ -40,6 +40,10 @@
> #define SVN_AUTH__AUTHFILE_PASSWORD_KEY "password"
> #define SVN_AUTH__AUTHFILE_PASSTYPE_KEY "passtype"
>
> +/* If you add a password type that stores the password on disk
> + * in encrypted form, remember to add it to the list in
> + * simple_save_creds_helper.
> + */
> #define SVN_AUTH__SIMPLE_PASSWORD_TYPE "simple"
> #define SVN_AUTH__WINCRYPT_PASSWORD_TYPE "wincrypt"
> #define SVN_AUTH__KEYCHAIN_PASSWORD_TYPE "keychain"
Ah, OK. Committed slightly reworded version in r30889.
Thanks!
--
Stefan Sperling <st...@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
Re: Review of dont-* branch
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Stefan Sperling wrote on Thu, 1 May 2008 at 19:39 +0200:
> On Thu, May 01, 2008 at 07:53:11PM +0300, Daniel Shahaf wrote:
> > Stefan Sperling wrote on Thu, 1 May 2008 at 17:28 +0200:
> > > On Tue, Apr 29, 2008 at 08:29:46PM +0300, Daniel Shahaf wrote:
> > > > > Index: subversion/libsvn_subr/simple_providers.c
> > > > > ===================================================================
> > > > > --- subversion/libsvn_subr/simple_providers.c (.../trunk) (revision 30801)
> > > > > +++ subversion/libsvn_subr/simple_providers.c (.../branches/dont-save-plaintext-passwords-by-default) (revision 30836)
> > > > > @@ -258,46 +272,117 @@ simple_save_creds_helper(svn_boolean_t *saved,
> > > > > + /* Don't store passwords in any form if the user has told
> > > > > + * us not to do so. */
> > > > > + if (! dont_store_passwords)
> > > > > {
> > > > > + svn_boolean_t may_save_password = FALSE;
> > > > >
> > > > > + /* If the password is going to be stored encrypted, go right
> > > > > + * ahead and store it to disk. Else determine whether saving
> > > > > + * in plaintext is OK. */
> > > > > + if (strcmp(passtype, SVN_AUTH__WINCRYPT_PASSWORD_TYPE) == 0
> > > > > + || strcmp(passtype, SVN_AUTH__KEYCHAIN_PASSWORD_TYPE) == 0)
> > > >
> > > > Do we want to hardcode which password types are encrypted?
> > >
> > > Well, what's the alternative?
> > >
> >
> > /me discovers that the PASSWORD_TYPE constants are private to simple_providers.c
> >
> > Document this? If someone adds a password type and greps for
> > SIMPLE_PASSWORD_TYPE (try this), they won't know that somewhere in the file
> > there is a list of "special" password types that SIMPLE isn't one of them...
>
> I'm not sure how exactly you want me to document this.
> Can you provide a patch?
>
>
Index: simple_providers.c
===================================================================
--- simple_providers.c (revision 30887)
+++ simple_providers.c (working copy)
@@ -40,6 +40,10 @@
#define SVN_AUTH__AUTHFILE_PASSWORD_KEY "password"
#define SVN_AUTH__AUTHFILE_PASSTYPE_KEY "passtype"
+/* If you add a password type that stores the password on disk
+ * in encrypted form, remember to add it to the list in
+ * simple_save_creds_helper.
+ */
#define SVN_AUTH__SIMPLE_PASSWORD_TYPE "simple"
#define SVN_AUTH__WINCRYPT_PASSWORD_TYPE "wincrypt"
#define SVN_AUTH__KEYCHAIN_PASSWORD_TYPE "keychain"
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Review of dont-* branch
Posted by Stefan Sperling <st...@elego.de>.
On Thu, May 01, 2008 at 07:53:11PM +0300, Daniel Shahaf wrote:
> Stefan Sperling wrote on Thu, 1 May 2008 at 17:28 +0200:
> > On Tue, Apr 29, 2008 at 08:29:46PM +0300, Daniel Shahaf wrote:
> > > > Index: subversion/libsvn_subr/simple_providers.c
> > > > ===================================================================
> > > > --- subversion/libsvn_subr/simple_providers.c (.../trunk) (revision 30801)
> > > > +++ subversion/libsvn_subr/simple_providers.c (.../branches/dont-save-plaintext-passwords-by-default) (revision 30836)
> > > > @@ -258,46 +272,117 @@ simple_save_creds_helper(svn_boolean_t *saved,
> > > > + /* Don't store passwords in any form if the user has told
> > > > + * us not to do so. */
> > > > + if (! dont_store_passwords)
> > > > {
> > > > + svn_boolean_t may_save_password = FALSE;
> > > >
> > > > + /* If the password is going to be stored encrypted, go right
> > > > + * ahead and store it to disk. Else determine whether saving
> > > > + * in plaintext is OK. */
> > > > + if (strcmp(passtype, SVN_AUTH__WINCRYPT_PASSWORD_TYPE) == 0
> > > > + || strcmp(passtype, SVN_AUTH__KEYCHAIN_PASSWORD_TYPE) == 0)
> > >
> > > Do we want to hardcode which password types are encrypted?
> >
> > Well, what's the alternative?
> >
>
> /me discovers that the PASSWORD_TYPE constants are private to simple_providers.c
>
> Document this? If someone adds a password type and greps for
> SIMPLE_PASSWORD_TYPE (try this), they won't know that somewhere in the file
> there is a list of "special" password types that SIMPLE isn't one of them...
I'm not sure how exactly you want me to document this.
Can you provide a patch?
--
Stefan Sperling <st...@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
Re: Review of dont-* branch
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Stefan Sperling wrote on Thu, 1 May 2008 at 17:28 +0200:
> On Tue, Apr 29, 2008 at 08:29:46PM +0300, Daniel Shahaf wrote:
> > > @@ -587,9 +627,12 @@ svn_error_t * svn_auth_next_credentials(void **cre
> > > /** Save a set of credentials.
> > > *
> > > * Ask @a state to store the most recently returned credentials,
> > > - * presumably because they successfully authenticated. Use @a pool
> > > - * for temporary allocation. If no credentials were ever returned, do
> > > - * nothing.
> > > + * presumably because they successfully authenticated.
> > > + * All allocations should be done in @a pool, which can be
> >
> > If we do not rev this API, we need to make sure it continues to work for
> > pre-1.6 callers as well. In this case, what happens if the pool does
> > *not* survive across RA sessions? Does it only not cache the user's
> > answers to the prompt, or could it segfault? (I think the former is
> > fine.)
>
> Mmmmmh, good question.
>
> I think the caching should only be done if the client provides a
> prompt callback. Currently the code tries to look up answers in the
> cache unconditionally. I will fix that, and make the prompt callback's
> docstring mention the pool lifetime issue wrt svn_auth_provider_t's
> save_credentials.
>
> This way, simple_save_creds_helper will segfault if, and only if:
>
> 1. The client provides a plaintext prompt callback
> 2. And the client passes a pool to save_credentials that does not
> survive across RA sessions
> 3. And the user causes two RA sessions to be opened (e.g. by
> supplying multiple URLs on the command line)
>
> I think this is a reasonable compromise, given that we change semantics
> of old API (save_credentials) only if new API is also used (the prompt
> callback), and that the three conditions taken together are quite some
> corner case. I don't think client developers will mind this much.
>
+1 to the compromise, due to item (1) on your list (not segfaulting for
clients that don't use the new-in-1.6 plaintext prompt callback).
> Unfortunately, it looks like we cannot provide the true semantics of the
> old API in this case without defaulting to storing plaintext passwords
> again.
Agreed: the old API defaulted to saving passwords.
> There is, AFAIK, no way for us to determine the lifetime of the
> pool we're handed.
>
Nor should there be, if I understand the pools chapter of HACKING correctly.
> > > Index: subversion/libsvn_subr/config_file.c
> > > ===================================================================
> > > --- subversion/libsvn_subr/config_file.c (.../trunk) (revision 30801)
> > > +++ subversion/libsvn_subr/config_file.c (.../branches/dont-save-plaintext-passwords-by-default) (revision 30836)
> > > - /* There are two different ways the user can disable disk caching
> > > - of credentials: either via --no-auth-cache, or in the config
> > > - file ('store-auth-creds = no'). */
> > > + /* Determine whether we are allowed to write to the auth/ area.
> > > + * This is the deprecated location for this option, the new
> > > + * location is SVN_CONFIG_CATEGORY_SERVERS. */
> >
> > So, if I understand correctly, the setting in ~/.subversion/config is
> > passed as an SVN_AUTH_PARAM_* parameter, and the setting(s) in
> > ~/.subversion/servers are read and used in svn_ra_open3()?
>
> I don't really understand what your question is getting at.
>
> svn_ra_open3 _also_ sets SVN_AUTH_PARAM_* parameters. It just gets
> the values from the servers file, instead of the config file.
>
Oops, right. I was confused. Did I look at svn_ra_open3 of trunk
(which, obviously, doesn't use these parameters)?
> The providers are shielded from the configuration details, e.g.
> which file parameters come from. They only ever see SVN_AUTH_PARAM_*
> parameters.
>
> Does that make sense?
Your answer makes sense; thanks.
> What was your question?
>
My question was, "This is how I understand the situation; did
I understand correctly".
> > It is not entirely obvious, when reading this function in isolation or
> > reading svn_ra_open3() in isolation, where the other config file's
> > options are handled. Maybe add cross-reference comments between them?
>
> I'm not comfortable with cross-referencing function names in comments
> in different files (let alone libraries), because the comments may get
> obsoleted and cause confusion rather then help, if not updated properly.
>
Agreed, but note that as you phrased it it's very generic; it applies to me,
too. (If I won't be updated properly (e.g., by being taught new cool
programming languages), I would eventually get obsoleted and confusing too ;))
> I will make the comments in svn_cmdline_setup_auth_baton mention
> that "the RA layer" may override auth settings. That should be
> generic enough to be valid for at least a good long while.
>
> I don't think putting a comment about options specified in 'config'
> into the RA layer code makes much sense. It should be clear that
> the RA layer gets its settings from 'servers', and that the command
> line client reads 'config' independently of what the RA layer is doing.
> If this is not apparent to the reader, than they simply haven't
> read enough of our code yet :)
>
> Note, for example, the code in libsvn_ra_neon/session.c, which has
> been reading 'servers' for ages, and never mentions 'config' in
> any way. When cross-referencing 'config' from svn_ra_open3(), we
> might as well add comments to libsvn_ra_neon about 'config'.
> And that does not really make sense, I think. We'd be documenting
> behaviour of one library in another.
>
+1; clearly explained.
The situation isn't symmetrical -- an libsvn_ra reader ought to suspect that a
config has already been read, but libsvn_ra itself doesn't care about this and
overrides the parameters unconditionally.
> > svn_cstring_casecmp() is:
> >
> > /**
> > * Compare two strings @a atr1 and @a atr2, treating case-equivalent
> > * unaccented Latin (ASCII subset) letters as equal.
> > */
> > int svn_cstring_casecmp(const char *str1, const char *str2);
> >
> > Do we want to also consider non-ASCII characters as equal regardless of case?
>
> I guess we could use apr_strnatcasecmp() instead.
> This maps characters to their upper-case equivalent via 'toupper',
> which works with non-ASCII characters on some system, based on what
> the C library does, whether the system supports locales, what day
> of week it is, whether the sun is out or whether it's raining,
> and apparently some other magic factors.
>
:)
So the choice is, on non-ASCII inputs, between never working correctly and
working correctly inconsistently. I don't have an opinion on this (both
consistency and correctness are important, and I don't know apr_strnatcasecmp
well enough.)
> > > Index: subversion/libsvn_subr/simple_providers.c
> > > ===================================================================
> > > --- subversion/libsvn_subr/simple_providers.c (.../trunk) (revision 30801)
> > > +++ subversion/libsvn_subr/simple_providers.c (.../branches/dont-save-plaintext-passwords-by-default) (revision 30836)
> > > @@ -258,46 +272,117 @@ simple_save_creds_helper(svn_boolean_t *saved,
> > > + /* Don't store passwords in any form if the user has told
> > > + * us not to do so. */
> > > + if (! dont_store_passwords)
> > > {
> > > + svn_boolean_t may_save_password = FALSE;
> > >
> > > + /* If the password is going to be stored encrypted, go right
> > > + * ahead and store it to disk. Else determine whether saving
> > > + * in plaintext is OK. */
> > > + if (strcmp(passtype, SVN_AUTH__WINCRYPT_PASSWORD_TYPE) == 0
> > > + || strcmp(passtype, SVN_AUTH__KEYCHAIN_PASSWORD_TYPE) == 0)
> >
> > Do we want to hardcode which password types are encrypted?
>
> Well, what's the alternative?
>
/me discovers that the PASSWORD_TYPE constants are private to simple_providers.c
Document this? If someone adds a password type and greps for
SIMPLE_PASSWORD_TYPE (try this), they won't know that somewhere in the file
there is a list of "special" password types that SIMPLE isn't one of them...
> > Thanks Stefan for your work on this branch; it looks good to me.
> >
> > Daniel
>
> Thanks heaps for your review :)
>
>
:)
Daniel
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Review of dont-* branch
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Apr 29, 2008 at 08:29:46PM +0300, Daniel Shahaf wrote:
> > /** A cancellation function/baton pair to be passed as the baton argument
> > * to the @c svn_cmdline_*_prompt functions.
> > *
> > * @since New in 1.4.
> > + * @deprecated Provided for backward compatibility with the 1.5 API.
> > */
> > typedef struct svn_cmdline_prompt_baton_t {
>
> Should svn_cmdline_prompt_baton_t's docstring point to svn_cmdline_prompt_baton2_t?
Yes, I will do that.
> > @@ -253,6 +266,18 @@ svn_cmdline_auth_ssl_client_cert_pw_prompt
> > svn_boolean_t may_save,
> > apr_pool_t *pool);
> >
> > +/** An implementation of @c svn_auth_plaintext_prompt_func_t that
> > + * prompts the user whether storing unencypted passwords to disk
> > + * is OK on the command line.
> ^^^^^^^^^^^^^^^^^^^
> "Storing unencrypted passwords ... on the command line"? As opposed to
> storing them (the passwords) on the kitchen table? ;)
:)
I will simply drop that part of the docstring.
> > +/** @brief Indicates whether providers may save passwords to disk in
> > + * plaintext. Property value can be either SVN_CONFIG_TRUE,
> > + * SVN_CONFIG_FALSE, or SVN_CONFIG_PROMPT. */
> ^^^^^^
> s/PROMPT/ASK/
Thanks, will fix.
> > @@ -587,9 +627,12 @@ svn_error_t * svn_auth_next_credentials(void **cre
> > /** Save a set of credentials.
> > *
> > * Ask @a state to store the most recently returned credentials,
> > - * presumably because they successfully authenticated. Use @a pool
> > - * for temporary allocation. If no credentials were ever returned, do
> > - * nothing.
> > + * presumably because they successfully authenticated.
> > + * All allocations should be done in @a pool, which can be
> ^^^^^^
> Since it's a function we implement, not a callback, might as well
> s/can be/is/.
Yes.
> If we do not rev this API, we need to make sure it continues to work for
> pre-1.6 callers as well. In this case, what happens if the pool does
> *not* survive across RA sessions? Does it only not cache the user's
> answers to the prompt, or could it segfault? (I think the former is
> fine.)
Mmmmmh, good question.
I think the caching should only be done if the client provides a
prompt callback. Currently the code tries to look up answers in the
cache unconditionally. I will fix that, and make the prompt callback's
docstring mention the pool lifetime issue wrt svn_auth_provider_t's
save_credentials.
This way, simple_save_creds_helper will segfault if, and only if:
1. The client provides a plaintext prompt callback
2. And the client passes a pool to save_credentials that does not
survive across RA sessions
3. And the user causes two RA sessions to be opened (e.g. by
supplying multiple URLs on the command line)
I think this is a reasonable compromise, given that we change semantics
of old API (save_credentials) only if new API is also used (the prompt
callback), and that the three conditions taken together are quite some
corner case. I don't think client developers will mind this much.
Unfortunately, it looks like we cannot provide the true semantics of the
old API in this case without defaulting to storing plaintext passwords
again. There is, AFAIK, no way for us to determine the lifetime of the
pool we're handed.
> > Index: subversion/libsvn_subr/config_file.c
> > ===================================================================
> > --- subversion/libsvn_subr/config_file.c (.../trunk) (revision 30801)
> > +++ subversion/libsvn_subr/config_file.c (.../branches/dont-save-plaintext-passwords-by-default) (revision 30836)
> > @@ -897,10 +900,19 @@ svn_config_ensure(const char *config_dir, apr_pool
> > + "[auth]" NL
> > "### Section for authentication and authorization customizations." NL
> > - "[auth]" NL
>
> Was this change involuntary?
Yes, thanks.
> > - /* There are two different ways the user can disable disk caching
> > - of credentials: either via --no-auth-cache, or in the config
> > - file ('store-auth-creds = no'). */
> > + /* Determine whether we are allowed to write to the auth/ area.
> > + * This is the deprecated location for this option, the new
> > + * location is SVN_CONFIG_CATEGORY_SERVERS. */
>
> So, if I understand correctly, the setting in ~/.subversion/config is
> passed as an SVN_AUTH_PARAM_* parameter, and the setting(s) in
> ~/.subversion/servers are read and used in svn_ra_open3()?
I don't really understand what your question is getting at.
svn_ra_open3 _also_ sets SVN_AUTH_PARAM_* parameters. It just gets
the values from the servers file, instead of the config file.
The providers are shielded from the configuration details, e.g.
which file parameters come from. They only ever see SVN_AUTH_PARAM_*
parameters.
Does that make sense? What was your question?
> It is not entirely obvious, when reading this function in isolation or
> reading svn_ra_open3() in isolation, where the other config file's
> options are handled. Maybe add cross-reference comments between them?
I'm not comfortable with cross-referencing function names in comments
in different files (let alone libraries), because the comments may get
obsoleted and cause confusion rather then help, if not updated properly.
I will make the comments in svn_cmdline_setup_auth_baton mention
that "the RA layer" may override auth settings. That should be
generic enough to be valid for at least a good long while.
I don't think putting a comment about options specified in 'config'
into the RA layer code makes much sense. It should be clear that
the RA layer gets its settings from 'servers', and that the command
line client reads 'config' independently of what the RA layer is doing.
If this is not apparent to the reader, than they simply haven't
read enough of our code yet :)
Note, for example, the code in libsvn_ra_neon/session.c, which has
been reading 'servers' for ages, and never mentions 'config' in
any way. When cross-referencing 'config' from svn_ra_open3(), we
might as well add comments to libsvn_ra_neon about 'config'.
And that does not really make sense, I think. We'd be documenting
behaviour of one library in another.
> svn_cstring_casecmp() is:
>
> /**
> * Compare two strings @a atr1 and @a atr2, treating case-equivalent
> * unaccented Latin (ASCII subset) letters as equal.
> */
> int svn_cstring_casecmp(const char *str1, const char *str2);
>
> Do we want to also consider non-ASCII characters as equal regardless of case?
I guess we could use apr_strnatcasecmp() instead.
This maps characters to their upper-case equivalent via 'toupper',
which works with non-ASCII characters on some system, based on what
the C library does, whether the system supports locales, what day
of week it is, whether the sun is out or whether it's raining,
and apparently some other magic factors.
> > @@ -386,10 +446,12 @@ svn_cmdline_prompt_user2(const char **result,
> > svn_cmdline_prompt_baton_t *baton,
> > apr_pool_t *pool)
> > {
> > - return prompt(result, prompt_str, FALSE /* don't hide input */, baton, pool);
> > + /* XXX: We know prompt doesn't use the new members
> > + * of svn_cmdline_prompt_baton2_t. */
> > + return prompt(result, prompt_str, FALSE /* don't hide input */,
> > + (svn_cmdline_prompt_baton2_t *)baton, pool);
>
> Add a reverse pointer, from prompt()'s docstring to this comment? If
> prompt() started using the new members, this function will need to be
> adjusted.
Yes, thanks.
> > Index: subversion/libsvn_subr/simple_providers.c
> > ===================================================================
> > --- subversion/libsvn_subr/simple_providers.c (.../trunk) (revision 30801)
> > +++ subversion/libsvn_subr/simple_providers.c (.../branches/dont-save-plaintext-passwords-by-default) (revision 30836)
> > @@ -258,46 +272,117 @@ simple_save_creds_helper(svn_boolean_t *saved,
> > + /* Don't store passwords in any form if the user has told
> > + * us not to do so. */
> > + if (! dont_store_passwords)
> > {
> > + svn_boolean_t may_save_password = FALSE;
> >
> > + /* If the password is going to be stored encrypted, go right
> > + * ahead and store it to disk. Else determine whether saving
> > + * in plaintext is OK. */
> > + if (strcmp(passtype, SVN_AUTH__WINCRYPT_PASSWORD_TYPE) == 0
> > + || strcmp(passtype, SVN_AUTH__KEYCHAIN_PASSWORD_TYPE) == 0)
>
> Do we want to hardcode which password types are encrypted?
Well, what's the alternative?
> > Index: TODO.branch
> > ===================================================================
> > --- TODO.branch (.../trunk) (revision 0)
> > +++ TODO.branch (.../branches/dont-save-plaintext-passwords-by-default) (revision 30836)
>
> (included for completeness) File should be removed before merging to trunk.
Sure, I will do that.
> Thanks Stefan for your work on this branch; it looks good to me.
>
> Daniel
Thanks heaps for your review :)
--
Stefan Sperling <st...@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