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

Hi Stefan,

Stefan Sperling wrote:
> Some comments inline.

Thanks for your comments. I am attaching an updated patch based on your comments.

>> @@ -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" ?

It should be "client certificate passphrases".

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

Yes this is fixed in the new patch.

>> + /* 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?

Oops that was a mistake. Removed :)

>> +/* 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?

The original function handles the caching of username and password, but here we
do only passphrase caching. Yes, it duplicates code, but it is difficult to
identify whether we are using this function for SVN_AUTH_CRED_SIMPLE or
SVN_AUTH_CRED_SSL_CLIENT_CERT_PW since it is hardcoded in the function. I haven
changed it in the updated patch.

>> +/* 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).

Yes this is merged now.

Updated log message.
[[[
Cache ssl client certificate passphrase.

* subversion/libsvn_ra/ra_loader.c
   (svn_ra_open3): Load config options for storing passphrase from servers
    config file.

* subversion/libsvn_subr/config_file.c
   (ensure_auth_dirs): Create new auth dir to store ssl client cert passphrase.
   (svn_config_ensure): Add doc for the new options in the servers file string.

* subversion/libsvn_subr/cmdline.c
   (get_auth_ssl_client_cert_pw_provider): New function to load ssl client cert
    password provider dynamically.
   (svn_cmdline_setup_auth_baton): If we have gnome keyring support get the
    corresponding ssl client cert passphrase provider.

* subversion/libsvn_subr/ssl_client_cert_pw_providers.c
   (): Include some private headers. Define SVN_AUTH__AUTHFILE_PASSPHRASE_KEY,
    SVN_AUTH__AUTHFILE_PASSTYPE_KEY.
   (ssl_client_cert_pw_file_provider_baton_t): New baton for ssl client cert
    passphrase provider.
   (simple_passphrase_get): New function to get plaintext passphrase.
   (simple_passphrase_set): New function to store plaintext passphrase.
   (ssl_client_cert_pw_file_first_credentials): Move logic to new helper.
   (ssl_client_cert_pw_file_save_credentials): Move logic to new helper.
   (svn_auth__ssl_client_cert_pw_file_first_creds_helper): New helper function
    for ssl_client_cert_pw_file_first_credentials.
   (svn_auth__ssl_client_cert_pw_file_save_creds_helper): New helper function
    for ssl_client_cert_pw_file_save_credentials.
   (ssl_client_cert_pw_file_provider): Add provision for saving credentials.
   (svn_auth_get_ssl_client_cert_pw_file_provider2): New public API which has
    a prompt function now.
   (svn_auth_get_ssl_client_cert_pw_file_provider): Update API for the above.

* subversion/libsvn_subr/prompt.c
   (plaintext_prompt_helper): New function which has the logic for plaintext
    promting functions.
   (svn_cmdline_auth_plaintext_prompt): Move logic to above function.
   (svn_cmdline_auth_plaintext_passphrase_prompt): New prompt function for
    plaintext passphrase prompt.

* subversion/libsvn_auth_gnome_keyring/gnome_keyring.c
   (gnome_keyring_ssl_client_cert_pw_first_creds): New function to get ssl
    client cert passphrase from encrypted credentials.
   (gnome_keyring_ssl_client_cert_pw_save_creds): New function to save
    encrypted ssl client cert passphrase.
   (gnome_keyring_ssl_client_cert_pw_provider): New baton.
   (svn_auth_get_gnome_keyring_ssl_client_cert_pw_provider): New public API for
    gnome keyring based ssl client cert passphrase storage and retrieval.

* subversion/include/svn_config.h
   (SVN_CONFIG_OPTION_STORE_SSL_CLIENT_CERT_PP): New option to store ssl client
    cert passphrase.
   (SVN_CONFIG_OPTION_STORE_PLAINTEXT_PASSPHRASE): New option to store plaintext
    passphrase.
   (SVN_CONFIG_DEFAULT_OPTION_STORE_PASSPHRASE): New default option for storing
    passphrase set to 'yes'.
   (SVN_CONFIG_DEFAULT_OPTION_STORE_PLAINTEXT_PASSPHRASE): New default option to
    store plaintext passphrase set to 'ask'.

* subversion/include/svn_cmdline.h
   (svn_cmdline_auth_plaintext_passphrase_prompt): New public API added to
    prompt for storing plaintext passphrases.

* subversion/include/private/svn_auth_private.h
   (svn_auth__ssl_client_cert_pw_file_first_creds_helper): New private function.
   (svn_auth__ssl_client_cert_pw_file_save_creds_helper): New private function.

* subversion/include/svn_auth.h
   (svn_auth_ssl_client_cert_pw_provider_func_t): Define function type for the
    provider.
   (svn_auth_plaintext_passphrase_prompt_func_t): New function prototype.
   (SVN_AUTH_PARAM_DONT_STORE_PASSPHRASE): New constant.
   (SVN_AUTH_PARAM_STORE_PLAINTEXT_PASSPHRASE): New constant.
   (svn_auth_get_ssl_client_cert_pw_file_provider2): New public API.

* subversion/libsvn_ra_neon/session.c
   (client_ssl_decrypt_cert): Call svn_auth_save_credentials to save the ssl
    client certificate passphrase.

Patch by: stylesen
]]]

Thank You.

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