Re: [PATCH] Cache ssl client certificate passphrases

Hi Bhuvan,

Bhuvaneswaran A wrote:
> 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

Thanks for the review.

>> #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?

That is used by doxygen (http://svn.collab.net/svn-doxygen/) in order to give a
brief summary about this parameter.

>> + 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)

This is basically to skip the error and continue, in this case to create the
directory if it does not exists, even if we are unable to create the directory
for some reason we don't want to stop there. If we have SVN_ERR, then the
application will stop there.

>> 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)

Yes I shall do it in my next patch.

>> + 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.

No. This is not true. This is checked via creds->may_save variable. If that
variable is false then only we are saving the credentials. Its done in the
following line:

<snip>
  svn_boolean_t no_auth_cache =
     (! creds->may_save) || (apr_hash_get(parameters,
                                          SVN_AUTH_PARAM_NO_AUTH_CACHE,
                                          APR_HASH_KEY_STRING) != NULL);
</snip>

>> 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));

Again the same thing as explained above.

-- 
Senthil Kumaran S
http://www.stylesen.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org