Martin Kosek wrote:
On Wed, 2011-07-06 at 11:03 -0400, Rob Crittenden wrote:
Some client errors were rather generic or outright misleading. This
cleans up some return values and displays output from the ipa-enrollment
extended operation.

ticket https://fedorahosted.org/freeipa/ticket/1417

NACK.

Good patch, but I found one issue:

ipa-client/ipa-install/ipa-client-install:
-    if ret == -1 or not ds.getDomainName():
+    if ret == ipadiscovery.NO_LDAP_SERVER or not ds.getDomainName():

You check for another error. That way the domain problem will not get
caught there.

Martin


Updated patch attached, catching NOT_FQDN now.

rob
>From 101ab0717e6655d9b2f5918f6c9c6d697c654e1a Mon Sep 17 00:00:00 2001
From: Rob Crittenden <rcrit...@redhat.com>
Date: Wed, 6 Jul 2011 10:30:24 -0400
Subject: [PATCH] Make ipa-client-install error messages more understandable and relevant.

* Check remote LDAP server to see if it is a V2 server
* Replace numeric return values with alphanumeric constants
* Display the error message from the ipa-enrollment extended op
* Remove generic join failed error message when XML-RPC fails
* Don't display Certificate subject base when enrollment fails
* Return proper error message when LDAP bind fails

https://fedorahosted.org/freeipa/ticket/1417
---
 ipa-client/ipa-install/ipa-client-install |   28 +++++++++++----------
 ipa-client/ipa-join.c                     |   24 +++++++++---------
 ipa-client/ipaclient/ipadiscovery.py      |   37 ++++++++++++++++++++--------
 3 files changed, 53 insertions(+), 36 deletions(-)

diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install
index 6bdeb8796b677c3a604083aad54f086c79af322b..6fe94231c6edca95ba189941899cf5c4d49c82bb 100755
--- a/ipa-client/ipa-install/ipa-client-install
+++ b/ipa-client/ipa-install/ipa-client-install
@@ -28,7 +28,7 @@ try:
     import logging
     import tempfile
     import getpass
-    import ipaclient.ipadiscovery
+    from ipaclient import ipadiscovery
     import ipaclient.ipachangeconf
     import ipaclient.ntpconf
     from ipapython.ipautil import run, user_input, CalledProcessError, file_exists
@@ -697,15 +697,18 @@ def main():
         sys.exit('Invalid hostname \'%s\', must be lower-case.' % hostname)
 
     # Create the discovery instance
-    ds = ipaclient.ipadiscovery.IPADiscovery()
+    ds = ipadiscovery.IPADiscovery()
 
-    ret = ds.search(domain=options.domain, server=options.server)
+    ret = ds.search(domain=options.domain, server=options.server, hostname=hostname)
 
-    if ret == -10:
+    if ret == ipadiscovery.BAD_HOST_CONFIG:
         print >>sys.stderr, "Can't get the fully qualified name of this host"
         print >>sys.stderr, "Please check that the client is properly configured"
         return ret
-    if ret == -1 or not ds.getDomainName():
+    if ret == ipadiscovery.NOT_FQDN:
+        print >>sys.stderr, "%s is not a fully-qualified hostname" % hostname
+        return ret
+    if ret == ipadiscovery.NO_LDAP_SERVER or not ds.getDomainName():
         logging.debug("Domain not found")
         if options.domain:
             cli_domain = options.domain
@@ -716,14 +719,14 @@ def main():
             print "DNS discovery failed to determine your DNS domain"
             cli_domain = user_input("Please provide the domain name of your IPA server (ex: example.com)", allow_empty = False)
             logging.debug("will use domain: %s\n", cli_domain)
-        ret = ds.search(domain=cli_domain, server=options.server)
+        ret = ds.search(domain=cli_domain, server=options.server, hostname=hostname)
 
     if not cli_domain:
         if ds.getDomainName():
             cli_domain = ds.getDomainName()
             logging.debug("will use domain: %s\n", cli_domain)
 
-    if ret == -2 or not ds.getServerName():
+    if ret == ipadiscovery.NO_LDAP_SERVER or not ds.getServerName():
         logging.debug("IPA Server not found")
         if options.server:
             cli_server = options.server
@@ -734,7 +737,7 @@ def main():
             print "DNS discovery failed to find the IPA Server"
             cli_server = user_input("Please provide your IPA server name (ex: ipa.example.com)", allow_empty = False)
             logging.debug("will use server: %s\n", cli_server)
-        ret = ds.search(domain=cli_domain, server=cli_server)
+        ret = ds.search(domain=cli_domain, server=cli_server, hostname=hostname)
     else:
         dnsok = True
     if not cli_server:
@@ -742,6 +745,9 @@ def main():
             cli_server = ds.getServerName()
             logging.debug("will use server: %s\n", cli_server)
 
+    if ret == ipadiscovery.NOT_IPA_SERVER:
+        print >>sys.stderr, "%s is not an IPA v2 Server." % cli_server
+        return ret
     if ret != 0:
         print >>sys.stderr, "Failed to verify that "+cli_server+" is an IPA Server."
         print >>sys.stderr, "This may mean that the remote server is not up or is not reachable"
@@ -855,11 +861,7 @@ def main():
             (stdout, stderr, returncode) = run(join_args, raiseonerr=False, env=env)
 
             if returncode != 0:
-                if returncode == 17:    # XML-RPC fault - possible IPA v1/v2 incompatibility
-                    print "Joining realm failed because of failing XML-RPC request."
-                    print "  This error may be caused by incompatible server/client major versions."
-                else:
-                    print >>sys.stderr, "Joining realm failed: %s" % stderr,
+                print >>sys.stderr, "Joining realm failed: %s" % stderr,
                 if not options.force:
                     return 1
                 print "  Use ipa-getkeytab to obtain a host principal for this server."
diff --git a/ipa-client/ipa-join.c b/ipa-client/ipa-join.c
index 21c087b68ca8813f0e89281b770772915c0dc543..95f2939cd9812d70aab6d29fb526ac9eb7b5479d 100644
--- a/ipa-client/ipa-join.c
+++ b/ipa-client/ipa-join.c
@@ -475,15 +475,9 @@ join_ldap(const char *ipaserver, char *hostname, const char ** binddn, const cha
     /* Now rebind as the host */
     ld = connect_ldap(ipaserver, *binddn, bindpw);
     if (!ld) {
-        if (has_principal) {
-            if (!quiet)
-                fprintf(stderr, _("Host is already joined.\n"));
-            rval = 13;
-        } else {
-            if (!quiet)
-                fprintf(stderr, _("Incorrect password.\n"));
-            rval = 15;
-        }
+        if (!quiet)
+            fprintf(stderr, _("Incorrect password.\n"));
+        rval = 15;
         goto done;
     }
 
@@ -491,13 +485,19 @@ join_ldap(const char *ipaserver, char *hostname, const char ** binddn, const cha
     valrequest.bv_len = strlen(hostname);
 
     if ((rc = ldap_extended_operation_s(ld, JOIN_OID, &valrequest, NULL, NULL, &oidresult, &valresult)) != LDAP_SUCCESS) {
+        char *s = NULL;
+#ifdef LDAP_OPT_DIAGNOSTIC_MESSAGE
+        ldap_get_option(ld, LDAP_OPT_DIAGNOSTIC_MESSAGE, &s);
+#else
+        ldap_get_option(ld, LDAP_OPT_ERROR_STRING, &s);
+#endif
         if (!quiet)
-            fprintf(stderr, _("principal not found in host entry\n"));
+            fprintf(stderr, _("Enrollment failed. %s\n"), s);
         if (debug) {
             fprintf(stderr, "ldap_extended_operation_s failed: %s",
                             ldap_err2string(rc));
         }
-        rval = 18;
+        rval = 13;
         goto ldap_done;
     }
 
@@ -1003,7 +1003,7 @@ join(const char *server, const char *hostname, const char *bindpw, const char *k
     }
 
 cleanup:
-    if (NULL != subject && !quiet)
+    if (NULL != subject && !quiet && rval == 0)
         fprintf(stderr, _("Certificate subject base is: %s\n"), subject);
 
     free((char *)princ);
diff --git a/ipa-client/ipaclient/ipadiscovery.py b/ipa-client/ipaclient/ipadiscovery.py
index d149412ec7907c0a72ad5a237bd14279e1bfcf51..77727b28e02aee100e7f772a7c1ecd0425983218 100644
--- a/ipa-client/ipaclient/ipadiscovery.py
+++ b/ipa-client/ipaclient/ipadiscovery.py
@@ -26,6 +26,13 @@ import ldap
 from ldap import LDAPError
 from ipapython.ipautil import run, CalledProcessError
 
+
+NOT_FQDN = -1
+NO_LDAP_SERVER = -2
+REALM_NOT_FOUND = -3
+NOT_IPA_SERVER = -4
+BAD_HOST_CONFIG = -10
+
 class IPADiscovery:
 
     def __init__(self):
@@ -95,8 +102,7 @@ class IPADiscovery:
                 domain = domain[p+1:]
         return (None, None)
 
-    def search(self, domain = "", server = ""):
-        hostname = ""
+    def search(self, domain = "", server = "", hostname=None):
         qname = ""
         results = []
         result = []
@@ -108,14 +114,15 @@ class IPADiscovery:
             if not domain: #domain not provided do full DNS discovery
 
                 # get the local host name
-                hostname = socket.getfqdn()
                 if not hostname:
-                    return -10 #bad host configuration
+                    hostname = socket.getfqdn()
+                if not hostname:
+                    return BAD_HOST_CONFIG
 
                 # first, check for an LDAP server for the local domain
                 p = hostname.find(".")
                 if p == -1: #no domain name
-                    return -1
+                    return NOT_FQDN
                 domain = hostname[p+1:]
 
                 # Get the list of domains from /etc/resolv.conf, we'll search
@@ -133,14 +140,14 @@ class IPADiscovery:
                         self.domain = domain
                         break
                 if not self.domain: #no ldap server found
-                    return -1
+                    return NO_LDAP_SERVER
             else:
                 logging.debug("[ipadnssearchldap]")
                 self.server = self.ipadnssearchldap(domain)
                 if self.server:
                     self.domain = domain
                 else:
-                    return -2 #no ldap server found
+                    return NO_LDAP_SERVER
 
         else: #server forced on us, this means DNS doesn't work :/
 
@@ -151,7 +158,7 @@ class IPADiscovery:
         logging.debug("[ipadnssearchkrb]")
         krbret = self.ipadnssearchkrb(self.domain)
         if not server and not krbret[0]:
-            return -3 # realm for autodiscovery not found
+            return REALM_NOT_FOUND
 
         self.realm = krbret[0]
         self.kdc = krbret[1]
@@ -161,7 +168,7 @@ class IPADiscovery:
         ldapret = self.ipacheckldap(self.server, self.realm)
 
         if not ldapret:
-            return -4 # not an IPA server (or broken config)
+            return NOT_IPA_SERVER
 
         self.server = ldapret[0]
         self.realm = ldapret[1]
@@ -169,6 +176,14 @@ class IPADiscovery:
         return 0
 
     def ipacheckldap(self, thost, trealm):
+        """
+        Given a host and kerberos realm verify that it is an IPA LDAP
+        server hosting the realm. The connection is an SSL connection
+        so the remote IPA CA cert must be available at
+        http://HOST/ipa/config/ca.crt
+
+        Returns a list [host, realm] or an empty list on error.
+        """
 
         lret = []
         lres = []
@@ -219,7 +234,7 @@ class IPADiscovery:
                     linfo = lret[0][1][lattr][0].lower()
                     break
 
-            if not linfo:
+            if not linfo or linfo.lower() != 'ipa v2.0':
                 return []
 
             #search and return known realms
@@ -323,5 +338,5 @@ class IPADiscovery:
 
             if not kdc:
                 logging.debug("SRV record for KDC not found! Realm: %s, SRV record: %s" % (realm, qname))
-        
+
         return [realm, kdc]
-- 
1.7.4

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to