Re: [PATCH] Cache ssl client cert passphrase in gnome-keyring

On Wed, May 28, 2008 at 04:35:36PM +0530, Senthil Kumaran S wrote:
> Hi,
>
> As a follow up to the patch posted here
> http://svn.haxx.se/dev/archive-2008-05/0337.shtml and the discussion went
> on from there, I am posting a new patch which uses the gnome-keyring to
> store the ssl client cert passphrase. Apart from this when we don't have
> gnome-keyring support enabled, this patch makes it possible for the user
> to cache the passphrase in the plaintext form in auth area, _only_ if the
> user desires.
>
> This patch is based on the work done on
> "dont-save-plaintext-passwords-by-default" and "gnome-keyring" branches.
> It implements the caching of the passphrase in the same way as done in
> these branches.
>
> I will start working on extending this for "KWallet" and "CryptAPI",
> before which I would like to receive comments about this patch and get
> this into trunk, if this looks fine.

Some comments inline.

> @@ -780,6 +794,24 @@
> "### this option has no effect if either 'store-passwords' or " NL
> "### 'store-auth-creds' is set to 'no'." NL
> "###" NL
> + "### Set store-ssl-client-cert-pp to 'no' to avoid storing ssl" NL
> + "### client certificate passphrase keys in the auth/ area of your" NL

What are "client certificate passphrase keys"?

Shouldn't this be "client certificate key passphrases" or just "client
certificate passphrases" ?

> + if (non_interactive == FALSE)
> + {
> + /* This provider is odd in that it isn't a prompting provider in
> + the classic sense. That is, it doesn't need to prompt in
> + order to get creds, but it *does* need to prompt the user
> + regarding the *cache storage* of creds. */
> + svn_auth_get_ssl_client_cert_pw_file_provider2
> + (&provider, svn_cmdline_auth_plaintext_passphrase_prompt,
> + pb, pool);
> + }

I would argue that we don't need this comment anymore. With this
patch, we already have two providers prompting the user regarding
cache storage.

I suppose we could change it to something like this instead:

     /* This provider doesn't prompt the user in order to get creds.
      * It prompts the user regarding the caching of creds. */

This is equally informative, and does not assume that the reader is
making a distinction between "odd" and "classic" providers.

The same comment appears where the plaintext password prompt provider
is initialised. Please fix it there, also.

> + /* Try to load passphrase from the auth/ cache. */
> + err = svn_config_read_auth_data(&creds_hash,
> + SVN_AUTH_CRED_SSL_CLIENT_CERT_PW,
> + realmstring, config_dir, pool);
> + svn_error_clear(err);
> + if (! err && creds_hash)
> + {
> + if (!passphrase_get(&password, creds_hash, realmstring,
> + NULL, non_interactive, pool))
> + password = NULL;
> +
> +/* svn_string_t *str; */
> +/* str = apr_hash_get(creds_hash, SVN_AUTH__AUTHFILE_PASSPHRASE_KEY, */
> +/* APR_HASH_KEY_STRING); */
> +/* if (str && str->data) */
> +/* password = str->data; */
> + }

Why are these lines commented out?

> +/* Save passphrase for a client certificate in auth/ cache */
> +svn_error_t *
> +svn_auth__ssl_client_cert_pw_file_save_creds_helper
> + (svn_boolean_t *saved,
> + void *credentials,
> + void *provider_baton,
> + apr_hash_t *parameters,
> + const char *realmstring,
> + svn_auth__password_set_t passphrase_set,
> + const char *passtype,
> + apr_pool_t *pool)
> +{

This function duplicates a lot of code. Would it be feasible to merge it
with the function it is based on?

> +/* This implements 'svn_auth_plaintext_passphrase_prompt_func_t'. */
> +svn_error_t *
> +svn_cmdline_auth_plaintext_passphrase_prompt(svn_boolean_t *may_save_plaintext,
> + const char *realmstring,
> + void *baton,
> + apr_pool_t *pool)
> +{

This one also duplicates some code.
Could we merge this function with the one it is based on?
The merged function could take more parameters than the original (e.g.
the banner above the prompt could be a new parameter).

> +/** The type of function returning authentication provider. */
> +typedef void (*svn_auth_ssl_client_cert_pw_provider_func_t)
> + (svn_auth_provider_object_t **provider,
> + apr_pool_t *pool);
> +

This docstring is a bit terse. What about:

/** A function returning an SSL client certificate passphrase provider. */

Thanks,
Stefan