On 11.7.2013 20:51, Rob Crittenden wrote:
Jan Cholasta wrote:
Hi,

the attached patches fix <https://fedorahosted.org/freeipa/ticket/3717>.

Also added a small patch to fix a formatting issue with
installutils.read_password.

Honza

Functionally ok but I found it very jarring the way the passwords were
prompted for. I think they should be moved after the realm question and
the text should be more than just the path to the filename.

rob


Moved the prompt and changed the text to "Enter <file> unlock password".

Updated patches attached.

Honza

--
Jan Cholasta
>From 992080e556036afe35c39092a5326c3267d8edf1 Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Tue, 9 Jul 2013 10:23:47 +0000
Subject: [PATCH 1/3] Ask for PKCS#12 password interactively in
 ipa-server-install.

https://fedorahosted.org/freeipa/ticket/3717
---
 install/tools/ipa-server-install | 73 ++++++++++++++++++++++++++--------------
 1 file changed, 48 insertions(+), 25 deletions(-)

diff --git a/install/tools/ipa-server-install b/install/tools/ipa-server-install
index cc88a0b..fed2004 100755
--- a/install/tools/ipa-server-install
+++ b/install/tools/ipa-server-install
@@ -276,14 +276,20 @@ def parse_options():
             if not options.forwarders and not options.no_forwarders:
                 parser.error("You must specify at least one --forwarder option or --no-forwarders option")
 
-    # If any of the PKCS#12 options are selected, all are required. Create a
-    # list of the options and count it to enforce that all are required without
-    # having a huge set of it blocks.
-    pkcs12 = [options.dirsrv_pkcs12, options.http_pkcs12, options.dirsrv_pin, options.http_pin]
-    cnt = pkcs12.count(None)
-    if cnt > 0 and cnt < 4:
+    # If any of the PKCS#12 options are selected, all are required.
+    pkcs12_req = (options.dirsrv_pkcs12, options.http_pkcs12)
+    pkcs12_opt = (options.pkinit_pkcs12,)
+    if any(pkcs12_req + pkcs12_opt) and not all(pkcs12_req):
         parser.error("All PKCS#12 options are required if any are used.")
 
+    if options.unattended:
+        if options.dirsrv_pkcs12 and not options.dirsrv_pin:
+            parser.error("You must specify --dirsrv_pin with --dirsrv_pkcs12")
+        if options.http_pkcs12 and not options.http_pin:
+            parser.error("You must specify --http_pin with --http_pkcs12")
+        if options.pkinit_pkcs12 and not options.pkinit_pin:
+            parser.error("You must specify --pkinit_pin with --pkinit_pkcs12")
+
     if options.dirsrv_pkcs12 and not options.root_ca_file:
         parser.error(
             "--root-ca-file must be given with the PKCS#12 options.")
@@ -704,18 +710,6 @@ def main():
                 sys.exit(1)
             cert = certdict[certissuer]
 
-    if options.http_pkcs12:
-        http_pin_file = ipautil.write_tmp_file(options.http_pin)
-        http_pkcs12_info = (options.http_pkcs12, http_pin_file.name)
-
-    if options.dirsrv_pkcs12:
-        dirsrv_pin_file = ipautil.write_tmp_file(options.dirsrv_pin)
-        dirsrv_pkcs12_info = (options.dirsrv_pkcs12, dirsrv_pin_file.name)
-
-    if options.pkinit_pkcs12:
-        pkinit_pin_file = ipautil.write_tmp_file(options.pkinit_pin)
-        pkinit_pkcs12_info = (options.pkinit_pkcs12, pkinit_pin_file.name)
-
     # We only set up the CA if the PKCS#12 options are not given.
     if options.dirsrv_pkcs12:
         setup_ca = False
@@ -834,13 +828,6 @@ def main():
     else:
         domain_name = options.domain_name
 
-    if options.http_pkcs12:
-        # Check the given PKCS#12 files
-        ca_file = options.root_ca_file
-        check_pkcs12 = installutils.check_pkcs12
-        http_cert_name = check_pkcs12(http_pkcs12_info, ca_file, host_name)
-        dirsrv_cert_name = check_pkcs12(dirsrv_pkcs12_info, ca_file, host_name)
-
     domain_name = domain_name.lower()
 
     ip = get_server_ip_address(host_name, fstore, options.unattended, options)
@@ -858,6 +845,42 @@ def main():
     if not options.subject:
         options.subject = DN(('O', realm_name))
 
+    ca_file = options.root_ca_file
+
+    if options.http_pkcs12:
+        if not options.http_pin:
+            options.http_pin = installutils.read_password(
+                "Enter %s unlock" % options.http_pkcs12,
+                confirm=False, validate=False)
+            if options.http_pin is None:
+                sys.exit("%s unlock password required" % options.http_pkcs12)
+        http_pin_file = ipautil.write_tmp_file(options.http_pin)
+        http_pkcs12_info = (options.http_pkcs12, http_pin_file.name)
+        http_cert_name = installutils.check_pkcs12(
+            http_pkcs12_info, ca_file, host_name)
+
+    if options.dirsrv_pkcs12:
+        if not options.dirsrv_pin:
+            options.dirsrv_pin = installutils.read_password(
+                "Enter %s unlock" % options.dirsrv_pkcs12,
+                confirm=False, validate=False)
+            if options.dirsrv_pin is None:
+                sys.exit("%s unlock password required" % options.dirsrv_pkcs12)
+        dirsrv_pin_file = ipautil.write_tmp_file(options.dirsrv_pin)
+        dirsrv_pkcs12_info = (options.dirsrv_pkcs12, dirsrv_pin_file.name)
+        dirsrv_cert_name = installutils.check_pkcs12(
+            dirsrv_pkcs12_info, ca_file, host_name)
+
+    if options.pkinit_pkcs12:
+        if not options.pkinit_pin:
+            options.pkinit_pin = installutils.read_password(
+                "Enter %s unlock" % options.pkinit_pkcs12,
+                confirm=False, validate=False)
+            if options.pkinit_pin is None:
+                sys.exit("%s unlock password required" % options.pkinit_pkcs12)
+        pkinit_pin_file = ipautil.write_tmp_file(options.pkinit_pin)
+        pkinit_pkcs12_info = (options.pkinit_pkcs12, pkinit_pin_file.name)
+
     if not options.dm_password:
         dm_password = read_dm_password()
 
-- 
1.8.3.1

>From 75bd0b50a5317e8bcd540c4fdc435fea6988efd2 Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Tue, 9 Jul 2013 10:24:14 +0000
Subject: [PATCH 2/3] Ask for PKCS#12 password interactively in
 ipa-replica-prepare.

https://fedorahosted.org/freeipa/ticket/3717
---
 ipaserver/install/ipa_replica_prepare.py | 46 ++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 14 deletions(-)

diff --git a/ipaserver/install/ipa_replica_prepare.py b/ipaserver/install/ipa_replica_prepare.py
index f6af28e..3135b93 100644
--- a/ipaserver/install/ipa_replica_prepare.py
+++ b/ipaserver/install/ipa_replica_prepare.py
@@ -103,15 +103,9 @@ class ReplicaPrepare(admintool.AdminTool):
         options.setup_pkinit = False
 
         # If any of the PKCS#12 options are selected, all are required.
-        pkcs12_opts = [options.dirsrv_pkcs12, options.dirsrv_pin,
-                    options.http_pkcs12, options.http_pin]
-        if options.setup_pkinit:
-            pkcs12_opts.extend([options.pkinit_pkcs12, options.pkinit_pin])
-        if pkcs12_opts[0]:
-            pkcs12_okay = all(opt for opt in pkcs12_opts)
-        else:
-            pkcs12_okay = all(opt is None for opt in pkcs12_opts)
-        if not pkcs12_okay:
+        pkcs12_req = (options.dirsrv_pkcs12, options.http_pkcs12)
+        pkcs12_opt = (options.pkinit_pkcs12,)
+        if any(pkcs12_req + pkcs12_opt) and not all(pkcs12_req):
             self.option_parser.error(
                 "All PKCS#12 options are required if any are used.")
 
@@ -136,11 +130,6 @@ class ReplicaPrepare(admintool.AdminTool):
                 "--http_pkcs12, --dirsrv_pkcs12 options to provide custom "
                 "certificates.")
 
-        if options.http_pkcs12:
-            # Check the given PKCS#12 files
-            self.check_pkcs12(options.http_pkcs12, options.http_pin)
-            self.check_pkcs12(options.dirsrv_pkcs12, options.dirsrv_pin)
-
         config_dir = dsinstance.config_dirname(
             dsinstance.realm_to_serverid(api.env.realm))
         if not ipautil.dir_exists(config_dir):
@@ -220,6 +209,35 @@ class ReplicaPrepare(admintool.AdminTool):
                     options.reverse_zone, options.ip_address):
                 raise admintool.ScriptError("Invalid reverse zone")
 
+        if options.http_pkcs12:
+            if not options.http_pin:
+                options.http_pin = installutils.read_password(
+                    "Enter %s unlock" % options.http_pkcs12,
+                    confirm=False, validate=False)
+                if options.http_pin is None:
+                    raise admintool.ScriptError(
+                        "%s unlock password required" % options.http_pkcs12)
+            self.check_pkcs12(options.http_pkcs12, options.http_pin)
+
+        if options.dirsrv_pkcs12:
+            if not options.dirsrv_pin:
+                options.dirsrv_pin = installutils.read_password(
+                    "Enter %s unlock" % options.dirsrv_pkcs12,
+                    confirm=False, validate=False)
+                if options.dirsrv_pin is None:
+                    raise admintool.ScriptError(
+                        "%s unlock password required" % options.dirsrv_pkcs12)
+            self.check_pkcs12(options.dirsrv_pkcs12, options.dirsrv_pin)
+
+        if options.pkinit_pkcs12:
+            if not options.pkinit_pin:
+                options.pkinit_pin = installutils.read_password(
+                    "Enter %s unlock" % options.pkinit_pkcs12,
+                    confirm=False, validate=False)
+                if options.pkinit_pin is None:
+                    raise admintool.ScriptError(
+                        "%s unlock password required" % options.pkinit_pkcs12)
+
         if (not ipautil.file_exists(
                     dogtag.configured_constants().CS_CFG_PATH) and
                 not options.dirsrv_pin):
-- 
1.8.3.1

>From b59d5cadad99d3314e273a06b862fb28bf90c3f9 Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Tue, 9 Jul 2013 10:29:21 +0000
Subject: [PATCH 3/3] Print newline after receiving EOF in
 installutils.read_password.

---
 install/tools/ipa-ca-install         | 2 +-
 install/tools/ipa-compat-manage      | 2 +-
 install/tools/ipa-csreplica-manage   | 2 +-
 install/tools/ipa-dns-install        | 2 +-
 install/tools/ipa-managed-entries    | 2 +-
 install/tools/ipa-nis-manage         | 2 +-
 install/tools/ipa-replica-conncheck  | 2 +-
 install/tools/ipa-replica-install    | 2 +-
 install/tools/ipa-replica-manage     | 2 +-
 install/tools/ipa-server-certinstall | 2 +-
 install/tools/ipa-server-install     | 6 +++---
 ipaserver/install/installutils.py    | 3 ++-
 12 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/install/tools/ipa-ca-install b/install/tools/ipa-ca-install
index 1fd59ec..636f63d 100755
--- a/install/tools/ipa-ca-install
+++ b/install/tools/ipa-ca-install
@@ -125,7 +125,7 @@ def main():
         except KeyboardInterrupt:
             sys.exit(0)
         if dirman_password is None:
-            sys.exit("\nDirectory Manager password required")
+            sys.exit("Directory Manager password required")
 
     if not options.admin_password and not options.skip_conncheck and \
         options.unattended:
diff --git a/install/tools/ipa-compat-manage b/install/tools/ipa-compat-manage
index 87fa47f..7061a3e 100755
--- a/install/tools/ipa-compat-manage
+++ b/install/tools/ipa-compat-manage
@@ -98,7 +98,7 @@ def main():
     else:
         dirman_password = get_dirman_password()
         if dirman_password is None:
-            sys.exit("\nDirectory Manager password required")
+            sys.exit("Directory Manager password required")
 
     api.bootstrap(context='cli', debug=options.debug)
     api.finalize()
diff --git a/install/tools/ipa-csreplica-manage b/install/tools/ipa-csreplica-manage
index 4e11ffd..ce027be 100755
--- a/install/tools/ipa-csreplica-manage
+++ b/install/tools/ipa-csreplica-manage
@@ -401,7 +401,7 @@ def main():
         dirman_passwd = installutils.read_password("Directory Manager", confirm=False,
             validate=False, retry=False)
         if dirman_passwd is None:
-            sys.exit("\nDirectory Manager password required")
+            sys.exit("Directory Manager password required")
 
     options.dirman_passwd = dirman_passwd
 
diff --git a/install/tools/ipa-dns-install b/install/tools/ipa-dns-install
index 47bc31b..275e699 100755
--- a/install/tools/ipa-dns-install
+++ b/install/tools/ipa-dns-install
@@ -142,7 +142,7 @@ def main():
     dm_password = options.dm_password or read_password("Directory Manager",
                                              confirm=False, validate=False)
     if dm_password is None:
-        sys.exit("\nDirectory Manager password required")
+        sys.exit("Directory Manager password required")
     bind = bindinstance.BindInstance(fstore, dm_password)
 
     # try the connection
diff --git a/install/tools/ipa-managed-entries b/install/tools/ipa-managed-entries
index 5bf3ad6..2cf37e2 100755
--- a/install/tools/ipa-managed-entries
+++ b/install/tools/ipa-managed-entries
@@ -95,7 +95,7 @@ def main():
     except errors.ACIError:
         dirman_password = get_dirman_password()
         if dirman_password is None:
-            sys.exit("\nDirectory Manager password required")
+            sys.exit("Directory Manager password required")
         try:
             conn.do_simple_bind(bindpw=dirman_password)
         except errors.ACIError:
diff --git a/install/tools/ipa-nis-manage b/install/tools/ipa-nis-manage
index a35e19f..71c0761 100755
--- a/install/tools/ipa-nis-manage
+++ b/install/tools/ipa-nis-manage
@@ -108,7 +108,7 @@ def main():
     else:
         dirman_password = get_dirman_password()
         if dirman_password is None:
-            sys.exit("\nDirectory Manager password required")
+            sys.exit("Directory Manager password required")
 
     if not dirman_password:
         sys.exit("No password supplied")
diff --git a/install/tools/ipa-replica-conncheck b/install/tools/ipa-replica-conncheck
index 3b0b1d0..583b5d5 100755
--- a/install/tools/ipa-replica-conncheck
+++ b/install/tools/ipa-replica-conncheck
@@ -340,7 +340,7 @@ def main():
                 password = installutils.read_password(principal, confirm=False,
                            validate=False, retry=False)
                 if password is None:
-                    sys.exit("\nPrincipal password required")
+                    sys.exit("Principal password required")
 
 
             stderr=''
diff --git a/install/tools/ipa-replica-install b/install/tools/ipa-replica-install
index c013c29..79f8a7a 100755
--- a/install/tools/ipa-replica-install
+++ b/install/tools/ipa-replica-install
@@ -480,7 +480,7 @@ def main():
         except KeyboardInterrupt:
             sys.exit(0)
         if dirman_password is None:
-            sys.exit("\nDirectory Manager password required")
+            sys.exit("Directory Manager password required")
 
     try:
         top_dir, dir = expand_replica_info(filename, dirman_password)
diff --git a/install/tools/ipa-replica-manage b/install/tools/ipa-replica-manage
index 2fb32fa..e2bd38e 100755
--- a/install/tools/ipa-replica-manage
+++ b/install/tools/ipa-replica-manage
@@ -1178,7 +1178,7 @@ def main():
             dirman_passwd = installutils.read_password("Directory Manager",
                 confirm=False, validate=False, retry=False)
             if dirman_passwd is None:
-                sys.exit("\nDirectory Manager password required")
+                sys.exit("Directory Manager password required")
 
     options.dirman_passwd = dirman_passwd
 
diff --git a/install/tools/ipa-server-certinstall b/install/tools/ipa-server-certinstall
index bc4dde2..5b498b1 100755
--- a/install/tools/ipa-server-certinstall
+++ b/install/tools/ipa-server-certinstall
@@ -141,7 +141,7 @@ def main():
             dm_password = installutils.read_password("Directory Manager",
                 confirm=False, validate=False, retry=False)
             if dm_password is None:
-                sys.exit("\nDirectory Manager password required")
+                sys.exit("Directory Manager password required")
             realm = get_realm_name()
             dirname = dsinstance.config_dirname(dsinstance.realm_to_serverid(realm))
             fd = open(dirname + "/pwdfile.txt")
diff --git a/install/tools/ipa-server-install b/install/tools/ipa-server-install
index fed2004..3fff521 100755
--- a/install/tools/ipa-server-install
+++ b/install/tools/ipa-server-install
@@ -665,7 +665,7 @@ def main():
         else:
             dm_password = read_password("Directory Manager", confirm=False)
         if dm_password is None:
-            sys.exit("\nDirectory Manager password required")
+            sys.exit("Directory Manager password required")
         try:
             options._update_loose(read_cache(dm_password))
         except Exception, e:
@@ -885,7 +885,7 @@ def main():
         dm_password = read_dm_password()
 
         if dm_password is None:
-            sys.exit("\nDirectory Manager password required")
+            sys.exit("Directory Manager password required")
     else:
         dm_password = options.dm_password
 
@@ -897,7 +897,7 @@ def main():
     if not options.admin_password:
         admin_password = read_admin_password()
         if admin_password is None:
-            sys.exit("\nIPA admin password required")
+            sys.exit("IPA admin password required")
     else:
         admin_password = options.admin_password
 
diff --git a/ipaserver/install/installutils.py b/ipaserver/install/installutils.py
index 278240f..f1e3184 100644
--- a/ipaserver/install/installutils.py
+++ b/ipaserver/install/installutils.py
@@ -291,7 +291,8 @@ def read_password(user, confirm=True, validate=True, retry=True, validator=_read
                 correct = True
     except EOFError:
         return None
-    print ""
+    finally:
+        print ""
     return pwd
 
 def update_file(filename, orig, subst):
-- 
1.8.3.1

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

Reply via email to