Require an ipa-ca SAN on 3rd party certs if ACME is enabled #5119

rcritten · 2020-09-18T15:17:25Z

Require an ipa-ca SAN on 3rd party certs if ACME is enabled

ACME requires an ipa-ca SAN to have a fixed URL to connect to.
If the Apache certificate is replaced by a 3rd party cert then
it must provide this SAN otherwise it will break ACME.

Add a status option to ipa-acme-manage.

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

Marking as ipa-next since I'm sure yet if ACME is going to be backported to ipa-4-8.

mrizwan93

Hello @rcritten ,
Please find comments inline.

mrizwan93 · 2020-09-21T06:18:46Z

ipatests/test_integration/test_acme.py

@@ -93,6 +131,9 @@ def test_acme_service_not_yet_enabled(self):
            ['curl', '--fail', self.acme_server],
            ok_returncode=22,
        )
+        result = self.master.run_command(['ipa-acme-manage', 'status'])
+        assert result.returncode == 0


This check can be removed., the return code for status command will be 0 anyways.

Yes, I suppose any exception is sufficient in case something goes wrong.

mrizwan93 · 2020-09-21T06:20:59Z

ipatests/test_integration/test_acme.py

@@ -111,6 +152,10 @@ def test_enable_acme_service(self):
        else:
            raise exc

+        result = self.master.run_command(['ipa-acme-manage', 'status'])
+        assert result.returncode == 0


This check can be removed., the return code for status command will be 0 anyways.

mrizwan93 · 2020-09-21T06:21:29Z

ipatests/test_integration/test_acme.py

@@ -252,3 +298,55 @@ def test_disable_acme_service(self):
            ['curl', '--fail', self.acme_server],
            ok_returncode=22,
        )
+        result = self.master.run_command(['ipa-acme-manage', 'status'])
+        assert result.returncode == 0


This check can be removed., the return code for status command will be 0 anyways.

mrizwan93 · 2020-09-21T06:31:08Z

ipatests/test_integration/test_acme.py

+        # Enable ACME which should fail since the Apache cert lacks the SAN
+        result = self.master.run_command(['ipa-acme-manage', 'enable'],
+                                         raiseonerr=False)
+        assert result.returncode == 1


Shall we make ipa-acme-manage enable command more robust in this case? cause return code 1 implies "CA is not installed on this server."

I'll look into a new return value for this and ipa-server-certinstall for invalid certificates.

I think I'm going to adjust the return values in ipa-acme-manage 0 is success, 1 is an error, and 2 and 3 replace the current 1/2. ipa-server-certinstall only returns 0 == ok, 1 == error. Since ACME hasn't shipped in an official release IMHO it's fine to change this.

I'm fine with that change also.

mrizwan93 · 2020-09-22T09:10:38Z

ipatests/test_integration/test_acme.py

+        """Require ipa-ca SAN on replacement web certificates"""
+
+        result = self.master.run_command(['ipa-acme-manage', 'enable'])
+        assert result.returncode == 0


This can be removed as raiseonerr=True is set by default.

mrizwan93 · 2020-09-22T09:11:15Z

ipatests/test_integration/test_acme.py

+        assert result.returncode == 1
+
+        result = self.master.run_command(['ipa-acme-manage', 'disable'])
+        assert result.returncode == 0


This can be removed as raiseonerr=True is set by default.

mrizwan93

Just a nitpick, else LGTM.

mrizwan93 · 2020-09-23T09:30:16Z

ipatests/test_integration/test_acme.py

+    def test_third_party_certs(self):
+        """Require ipa-ca SAN on replacement web certificates"""
+
+        result = self.master.run_command(['ipa-acme-manage', 'enable'])


variable result is not used.

mrizwan93 · 2020-09-23T09:30:43Z

ipatests/test_integration/test_acme.py

+        result = self.certinstall()
+        assert result.returncode == 1
+
+        result = self.master.run_command(['ipa-acme-manage', 'disable'])


variable result is not used.

rcritten · 2020-09-28T12:31:09Z

@frasertweedale do you have any opionions?

rcritten · 2020-10-05T14:58:46Z

Marking this as WIP as the status option will be dependent upon #5170

frasertweedale

Haven't tested but the approach seems good to me.

ipaserver/install/cainstance.py

frasertweedale · 2020-10-06T11:40:21Z

ipatests/test_integration/test_acme.py

+        # Enable ACME which should fail since the Apache cert lacks the SAN
+        result = self.master.run_command(['ipa-acme-manage', 'enable'],
+                                         raiseonerr=False)
+        assert result.returncode == 1


I'm fine with that change also.

rcritten · 2020-10-26T19:04:03Z

Rebased on top of PR #5170 so we can potentially merge this at or near the same time.

ACME requires an ipa-ca SAN to have a fixed URL to connect to. If the Apache certificate is replaced by a 3rd party cert then it must provide this SAN otherwise it will break ACME. https://pagure.io/freeipa/issue/8498 Signed-off-by: Rob Crittenden <rcritten@redhat.com>

Traditionally in IPA 0 = success, 1 = error and then specific error messages follow from that. Shift the ipa-acme-manage return codes for "not installed" and "not a CA" up by one. https://pagure.io/freeipa/issue/8498 Signed-off-by: Rob Crittenden <rcritten@redhat.com>

Test that: 1. With ACME enabled, SAN is required 2. With ACME disabled, SAN is not required Also verify the ipa-acme-manage status command. https://pagure.io/freeipa/issue/8498 Signed-off-by: Rob Crittenden <rcritten@redhat.com>

flo-renaud · 2020-11-02T18:49:42Z

ACK'ing based on @frasertweedale and @mrizwan93 comments

rcritten · 2020-11-02T19:01:50Z

master:

2768b0d Require an ipa-ca SAN on 3rd party certs if ACME is enabled
e0ff82c Change the return codes of ipa-acme-manage
c8f13cd ipatests: Add tests for requiring ipa-ca SAN when ACME is enabled

rcritten added the ipa-next Mark as master (4.12) only label Sep 18, 2020

rcritten force-pushed the issue_8498 branch 4 times, most recently from d3b68b2 to 837fef2 Compare September 18, 2020 20:12

mrizwan93 reviewed Sep 21, 2020

View reviewed changes

rcritten force-pushed the issue_8498 branch from 837fef2 to 351f771 Compare September 21, 2020 14:37

mrizwan93 reviewed Sep 22, 2020

View reviewed changes

rcritten force-pushed the issue_8498 branch from 351f771 to b446090 Compare September 22, 2020 12:32

rcritten added the re-run Trigger a new run of PR-CI label Sep 22, 2020

freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Sep 22, 2020

mrizwan93 reviewed Sep 23, 2020

View reviewed changes

rcritten force-pushed the issue_8498 branch from b446090 to a50c10d Compare September 23, 2020 15:46

rcritten added needs review Pull Request is waiting for a review WIP Work in progress - not ready yet for review and removed needs review Pull Request is waiting for a review labels Sep 28, 2020

frasertweedale reviewed Oct 6, 2020

View reviewed changes

rcritten force-pushed the issue_8498 branch from a50c10d to 943856c Compare October 6, 2020 13:53

mrizwan93 mentioned this pull request Oct 13, 2020

Centralize enable/disable of the ACME service #5170

Closed

rcritten added needs review Pull Request is waiting for a review and removed WIP Work in progress - not ready yet for review labels Oct 13, 2020

rcritten force-pushed the issue_8498 branch from 943856c to 6f99117 Compare October 26, 2020 19:03

rcritten force-pushed the issue_8498 branch 3 times, most recently from ba6235b to ddb3520 Compare October 27, 2020 12:30

mrizwan93 mentioned this pull request Oct 27, 2020

External-CA scenarios for ACME service #5147

Closed

freeipa-pr-ci added the needs rebase Pull Request cannot be automatically merged - needs to be rebased label Nov 2, 2020

rcritten added 3 commits November 2, 2020 10:46

rcritten force-pushed the issue_8498 branch from ddb3520 to 1a2ac0c Compare November 2, 2020 15:49

rcritten added prioritized Pull Request has higher priority for PR-CI and removed needs rebase Pull Request cannot be automatically merged - needs to be rebased needs review Pull Request is waiting for a review labels Nov 2, 2020

flo-renaud added the ack Pull Request approved, can be merged label Nov 2, 2020

rcritten added the pushed Pull Request has already been pushed label Nov 2, 2020

rcritten closed this Nov 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require an ipa-ca SAN on 3rd party certs if ACME is enabled #5119

Require an ipa-ca SAN on 3rd party certs if ACME is enabled #5119

rcritten commented Sep 18, 2020

mrizwan93 left a comment

mrizwan93 Sep 21, 2020

rcritten Sep 21, 2020

mrizwan93 Sep 21, 2020

rcritten Sep 22, 2020

mrizwan93 Sep 21, 2020

rcritten Sep 22, 2020

mrizwan93 Sep 21, 2020

rcritten Sep 21, 2020

rcritten Sep 21, 2020

frasertweedale Oct 6, 2020

mrizwan93 Sep 22, 2020

mrizwan93 Sep 22, 2020

mrizwan93 left a comment

mrizwan93 Sep 23, 2020

rcritten Sep 23, 2020

mrizwan93 Sep 23, 2020

rcritten commented Sep 28, 2020

rcritten commented Oct 5, 2020

frasertweedale left a comment

frasertweedale Oct 6, 2020

rcritten commented Oct 26, 2020

flo-renaud commented Nov 2, 2020

rcritten commented Nov 2, 2020

Require an ipa-ca SAN on 3rd party certs if ACME is enabled #5119

Require an ipa-ca SAN on 3rd party certs if ACME is enabled #5119

Conversation

rcritten commented Sep 18, 2020

mrizwan93 left a 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

mrizwan93 left a 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

rcritten commented Sep 28, 2020

rcritten commented Oct 5, 2020

frasertweedale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcritten commented Oct 26, 2020

flo-renaud commented Nov 2, 2020

rcritten commented Nov 2, 2020