You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2013/10/07 07:02:00 UTC
svn commit: r1529748 - in /subversion/trunk/subversion/svnserve: serve.c
server.h
Author: stefan2
Date: Mon Oct 7 05:02:00 2013
New Revision: 1529748
URL: http://svn.apache.org/r1529748
Log:
Make authz and password config file readers in svnserve only depend on
repository information. Defer error logging to the caller level that
will have a server baton.
* subversion/svnserve/server.h
(load_pwdb_config): remove from header
* subversion/svnserve/serve.c
(load_pwdb_config,
load_authz_config): restrict parameters to repository-specific ones;
defer error logging to parent
(handle_config_error): new utility for deferred error logging
(find_repos): update and simplify caller
(serve): handle deferred error logging
Modified:
subversion/trunk/subversion/svnserve/serve.c
subversion/trunk/subversion/svnserve/server.h
Modified: subversion/trunk/subversion/svnserve/serve.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svnserve/serve.c?rev=1529748&r1=1529747&r2=1529748&view=diff
==============================================================================
--- subversion/trunk/subversion/svnserve/serve.c (original)
+++ subversion/trunk/subversion/svnserve/serve.c Mon Oct 7 05:02:00 2013
@@ -283,16 +283,19 @@ log_authz_denied(const char *path,
return log_write(b->log_file, line, strlen(line), pool);
}
-
-svn_error_t *load_pwdb_config(server_baton_t *server,
- svn_ra_svn_conn_t *conn,
- apr_pool_t *pool)
+/* If CFG specifies a path to the password DB, read that DB and store it
+ * in REPOSITORY->PWDB.
+ */
+static svn_error_t *
+load_pwdb_config(repository_t *repository,
+ svn_config_t *cfg,
+ apr_pool_t *pool)
{
const char *pwdb_path;
svn_error_t *err;
- repository_t *repository = server->repository;
- svn_config_get(repository->cfg, &pwdb_path, SVN_CONFIG_SECTION_GENERAL,
+ svn_config_get(cfg, &pwdb_path,
+ SVN_CONFIG_SECTION_GENERAL,
SVN_CONFIG_OPTION_PASSWORD_DB, NULL);
repository->pwdb = NULL;
@@ -305,8 +308,6 @@ svn_error_t *load_pwdb_config(server_bat
FALSE, FALSE, pool);
if (err)
{
- log_server_error(err, server, conn, pool);
-
/* Because it may be possible to read the pwdb file with some
access methods and not others, ignore errors reading the pwdb
file and just don't present password authentication as an
@@ -319,11 +320,7 @@ svn_error_t *load_pwdb_config(server_bat
if (err->apr_err != SVN_ERR_BAD_FILENAME
&& ! APR_STATUS_IS_EACCES(err->apr_err))
{
- /* Now that we've logged the error, clear it and return a
- * nice, generic error to the user:
- * http://subversion.tigris.org/issues/show_bug.cgi?id=2271 */
- svn_error_clear(err);
- return svn_error_create(SVN_ERR_AUTHN_FAILED, NULL, NULL);
+ return svn_error_create(SVN_ERR_AUTHN_FAILED, err, NULL);
}
else
/* Ignore SVN_ERR_BAD_FILENAME and APR_EACCES and proceed. */
@@ -370,22 +367,21 @@ canonicalize_access_file(const char **ac
SERVER and CONN must not be NULL. The real errors will be logged with
SERVER and CONN but return generic errors to the client. */
-static
-svn_error_t *load_authz_config(server_baton_t *server,
- svn_ra_svn_conn_t *conn,
- const char *repos_root,
- apr_pool_t *pool)
+static svn_error_t *
+load_authz_config(repository_t *repository,
+ const char *repos_root,
+ svn_config_t *cfg,
+ apr_pool_t *pool)
{
const char *authzdb_path;
const char *groupsdb_path;
svn_error_t *err;
- repository_t *repository = server->repository;
/* Read authz configuration. */
- svn_config_get(repository->cfg, &authzdb_path, SVN_CONFIG_SECTION_GENERAL,
+ svn_config_get(cfg, &authzdb_path, SVN_CONFIG_SECTION_GENERAL,
SVN_CONFIG_OPTION_AUTHZ_DB, NULL);
- svn_config_get(repository->cfg, &groupsdb_path, SVN_CONFIG_SECTION_GENERAL,
+ svn_config_get(cfg, &groupsdb_path, SVN_CONFIG_SECTION_GENERAL,
SVN_CONFIG_OPTION_GROUPS_DB, NULL);
if (authzdb_path)
@@ -406,15 +402,11 @@ svn_error_t *load_authz_config(server_ba
groupsdb_path, TRUE, pool);
if (err)
- {
- log_server_error(err, server, conn, pool);
- svn_error_clear(err);
- return svn_error_create(SVN_ERR_AUTHZ_INVALID_CONFIG, NULL, NULL);
- }
+ return svn_error_create(SVN_ERR_AUTHZ_INVALID_CONFIG, err, NULL);
/* Are we going to be case-normalizing usernames when we consult
* this authz file? */
- svn_config_get(repository->cfg, &case_force_val,
+ svn_config_get(cfg, &case_force_val,
SVN_CONFIG_SECTION_GENERAL,
SVN_CONFIG_OPTION_FORCE_USERNAME_CASE, NULL);
if (case_force_val)
@@ -436,6 +428,33 @@ svn_error_t *load_authz_config(server_ba
return SVN_NO_ERROR;
}
+/* If ERROR is a AUTH* error as returned by load_pwdb_config or
+ * load_authz_config, write it to SERVER's log file and use CONN for
+ * context information. Return a sanitized version of ERROR.
+ */
+static svn_error_t *
+handle_config_error(svn_error_t *error,
+ server_baton_t *server,
+ svn_ra_svn_conn_t *conn,
+ apr_pool_t *pool)
+{
+ if ( error
+ && ( error->apr_err == SVN_ERR_AUTHZ_INVALID_CONFIG
+ || error->apr_err == SVN_ERR_AUTHN_FAILED))
+ {
+ apr_status_t apr_err = error->apr_err;
+ log_server_error(error, server, conn, pool);
+
+ /* Now that we've logged the error, clear it and return a
+ * nice, generic error to the user:
+ * http://subversion.tigris.org/issues/show_bug.cgi?id=2271 */
+ svn_error_clear(error);
+ return svn_error_create(apr_err, NULL, NULL);
+ }
+
+ return error;
+}
+
/* Set *FS_PATH to the portion of URL that is the path within the
repository, if URL is inside REPOS_URL (if URL is not inside
REPOS_URL, then error, with the effect on *FS_PATH undefined).
@@ -3385,17 +3404,11 @@ static svn_error_t *find_repos(const cha
FALSE, /* section_names_case_sensitive */
FALSE, /* option_names_case_sensitive */
pool));
- SVN_ERR(load_pwdb_config(b, conn, pool));
- SVN_ERR(load_authz_config(b, conn, repos_root, pool));
- }
- /* svnserve.conf has been loaded via the --config-file option so need
- * to load pwdb and authz. */
- else
- {
- SVN_ERR(load_pwdb_config(b, conn, pool));
- SVN_ERR(load_authz_config(b, conn, repos_root, pool));
}
+ SVN_ERR(load_pwdb_config(repository, repository->cfg, pool));
+ SVN_ERR(load_authz_config(repository, repos_root, repository->cfg, pool));
+
#ifdef SVN_HAVE_SASL
/* Should we use Cyrus SASL? */
SVN_ERR(svn_config_get_bool(repository->cfg, &repository->use_sasl,
@@ -3715,7 +3728,8 @@ svn_error_t *serve(svn_ra_svn_conn_t *co
}
}
- err = find_repos(client_url, params->root, &b, conn, cap_words, pool);
+ err = handle_config_error(find_repos(client_url, params->root, &b, conn,
+ cap_words, pool), &b, conn, pool);
if (!err)
{
SVN_ERR(auth_request(conn, pool, &b, READ_ACCESS, FALSE));
Modified: subversion/trunk/subversion/svnserve/server.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svnserve/server.h?rev=1529748&r1=1529747&r2=1529748&view=diff
==============================================================================
--- subversion/trunk/subversion/svnserve/server.h (original)
+++ subversion/trunk/subversion/svnserve/server.h Mon Oct 7 05:02:00 2013
@@ -159,15 +159,6 @@ client_info_t * get_client_info(svn_ra_s
svn_error_t *serve(svn_ra_svn_conn_t *conn, serve_params_t *params,
apr_pool_t *pool);
-/* Load the password database for the listening server based on the
- entries in the SERVER struct.
-
- SERVER and CONN must not be NULL. The real errors will be logged with
- SERVER and CONN but return generic errors to the client. */
-svn_error_t *load_pwdb_config(server_baton_t *server,
- svn_ra_svn_conn_t *conn,
- apr_pool_t *pool);
-
/* Initialize the Cyrus SASL library. POOL is used for allocations. */
svn_error_t *cyrus_init(apr_pool_t *pool);
Re: svn commit: r1529748 - in /subversion/trunk/subversion/svnserve:
serve.c server.h
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Mon, Oct 7, 2013 at 1:15 PM, Bert Huijben <be...@qqmail.nl> wrote:
>
> > - svn_config_get(repository->cfg, &pwdb_path,
> > SVN_CONFIG_SECTION_GENERAL,
> > + svn_config_get(cfg, &pwdb_path,
> > + SVN_CONFIG_SECTION_GENERAL,
> > SVN_CONFIG_OPTION_PASSWORD_DB, NULL);
> >
> > repository->pwdb = NULL;
> > @@ -305,8 +308,6 @@ svn_error_t *load_pwdb_config(server_bat
> > FALSE, FALSE, pool);
> > if (err)
> > {
> > - log_server_error(err, server, conn, pool);
> > -
> > /* Because it may be possible to read the pwdb file with some
> > access methods and not others, ignore errors reading the
> pwdb
> > file and just don't present password authentication as an
> > @@ -319,11 +320,7 @@ svn_error_t *load_pwdb_config(server_bat
> > if (err->apr_err != SVN_ERR_BAD_FILENAME
> > && ! APR_STATUS_IS_EACCES(err->apr_err))
> > {
> > - /* Now that we've logged the error, clear it and return
> a
> > - * nice, generic error to the user:
> > - *
> http://subversion.tigris.org/issues/show_bug.cgi?id=2271 */
> > - svn_error_clear(err);
> > - return svn_error_create(SVN_ERR_AUTHN_FAILED, NULL,
> NULL);
> > + return svn_error_create(SVN_ERR_AUTHN_FAILED, err, NULL);
>
>
> I like the change for debugging, but this clearly re-introduces issue
> #2271 " svnserve expose the path to the password file if it is not found"
>
> Is this message cleaned in a different place now?
>
>
Serve() now calls the new handle_config_error() to sanitize the errors.
BTW, thanks for reviewing!
-- Stefan^2.
RE: svn commit: r1529748 - in /subversion/trunk/subversion/svnserve: serve.c server.h
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: stefan2@apache.org [mailto:stefan2@apache.org]
> Sent: maandag 7 oktober 2013 07:02
> To: commits@subversion.apache.org
> Subject: svn commit: r1529748 - in /subversion/trunk/subversion/svnserve:
> serve.c server.h
>
> Author: stefan2
> Date: Mon Oct 7 05:02:00 2013
> New Revision: 1529748
>
> URL: http://svn.apache.org/r1529748
> Log:
> Make authz and password config file readers in svnserve only depend on
> repository information. Defer error logging to the caller level that
> will have a server baton.
>
> * subversion/svnserve/server.h
> (load_pwdb_config): remove from header
>
> * subversion/svnserve/serve.c
> (load_pwdb_config,
> load_authz_config): restrict parameters to repository-specific ones;
> defer error logging to parent
> (handle_config_error): new utility for deferred error logging
> (find_repos): update and simplify caller
> (serve): handle deferred error logging
>
> Modified:
> subversion/trunk/subversion/svnserve/serve.c
> subversion/trunk/subversion/svnserve/server.h
>
> Modified: subversion/trunk/subversion/svnserve/serve.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/svnserve/serve
> .c?rev=1529748&r1=1529747&r2=1529748&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/svnserve/serve.c (original)
> +++ subversion/trunk/subversion/svnserve/serve.c Mon Oct 7 05:02:00 2013
> @@ -283,16 +283,19 @@ log_authz_denied(const char *path,
> return log_write(b->log_file, line, strlen(line), pool);
> }
>
> -
> -svn_error_t *load_pwdb_config(server_baton_t *server,
> - svn_ra_svn_conn_t *conn,
> - apr_pool_t *pool)
> +/* If CFG specifies a path to the password DB, read that DB and store it
> + * in REPOSITORY->PWDB.
> + */
> +static svn_error_t *
> +load_pwdb_config(repository_t *repository,
> + svn_config_t *cfg,
> + apr_pool_t *pool)
> {
> const char *pwdb_path;
> svn_error_t *err;
> - repository_t *repository = server->repository;
>
> - svn_config_get(repository->cfg, &pwdb_path,
> SVN_CONFIG_SECTION_GENERAL,
> + svn_config_get(cfg, &pwdb_path,
> + SVN_CONFIG_SECTION_GENERAL,
> SVN_CONFIG_OPTION_PASSWORD_DB, NULL);
>
> repository->pwdb = NULL;
> @@ -305,8 +308,6 @@ svn_error_t *load_pwdb_config(server_bat
> FALSE, FALSE, pool);
> if (err)
> {
> - log_server_error(err, server, conn, pool);
> -
> /* Because it may be possible to read the pwdb file with some
> access methods and not others, ignore errors reading the pwdb
> file and just don't present password authentication as an
> @@ -319,11 +320,7 @@ svn_error_t *load_pwdb_config(server_bat
> if (err->apr_err != SVN_ERR_BAD_FILENAME
> && ! APR_STATUS_IS_EACCES(err->apr_err))
> {
> - /* Now that we've logged the error, clear it and return a
> - * nice, generic error to the user:
> - * http://subversion.tigris.org/issues/show_bug.cgi?id=2271 */
> - svn_error_clear(err);
> - return svn_error_create(SVN_ERR_AUTHN_FAILED, NULL, NULL);
> + return svn_error_create(SVN_ERR_AUTHN_FAILED, err, NULL);
I like the change for debugging, but this clearly re-introduces issue #2271 " svnserve expose the path to the password file if it is not found"
Is this message cleaned in a different place now?
Bert