Re: [PATCH] Cache ssl client certificate passphrases

Hi Senthil,

Nice patch! I did not verify the functionality of the patch, but I've
got few comments while I skimmed through the patch. Please see my
comments inline.

On Thu, 2008-05-08 at 14:37 +0530, Senthil Kumaran S wrote:
> Hi Stefan,
>
> Stefan Sperling wrote:
> > could we have the default be 'no' (easy), or add a prompt similar to
>
> I am attaching an updated patch which has the default as 'no'.
>
> The log message remains the same as in my previous email :)

> Index: subversion/include/svn_auth.h
> ===================================================================
> --- subversion/include/svn_auth.h (revision 31082)
> +++ subversion/include/svn_auth.h (working copy)
> @@ -568,6 +568,12 @@
> #define SVN_AUTH_PARAM_STORE_PLAINTEXT_PASSWORDS
> SVN_AUTH_PARAM_PREFIX \
>
> "store-plaintext-passwords"
>
> +/** @brief The application wants providers to save ssl client
       ^^^^^^
You may want to remove this word?

> Index: subversion/libsvn_subr/config_file.c
> ===================================================================
> --- subversion/libsvn_subr/config_file.c (revision 31082)
> +++ subversion/libsvn_subr/config_file.c (working copy)
> @@ -497,6 +497,15 @@
> svn_error_clear(err);
> svn_error_clear(svn_io_dir_make(auth_subdir, APR_OS_DEFAULT,
> pool));
> }
> +
> + auth_subdir = svn_path_join_many(pool, auth_dir,
> + SVN_AUTH_CRED_SSL_CLIENT_CERT_PW,
> NULL);
> + err = svn_io_check_path(auth_subdir, &kind, pool);
> + if (err || kind == svn_node_none)

It can be better written as, or i would say that's how this function has
been used throughout the code base:

     SVN_ERR(svn_io_check_path(auth_subdir, &kind, pool));
     if (kind == svn_node_noe)

> Index: subversion/libsvn_subr/ssl_client_cert_pw_providers.c
> ===================================================================

> + if (! password)
> + {
> + svn_error_t *err;
> + apr_hash_t *creds_hash = NULL;
> + const char *config_dir = apr_hash_get(parameters,
> +
> SVN_AUTH_PARAM_CONFIG_DIR,
> + APR_HASH_KEY_STRING);
> +
> + /* 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)
> + {
> + 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;
> + }
> + }
> +
> if (password)

You can make it as a else loop, because earlier you are checking for:
    if (! password)

> @@ -67,11 +94,63 @@
> }
>
>
> +/* Save passphrase for a client certificate in auth/ cache */
> +static svn_error_t *
> +ssl_client_cert_pw_file_save_credentials(svn_boolean_t *saved,
> + void *credentials,
> + void *provider_baton,
> + apr_hash_t *parameters,
> + const char *realmstring,
> + apr_pool_t *pool)
> +{
> + svn_auth_cred_ssl_client_cert_pw_t *creds = credentials;
> + apr_hash_t *creds_hash = NULL;
> + const char *config_dir;
> + svn_error_t *err;
> +
> + svn_boolean_t store_ssl_client_cert_passphrase =
> + apr_hash_get(parameters,
> + SVN_AUTH_PARAM_STORE_SSL_CLIENT_CERT_PP,
> + APR_HASH_KEY_STRING) != NULL;
> +
> + svn_boolean_t no_auth_cache =
> + (! creds->may_save) || (apr_hash_get(parameters,
> +
> SVN_AUTH_PARAM_NO_AUTH_CACHE,
> + APR_HASH_KEY_STRING) !=
> NULL);
> +
> + *saved = FALSE;
> +
> + if (no_auth_cache)
> + return SVN_NO_ERROR;
> +
> + config_dir = apr_hash_get(parameters,
> + SVN_AUTH_PARAM_CONFIG_DIR,
> + APR_HASH_KEY_STRING);
> +
> + creds_hash = apr_hash_make(pool);
> +
> + if (store_ssl_client_cert_passphrase)
> + {
> + apr_hash_set(creds_hash, SVN_AUTH__AUTHFILE_PASSPHRASE_KEY,
> + APR_HASH_KEY_STRING,
> + svn_string_create(creds->password, pool));
> + /* Save credentials to disk. */
> + err = svn_config_write_auth_data(creds_hash,

I smell you write the credentials to cache even if passphrase is not
changed. Do you want to do that? How about verifying the passphrase
before saving it in cache? If it is changed, cache it; otherwise do not
cache it.

> Index: subversion/libsvn_ra_neon/session.c
> ===================================================================
> --- subversion/libsvn_ra_neon/session.c (revision 31082)
> +++ subversion/libsvn_ra_neon/session.c (working copy)
> @@ -290,6 +290,10 @@
>
> if (ne_ssl_clicert_decrypt(clicert, pw_creds->password) == 0)
> {
> + error = svn_auth_save_credentials(state, pool);
> + if (error)
> + svn_error_clear(error);

Can we rewrite it as follows (it is more of a question)?
   SVN_ERR(svn_auth_save_credentials(state, pool));

Thanks!

-- 
Regards,
Bhuvaneswaran