Add password file to certutil calls in ipapython.certdb module #446

stlaz · 2017-02-08T15:25:11Z

With this patchset, ipa-client-install should not ask for NSS database password.

Prerequisite for it to work in FIPS:
#367

edit: This was a part of a bigger branch and might be missing some parts.

HonzaCholasta · 2017-02-09T06:52:06Z

ipapython/certdb.py

@@ -83,7 +83,8 @@ class NSSDatabase(object):
    # got too tied to IPA server details, killing reusability.
    # BaseCertDB is a class that knows nothing about IPA.
    # Generic NSS DB code should be moved here.
-    def __init__(self, nssdir=None):
+    def __init__(self, nssdir=None, password_filename=None):
+        self.password_filename = password_filename


Could we default to os.path.join(nssdir, 'pwdfile.txt')?

We might but password file does not need to exist in general. I think this assumption is made in CertDB instead.
I can add it nonetheless.

HonzaCholasta · 2017-02-09T06:54:23Z

ipapython/certdb.py

        return ipautil.run(new_args, stdin, **kwargs)

    def create_db(self, password_filename):
        """Create cert DB

        :param password_filename: Name of file containing the database password
        """
-        self.run_certutil(["-N", "-f", password_filename])
+        # run_certutil will use self.password_filename to setup the db
+        self.password_filename = password_filename


Now that the filename is specified via __init__() argument, I see very little point in keeping the argument in create_db() as well.

As I started doing this I realized that this change actually has to be done along with the one proposed on line 87, will do both, then.

HonzaCholasta · 2017-02-09T06:55:16Z

ipaserver/install/custodiainstance.py

@@ -159,7 +159,7 @@ def __get_keys(self, ca_host, cacerts_file, cacerts_pwd, data):
                             '-w', pk12pwfile])

            # Add CA certificates
-            tmpdb = CertDB(self.realm, nssdir=tmpnssdir)
+            tmpdb = NSSDatabase(tmpnssdir, password_filename=nsspwfile)


Why is this necessary?

The path to CertDB password file in this case would be $tmpnssdir"/pwdfile.txt". This file would be then passed to CertDB's inner NSSDatabase - but there is no such file. As the author of custodiainstance creates the password file in $tmpnssdir"/pk12pwfile" it was my assumption that they never meant to use IPA-specific NSS database representation.

stlaz · 2017-02-10T14:12:34Z

ipaclient/install/client.py

-    db = certdb.NSSDatabase(paths.IPA_NSSDB_DIR)
-    pwdfile = os.path.join(db.secdir, 'pwdfile.txt')
+    db = certdb.NSSDatabase(paths.IPA_NSSDB_DIR,
+                            os.path.join(paths.IPA_NSSDB_DIR, 'pwdfile.txt'))


I could do this and according changes properly by replacing NSSDatabase with CertDB but that would mean having certs.py moved to ipalib/install so that it's in a reasonable spot which I am not sure is worth it.

stlaz · 2017-02-14T15:26:42Z

NSSDatabase now defaults its .password_file to .sec_dir + 'pwdfile.txt'. It's necessary to create a pwdfile.txt in Dogtag cert store so that actions like CA renew function properly even with FIPS.

HonzaCholasta · 2017-02-15T07:50:36Z

ipaserver/install/server/upgrade.py

@@ -1521,6 +1521,11 @@ def upgrade_configuration():
            api.env.realm, certs.NSS_DIR, host_name=api.env.host)
    ca_running = ca.is_running()

+    # create passswd.txt file in PKI_TOMCAT_ALIAS_DIR if it does not exist
+    # this file will be required on most actions over this NSS DB in FIPS
+    if not os.path.exists(paths.PKI_TOMCAT_ALIAS_DIR, 'passwd.txt'):


It's pwdfile.txt.

Seems like I got somehow obsessed with passwd.txt for some reason. Fixed.

stlaz · 2017-02-15T09:16:58Z

This patchset seems more like a cleanup after the privilege separation one, although adding a password to certutil calls is still the main topic here.

tkrizek

Everything except the server upgrade bit looks fine.

tkrizek · 2017-02-16T15:23:19Z

ipaserver/install/server/upgrade.py

@@ -1543,6 +1543,11 @@ def upgrade_configuration():
            api.env.realm, paths.IPA_RADB_DIR, host_name=api.env.host)
    ca_running = ca.is_running()

+    # create passswd.txt file in PKI_TOMCAT_ALIAS_DIR if it does not exist
+    # this file will be required on most actions over this NSS DB in FIPS
+    if not os.path.exists(paths.PKI_TOMCAT_ALIAS_DIR, 'pwdfile.txt'):


os.path.exists takes only a single argument, the file path needs to be joined.

That was the original intention, thanks.

NSSDatabases should call certutil with a password. Also, removed `password_filename` argument from `.create_db()`. https://fedorahosted.org/freeipa/ticket/5695

Replaced CertDB with NSSDatabase.

MartinBasti · 2017-02-17T09:14:41Z

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/ca457eb5ce12291f555f1bf771114d6d7d191987
https://fedorahosted.org/freeipa/changeset/b20b0489ea06931bfa7d46bdbd6623bc3f09219b

stlaz changed the title ~~Certdb passwd~~ No NSS database passwords in ipa-client-install Feb 8, 2017

tkrizek self-assigned this Feb 8, 2017

HonzaCholasta reviewed Feb 9, 2017

View reviewed changes

stlaz force-pushed the certdb_passwd branch 3 times, most recently from c5547d4 to 5d7079f Compare February 10, 2017 13:22

stlaz commented Feb 10, 2017

View reviewed changes

stlaz force-pushed the certdb_passwd branch from 5d7079f to a29b529 Compare February 14, 2017 08:54

stlaz closed this Feb 14, 2017

stlaz reopened this Feb 14, 2017

stlaz force-pushed the certdb_passwd branch from a29b529 to 1bd6133 Compare February 14, 2017 14:49

stlaz force-pushed the certdb_passwd branch from 1bd6133 to ca9a33b Compare February 14, 2017 15:33

HonzaCholasta reviewed Feb 15, 2017

View reviewed changes

stlaz force-pushed the certdb_passwd branch from ca9a33b to cb101d0 Compare February 15, 2017 09:15

stlaz changed the title ~~No NSS database passwords in ipa-client-install~~ Add password file to certutil calls in ipapython.certdb module Feb 15, 2017

tkrizek suggested changes Feb 16, 2017

View reviewed changes

stlaz added 2 commits February 16, 2017 17:54

Add password to certutil calls in NSSDatabase

301a98b

NSSDatabases should call certutil with a password. Also, removed `password_filename` argument from `.create_db()`. https://fedorahosted.org/freeipa/ticket/5695

custodiainstance: don't use IPA-specific CertDB

8a13b39

Replaced CertDB with NSSDatabase.

stlaz force-pushed the certdb_passwd branch from cb101d0 to 8a13b39 Compare February 16, 2017 16:54

tkrizek added the ack Pull Request approved, can be merged label Feb 17, 2017

MartinBasti added the pushed Pull Request has already been pushed label Feb 17, 2017

MartinBasti closed this Feb 17, 2017

stlaz deleted the certdb_passwd branch September 11, 2017 10:48

Add password file to certutil calls in ipapython.certdb module #446

Add password file to certutil calls in ipapython.certdb module #446

stlaz commented Feb 8, 2017 •

edited by MartinBasti

HonzaCholasta Feb 9, 2017

stlaz Feb 9, 2017

HonzaCholasta Feb 9, 2017

stlaz Feb 9, 2017

stlaz Feb 9, 2017

HonzaCholasta Feb 9, 2017

stlaz Feb 9, 2017

stlaz Feb 10, 2017 •

edited

stlaz commented Feb 14, 2017 •

edited

HonzaCholasta Feb 15, 2017

stlaz Feb 15, 2017

stlaz commented Feb 15, 2017

tkrizek left a comment

tkrizek Feb 16, 2017

stlaz Feb 16, 2017

MartinBasti commented Feb 17, 2017

Add password file to certutil calls in ipapython.certdb module #446

Add password file to certutil calls in ipapython.certdb module #446

Conversation

stlaz commented Feb 8, 2017 • edited by MartinBasti

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stlaz Feb 10, 2017 • edited

Choose a reason for hiding this comment

stlaz commented Feb 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stlaz commented Feb 15, 2017

tkrizek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinBasti commented Feb 17, 2017

stlaz commented Feb 8, 2017 •

edited by MartinBasti

stlaz Feb 10, 2017 •

edited

stlaz commented Feb 14, 2017 •

edited