FIPS: replica install check #437

tkrizek · 2017-02-06T15:58:49Z

PR depends on the rest of the FIPS patches.

MartinBasti · 2017-02-06T16:52:33Z

ipaserver/install/server/replicainstall.py

-            "Cannot install replica of a server of higher version ({}) than"
-            "the local version ({})".format(remote_version, api_version))
+        raise ScriptError(
+            _("Cannot install replica of a server of higher version "


We don't use ugettext in installers, only in plugins

MartinBasti · 2017-02-06T16:53:12Z

ipaserver/install/server/replicainstall.py

+    if fips_mode != remote_fips_mode:
+        if fips_mode:
+            raise ScriptError(
+                _("Cannot join FIPS-enabled replica into existing topology: "


MartinBasti · 2017-02-06T16:53:45Z

ipaserver/install/server/replicainstall.py

+                  "FIPS is not enabled on the master server."))
+        else:
+            raise ScriptError(
+                _("Cannot join replica into existing FIPS-enabled topology: "


MartinBasti · 2017-02-06T16:58:07Z

I'm still afraid that users may want to create a FIPS replica from the non-FIPS master, even if it is not recommended due security. How can be this achieved?

tkrizek · 2017-02-06T17:05:35Z

@MartinBasti Since this check is performed only during installation, the user could simply install non-FIPS replica and then turn FIPS on afterwards. There might be issues with this approach and thus it is neither recommended nor supported, as stated in the documentation.

MartinBasti · 2017-02-06T19:01:22Z

@tomaskrizek on current versions of RHEL and fedora IPA doesn't start in FIPS, but upgrading first and then enabling FIPS might be the way

HonzaCholasta · 2017-02-07T06:20:01Z

ipalib/config.py

@@ -44,6 +44,7 @@
 from ipalib.constants import CONFIG_SECTION
 from ipalib.constants import OVERRIDE_ERROR, SET_ERROR, DEL_ERROR
 from ipalib import errors
+from ipaplatform.tasks import tasks


Importing from ipaplatform to ipalib is forbidden, this will have to be worked around.

HonzaCholasta · 2017-02-07T06:20:20Z

ipaserver/install/server/replicainstall.py

+        raise ScriptError(
+            "Cannot install replica of a server of higher version "
+            "(%(remote_version)s) than the local version (%(api_version)s)"
+            % dict(remote_version=remote_version, api_version=api_version))


NACK, .format() is the preferred way of formatting strings.

HonzaCholasta · 2017-02-07T06:22:40Z

ipaserver/install/server/replicainstall.py

@@ -1077,7 +1099,7 @@ def promote_check(installer):
    remote_api.finalize()
    installer._remote_api = remote_api

-    check_remote_version(remote_api)
+    check_remote_compatibility(remote_api)


This check is executed only in replica promotion. There should be a check for classic replica install as well; IMO it should reject install when FIPS is enabled.

Yes, that's what happens now with the is_fips_enabled() guards. Since FIPS is not available on DL0, it should fail.

stlaz · 2017-02-07T07:13:01Z

ipalib/config.py

@@ -497,6 +498,10 @@ def _bootstrap(self, **overrides):
        if 'plugins_on_demand' not in self:
            self.plugins_on_demand = (self.context == 'cli')

+        # Set fips_mode:
+        if 'fips_mode' not in self:
+            self.fips_mode = tasks.is_fips_enabled()


Do we want the user to be able to set env.fips_mode during api bootstrapping? I do not think so. This should be set amongst other run-time variables.

stlaz · 2017-02-07T07:20:05Z

ipaserver/install/server/replicainstall.py

+
+    # Check FIPS mode compatibility
+    remote_fips_mode = env['fips_mode']
+    fips_mode = tasks.is_fips_enabled()


I think you could rather pass use api.env.fips_mode similar to what's done in the api_version.

stlaz · 2017-02-07T07:29:25Z

ipaserver/install/server/replicainstall.py

+            % dict(remote_version=remote_version, api_version=api_version))
+
+    # Check FIPS mode compatibility
+    remote_fips_mode = env['fips_mode']


env does not necessarily contain "fips_mode" key.

martbab · 2017-02-07T07:43:01Z

@tomaskrizek since you added a new key to the Env object, you will have to fix test_ipalib/test_config.py to account for this change, see https://travis-ci.org/freeipa/freeipa/jobs/198916106#L443

pvoborni · 2017-02-07T09:49:55Z

@MartinBasti I'm not sure from your comment if you would like to provide a way to change non-FIPS server into a FIPS server or just brainstorming ways how it can be worked around. In any case this path is not a goal and actually should be discouraged. http://www.freeipa.org/page/V4/FreeIPA-on-FIPS#Design

MartinBasti · 2017-02-07T11:01:21Z

@pvoborni more or less brainstorming, as I'm almost sure that people will want to migrate current deployments to FIPS mode

tkrizek · 2017-02-07T15:33:07Z

Thanks for the feedback. Hopefully I addressed all the concerns above in the update.

stlaz · 2017-02-08T08:16:21Z

ipalib/config.py

@@ -440,6 +444,9 @@ def _bootstrap(self, **overrides):
        self.bin = path.dirname(self.script)
        self.home = os.environ.get('HOME', None)

+        if tasks is not None:
+            self.fips_mode = tasks.is_fips_enabled()


This needs an explanation in a comment.

stlaz · 2017-02-08T08:19:16Z

ipaserver/install/server/replicainstall.py

+    """
+    env = client.forward(u'env', u'fips_mode')['result']
+    remote_fips_mode = env.get('fips_mode', False)
+    fips_mode = tasks.is_fips_enabled()


I believe you left some mess here.

stlaz · 2017-02-08T08:24:06Z

ipaserver/install/server/replicainstall.py

+    Verify remote server's version is not higher than this server's version
+
+    :param client: RPC client
+


Please fill this empty line with the other parameter documentation. Don't leave a blank line before :raises, it's ugly and it's not usually done this way pythonhosted:Sphinx.

stlaz · 2017-02-08T08:26:07Z

ipaserver/install/server/replicainstall.py

+
+    :param client: RPC client
+
+    :raises: ``ScriptError`` if the checks fails


Should be :raises: ScripError: blahblah

stlaz · 2017-02-08T08:30:15Z

ipaserver/install/server/replicainstall.py

@@ -747,6 +786,10 @@ def install_check(installer):
                         ldap_uri=ldapuri)
    remote_api.finalize()
    installer._remote_api = remote_api
+
+    with rpc_client(remote_api) as client:
+        check_remote_fips_mode(client, api.env.fips_mode)


Why are you checking fips mode twice in DL1? Also, if this is supposed to address the issue "Explode if trying to install in FIPS on DL0", this won't work.

The DL1 twice checking remark is false, thought this code was in common_check(). The other part still applies, though.

stlaz · 2017-02-08T09:44:44Z

LGTM

HonzaCholasta · 2017-02-08T10:34:35Z

ipalib/constants.py

@@ -215,6 +215,7 @@
    ('script', object),  # sys.argv[0]
    ('bin', object),  # The directory containing the script
    ('home', object),  # $HOME
+    ('fips_mode', object),  # FIPS-mode of the server


Won't this cause api.env.fips_mode to be object rather than unset when ipaplatform is not installed?

You're right. I didn't notice these settings are global, not just test-specific. I removed this bit and fixed the test instead.

HonzaCholasta · 2017-02-09T07:01:06Z

ipaserver/install/server/replicainstall.py

-    if remote_version > api_version:
-        raise RuntimeError(
+    if remote_version > local_version:
+        raise ScriptError(


Note that ScriptError is really only necessary when you need the script to fail with a specific exit code. If you don't need that and are fine with exit code 1, RuntimeError is all you need.

HonzaCholasta · 2017-02-09T07:01:55Z

ipatests/test_ipalib/test_config.py

@@ -563,7 +563,7 @@ def test_finalize_core(self):
        # Test using DEFAULT_CONFIG:
        defaults = dict(constants.DEFAULT_CONFIG)
        (o, home) = self.finalize_core(None, **defaults)
-        assert list(o) == sorted(defaults)
+        assert set(o).issuperset(set(defaults))


I would rather keep the original check, but make sure that o does not contain fips_mode before doing it.

I'm not sure how you'd like to make sure o doesn't contain fips_mode. Env object does not allow deletion and the only way I can think of would be to violate the object's "private" properties.

I propose we either:

always set fips_mode: to True, False or None

accept that Env can now have optional attributes and use the test proposed above

You can just add the fips_mode into ipalib/constants and set its value to object. We had to do this workaround when @tiran added nss_dir to the config (see https://github.com/freeipa/freeipa/blob/master/ipalib/constants.py#L228). The test in the original from will then just work(R).

@martbab I tried to do that, but that also sets the fips_mode key for the env object, doesn't it? We want the fips_mode key to not be present if ipaplatform wasn't imported.

I think this should work:

defaults = dict(constants.DEFAULT_CONFIG) defaults['fips_mode'] = None (o, home) = self.finalize_core(None, **defaults) assert list(o) == sorted(defaults)

Ah, of course! fixed

HonzaCholasta · 2017-02-16T08:01:18Z

ipaserver/install/server/replicainstall.py

@@ -738,6 +775,7 @@ def install_check(installer):
                         ldap_uri=ldapuri)
    remote_api.finalize()
    installer._remote_api = remote_api
+


Why was the "FIPS is not allowed in domain level 0" check removed?

It is still there: https://github.com/freeipa/freeipa/pull/437/files#diff-4fc382c22d987062a45dddeaac8bb461L648

https://github.com/tomaskrizek/freeipa/blob/28c57e48423e77756eb152068db44933cd0540f7/ipaserver/install/server/replicainstall.py#L685

HonzaCholasta · 2017-02-21T12:36:50Z

ipatests/test_ipalib/test_config.py

@@ -562,6 +562,7 @@ def test_finalize_core(self):

        # Test using DEFAULT_CONFIG:
        defaults = dict(constants.DEFAULT_CONFIG)
+        defaults['fips_mode'] = object


object. Huh?

Yes, to skip the value check (few lines below - https://github.com/freeipa/freeipa/pull/437/files/28c57e48423e77756eb152068db44933cd0540f7#diff-c5fcc8900faeb561ad5f2a58fef94c85L568 ). If we set it to None, the test will check whether fips_mode value is None - which is not the desired behavior.

HonzaCholasta · 2017-02-21T12:37:19Z

LGTM.

stlaz · 2017-02-21T12:52:31Z

3 LGTM + tests passing seems like a good enough reason for ACK to me.

MartinBasti · 2017-02-21T14:35:47Z

Needs rebase

Variable fips_mode indicating whether machine is running in FIPS-enabled mode was added to env. https://fedorahosted.org/freeipa/ticket/5695

Add optional key fips_mode to Env object in tests. https://fedorahosted.org/freeipa/ticket/5695

Refactor function to use ScriptError exception and provide docstring.

Abstract creating rpc client into a context manager to allow re-use.

Check status of remote server's FIPS mode and proceed with installation only if it matches the current replica's FIPS mode. https://fedorahosted.org/freeipa/ticket/5695

MartinBasti · 2017-02-21T16:09:24Z

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/3372ad2766c0d182fa88c8bc28cf43477dc4cb3b
https://fedorahosted.org/freeipa/changeset/7292890042677ae40faa44753ebf570db6c19e7c
https://fedorahosted.org/freeipa/changeset/62e884ff7f037a28a15d61cc9fa9c46e5c40cda5
https://fedorahosted.org/freeipa/changeset/397ca71e897b42a23ed4ef294fca367c1542a2aa
https://fedorahosted.org/freeipa/changeset/cf25ea7e300cdada57bd964acb4393cc11ad333e

tkrizek force-pushed the t5695-fips-install-check branch from b1eeace to 1dacf22 Compare February 6, 2017 16:36

MartinBasti suggested changes Feb 6, 2017

View reviewed changes

tkrizek force-pushed the t5695-fips-install-check branch from 1dacf22 to 8b07c3b Compare February 6, 2017 17:27

HonzaCholasta reviewed Feb 7, 2017

View reviewed changes

stlaz reviewed Feb 7, 2017

View reviewed changes

tkrizek force-pushed the t5695-fips-install-check branch from 8b07c3b to b2483d4 Compare February 7, 2017 15:29

stlaz reviewed Feb 8, 2017

View reviewed changes

stlaz requested changes Feb 8, 2017

View reviewed changes

tkrizek force-pushed the t5695-fips-install-check branch from b2483d4 to 88f6d90 Compare February 8, 2017 09:23

stlaz approved these changes Feb 8, 2017

View reviewed changes

HonzaCholasta reviewed Feb 8, 2017

View reviewed changes

tkrizek force-pushed the t5695-fips-install-check branch from 88f6d90 to bf8ee49 Compare February 8, 2017 16:14

HonzaCholasta reviewed Feb 9, 2017

View reviewed changes

tkrizek force-pushed the t5695-fips-install-check branch 3 times, most recently from 3c816db to af915d1 Compare February 15, 2017 14:55

tkrizek force-pushed the t5695-fips-install-check branch 2 times, most recently from 144bbed to 28c57e4 Compare February 15, 2017 15:53

HonzaCholasta reviewed Feb 16, 2017

View reviewed changes

MartinBasti approved these changes Feb 16, 2017

View reviewed changes

stlaz mentioned this pull request Feb 20, 2017

Remove MD5 certificate fingerprints #482

Closed

HonzaCholasta reviewed Feb 21, 2017

View reviewed changes

stlaz added the ack Pull Request approved, can be merged label Feb 21, 2017

Tomas Krizek added 5 commits February 21, 2017 16:48

Add fips_mode variable to env

85da41c

Variable fips_mode indicating whether machine is running in FIPS-enabled mode was added to env. https://fedorahosted.org/freeipa/ticket/5695

test_config: fix tests for env.fips_mode

d34bfe0

Add optional key fips_mode to Env object in tests. https://fedorahosted.org/freeipa/ticket/5695

check_remote_version: update exception and docstring

3c98fa8

Refactor function to use ScriptError exception and provide docstring.

replicainstall: add context manager for rpc client

1acadff

Abstract creating rpc client into a context manager to allow re-use.

FIPS: perform replica installation check

33b0817

Check status of remote server's FIPS mode and proceed with installation only if it matches the current replica's FIPS mode. https://fedorahosted.org/freeipa/ticket/5695

tkrizek force-pushed the t5695-fips-install-check branch from 28c57e4 to 33b0817 Compare February 21, 2017 15:50

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

MartinBasti closed this Feb 21, 2017

tkrizek deleted the t5695-fips-install-check branch September 15, 2017 14:14

		Verify remote server's version is not higher than this server's version

		:param client: RPC client


		:param client: RPC client

		:raises: ``ScriptError`` if the checks fails

FIPS: replica install check #437

FIPS: replica install check #437

Conversation

tkrizek commented Feb 6, 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

MartinBasti commented Feb 6, 2017

tkrizek commented Feb 6, 2017

MartinBasti commented Feb 6, 2017

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 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martbab commented Feb 7, 2017

pvoborni commented Feb 7, 2017

MartinBasti commented Feb 7, 2017

tkrizek commented Feb 7, 2017

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 commented Feb 8, 2017

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

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

HonzaCholasta commented Feb 21, 2017

stlaz commented Feb 21, 2017

MartinBasti commented Feb 21, 2017

MartinBasti commented Feb 21, 2017

tkrizek commented Feb 6, 2017 •

edited by MartinBasti

stlaz Feb 7, 2017 •

edited