ca, kra install: validate DM password #757

nicki-krizek · 2017-05-03T12:33:54Z

Prevent CA and KRA installation from proceeding if provided DM password is invalid to avoid broken installations with no possibility to uninstall CA or KRA.

https://pagure.io/freeipa/issue/6892

tiran · 2017-05-03T12:52:01Z

install/tools/ipa-ca-install

+                "Directory Manager (existing master)", confirm=False,
+                validate=False)
+        except KeyboardInterrupt:
+            sys.exit(0)


keyboard interrupt should not result in exit code 0. A common exit code is 130 (128 + SIGINT).

I'm afraid that this is inconsistent with other parts of code. The rest will return error code 1 in case of SIGINT.

If you want to keep new error code, then manpage should be updated as well.

Or even better, don't catch that exception and let installutils.run_script() to handle it in the same way for every script.

MartinBasti · 2017-05-03T14:58:12Z

ipaserver/install/installutils.py

+    try:
+        client.simple_bind(DIRMAN_DN, password)
+    except errors.ACIError:
+        raise ValueError("Invalid Directory Manager password")


I'm sorry but I think you may miss following code there:

else: client.unbind()

Also I would rather use verbs to name functions/methods, like validate_dm_password (but I have a suspicion that one is already taken).

martbab · 2017-05-03T14:59:00Z

install/tools/ipa-ca-install

+    try:
+        installutils.dm_password_validator(password)
+    except ValueError:
+        sys.exit("Directory Manager password is invalid")


Please don't use sys.exit() explicitly here, either let the exception propagate up the stack to the caller or re-raise RuntimeError as you do in ipa-kra-install

stlaz · 2017-05-04T07:04:14Z

There will be no more sys.exits. This patchset shall not be ACKed until all have been removed.

stlaz

Don't use sys.exits

Extract copy-pasted code to a single function. Related https://pagure.io/freeipa/issue/6892 Signed-off-by: Tomas Krizek <tkrizek@redhat.com>

Add a validator that checks whether provided Directory Manager is valid by attempting to connect to LDAP. Related https://pagure.io/freeipa/issue/6892 Signed-off-by: Tomas Krizek <tkrizek@redhat.com>

nicki-krizek · 2017-05-05T13:50:19Z

Thanks for the feedback, hopefully I addressed all the issues.

MartinBasti

Please fix nitpicks, and ehm..ehm maybe add a CI test? Works for me otherwise

MartinBasti · 2017-05-05T16:35:22Z

install/tools/ipa-ca-install

+def _get_dirman_password(password=None, unattended=False):
+    if not password:
+        if unattended:
+            raise RuntimeError('Directory Manager password required')


Please use script error to be consistent

ipaserver/install/server/install.py: raise ScriptError("Directory Manager password required")

MartinBasti · 2017-05-05T16:35:54Z

install/tools/ipa-ca-install

+    try:
+        installutils.validate_dm_password_ldap(password)
+    except ValueError:
+        raise RuntimeError("Directory Manager password is invalid")


Please use ScriptError here as well.

MartinBasti · 2017-05-05T16:38:10Z

ipaserver/install/ipa_kra_install.py

+        try:
+            installutils.validate_dm_password_ldap(self.options.password)
+        except ValueError:
+            raise RuntimeError("Directory Manager password is invalid")


ScriptError please :)

nicki-krizek · 2017-05-10T15:12:49Z

Implementing the tests shouldn't block us from pushing this fix. I opened a ticket for it: https://pagure.io/freeipa/issue/6941

MartinBasti · 2017-05-11T14:44:36Z

We have to use sys.exit() in this case, because I forgot that CA still uses old style installer. Without sys.exit() ti will always suggest user to uninstall server:

Your system may be partly configured.
Run /usr/sbin/ipa-server-install --uninstall to clean up.

We don't want to uninstall server due typo in password

stlaz · 2017-05-12T12:00:20Z

You forgot an import in ipa-ca-install:

************* Module ipa-ca-install

install/tools/ipa-ca-install:37: [W0611(unused-import), ] Unused ScriptError imported from ipapython.admintool)

Before proceeding with installation, validate DM password. If the provided DM password is invalid, abort the installation. Fixes https://pagure.io/freeipa/issue/6892 Signed-off-by: Tomas Krizek <tkrizek@redhat.com>

MartinBasti · 2017-05-17T11:01:38Z

master:

80d61c2 ca install: merge duplicated code for DM password
7a4a368 installutils: add DM password validator
1b1bace ca, kra install: validate DM password

ipa-4-5:

282fc0c ca install: merge duplicated code for DM password
4c12b71 installutils: add DM password validator
b8bcaa6 ca, kra install: validate DM password

tiran requested changes May 3, 2017

View reviewed changes

nicki-krizek force-pushed the validate-dm-password branch from 23c0ca9 to 03e8494 Compare May 3, 2017 13:40

MartinBasti self-assigned this May 3, 2017

MartinBasti reviewed May 3, 2017

View reviewed changes

martbab reviewed May 3, 2017

View reviewed changes

stlaz requested changes May 4, 2017

View reviewed changes

Tomas Krizek added 2 commits May 5, 2017 15:22

ca install: merge duplicated code for DM password

2cce230

Extract copy-pasted code to a single function. Related https://pagure.io/freeipa/issue/6892 Signed-off-by: Tomas Krizek <tkrizek@redhat.com>

installutils: add DM password validator

e3e8f05

Add a validator that checks whether provided Directory Manager is valid by attempting to connect to LDAP. Related https://pagure.io/freeipa/issue/6892 Signed-off-by: Tomas Krizek <tkrizek@redhat.com>

nicki-krizek force-pushed the validate-dm-password branch from 03e8494 to 6f3e293 Compare May 5, 2017 13:49

MartinBasti suggested changes May 5, 2017

View reviewed changes

nicki-krizek force-pushed the validate-dm-password branch 2 times, most recently from 4677428 to 8d13f2a Compare May 10, 2017 15:04

nicki-krizek force-pushed the validate-dm-password branch 2 times, most recently from 6eef190 to fa43013 Compare May 11, 2017 14:50

stlaz approved these changes May 12, 2017

View reviewed changes

ca, kra install: validate DM password

630829e

Before proceeding with installation, validate DM password. If the provided DM password is invalid, abort the installation. Fixes https://pagure.io/freeipa/issue/6892 Signed-off-by: Tomas Krizek <tkrizek@redhat.com>

nicki-krizek force-pushed the validate-dm-password branch from fa43013 to 630829e Compare May 12, 2017 14:22

nicki-krizek added the prioritized Pull Request has higher priority for PR-CI label May 17, 2017

MartinBasti added ack Pull Request approved, can be merged and removed prioritized Pull Request has higher priority for PR-CI labels May 17, 2017

MartinBasti approved these changes May 17, 2017

View reviewed changes

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

MartinBasti closed this May 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ca, kra install: validate DM password #757

ca, kra install: validate DM password #757

nicki-krizek commented May 3, 2017 •

edited by MartinBasti

Loading

tiran May 3, 2017

MartinBasti May 3, 2017

MartinBasti May 3, 2017

MartinBasti May 3, 2017 •

edited

Loading

martbab May 3, 2017

martbab May 3, 2017

stlaz commented May 4, 2017

stlaz left a comment

nicki-krizek commented May 5, 2017

MartinBasti left a comment •

edited

Loading

MartinBasti May 5, 2017

MartinBasti May 5, 2017

MartinBasti May 5, 2017

nicki-krizek commented May 10, 2017

MartinBasti commented May 11, 2017

stlaz commented May 12, 2017

MartinBasti commented May 17, 2017

ca, kra install: validate DM password #757

ca, kra install: validate DM password #757

Conversation

nicki-krizek commented May 3, 2017 • edited by MartinBasti Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinBasti May 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stlaz commented May 4, 2017

stlaz left a comment

Choose a reason for hiding this comment

nicki-krizek commented May 5, 2017

MartinBasti left a comment • edited Loading

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

nicki-krizek commented May 10, 2017

MartinBasti commented May 11, 2017

stlaz commented May 12, 2017

MartinBasti commented May 17, 2017

nicki-krizek commented May 3, 2017 •

edited by MartinBasti

Loading

MartinBasti May 3, 2017 •

edited

Loading

MartinBasti left a comment •

edited

Loading