You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Barry Scott <ba...@barrys-emacs.org> on 2004/01/17 19:25:59 UTC
svncpp crash in ssl_server_trust_file_save_credentials
I'm testing SSL callbacks for the pysvn python extension and I'm seeing a
reproducible crash with 0.36.0.
Pysvn uses the svncpp library from rapidsvn patched to support 0.36.0.
I don't know if its the svncpp code or the svn code that is in
error here. Can an SSL auth export comment on where a fix needs
to be made please?
The crash occurs in svn_config_write_auth_data that is passed the
realmstring as NULL from ssl_server_trust_file_save_credentials
(ssl_server_providers.c).
This is because pb->realmstring is NULL in
ssl_server_trust_file_save_credential.
With the subversion client pb->realmstring will be setup by the code in
ssl_server_trust_file_first_credentials.
But the svncpp code has its own provider that implements the
first_credentials callback but not the save_credentials callback.
The code in svn_auth_save_credentials (libsvn_subr/auth.c) is designed
to hunt for a save_creditials callback. It loops over the providers,
and ends up calling ssl_server_trust_file_save_credentials has no
way of being given the realmstring and a crash occurs.
0.35.1 did not crash, it also did not save credential at all. I'm
guess that bug was fixed and exposed this new one.
The svncpp code that handles this is in src\svncpp\context.cpp in
the rapidsvn repository. I've applied this patch against the latest
version to support 0.36.0.
Barry
Index: context.cpp
===================================================================
--- context.cpp (revision 6998)
+++ context.cpp (working copy)
@@ -144,8 +144,9 @@
*(svn_auth_provider_object_t **)apr_array_push (providers) =
provider;
+ // plugged in 3 as the retry limit - what is a good limit?
svn_client_get_ssl_client_cert_pw_prompt_provider (
- &provider, onSslClientCertPwPrompt, this, pool);
+ &provider, onSslClientCertPwPrompt, this, 3, pool);
*(svn_auth_provider_object_t **)apr_array_push (providers) =
provider;
@@ -285,12 +286,13 @@
void *baton,
const char *realm,
const char *username,
+ svn_boolean_t may_save,
apr_pool_t *pool)
{
Data * data;
SVN_ERR (getData (baton, &data));
- if (!data->retrieveLogin (username, realm))
+ if (!data->retrieveLogin (username, realm, may_save != 0))
return svn_error_create (SVN_ERR_CANCELLED, NULL, "");
svn_auth_cred_simple_t* lcred = (svn_auth_cred_simple_t*)
@@ -316,6 +318,7 @@
const char *realm,
apr_uint32_t failures,
const svn_auth_ssl_server_cert_info_t *info,
+ svn_boolean_t may_save,
apr_pool_t *pool)
{
Data * data;
@@ -329,11 +332,12 @@
trustData.validFrom = info->valid_from;
trustData.validUntil = info->valid_until;
trustData.issuerDName = info->issuer_dname;
+ trustData.maySave = may_save != 0;
apr_uint32_t acceptedFailures;
ContextListener::SslServerTrustAnswer answer =
data->listener->contextSslServerTrustPrompt (
- trustData, acceptedFailures);
+ trustData, acceptedFailures );
if(answer == ContextListener::DONT_ACCEPT)
*cred = NULL;
@@ -345,7 +349,7 @@
if (answer == ContextListener::ACCEPT_PERMANENTLY)
{
- cred_->trust_permanently = 1;
+ cred_->may_save = 1;
cred_->accepted_failures = acceptedFailures;
}
*cred = cred_;
@@ -390,13 +394,15 @@
onSslClientCertPwPrompt (
svn_auth_cred_ssl_client_cert_pw_t **cred,
void *baton,
+ const char *realm,
+ svn_boolean_t may_save,
apr_pool_t *pool)
{
Data * data;
SVN_ERR (getData (baton, &data));
std::string password ("");
- if (!data->listener->contextSslClientCertPwPrompt (password))
+ if (!data->listener->contextSslClientCertPwPrompt (password, realm,
may_save != 0))
return svn_error_create (SVN_ERR_CANCELLED, NULL, "");
svn_auth_cred_ssl_client_cert_pw_t *cred_ =
@@ -473,7 +479,8 @@
*/
bool
retrieveLogin (const char * username_,
- const char * realm)
+ const char * realm,
+ bool may_save)
{
bool ok;
@@ -485,7 +492,7 @@
else
username = username_;
- ok = listener->contextGetLogin (realm, username, password);
+ ok = listener->contextGetLogin (realm, username, password, may_save);
return ok;
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svncpp crash in ssl_server_trust_file_save_credentials
Posted by Tobias Ringstrom <to...@ringstrom.mine.nu>.
Barry Scott wrote:
> I'm testing SSL callbacks for the pysvn python extension and I'm seeing a
> reproducible crash with 0.36.0.
>
> Pysvn uses the svncpp library from rapidsvn patched to support 0.36.0.
>
> I don't know if its the svncpp code or the svn code that is in
> error here. Can an SSL auth export comment on where a fix needs
> to be made please?
>
> The crash occurs in svn_config_write_auth_data that is passed the
> realmstring as NULL from ssl_server_trust_file_save_credentials
> (ssl_server_providers.c).
>
> This is because pb->realmstring is NULL in
> ssl_server_trust_file_save_credential.
>
> With the subversion client pb->realmstring will be setup by the code in
> ssl_server_trust_file_first_credentials.
>
> But the svncpp code has its own provider that implements the
> first_credentials callback but not the save_credentials callback.
Interesting. Yes, this is a horrible (and quite old) misuse of the
provider baton. I can see two ways to fix this: We can either add the
realmstring to all credential structs, or we can add a realmstring to
the next and save providers functions. The cred_kind/realmstring pair is
the key when you search for credentials, so it makes a lot of sense to
go for the second alternative, i.e. to add a realmstring parameter to
the next and save functions.
Any objections?
/Tobias
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svncpp crash in ssl_server_trust_file_save_credentials
Posted by Tobias Ringstrom <to...@ringstrom.mine.nu>.
Barry Scott wrote:
> At 21-01-2004 13:42, Tobias Ringstrom wrote:
> Let me check I understand.
>
> With a well designed GUI the store-auth-creds in the config file is not
> required.
Sure. I wanted to remove the whole option, but the idea met some
resistance. Since we are stuck with the option, it's good to indicate to
the user that saving has been globally disabled.
> I have this in my config:
>
> [auth]
> store-password = no
> store-auth-creds = no
>
> and may_save is passed in as 1 when I access a repo via HTTP:
> that needs a username/password.
It could be that your client does not check the config file and updates
the auth parameters as the cmdline client does, i.e.
/* 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'). */
if ((err = svn_config_get_bool (cfg, &store_password_val,
SVN_CONFIG_SECTION_AUTH,
SVN_CONFIG_OPTION_STORE_AUTH_CREDS,
TRUE)))
svn_handle_error (err, stderr, TRUE);
if (opt_state.no_auth_cache || !store_password_val)
svn_auth_set_parameter(ab, SVN_AUTH_PARAM_NO_AUTH_CACHE,
(void *) "");
This should really be done by libsvn_subr, but fixing that requires
another API change. Sussman and I discussed this problem on IRC a week ago.
/Tobias
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svncpp crash in ssl_server_trust_file_save_credentials
Posted by Barry Scott <ba...@barrys-emacs.org>.
Hmmm, I've broken pysvn/svnpcpp somewhere in supporting may_save. I cannot
save credentials at all now... I'll debug the save logic and test the use
cases to find out how may_save and the config file options work together.
Barry
At 21-01-2004 23:16, you wrote:
>At 21-01-2004 13:42, Tobias Ringstrom wrote:
>Let me check I understand.
>>>The may_save means that there is no provider capable of saving the
>>>credentials.
>>>However in the case of the svncpp library I will always see may_save as
>>>true as it adds the file providers. Hence I could remove the may_save
>>>flag for the API and simplify the C++ and pysvn API.
>>>I'm guessing you are going to say I have this wrong :-)
>>
>>I'm afraid I have to. It is possible to globally disable saving of
>>authentication credentials by setting store-auth-creds to "no" in the
>>config file. The command line client also has an option --no-auth-cache
>>which does the same thing. The may_save that is passed to the prompt
>>function is set to FALSE if any of these options are used.
>
>With a well designed GUI the store-auth-creds in the config file is not
>required.
>I can see I'll end up with a FAQ pointing out the problems that
>store-auth-creds
>cause a GUI client.
>
>I have this in my config:
>
>[auth]
>store-password = no
>store-auth-creds = no
>
>and may_save is passed in as 1 when I access a repo via HTTP:
>that needs a username/password. But as you say the creds are not
>stored when I set may_save to 1 in the creds struct.
>
>Being passed 1 for may_save in the case is a bug with svn auth?
>
>Barry
>
>
>
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: dev-unsubscribe@rapidsvn.tigris.org
>For additional commands, e-mail: dev-help@rapidsvn.tigris.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svncpp crash in ssl_server_trust_file_save_credentials
Posted by Barry Scott <ba...@barrys-emacs.org>.
At 21-01-2004 13:42, Tobias Ringstrom wrote:
Let me check I understand.
>>The may_save means that there is no provider capable of saving the
>>credentials.
>>However in the case of the svncpp library I will always see may_save as
>>true as it adds the file providers. Hence I could remove the may_save
>>flag for the API and simplify the C++ and pysvn API.
>>I'm guessing you are going to say I have this wrong :-)
>
>I'm afraid I have to. It is possible to globally disable saving of
>authentication credentials by setting store-auth-creds to "no" in the
>config file. The command line client also has an option --no-auth-cache
>which does the same thing. The may_save that is passed to the prompt
>function is set to FALSE if any of these options are used.
With a well designed GUI the store-auth-creds in the config file is not
required.
I can see I'll end up with a FAQ pointing out the problems that
store-auth-creds
cause a GUI client.
I have this in my config:
[auth]
store-password = no
store-auth-creds = no
and may_save is passed in as 1 when I access a repo via HTTP:
that needs a username/password. But as you say the creds are not
stored when I set may_save to 1 in the creds struct.
Being passed 1 for may_save in the case is a bug with svn auth?
Barry
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svncpp crash in ssl_server_trust_file_save_credentials
Posted by Tobias Ringstrom <to...@ringstrom.mine.nu>.
Barry Scott wrote:
>
>> No, that is not how it works. If the may_save argument to the prompt
>> function is FALSE, it means that it is impossible to save. Even if you
>> set may_save in the creds struct to TRUE, it will not be saved. If the
>> application wants to remember a default setting for the user, it must
>> do so in an application specific way (probably utilizing the prompt
>> baton). That has nothing to do with the svn libs.
>
>
> Let me check I understand.
>
> The may_save means that there is no provider capable of saving the
> credentials.
>
> However in the case of the svncpp library I will always see may_save as
> true as it adds the file providers. Hence I could remove the may_save flag
> for the API and simplify the C++ and pysvn API.
>
> I'm guessing you are going to say I have this wrong :-)
I'm afraid I have to. It is possible to globally disable saving of
authentication credentials by setting store-auth-creds to "no" in the
config file. The command line client also has an option --no-auth-cache
which does the same thing. The may_save that is passed to the prompt
function is set to FALSE if any of these options are used.
/Tobias
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svncpp crash in ssl_server_trust_file_save_credentials
Posted by Barry Scott <ba...@barrys-emacs.org>.
>No, that is not how it works. If the may_save argument to the prompt
>function is FALSE, it means that it is impossible to save. Even if you set
>may_save in the creds struct to TRUE, it will not be saved. If the
>application wants to remember a default setting for the user, it must do
>so in an application specific way (probably utilizing the prompt baton).
>That has nothing to do with the svn libs.
Let me check I understand.
The may_save means that there is no provider capable of saving the credentials.
However in the case of the svncpp library I will always see may_save as true
as it adds the file providers. Hence I could remove the may_save flag for the
API and simplify the C++ and pysvn API.
I'm guessing you are going to say I have this wrong :-)
Barry
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svncpp crash in ssl_server_trust_file_save_credentials
Posted by Tobias Ringstrom <to...@ringstrom.mine.nu>.
Barry Scott wrote:
> See issue #153 it has the full text of the patch, including the fix.
Thanks!
>> a little hard to read the patch. I'm very interested in how you handle
>> the may_save boolean in the C++ API. The idea is that the may_save
>> boolean that is passed to the prompt function is to be used to enable
>> or disable (i.e. gray out) a "remember name and password" checkbox in
>> a prompt dialog. If the user checks that box, the may_save in the
>> creds struct shall be set to TRUE. It's handled in the same way for
>> most (if not all) providers. Just send me an email if I can help.
>
> The may_save should default the check box to save/don't save as the users
> preference. Why would I force the user to edit an icky text file before
> the GUI allows them to save credientials?
No, that is not how it works. If the may_save argument to the prompt
function is FALSE, it means that it is impossible to save. Even if you
set may_save in the creds struct to TRUE, it will not be saved. If the
application wants to remember a default setting for the user, it must do
so in an application specific way (probably utilizing the prompt baton).
That has nothing to do with the svn libs.
/Tobias
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svncpp crash in ssl_server_trust_file_save_credentials
Posted by Barry Scott <ba...@barrys-emacs.org>.
At 20-01-2004 22:25, Tobias Ringström wrote:
>Barry Scott wrote:
>>Thanks for taking the trouble to look at the svncpp code.
>
>It's was a pleasure. Getting rapidsvn/svncpp up to date is a very
>worthwhile goal.
Or in my case svncpp and pysvn, I'm working on the python svn GUI Workbench
rather then rapidsvn.
>>Reordering the providers to put the file ones first fixes the crash.
>>I'll update the svncpp patch for 0.36.0 support to include this fix.
>
>Cool. I couldn't get your patch to apply properly (probably due to MUA
>damage) so it was
See issue #153 it has the full text of the patch, including the fix.
>a little hard to read the patch. I'm very interested in how you handle the
>may_save boolean in the C++ API. The idea is that the may_save boolean
>that is passed to the prompt function is to be used to enable or disable
>(i.e. gray out) a "remember name and password" checkbox in a prompt
>dialog. If the user checks that box, the may_save in the creds struct
>shall be set to TRUE. It's handled in the same way for most (if not all)
>providers. Just send me an email if I can help.
The may_save should default the check box to save/don't save as the users
preference. Why would I force the user to edit an icky text file before
the GUI allows them to save credientials?
The alternative design to the may_save flag is an API to query the svn
preferences. Is there such an API? I have not looked.
Barry
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svncpp crash in ssl_server_trust_file_save_credentials
Posted by Tobias Ringström <to...@ringstrom.mine.nu>.
Barry Scott wrote:
> Thanks for taking the trouble to look at the svncpp code.
It's was a pleasure. Getting rapidsvn/svncpp up to date is a very
worthwhile goal.
> Reordering the providers to put the file ones first fixes the crash.
> I'll update the svncpp patch for 0.36.0 support to include this fix.
Cool. I couldn't get your patch to apply properly (probably due to MUA
damage) so it was a little hard to read the patch. I'm very interested
in how you handle the may_save boolean in the C++ API. The idea is that
the may_save boolean that is passed to the prompt function is to be used
to enable or disable (i.e. gray out) a "remember name and password"
checkbox in a prompt dialog. If the user checks that box, the may_save
in the creds struct shall be set to TRUE. It's handled in the same way
for most (if not all) providers. Just send me an email if I can help.
/Tobias
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svncpp crash in ssl_server_trust_file_save_credentials
Posted by Barry Scott <ba...@barrys-emacs.org>.
At 20-01-2004 14:02, Tobias Ringstrom wrote:
>Barry Scott wrote:
>>With the subversion client pb->realmstring will be setup by the code in
>>ssl_server_trust_file_first_credentials.
>>But the svncpp code has its own provider that implements the
>>first_credentials callback but not the save_credentials callback.
>
>I've just looked at the svncpp code, and it looks like you're installing
>the ssl_server_trust and ssl_client_cert_pw prompt providers before the
>corresponding file providers. That means that the prompt functions will
>always be called, even if there are cached credentials available. If you
>fix the ordering problem, you will also work around the auth bug you reported.
Thanks for taking the trouble to look at the svncpp code.
Reordering the providers to put the file ones first fixes the crash.
I'll update the svncpp patch for 0.36.0 support to include this fix.
>There is no doubt that you did find a bug in the authentication design,
>though. Personally I'd like to have it fixed because it's not good to have
>a broken design, and the risk is pretty small. It will have to be voted
>for though, so anything can happen. But even if that bug is fixed, you
>need to change the order of the providers in svncpp.
Yes it would be good to fix the design bug.
Barry
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svncpp crash in ssl_server_trust_file_save_credentials
Posted by Tobias Ringstrom <to...@ringstrom.mine.nu>.
Barry Scott wrote:
> With the subversion client pb->realmstring will be setup by the code in
> ssl_server_trust_file_first_credentials.
>
> But the svncpp code has its own provider that implements the
> first_credentials callback but not the save_credentials callback.
I've just looked at the svncpp code, and it looks like you're installing
the ssl_server_trust and ssl_client_cert_pw prompt providers before the
corresponding file providers. That means that the prompt functions will
always be called, even if there are cached credentials available. If you
fix the ordering problem, you will also work around the auth bug you
reported.
There is no doubt that you did find a bug in the authentication design,
though. Personally I'd like to have it fixed because it's not good to
have a broken design, and the risk is pretty small. It will have to be
voted for though, so anything can happen. But even if that bug is fixed,
you need to change the order of the providers in svncpp.
/Tobias
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org