Turn on NSSOCSP check in mod_nss conf #729

pvomacka · 2017-04-25T11:11:16Z

Turn on NSSOCSP directive during install/replica install/upgrade.
That check whether the certificate which is used for login is
revoked or not using OSCP.

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

rcritten · 2017-04-25T14:19:35Z

ipaserver/install/httpinstance.py

@@ -156,6 +157,7 @@ def create_instance(self, realm, fqdn, domain_name, pkcs12_info=None,
                  self.set_mod_nss_protocol)
        self.step("setting mod_nss password file", self.__set_mod_nss_passwordfile)
        self.step("enabling mod_nss renegotiate", self.enable_mod_nss_renegotiate)
+        self.step("enabling mod_nss NSSOCSP", self.enable_mod_nss_ocsp)


nit: I think OCSP is preferrable over NSSOCSP which may confuse some people.

Thats true, I'll change that.

rcritten · 2017-04-25T14:28:10Z

ipaserver/install/httpinstance.py

+            aug.set(os.path.join(ocsp_path, 'directive[last()+1]'), 'NSSOCSP')
+
+        aug.set(os.path.join(ocsp_path, 'directive[. = "NSSOCSP"]/arg'), 'on')
+        aug.save()


I pulled this into a separate file and it didn't seem to work in F-25, could be something I did wrong...

I tried it during IPA installation, not alone and it worked.

Ok, I'll chalk it up to a copy-paste problem on my end assuming others do an end-to-end test and confirm all three possible states (No starting NSSOCSP, NSSOCSP off, and NSSOCSP already on.

rcritten · 2017-04-25T14:29:24Z

ipaserver/install/server/upgrade.py

@@ -1375,7 +1375,7 @@ def remove_ds_ra_cert(subject_base):
 def fix_trust_flags():
    root_logger.info('[Fixing trust flags in %s]' % paths.HTTPD_ALIAS_DIR)

-    if sysupgrade.get_upgrade_state('http', 'fix_trust_flags'):
+    if sysupgrade.get_upgrade_state('http', 'fix_trust_flags2'):


I don't understand why you are changing the name of the state rather than adding a new state.

Yes, I could create new state, but is it really needed? My new code fixes trust flags in httpd/alias, thats what this state is for, so I don't see any point in copying (duplicating) it.

Rob is right that you should probably have a new state to do this, this one was supposed to do some changes and you're adding some more on top of that.

Ok, at the beginning I wanted to add it, too, but this solution (not add new state) was recommended me by @HonzaCholasta. So if @HonzaCholasta is ok with a new state, then I will create it.

rcritten · 2017-04-25T14:30:11Z

ipaserver/install/server/upgrade.py

@@ -1389,7 +1389,11 @@ def fix_trust_flags():
    if cert:
        db.trust_root_cert(nickname, 'CT,C,C')

-    sysupgrade.set_upgrade_state('http', 'fix_trust_flags', True)
+    sc_nickname = installutils.get_directive(paths.HTTPD_NSS_CONF,
+                                             "NSSNickname")


Should there be a None check here for paranoia reasons?

I'm not sure whether there can be a situation when NSSNickname is not specified. Is it possible or not?

Not sure but throughout the cert code whenever a cert is searched for in this way a sanity check follows.

rcritten · 2017-04-25T14:30:43Z

ipaserver/install/server/upgrade.py

@@ -1404,6 +1408,11 @@ def update_mod_nss_protocol(http):
    sysupgrade.set_upgrade_state('nss.conf', 'protocol_updated_tls12', True)


+def enable_mod_nss_ocsp(http):
+    root_logger.info('[Updating mod_nss enabling NSSOCSP]')


Similarly, I'd replace NSSOCSP with just OCSP.

flo-renaud

Hi @pvomacka
thank you for the patch. please find my comments inline.

flo-renaud · 2017-04-26T08:06:45Z

install/restart_scripts/restart_httpd

+    api.finalize()
+
+    db = certs.CertDB(api.env.realm, nssdir=paths.HTTPD_ALIAS_DIR)
+    server_certs = db.find_server_certs()


db.find_server_certs() returns all the certs in the DB with the u flag, and it may return more than one nickname, maybe not the one you are expecting. You could use installutils.get_directive(paths.HTTPD_NSS_CONF,'NSSNickname') instead in order to find the nickname of HTTPd server certificate.

Hello @flo-renaud, thank you for review.
Yes, I that will be better to use the installutils method.

flo-renaud · 2017-04-26T08:12:21Z

install/restart_scripts/restart_httpd

+
+    nickname = server_certs[0][0]
+    db.trust_root_cert(nickname, "P,,")
+


Is there a specific reason for changing the flag in this script (during restart of httpd) rather than during upgrade? Is it to make sure that certs configured with ipa-server-certinstall have the proper flag? Then a comment would help future developers.

This is the certmonger restart script. This is probably a belt-and-suspenders change to assure that the peer flag is retained. I agree a comment would be helpful.

Yes, @rcritten is right. And true is that I forgot to add it into commit message. I will add it there.

rcritten · 2017-04-26T21:51:31Z

install/restart_scripts/restart_httpd

+    nickname = installutils.get_directive(paths.HTTPD_NSS_CONF, "NSSNickname")
+
+    db.trust_root_cert(nickname, "P,,")
+


I think a one-line comment here is needed so future devs will know what this is about without having to dig thru git history.

rcritten · 2017-04-26T21:51:41Z

ipaserver/install/server/upgrade.py

+    sc_nickname = installutils.get_directive(paths.HTTPD_NSS_CONF,
+                                             "NSSNickname")
+    db.trust_root_cert(sc_nickname, "P,,")
+


This will ALWAYS fix the trust though. Is that what you want? I think it should be in a separate function, tracked separately, with a similar test for being already executed. I doubt it would cause any harm being run multiple times but it is bad form.

HonzaCholasta · 2017-04-27T07:05:00Z

ipaserver/install/httpinstance.py

+        if ocsp is None:
+            aug.set(os.path.join(ocsp_path, 'directive[last()+1]'), 'NSSOCSP')
+
+        aug.set(os.path.join(ocsp_path, 'directive[. = "NSSOCSP"]/arg'), 'on')


You can shorten this to:

aug.set('/files/etc/httpd/conf.d/nss.conf/VirtualHost/directive[. = "NSSOCSP"]', 'NSSOCSP') aug.set('/files/etc/httpd/conf.d/nss.conf/VirtualHost/directive[. = "NSSOCSP"]/arg', 'on')

Additionally, you can prepend the following to uncomment NSSOCSP:

aug.set('/files/etc/httpd/conf.d/nss.conf/VirtualHost/#comment[. =~ regexp("NSSOCSP .*")]', 'NSSOCSP') aug.rename('/files/etc/httpd/conf.d/nss.conf/VirtualHost/#comment[. = "NSSOCSP"]', 'directive')

HonzaCholasta · 2017-04-28T06:27:53Z

@pvomacka, CI fails because you forgot to include python-augeas in lint BuildRequires.

On a related note, the python-augeas Requires should be in python2-ipaserver and python3-ipaserver, not in freeipa-server.

HonzaCholasta · 2017-04-28T06:36:56Z

ipaserver/install/server/upgrade.py

@@ -1392,6 +1392,23 @@ def fix_trust_flags():
    sysupgrade.set_upgrade_state('http', 'fix_trust_flags', True)


+def fix_server_cert_trust_flags():


Nitpick, but I would rather name this http_trust_server_cert or similar.

HonzaCholasta · 2017-04-28T06:37:50Z

ipaserver/install/httpinstance.py

+        aug = augeas.Augeas()
+        ocsp_path = os.path.join('/files',
+                                 paths.HTTPD_NSS_CONF[1:],
+                                 'VirtualHost')


You should use format strings here as well:

path = '/files{}/VirtualHost'.format(paths.HTTPD_NSS_CONF)

HonzaCholasta · 2017-04-28T06:49:20Z

ipaserver/install/httpinstance.py

+        aug.rename('{path}/#comment[. = "NSSOCSP"]'.format(path=ocsp_path),
+                   'directive')
+        aug.set('{path}/directive[. = "NSSOCSP"]/arg'.format(path=ocsp_path),
+                'on')


Nitpick: You can fit each of these statements on a single line by removing some unnecessary stuff:

aug.set('{}/#comment[.=~regexp("NSSOCSP .*")]'.format(path), 'NSSOCSP') aug.rename('{}/#comment[.="NSSOCSP"]'.format(path), 'directive') aug.set('{}/directive[.="NSSOCSP"]/arg'.format(path), 'on')

HonzaCholasta · 2017-04-28T07:05:48Z

ipaserver/install/httpinstance.py

@@ -263,6 +265,20 @@ def enable_mod_nss_renegotiate(self):
        installutils.set_directive(paths.HTTPD_NSS_CONF, 'NSSRenegotiation', 'on', False)
        installutils.set_directive(paths.HTTPD_NSS_CONF, 'NSSRequireSafeNegotiation', 'on', False)

+    def enable_mod_nss_ocsp(self):
+        aug = augeas.Augeas()


By default, augeas loads all lenses and all configuration files it finds in the system. I guess we don't want it to do that and load only the httpd lens and the /etc/httpd/conf.d/nss.conf file, which can be done using:

aug = Augeas(flags=Augeas.NO_LOAD | Augeas.NO_MODL_AUTOLOAD) aug.set('/augeas/load/Httpd/lens', 'Httpd.lens') aug.set('/augeas/load/Httpd/incl', paths.HTTPD_NSS_CONF) aug.load()

tiran

Why are you using augeas instead of some simple Python code? Introduction of new dependencies should be discussed first.

tiran · 2017-04-28T07:12:59Z

freeipa.spec.in

@@ -306,6 +306,7 @@ Requires: oddjob
 Requires: gssproxy >= 0.7.0-2
 # 1.15.2: FindByNameAndCertificate (https://pagure.io/SSSD/sssd/issue/3050)
 Requires: sssd-dbus >= 1.15.2
+Requires: python-augeas


Do we really want to depend on a new Python library, C library and complex framework just to change a line in a config file? I'm -1. The task can be accomplished with a bit of Python code and regexp.

Yes, it would be possible to do it only with python regexp but it will be much nicer to do it using augeas. And python-augeas is available in RHEL and Fedora so it is not a big issue to add a dependency.

We want to migrate all config file handling to augeas eventually, and don't want introduce any new yet another barely maintainable ad-hoc bits of Python code to update a config file, so now is as good time as any to add the dependency.

flo-renaud · 2017-05-02T12:24:33Z

Hi @pvomacka
I tested your last update with a new install and with an upgraded instance, and both are functionally OK. Revoked certs do not allow to access IPA Web UI.

MartinBasti · 2017-05-02T15:13:13Z

augeas should be dependency of python2-ipaserver and python3-ipaserver (python3-augeas) packages

************* Module ipaserver.install.httpinstance
ipaserver/install/httpinstance.py:32: [E0401(import-error), ] Unable to import 'augeas')

pvomacka · 2017-05-02T15:25:21Z

Hello @flo-renaud, thank you for testing this.
Hello @MartinBasti, thank you for review. I just fixed that.

MartinBasti · 2017-05-02T16:38:46Z

And you also need python[3]-augeas as Pylint BuildDependency to pass pylint :), sorry I forgot about it.

MartinBasti · 2017-05-02T16:41:33Z

And you also need to add it in ipaserver/setup.py as dependency for our PyPI packages

Turn on NSSOCSP directive during install/replica install/upgrade. That check whether the certificate which is used for login is revoked or not using OSCP. Marks the server cert in httpd NSS DB as trusted peer ('P,,') to avoid chicken and egg problem when it is needed to contact the OCSP responder when httpd is starting. https://pagure.io/freeipa/issue/6370

pvomacka · 2017-05-03T08:39:06Z

@MartinBasti thank you for comments, fixed.

tkrizek · 2017-05-10T07:09:44Z

master:

e0b32da Turn on NSSOCSP check in mod_nss conf

ipa-4-5:

4aa7e70 Turn on NSSOCSP check in mod_nss conf

pvomacka requested review from HonzaCholasta and flo-renaud April 25, 2017 11:11

pvomacka force-pushed the t6370 branch from 5c1777f to 641a697 Compare April 25, 2017 13:02

rcritten reviewed Apr 25, 2017

View reviewed changes

flo-renaud reviewed Apr 26, 2017

View reviewed changes

HonzaCholasta self-assigned this Apr 26, 2017

pvomacka force-pushed the t6370 branch from 641a697 to dcd894b Compare April 26, 2017 16:19

rcritten reviewed Apr 26, 2017

View reviewed changes

HonzaCholasta reviewed Apr 27, 2017

View reviewed changes

pvomacka force-pushed the t6370 branch from dcd894b to d2a319e Compare April 27, 2017 13:53

HonzaCholasta reviewed Apr 28, 2017

View reviewed changes

tiran requested changes Apr 28, 2017

View reviewed changes

pvomacka force-pushed the t6370 branch 5 times, most recently from 740da4c to 9156542 Compare May 2, 2017 10:10

pvomacka force-pushed the t6370 branch from 9156542 to 9e8e55b Compare May 2, 2017 15:25

pvomacka force-pushed the t6370 branch from 9e8e55b to cbe2a84 Compare May 3, 2017 08:38

flo-renaud added the ack Pull Request approved, can be merged label May 9, 2017

tkrizek added the pushed Pull Request has already been pushed label May 10, 2017

tkrizek closed this May 10, 2017


		nickname = server_certs[0][0]
		db.trust_root_cert(nickname, "P,,")

		nickname = installutils.get_directive(paths.HTTPD_NSS_CONF, "NSSNickname")

		db.trust_root_cert(nickname, "P,,")

		@@ -1392,6 +1392,23 @@ def fix_trust_flags():
		sysupgrade.set_upgrade_state('http', 'fix_trust_flags', True)


		def fix_server_cert_trust_flags():

Turn on NSSOCSP check in mod_nss conf #729

Turn on NSSOCSP check in mod_nss conf #729

Conversation

pvomacka commented Apr 25, 2017 • edited by tkrizek

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

flo-renaud 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

HonzaCholasta commented Apr 28, 2017 • edited

Choose a reason for hiding this comment

HonzaCholasta Apr 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HonzaCholasta Apr 28, 2017 • edited

Choose a reason for hiding this comment

flo-renaud commented May 2, 2017

MartinBasti commented May 2, 2017

pvomacka commented May 2, 2017

MartinBasti commented May 2, 2017

MartinBasti commented May 2, 2017

pvomacka commented May 3, 2017

tkrizek commented May 10, 2017

pvomacka commented Apr 25, 2017 •

edited by tkrizek

HonzaCholasta commented Apr 28, 2017 •

edited

HonzaCholasta Apr 28, 2017 •

edited

HonzaCholasta Apr 28, 2017 •

edited