Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Date: Sat, 30 Apr 2016 00:53:50 +0000
From: Daniel Shahaf <d.s@daniel.shahaf.name>
To: dev@subversion.apache.org
Subject: [PATCH/RFC] use-sasl=true in --without-sasl builds: make that a fatal
 error?
Message-ID: <20160430005350.GA23087@tarsus.local2>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="oyUTqETQ0mS9luUI"
Content-Disposition: inline
User-Agent: Mutt/1.5.23 (2014-03-12)


--oyUTqETQ0mS9luUI
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline

Currently, if use-sasl=true is set in svnserve.conf but svnserve was
compiled without SASL support, SASL is silently not used.  That's
actually documented:

  svnserve.conf:
    [sasl]
    ### This option specifies whether you want to use the Cyrus SASL
    ### library for authentication. Default is false.
    ### This section will be ignored if svnserve is not built with Cyrus
    ### SASL support; to check, run 'svnserve --version' and look for a line
    ### reading 'Cyrus SASL authentication is available.'
    # use-sasl = true

But documentation notwithstanding, it seems like a misfeature.  Should
we change this, so --without-sasl builds will error out if use-sasl=true
is set?  The patch would be simple enough (attached).

Cheers,

Daniel

P.S. The 'password-db' setting is in the same boat: it's ignored
when SASL is enabled, and documented this way.  However, I'm not
convinced a change to that setting's handling is needed.


--oyUTqETQ0mS9luUI
Content-Type: text/plain; charset=utf-8
Content-Disposition: attachment; filename="use-sasl-true-fatal-error.txt"

[[[
svnserve: Make use-sasl=true a fatal error in SASL-less builds.

* subversion/svnserve/serve.c
  (find_repos): Check 'use-sasl' in SASL-less builds, too.

* subversion/libsvn_repos/repos.c
  (create_conf): Update documentation.

TODO: write release notes entry and/or notes/api-errata/1.10/svnserve001.txt
]]]

[[[
Index: subversion/svnserve/serve.c
===================================================================
--- subversion/svnserve/serve.c	(revision 1734828)
+++ subversion/svnserve/serve.c	(working copy)
@@ -3637,6 +3637,7 @@ find_repos(const char *url,
 {
   const char *path, *full_path, *fs_path, *hooks_env;
   svn_stringbuf_t *url_buf;
+  svn_boolean_t sasl_requested;
 
   /* Skip past the scheme and authority part. */
   path = skip_scheme_part(url);
@@ -3710,14 +3711,16 @@ find_repos(const char *url,
   SVN_ERR(load_authz_config(repository, repository->repos_root, cfg,
                             authz_pool, result_pool));
 
+  /* Should we use Cyrus SASL? */
+  SVN_ERR(svn_config_get_bool(cfg, &sasl_requested,
+                              SVN_CONFIG_SECTION_SASL,
+                              SVN_CONFIG_OPTION_USE_SASL, FALSE));
+  if (sasl_requested)
+    {
 #ifdef SVN_HAVE_SASL
-    {
       const char *val;
 
-      /* Should we use Cyrus SASL? */
-      SVN_ERR(svn_config_get_bool(cfg, &repository->use_sasl,
-                                  SVN_CONFIG_SECTION_SASL,
-                                  SVN_CONFIG_OPTION_USE_SASL, FALSE));
+      repository->use_sasl = sasl_requested;
 
       svn_config_get(cfg, &val, SVN_CONFIG_SECTION_SASL,
                     SVN_CONFIG_OPTION_MIN_SSF, "0");
@@ -3726,8 +3729,14 @@ find_repos(const char *url,
       svn_config_get(cfg, &val, SVN_CONFIG_SECTION_SASL,
                     SVN_CONFIG_OPTION_MAX_SSF, "256");
       SVN_ERR(svn_cstring_atoui(&repository->max_ssf, val));
+#else /* !SVN_HAVE_SASL */
+      return svn_error_createf(SVN_ERR_BAD_CONFIG_VALUE, NULL,
+                               _("SASL requested but not compiled in; "
+                                 "set '%s' to 'false' or recompile "
+                                 "svnserve with SASL support"),
+                               SVN_CONFIG_OPTION_USE_SASL);
+#endif /* SVN_HAVE_SASL */
     }
-#endif
 
   /* Use the repository UUID as the default realm. */
   SVN_ERR(svn_fs_get_uuid(repository->fs, &repository->realm, scratch_pool));
Index: subversion/libsvn_repos/repos.c
===================================================================
--- subversion/libsvn_repos/repos.c     (revision 1734828)
+++ subversion/libsvn_repos/repos.c     (working copy)
@@ -882,7 +882,7 @@ create_conf(svn_repos_t *repos, apr_pool_t *pool)
 "[sasl]"                                                                     NL
 "### This option specifies whether you want to use the Cyrus SASL"           NL
 "### library for authentication. Default is false."                          NL
-"### This section will be ignored if svnserve is not built with Cyrus"       NL
+"### Enabling this option requires svnserve to have been built with Cyrus"   NL
 "### SASL support; to check, run 'svnserve --version' and look for a line"   NL
 "### reading 'Cyrus SASL authentication is available.'"                      NL
 "# use-sasl = true"                                                          NL
]]]

--oyUTqETQ0mS9luUI--