3 commits - admserv/newinst mod_admserv/mod_admserv.c

[Richard Allen Megginson]

 admserv/newinst/src/AdminServer.pm.in |   23 ++++---
 mod_admserv/mod_admserv.c             |  103 ++++++++++++++++++++++++----------
 2 files changed, 87 insertions(+), 39 deletions(-)

New commits:
commit bd9522123a4935730b77ec4e2a4ae628b7b270aa
Author: Rich Megginson <rmeggins(a)redhat.com&gt;
Date:   Tue Oct 22 13:49:01 2013 -0600

    Ticket #47478 No groups file? error restarting Admin server
    
    https://fedorahosted.org/389/ticket/47478
    Reviewed by: nkinder (Thanks!)
    Branch: master
    Fix Description: When Apache runs the CGI, it has to run through all of the
    hooks/callbacks again, including the authz callbacks, via a sub-request.
    For the sub-request, admserv_check_authz was just returning DECLINED, which
    means "I decline to answer this request for authz, go to the next handler".
    It was then doing regular Apache authz handling, including looking at the
    groups file.  The fix is to remember what the original authz return value was,
    and just return that if being called as part of a sub-request.
    Platforms tested: RHEL6 x86_64
    Flag Day: no
    Doc impact: no

diff --git a/mod_admserv/mod_admserv.c b/mod_admserv/mod_admserv.c
index d104538..420e36b 100644
--- a/mod_admserv/mod_admserv.c
+++ b/mod_admserv/mod_admserv.c
@@ -1589,6 +1589,7 @@ populate_task_cache_entries(const char *userDN, LDAP *server)
 #define IS_START_CONFIG_DS_REQ(uri, serverid, userdn, user) \
     uri && !STRNCASECMP(uri, STARTDS_IDENTIFIER, strlen(STARTDS_IDENTIFIER))
&& \
     serverid && !STRCMP(serverid, "admin-serv") && /* is local
superuser */ !userdn && user
+#define ADMSERV_AUTHZ_RETVAL "admserv_authz_retval"
 
 static int
 admserv_check_authz(request_rec *r)
@@ -1608,19 +1609,25 @@ admserv_check_authz(request_rec *r)
     long            now;
     int             is_start_config_ds_req = 0;
     int tries = 0;
+    int retval = DECLINED;
+    char rvbuf[20];
+    const char *rvstr;
     admserv_config *cf = ap_get_module_config(r->per_dir_config,
                                               &admserv_module);
 
     ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
                   "admserv_check_authz: request for uri [%s]", r->uri);
 
-    /* for some reason, we get sub requests for our tasks which we can ignore */
-    if (r->main) {
+    /* sub requests need to reparse the uri, but do not need to go through the whole
authz again
+     * just return the result from the original authz request
+     */
+    if (r->main && (rvstr = apr_table_get(r->main->notes,
ADMSERV_AUTHZ_RETVAL))) {
         ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
-                      "admserv_check_authz: skipping sub request [%s] - filename
[%s] user [%s]",
+                      "admserv_check_authz: sub request [%s] - filename [%s] user
[%s] retval [%s]",
                       r->uri, r->main->filename ? r->main->filename :
"(null)",
-                      r->main->user ? r->main->user : "(null)");
-        return DECLINED;
+                      r->main->user ? r->main->user : "(null)",
rvstr);
+        retval = atoi(rvstr);
+        goto done;
     }
 
     uri = apr_pstrdup(r->pool, r->uri); /* might need unparsed_uri here? */
@@ -1628,7 +1635,8 @@ admserv_check_authz(request_rec *r)
     if (!(p = strchr(uri+1, '/'))) {
         ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
                       "admserv_check_authz(): Skipping invalid URI [%s]",
uri);
-        return DECLINED;
+        retval = DECLINED;
+        goto done;
     }
 
     serverid = uri+1;
@@ -1655,7 +1663,8 @@ admserv_check_authz(request_rec *r)
                 ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
                               "admserv_check_authz(): unable to find registered
server (%s)",
                               serverid);
-                return admserv_error(r, HTTP_BAD_REQUEST, "server not
registered"); /*i18n*/
+                retval = admserv_error(r, HTTP_BAD_REQUEST, "server not
registered"); /*i18n*/
+                goto done;
             }
         }
     }
@@ -1665,7 +1674,8 @@ admserv_check_authz(request_rec *r)
         ap_log_rerror(APLOG_MARK, APLOG_NOTICE, 0, r,
                       "admserv_check_authz(): passing [%s] to the userauth
handler",
                       r->uri);
-        return OK;
+        retval = OK;
+        goto done;
     }
 
     entryDN[0] = '\0';
@@ -1673,7 +1683,8 @@ admserv_check_authz(request_rec *r)
         ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
                       "admserv_check_authz(): unable to build DN from URL - bad URL
[%s]",
                       uri);
-        return admserv_error_std(r, "server not registered"); /*i18n*/
+        retval = admserv_error_std(r, "server not registered"); /*i18n*/
+        goto done;
     }
 
     convert_to_lower_case(entryDN);
@@ -1693,12 +1704,14 @@ admserv_check_authz(request_rec *r)
                           "admserv_check_authz: task [%s] not cached for local
superuser",
                           entryDN);
             if (configdsdown) {
-                return OK; /* let the admserv_config_ds_down handler handle this
situation */
+                retval = OK; /* let the admserv_config_ds_down handler handle this
situation */
+                goto done;
             }
             if (!send_response) {
-                return admserv_error_std(r, retmsg);
+                retval = admserv_error_std(r, retmsg);
+                goto done;
             }
-            return retval;
+            goto done;
         }
         goto found;
     }
@@ -1706,7 +1719,8 @@ admserv_check_authz(request_rec *r)
     if (!userdn || !pw) {
         ap_log_rerror(APLOG_MARK, APLOG_CRIT, 0, r,
                       "admserv_check_authz(): invalid userdn/pw");
-        return admserv_error_std(r, "invalid user/password"); /*i18n*/
+        retval = admserv_error_std(r, "invalid user/password"); /*i18n*/
+        goto done;
     }
 
     /* DT 3/10/98
@@ -1725,7 +1739,8 @@ admserv_check_authz(request_rec *r)
         ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
                       "admserv_check_authz(): unable to open LDAP connection to
%s:%d",
                       registryServer.host, registryServer.port);
-        return admserv_error_std(r, "unable to open LDAPConnection"); /*i18n*/
+        retval = admserv_error_std(r, "unable to open LDAPConnection");
/*i18n*/
+        goto done;
     }
 
     tries = 0;
@@ -1743,7 +1758,8 @@ admserv_check_authz(request_rec *r)
             ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
                           "admserv_check_authz(): unable to LDAP BIND as [%s] to
%s:%d",
                           userdn ? userdn : "(anon)", registryServer.host,
registryServer.port);
-            return admserv_error_std(r, "unable to open LDAPConnection");
/*i18n*/
+            retval = admserv_error_std(r, "unable to open LDAPConnection");
/*i18n*/
+            goto done;
         }
     } while (server != NULL && ++tries < 2);
 
@@ -1756,7 +1772,7 @@ admserv_check_authz(request_rec *r)
                 ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
                               "admserv_check_authz: could not find cached task [%s]
for [%s]",
                               entryDN, userdn ? userdn : "(anon)");
-                return retval;
+                goto done;
             }
             goto found;
         }
@@ -1764,7 +1780,8 @@ admserv_check_authz(request_rec *r)
         ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
                       "admserv_check_authz(): got LDAP error %d talking to %s:%d -
possible user [%s] not authorized",
                       ldapError, registryServer.host, registryServer.port, userdn ?
userdn : "(anon)");
-        return admserv_error(r, HTTP_UNAUTHORIZED, "invalid user credentials");
/*i18n*/
+        retval = admserv_error(r, HTTP_UNAUTHORIZED, "invalid user
credentials"); /*i18n*/
+        goto done;
     }
 
     /* Attempt to read the entry data -- involves DS eval of entry ACI for userDN.
@@ -1781,7 +1798,8 @@ admserv_check_authz(request_rec *r)
                   "admserv_check_authz(): Task [%s] not found for user [%s] -
either"
                   " the task was not registered or the user was not
authorized",
                   entryDN, userdn ? userdn : "(anon)");
-    return admserv_error(r, HTTP_UNAUTHORIZED, "task not found or
unauthorized"); /*i18n*/
+    retval = admserv_error(r, HTTP_UNAUTHORIZED, "task not found or
unauthorized"); /*i18n*/
+    goto done;
 
     /* This is remapping - converting the requested URI to the actual CGI filename
        We can't do this in a translate_name hook because that happens _before_
authenticating
@@ -1800,9 +1818,10 @@ found:
             ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
                           "admserv_check_authz(): No ADMCgiBinDir was specified for
location [%s]",
                           r->filename);
-            return admserv_error_std(r, apr_psprintf(r->pool,
+            retval = admserv_error_std(r, apr_psprintf(r->pool,
                                                      "No ADMCgiBinDir specified for
location [%s]",
                                                      r->filename));
+            goto done;
         }
 
         r->filename = apr_psprintf(r->pool, "%s%c%s",
@@ -1819,9 +1838,10 @@ found:
                 ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
                               "admserv_check_authz(): start config ds CGI is a
directory [%s]",
                               r->filename);
-                return admserv_error_std(r, apr_psprintf(r->pool,
+                retval = admserv_error_std(r, apr_psprintf(r->pool,
                                                          "Invalid URL [%s] is a
directory",
                                                          r->filename));
+                goto done;
             }
         }
 
@@ -1829,7 +1849,8 @@ found:
         ap_log_rerror(APLOG_MARK, APLOG_NOTICE, 0, r,
                       "admserv_check_authz(): StartConfigDS requested by the local
superuser");
 
-        return OK;
+        retval = OK;
+        goto done;
     }
  
     /* DT 3/15/98 Runtime command support. */
@@ -1842,7 +1863,8 @@ found:
         ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
                       "admserv_check_authz: mapped uri [%s] to command [%s]",
                       r->uri, name);
-        return OK;
+        retval = OK;
+        goto done;
     } else {
         ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
                       "admserv_check_authz: uri [%s] did not begin with [%s] - not a
command",
@@ -1859,9 +1881,10 @@ found:
             ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
                           "admserv_check_authz(): No ADMCgiBinDir was specified for
location [%s]",
                           r->filename);
-            return admserv_error_std(r, apr_psprintf(r->pool,
+            retval = admserv_error_std(r, apr_psprintf(r->pool,
                                                      "No ADMCgiBinDir specified for
location [%s]",
                                                      r->filename));
+            goto done;
         }
 
         /* set cgibindir in the Directory/Location section for this server */
@@ -1880,9 +1903,10 @@ found:
                 ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
                               "admserv_check_authz(): CGI is a directory
[%s]",
                               r->filename);
-                return admserv_error_std(r, apr_psprintf(r->pool,
+                retval = admserv_error_std(r, apr_psprintf(r->pool,
                                                          "Invalid URL [%s] is a
directory",
                                                          r->filename));
+                goto done;
             }
         }
 
@@ -1901,7 +1925,11 @@ found:
                   "admserv_check_authz: execute CGI [%s] args [%s]",
                   r->filename, r->args);
 
-    return OK; /* FIXME, does this matter? */
+    retval = OK;
+done:
+    snprintf(rvbuf, sizeof(rvbuf), "%d", retval);
+    apr_table_set(r->notes, ADMSERV_AUTHZ_RETVAL, rvbuf); /* remember authz result for
sub-requests */
+    return retval;
 }
 
 static int


commit 8d0482561ed95621e52710d74fb74dc6970a6f04
Author: Rich Megginson <rmeggins(a)redhat.com&gt;
Date:   Mon Oct 21 15:23:07 2013 -0600

    Ticket #47300 [RFE] remove-ds-admin.pl: redesign the behaviour
    
    https://fedorahosted.org/389/ticket/47300
    Reviewed by: mreynolds (Thanks!)
    Branch: master
    Fix Description: Replace these files with their defaults saved in the bakup
    directory: httpd.conf, console.conf, admserv.conf, nss.conf.
    Without the -a (all) flag, preserve these files: cert8.db,
    key3.db, secmod.db, password.conf.  With the -a flag, remove them.
    Platforms tested: RHEL6 x86_64
    Flag Day: no
    Doc impact: no

diff --git a/admserv/newinst/src/AdminServer.pm.in
b/admserv/newinst/src/AdminServer.pm.in
index 1d7d06f..7c7b511 100644
--- a/admserv/newinst/src/AdminServer.pm.in
+++ b/admserv/newinst/src/AdminServer.pm.in
@@ -355,6 +355,9 @@ sub registerASWithConfigDS {
     return @errs ? 0 : 1;
 }
 
+my @saveconffiles = qw(admserv.conf httpd.conf nss.conf console.conf);
+my @savesecfiles = qw(cert8.db key3.db secmod.db password.conf);
+
 # update other config files - these are the fields which users typically want to
 # change during an install or an upgrade, that also must be synced to the Apache
 # style config files - we use the config CGI in command line mode because it
@@ -412,14 +415,13 @@ sub updateHttpConfFiles {
         print CONSOLECONF @contents;
         close (CONSOLECONF);
     }
-    my @savefiles = qw(admserv.conf httpd.conf nss.conf console.conf cert8.db key3.db
secmod.db);
     if (! -d "$admConf->{configdir}/bakup") {
         if (system ("mkdir -p $admConf->{configdir}/bakup")) {
             debug(0, "Error backing up $admConf->{configdir}/console.conf failed:
$!");
         }
     }
     # backup savefiles for "remove-ds-admin.pl -a"
-    foreach my $savefile (@savefiles) {
+    foreach my $savefile (@saveconffiles, @savesecfiles) {
         if (! -f "$admConf->{configdir}/bakup/$savefile") {
             if (system ("cp -p $admConf->{configdir}/$savefile
$admConf->{configdir}/bakup")) {
                 debug(0, "Error backing up $admConf->{configdir}/$savefile
failed: $!");
@@ -740,7 +742,10 @@ sub removeAdminServer {
     }
 
     # remove config files
-    my @savefiles = qw(admserv.conf httpd.conf nss.conf console.conf cert8.db key3.db
secmod.db);
+    my @savefiles = @savesecfiles; # save security files by default
+    if ($all) {
+        @savefiles = (); # $all means remove everything, save nothing
+    }
     if (opendir(CONFDIR, $configdir)) {
         while ($file = readdir(CONFDIR)) {
             next if ($file eq '.' || $file eq '..');
@@ -756,13 +761,11 @@ sub removeAdminServer {
             }
         }
         closedir(CONFDIR);
-        if ($all) {
-            # restore backed up savefiles
-            foreach my $savefile (@savefiles) {
-                if (-f "$configdir/bakup/$savefile") {
-                    if (system ("mv $configdir/bakup/$savefile $configdir")) {
-                        debug(0, "Error Restoring $configdir/$savefile failed:
$!");
-                    }
+        # restore original conf files
+        foreach my $savefile (@saveconffiles) {
+            if (-f "$configdir/bakup/$savefile") {
+                if (system ("mv $configdir/bakup/$savefile $configdir")) {
+                    debug(0, "Error Restoring $configdir/$savefile failed:
$!");
                 }
             }
         }


commit d35d14f7b599ebbc927cf7c6a6d8b163917e70df
Author: Rich Megginson <rmeggins(a)redhat.com&gt;
Date:   Mon Oct 21 14:43:02 2013 -0600

    Ticket #434 admin-serv logs filling with "admserv_host_ip_check:
ap_get_remote_host could not resolve <ip address>"
    
    https://fedorahosted.org/389/ticket/434
    Reviewed by: mreynolds (Thanks!)
    Branch: master
    Fix Description: Only warn once per process, and give the admin more
    information about what the problem is and what to do about it.  Also added
    more debugging for access control, and turned down the verbosity of some of
    the other messages.
    Platforms tested: RHEL6 x86_64
    Flag Day: no
    Doc impact: no

diff --git a/mod_admserv/mod_admserv.c b/mod_admserv/mod_admserv.c
index d6d2e19..d104538 100644
--- a/mod_admserv/mod_admserv.c
+++ b/mod_admserv/mod_admserv.c
@@ -2013,6 +2013,7 @@ admserv_host_ip_check(request_rec *r)
     char * clientIP = r->connection->remote_ip;
 #endif
     char *msg;
+    static int warned = 0;
 
     if (clientIP) {
     } else {
@@ -2036,8 +2037,15 @@ admserv_host_ip_check(request_rec *r)
             }
         } else {
             PRNetAddr addr;
-            ap_log_rerror(APLOG_MARK, APLOG_NOTICE, 0, r,
-                          "admserv_host_ip_check: ap_get_remote_host could not
resolve %s", clientIP);
+            if (!warned) {
+                ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r,
+                              "admserv_host_ip_check: Access control based on
hostname [%s] is being used, "
+                              "but the server could not resolve the hostname of
client IP address [%s].  Either "
+                              "enable HostnameLookups in console.conf (by default it
is off for performance reasons), "
+                              "or turn off access control by host/domain name and
use access control by IP address only.",
+                              accessHosts, clientIP);
+                warned = 1; /* warn only once per process */
+            }
             if (PR_SUCCESS == PR_StringToNetAddr(clientIP, &addr)) {
                 char buf[PR_NETDB_BUF_SIZE];
                 PRHostEnt hEntry;
@@ -2045,16 +2053,19 @@ admserv_host_ip_check(request_rec *r)
                     if (APR_SUCCESS != admserv_match_list(apr_pstrdup(r->pool,
accessHosts),
                                                           hEntry.h_name, matchflags)) {
                         char ** x;
-                        ap_log_rerror(APLOG_MARK, APLOG_NOTICE, 0, r,
+                        ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
                                       "admserv_host_ip_check: host [%s] did not
match pattern [%s] -"
                                       "will scan aliases", hEntry.h_name,
accessHosts);
                         for (x = hEntry.h_aliases; x && *x; x++) {
                             if (APR_SUCCESS != admserv_match_list(apr_pstrdup(r->pool,
accessHosts),
                                                                   *x, matchflags)) {
-                                ap_log_rerror(APLOG_MARK, APLOG_NOTICE, 0, r,
+                                ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
                                               "admserv_host_ip_check: host alias
[%s] did not match pattern [%s]",
                                               *x, accessHosts);
                             } else {
+                                ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
+                                              "admserv_host_ip_check: host alias
[%s] matched pattern [%s] - access allowed",
+                                              *x, accessHosts);
                                 return DECLINED;
                             }
                         }
@@ -2077,7 +2088,13 @@ admserv_host_ip_check(request_rec *r)
         int matchflags = APR_FNM_PERIOD;
         apr_status_t rc = admserv_match_list(apr_pstrdup(r->pool, accessAddresses),
clientIP, matchflags);
         if (rc != APR_SUCCESS) {
+            ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
+                          "admserv_host_ip_check: client IP address [%s] did not
match pattern [%s] - access denied",
+                          clientIP, accessAddresses);
         } else {
+            ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
+                          "admserv_host_ip_check: client IP address [%s] matched
pattern [%s] - access allowed",
+                          clientIP, accessAddresses);
             return DECLINED;
         }
     }