Follow up also to serf-dev.
On Thu, Jun 20, 2013 at 11:05 PM, Lieven Govaerts <svnlgo_at_mobsol.be> wrote:
> On Thu, Jun 20, 2013 at 10:30 PM, Greg Stein <gstein_at_gmail.com> wrote:
>> On Thu, Jun 20, 2013 at 2:19 PM, Stefan Küng <tortoisesvn_at_gmail.com> wrote:
>>> Hi,
>>>
>>> Another crash that's climbing up in the crash report statistics for TSVN.
>>> Seems to be related to the previously discussed problem with checkouts in
>>> TSVN.
>
> Thanks Stefan.
>
>>> The stack trace:
>>>
>>> BowPad
>>>
>>> libsvn_tsvn.dll!svn_ra_serf__credentials_callback(char * *
>>> username=0xffffffffffffffff, char * * password=0x0000000002ba0210,
>>> serf_request_t * request=0x0000000002ba0280, void *
>>> baton=0x0000000002c12588, int code=407, const char *
>>> authn_type=0x000007fee0bf0b58, const char * realm=0x0000000002c12b60,
>>> apr_pool_t * pool=0x0000000002bb8258) Line 1789 C
>>
>> The 407 code means a proxy authorization is needed.
>>
>> The problem in this stack frame is the username address. It clearly
>> has a problematic value. It should only be about four bytes off from
>> the password value.
>>
>>> libsvn_tsvn.dll!serf__handle_basic_auth(int code=-524813808, serf_request_t
>>> * request=0x0000000002ba8368, serf_bucket_t * response=0x0000000000000001,
>>> const char * auth_hdr=0x0000000002bb30be, const char *
>>> auth_attr=0x0000000002bb30be, void * baton=0x0000000002bac270, apr_pool_t *
>>> pool=0x0000000002ba0198) Line 89 C
>>
>> The code parameter in this traceback is clearly incorrect. It should
>> be 407, just like what got passed to the callback.
>>
>> Either these tracebacks are wonky, or there may be some stack smashing
>> going on. Stefan?
>
> I guess it's optimized code.
>
>>>...
>>> The crash is in libsvn_ra_serf\util.c, Snippet
>>> svn_ra_serf__credentials_callback, line 1789:
>>> BowPad *username = apr_pstrdup(pool, session->proxy_username);
>>> *password = apr_pstrdup(pool, session->proxy_password);
>>
>> Was session bad? Or was the proxy_username bad?
>>
>
> Looks like it's the authentication handling when setting up a SSL
> tunnel that's at fault here, at least I can easily reproduce it with
> an apache http proxy connetion to a https repo.
>
> The ssl tunnel is started by a CONNECT request created by serf. When
> the proxy requests credentials, serf will call back to the
> application. As the application doesn't know about this request, it
> doesn't get a valid baton either, so can't get baton->session ...
>
> That baton it gets is the ctx used by the ssltunnel code.
>
> Hm, have to think about how we can solve this. Not sure it can be done
> with the existing API.
The existing credentials callback for Basic and Digest is
request-based, so when calling the application it passes it the
serf_request_t and associated handler_baton.
The issue here is that the ssl tunnel code creates a CONNECT request
within serf, but still calls the application if proxy authentication
with Basic/Digest is needed. The application doesn't know about the
request, so the handler baton is invalid for the application, so it
has no way to know for which serf context this credentials request is.
Proxies are configured at serf context level.
This is probably related to serf only supporting NTLM via Negotiate,
my guess is that these proxies are configured to use NTLM and Basic.
Why else would you want to use a proxy with Basic authentication to
setup a secure connection to a remote server?
As a solution, I propose to add a new type:
(typedef apr_status_t (*serf_proxy_credentials_callback_t)(
char **username,
char **password,
void *ctx_baton,
const char *authn_type,
const char *realm,
apr_pool_t *pool);
ctx_baton here is set in a call to the new API:
void serf_config_credentials_callback(serf_context_t *ctx,
serf_credentials_callback_t cred_cb,
void *ctx_baton);
The application can then use ctx_baton to retrieve context specific
information, e.g. its wrapper around the serf context.
The basic and digest authn code will use the proxy creds callback to
get username and password instead of the request creds callback. This
should fix the ssl tunnel case. When the proxy callback is not set,
these two authn scheme handlers should return an error in case of 407
response code.
This is a new API, so requires serf 1.3. We'll need one anyway for the
NTLM authn scheme handler (IMO this is a feature, so can't include in
a patch release).
Also, we'll probably want to run over all possible combinations of
http,https,proxy,tunnel x basic, digest, ntlm,
negotiate(ntlm,negotiate) x windows,*nix before we release these 2 new
features.
I can implement the proxy callback over the weekend.
Lieven
Received on 2013-06-21 08:56:06 CEST