You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Shlomi Fish <sh...@vipe.stud.technion.ac.il> on 2003/07/19 14:40:43 UTC
[PATCH] Fix for Issue # 1267
Here's a revised patch for latest revisions of Subversion.
Here's the log:
<<<
Resolves Issue #1267 (Have to say "No" several times to reject SSL
certificate.)
* session.c
(server_ssl_callback): modified this function to set the ssl_certificate
flags in the session baton.
* ra_dav.h
Added the ssl_certificate flags to the svn_ra_session_t baton.
* util.c
(svn_ra_dav__parsed_request): reset the ssl_certificate flags at the
beginning of the request. Check for an SSL rejection and if so,
return an authorization error.
>>>
Regards,
Shlomi Fish
----------------------------------------------------------------------
Shlomi Fish shlomif@vipe.technion.ac.il
Home Page: http://t2.technion.ac.il/~shlomif/
An apple a day will keep a doctor away. Two apples a day will keep two
doctors away.
Falk Fish
Re: [PATCH] Fix for Issue # 1267
Posted by Shlomi Fish <sh...@vipe.stud.technion.ac.il>.
On Sun, 20 Jul 2003, Ben Collins-Sussman wrote:
> Shlomi Fish <sh...@vipe.technion.ac.il> writes:
>
> > Resolves Issue #1267 (Have to say "No" several times to reject SSL
> > certificate.)
> >
> > * session.c
> > (server_ssl_callback): modified this function to set the ssl_certificate
> > flags in the session baton.
> >
> > * ra_dav.h
> > Added the ssl_certificate flags to the svn_ra_session_t baton.
> >
> > * util.c
> > (svn_ra_dav__parsed_request): reset the ssl_certificate flags at the
> > beginning of the request. Check for an SSL rejection and if so,
> > return an authorization error.
>
> OK, I've finally studied this patch, and now it's *my* turn to object
> to it. (Poor Shlomi... :-) )
>
> This patch adds a global cache to the RA session object, so that
> svn_ra_dav__parsed_request can return SVN_ERR_RA_NOT_AUTHORIZED. This
> causes svn_ra_dav__search_for_starting_props to stop retrying PROPFIND
> requests on parent directories, because, at the moment, it's only
> ignoring the more generic SVN_ERR_RA_DAV_REQUEST_FAILED.
>
> But I don't like the RA-session cache at all; it seems to me like
> somehow, we're failing to use neon properly (or neon has a bug?).
> Notice that svn_ra_dav__parsed_request calls
> svn_ra_dav__convert_error, which tries to map neon return-values to
> svn error codes. In particular, a NE_AUTH failure code gets mapped to
> SVN_ERR_RA_NOT_AUTHORIZED. But this mapping isn't happening, because
> for some reason, neon is returning a generic error (NE_ERROR).
>
It does:
Studying the source code of Neon (check the ne_negotiate_ssl() function in
ne_session.c) seems that Neoen is returning NE_ERROR if the SSL
Certificate did not past the test. (In fact, it seems it is the only error
it returns).
My patch is the only way to determine if this happened because of a
rejection of an SSL certificate, because Neon does not return a special
error in this case.
It might be a Neon mis-behaviour, but we should deal with it somehow.
Regards,
Shlomi Fish
----------------------------------------------------------------------
Shlomi Fish shlomif@vipe.technion.ac.il
Home Page: http://t2.technion.ac.il/~shlomif/
An apple a day will keep a doctor away. Two apples a day will keep two
doctors away.
Falk Fish
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
[PATCH] Fix for Issue #1267 Rev. 2 [was Re: [PATCH] Fix for Issue
#1267]
Posted by Shlomi Fish <sh...@vipe.stud.technion.ac.il>.
Here's an updated patch that distinguishes between 404 error codes and
everything else:
Log:
<<<
Resolves Issue #1267 (Have to say "No" several times to reject SSL
certificate.) and probably other misbehaviours.
* svn_error_codes.h
Added the SVN_ERR_RA_DAV_PATH_NOT_FOUND error code.
* props.c
(svn_ra_dav__search_for_starting_props): now ignoring on the PATH_NOT_FOUND
error instead of the more generic REQUEST_FAILED error.
* util.c
(svn_ra_dav__parsed_request): check if the error code is 404 and
if so send a PATH_NOT_FOUND error. Else send a generic error.
>>>
Regards,
Shlomi Fish
On Mon, 21 Jul 2003, Ben Collins-Sussman wrote:
> Shlomi Fish <sh...@vipe.technion.ac.il> writes:
>
> > OK, can I add an error code to svn_error_codes.h?
>
> Sure, create SVN_ERR_RA_DAV_PATH_NOT_FOUND. That's the main idea.
> Map 404 to that error code.
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
----------------------------------------------------------------------
Shlomi Fish shlomif@vipe.technion.ac.il
Home Page: http://t2.technion.ac.il/~shlomif/
An apple a day will keep a doctor away. Two apples a day will keep two
doctors away.
Falk Fish
Re: [PATCH] Fix for Issue # 1267
Posted by Ben Collins-Sussman <su...@collab.net>.
Shlomi Fish <sh...@vipe.technion.ac.il> writes:
> OK, can I add an error code to svn_error_codes.h?
Sure, create SVN_ERR_RA_DAV_PATH_NOT_FOUND. That's the main idea.
Map 404 to that error code.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix for Issue # 1267
Posted by Shlomi Fish <sh...@vipe.stud.technion.ac.il>.
On Mon, 21 Jul 2003, Ben Collins-Sussman wrote:
> Joe Orton <jo...@manyfish.co.uk> writes:
>
> > It sounds like the real problem is here; this function should walk up
> > the parents if and only if the PROPFIND request gets a 404 response
> > (when the given path no longer exists in HEAD).
>
> Totally agree. Let's convert 404 status codes into a very specific
> svn errorcode. Then make search_for_starting_props *only* retry on
> that error.
>
OK, can I add an error code to svn_error_codes.h?
My first try converted any other error besides 404 to some obscure error I
found there, but I think it's a sub-optimal solution. I'll work on it some
more today. Stay tuned.
> Thanks, Joe.... Shlomi, wanna try again? :-)
>
I'm right on it.
Regards,
Shlomi Fish
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
----------------------------------------------------------------------
Shlomi Fish shlomif@vipe.technion.ac.il
Home Page: http://t2.technion.ac.il/~shlomif/
An apple a day will keep a doctor away. Two apples a day will keep two
doctors away.
Falk Fish
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix for Issue # 1267
Posted by Ben Collins-Sussman <su...@collab.net>.
Joe Orton <jo...@manyfish.co.uk> writes:
> It sounds like the real problem is here; this function should walk up
> the parents if and only if the PROPFIND request gets a 404 response
> (when the given path no longer exists in HEAD).
Totally agree. Let's convert 404 status codes into a very specific
svn errorcode. Then make search_for_starting_props *only* retry on
that error.
Thanks, Joe.... Shlomi, wanna try again? :-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix for Issue # 1267
Posted by Joe Orton <jo...@manyfish.co.uk>.
On Sun, Jul 20, 2003 at 08:18:17AM -0500, Ben Collins-Sussman wrote:
> Shlomi Fish <sh...@vipe.technion.ac.il> writes:
>
> > Resolves Issue #1267 (Have to say "No" several times to reject SSL
> > certificate.)
> >
> > * session.c
> > (server_ssl_callback): modified this function to set the ssl_certificate
> > flags in the session baton.
> >
> > * ra_dav.h
> > Added the ssl_certificate flags to the svn_ra_session_t baton.
> >
> > * util.c
> > (svn_ra_dav__parsed_request): reset the ssl_certificate flags at the
> > beginning of the request. Check for an SSL rejection and if so,
> > return an authorization error.
>
> OK, I've finally studied this patch, and now it's *my* turn to object
> to it. (Poor Shlomi... :-) )
>
> This patch adds a global cache to the RA session object, so that
> svn_ra_dav__parsed_request can return SVN_ERR_RA_NOT_AUTHORIZED. This
> causes svn_ra_dav__search_for_starting_props to stop retrying PROPFIND
> requests on parent directories, because, at the moment, it's only
> ignoring the more generic SVN_ERR_RA_DAV_REQUEST_FAILED.
It sounds like the real problem is here; this function should walk up
the parents if and only if the PROPFIND request gets a 404 response
(when the given path no longer exists in HEAD).
Currently this function is carrying on past all the fatal errors neon is
returning, e.g. NE_CONNECT, NE_LOOKUP, etc ...
$ strace -e connect svn co http://localhost:666/ag/aga/ag
connect(3, {sa_family=AF_UNIX, path="/var/run/.nscd_socket"}, 110) = -1
ENOENT (No such file or directory)
connect(3, {sa_family=AF_INET, sin_port=htons(666),
sin_addr=inet_addr("127.0.0.1")}, 16) = -1 ECONNREFUSED (Connection refused)
connect(3, {sa_family=AF_INET, sin_port=htons(666),
sin_addr=inet_addr("127.0.0.1")}, 16) = -1 ECONNREFUSED (Connection refused)
connect(3, {sa_family=AF_INET, sin_port=htons(666),
sin_addr=inet_addr("127.0.0.1")}, 16) = -1 ECONNREFUSED (Connection refused)
connect(3, {sa_family=AF_INET, sin_port=htons(666),
sin_addr=inet_addr("127.0.0.1")}, 16) = -1 ECONNREFUSED (Connection refused)
svn: RA layer request failed
svn: The path was not part of a repository
svn: PROPFIND request failed on '/'
svn: PROPFIND of '/': could not connect to server (http://localhost:666)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix for Issue # 1267
Posted by Ben Collins-Sussman <su...@collab.net>.
Shlomi Fish <sh...@vipe.technion.ac.il> writes:
> Resolves Issue #1267 (Have to say "No" several times to reject SSL
> certificate.)
>
> * session.c
> (server_ssl_callback): modified this function to set the ssl_certificate
> flags in the session baton.
>
> * ra_dav.h
> Added the ssl_certificate flags to the svn_ra_session_t baton.
>
> * util.c
> (svn_ra_dav__parsed_request): reset the ssl_certificate flags at the
> beginning of the request. Check for an SSL rejection and if so,
> return an authorization error.
OK, I've finally studied this patch, and now it's *my* turn to object
to it. (Poor Shlomi... :-) )
This patch adds a global cache to the RA session object, so that
svn_ra_dav__parsed_request can return SVN_ERR_RA_NOT_AUTHORIZED. This
causes svn_ra_dav__search_for_starting_props to stop retrying PROPFIND
requests on parent directories, because, at the moment, it's only
ignoring the more generic SVN_ERR_RA_DAV_REQUEST_FAILED.
But I don't like the RA-session cache at all; it seems to me like
somehow, we're failing to use neon properly (or neon has a bug?).
Notice that svn_ra_dav__parsed_request calls
svn_ra_dav__convert_error, which tries to map neon return-values to
svn error codes. In particular, a NE_AUTH failure code gets mapped to
SVN_ERR_RA_NOT_AUTHORIZED. But this mapping isn't happening, because
for some reason, neon is returning a generic error (NE_ERROR).
So my question is: why is this happening? It seems to me that if our
ssl callback returns failure to neon, neon should return NE_AUTH
failure, not NE_ERROR. If this were happening,
search_for_starting_props would be bailing out properly, rather than
retrying.
Joe Orton, any thoughts?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org