<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
thanks comments inline<br>
<br>
<div class="moz-cite-prefix">On 10/07/2015 12:51 PM, Tomas Babej
wrote:<br>
</div>
<blockquote cite="mid:20151007105110.GA27006@redhat.com" type="cite">
<pre wrap="">On Tue, Oct 06, 2015 at 09:58:04PM +0300, Timo Aaltonen wrote:
</pre>
<blockquote type="cite">
<pre wrap="">
Hi
So here's the first batch of quick patches for ticket #5343. They're
only compile-tested so far (so no stupid mistakes I hope), as I don't
have 4.2+ working yet. Wonder how the quotes in the last patch work, but
at least make-lint didn't laugh too hard..
--
t
</pre>
</blockquote>
<pre wrap="">
Hi,
overall this looks good, couple of comments inline.
</pre>
<blockquote type="cite">
<pre wrap="">From 15b30829c53a7e02ddc997c17559d755b751c9d6 Mon Sep 17 00:00:00 2001
From: Timo Aaltonen <a class="moz-txt-link-rfc2396E" href="mailto:tjaalton@debian.org"><tjaalton@debian.org></a>
Date: Tue, 6 Oct 2015 16:02:37 +0300
Subject: [PATCH 1/2] ipaplatform: Add HTTPD_USER to constants
<a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/5343">https://fedorahosted.org/freeipa/ticket/5343</a>
---
ipaplatform/base/constants.py | 1 +
ipaserver/install/cainstance.py | 3 ++-
ipaserver/install/certs.py | 3 ++-
ipaserver/install/httpinstance.py | 11 ++++++-----
ipaserver/install/ipa_server_certinstall.py | 3 ++-
5 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/ipaplatform/base/constants.py b/ipaplatform/base/constants.py
index cef829e2d3886db00ae6d0299ddcf325d1add80e..3f78822f99d9fbe815901301f4e6855105e73eea 100644
--- a/ipaplatform/base/constants.py
+++ b/ipaplatform/base/constants.py
@@ -8,4 +8,5 @@ This base platform module exports platform dependant constants.
class BaseConstantsNamespace(object):
+ HTTPD_USER = "apache"
IPA_DNS_PACKAGE_NAME = "freeipa-server-dns<a class="moz-txt-link-rfc2396E" href="mailto:diff--gita/ipaserver/install/cainstance.pyb/ipaserver/install/cainstance.pyindexc4788816ab702e9409c9bc44a91fcbd95dce018d..6deaef57c025cb55da9fcaf7620a54565f6701c7100644---a/ipaserver/install/cainstance.py+++b/ipaserver/install/cainstance.py@@-48,6+48,7@@fromipalibimportpkcs10,x509fromipalibimporterrorsfromipaplatformimportservices+fromipaplatform.constantsimportconstantsfromipaplatform.pathsimportpathsfromipaplatform.tasksimporttasks@@-1103,7+1104,7@@classCAInstance(DogtagInstance):os.chmod(self.ra_agent_db+">"
diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index c4788816ab702e9409c9bc44a91fcbd95dce018d..6deaef57c025cb55da9fcaf7620a54565f6701c7 100644
--- a/ipaserver/install/cainstance.py
+++ b/ipaserver/install/cainstance.py
@@ -48,6 +48,7 @@ from ipalib import pkcs10, x509
from ipalib import errors
from ipaplatform import services
+from ipaplatform.constants import constants
from ipaplatform.paths import paths
from ipaplatform.tasks import tasks
@@ -1103,7 +1104,7 @@ class CAInstance(DogtagInstance):
os.chmod(self.ra_agent_db + "</a>/key3.db", 0o640)
os.chmod(self.ra_agent_db + "/secmod.db", 0o640)
- pent = pwd.getpwnam("apache")
+ pent = pwd.getpwnam(constants.HTTPD_USER)
os.chown(self.ra_agent_db + "/cert8.db", 0, pent.pw_gid )
os.chown(self.ra_agent_db + "/key3.db", 0, pent.pw_gid )
os.chown(self.ra_agent_db + "/secmod.db", 0, pent.pw_gid )
diff --git a/ipaserver/install/certs.py b/ipaserver/install/certs.py
index 3e07ee398fa47beb02f54940a0246d58ae2267ae..d85344ede993840845af63c377525699425a9382 100644
--- a/ipaserver/install/certs.py
+++ b/ipaserver/install/certs.py
@@ -42,6 +42,7 @@ from ipalib import pkcs10, x509, api
from ipalib.errors import CertificateOperationError
from ipalib.text import _
from ipaplatform import services
+from ipaplatform.constants import constants
from ipaplatform.paths import paths
# Apache needs access to this database so we need to create it
@@ -519,7 +520,7 @@ class CertDB(object):
f.close()
pwdfile.close()
# TODO: replace explicit uid by a platform-specific one</pre>
</blockquote>
</blockquote>
This TODO can be removed with this patch<br>
<blockquote cite="mid:20151007105110.GA27006@redhat.com" type="cite">
<blockquote type="cite">
<pre wrap="">
- self.set_perms(self.pwd_conf, uid="apache")
+ self.set_perms(self.pwd_conf, uid=constants.HTTPD_USER)
def find_root_cert(self, nickname):
""<a class="moz-txt-link-rfc2396E" href="mailto:diff--gita/ipaserver/install/httpinstance.pyb/ipaserver/install/httpinstance.pyindexee4853a3f9a8a42bd050fd8b208fc2419c323512..a7fdfb1a21a8c62f57503cfaca68b30e4f26244f100644---a/ipaserver/install/httpinstance.py+++b/ipaserver/install/httpinstance.py@@-41,6+41,7@@importipapython.errorsfromipaserver.installimportsysupgradefromipalibimportapifromipalibimporterrors+fromipaplatform.constantsimportconstantsfromipaplatform.tasksimporttasksfromipaplatform.pathsimportpathsfromipaplatformimportservices@@-52,7+53,7@@SELINUX_BOOLEAN_SETTINGS=dict()KDCPROXY_USER='kdcproxy'-+HTTPD_USER=constants.HTTPD_USERdefhttpd_443_configured():">"
diff --git a/ipaserver/install/httpinstance.py b/ipaserver/install/httpinstance.py
index ee4853a3f9a8a42bd050fd8b208fc2419c323512..a7fdfb1a21a8c62f57503cfaca68b30e4f26244f 100644
--- a/ipaserver/install/httpinstance.py
+++ b/ipaserver/install/httpinstance.py
@@ -41,6 +41,7 @@ import ipapython.errors
from ipaserver.install import sysupgrade
from ipalib import api
from ipalib import errors
+from ipaplatform.constants import constants
from ipaplatform.tasks import tasks
from ipaplatform.paths import paths
from ipaplatform import services
@@ -52,7 +53,7 @@ SELINUX_BOOLEAN_SETTINGS = dict(
)
KDCPROXY_USER = 'kdcproxy'
-
+HTTPD_USER = constants.HTTPD_USER
def httpd_443_configured():
"</a>"<a class="moz-txt-link-rfc2396E" href="mailto:@@-188,14+189,14@@classHTTPInstance(service.Service):self.move_service(self.principal)self.add_cert_to_service()-pent=pwd.getpwnam(">"
@@ -188,14 +189,14 @@ class HTTPInstance(service.Service):
self.move_service(self.principal)
self.add_cert_to_service()
- pent = pwd.getpwnam("</a>apache")
+ pent = pwd.getpwnam(HTTPD_USER)
os.chown(paths.IPA_KEYTAB, pent.pw_uid, pent.pw_gid)
def remove_httpd_ccache(self):
# Clean up existing ccache
# Make sure that empty env is passed to avoid passing KRB5CCNAME from
# current env
- ipautil.run(['kdestroy', '-A'], runas='apache', raiseonerr=False, env={})
+ ipautil.run(['kdestroy', '-A'], runas=HTTPD_USER, raiseonerr=False, env={})
def __configure_http(self):
target_fname = paths.HTTPD_IPA_CONF
@@ -324,7 +325,7 @@ class HTTPInstance(service.Service):
os.chmod(certs.NSS_DIR + "/secmod.db", 0o660)
os.chmod(certs.NSS_DIR + "/pwdfile.txt", 0o660)
- pent = pwd.getpwnam("apache")
+ pent = pwd.getpwnam(HTTPD_USER)
os.chown(certs.NSS_DIR + "/cert8.db", 0, pent.pw_gid )
os.chown(certs.NSS_DIR + "/key3.db", 0, pent.pw_gid )
os.chown(certs.NSS_DIR + "/secmod.db", 0, pent.pw_gid )
@@ -493,7 +494,7 @@ class HTTPInstance(service.Service):
pass
# Remove the ccache file for the HTTPD service
- ipautil.run([paths.KDESTROY, '-c', paths.KRB5CC_HTTPD], runas='apache',
+ ipautil.run([paths.KDESTROY, '-c', paths.KRB5CC_HTTPD], runas=HTTPD_USER,
raiseonerr=False)
# Remove the configuration files we create
diff --git a/ipaserver/install/ipa_server_certinstall.py b/ipaserver/install/ipa_server_certinstall.py
index e90b2abd6644c71bc3b567af5ac74c8368df1b15..ac0b0274e4e36db4ea6fb695afb527e2b83a8c77 100644
--- a/ipaserver/install/ipa_server_certinstall.py
+++ b/ipaserver/install/ipa_server_certinstall.py
@@ -24,6 +24,7 @@ import os.path
import pwd
import optparse
+from ipaplatform.constants import constants
from ipaplatform.paths import paths
from ipapython import admintool
from ipapython.dn import DN
@@ -151,7 +152,7 @@ class ServerCertInstall(admintool.AdminTool):
os.chmod(os.path.join(dirname, 'key3.db'), 0o640)
os.chmod(os.path.join(dirname, 'secmod.db'), 0o640)
- pent = pwd.getpwnam("apache")
+ pent = pwd.getpwnam(constants.HTTPD_USER)
os.chown(os.path.join(dirname, 'cert8.db'), 0, pent.pw_gid)
os.chown(os.path.join(dirname, 'key3.db'), 0, pent.pw_gid)
os.chown(os.path.join(dirname, 'secmod.db'), 0, pent.pw_gid)
--
2.5.0
</pre>
</blockquote>
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap="">From 77be9a8b67a49ca263e82dde5bf87d432ca64922 Mon Sep 17 00:00:00 2001
From: Timo Aaltonen <a class="moz-txt-link-rfc2396E" href="mailto:tjaalton@debian.org"><tjaalton@debian.org></a>
Date: Tue, 6 Oct 2015 16:27:21 +0300
Subject: [PATCH 2/2] ipaplatform: Add NAMED_USER to constants
<a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/5343">https://fedorahosted.org/freeipa/ticket/5343</a>
---
ipaplatform/base/constants.py | 1 +
ipaserver/install/bindinstance.py | 3 ++-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/ipaplatform/base/constants.py b/ipaplatform/base/constants.py
index 3f78822f99d9fbe815901301f4e6855105e73eea..9a1237106d47b93c6cbe50b139b92cbcc0a745ff 100644
--- a/ipaplatform/base/constants.py
+++ b/ipaplatform/base/constants.py
@@ -10,3 +10,4 @@ This base platform module exports platform dependant constants.
class BaseConstantsNamespace(object):
HTTPD_USER = "apache"
IPA_DNS_PACKAGE_NAME = "freeipa-server-dns"
+ NAMED_USER = "named<a class="moz-txt-link-rfc2396E" href="mailto:diff--gita/ipaserver/install/bindinstance.pyb/ipaserver/install/bindinstance.pyindexe8fdb3b83317f996959e4123b481f353c2f056c9..2cbf30202f30bd80c01a6399ecff3a6406316825100644---a/ipaserver/install/bindinstance.py+++b/ipaserver/install/bindinstance.py@@-39,6+39,7@@fromipapython.dnimportDNimportipalibfromipalibimportapi,errorsfromipaplatformimportservices+fromipaplatform.constantsimportconstantsfromipaplatform.pathsimportpathsfromipaplatform.tasksimporttasksfromipalib.utilimport(validate_zonemgr_str,normalize_zonemgr,@@-561,7+562,7@@classBindInstance(service.Service):suffix=ipautil.dn_attribute_property('_suffix')defsetup(self,fqdn,ip_addresses,realm_name,domain_name,forwarders,ntp,-reverse_zones,named_user=">"
diff --git a/ipaserver/install/bindinstance.py b/ipaserver/install/bindinstance.py
index e8fdb3b83317f996959e4123b481f353c2f056c9..2cbf30202f30bd80c01a6399ecff3a6406316825 100644
--- a/ipaserver/install/bindinstance.py
+++ b/ipaserver/install/bindinstance.py
@@ -39,6 +39,7 @@ from ipapython.dn import DN
import ipalib
from ipalib import api, errors
from ipaplatform import services
+from ipaplatform.constants import constants
from ipaplatform.paths import paths
from ipaplatform.tasks import tasks
from ipalib.util import (validate_zonemgr_str, normalize_zonemgr,
@@ -561,7 +562,7 @@ class BindInstance(service.Service):
suffix = ipautil.dn_attribute_property('_suffix')
def setup(self, fqdn, ip_addresses, realm_name, domain_name, forwarders, ntp,
- reverse_zones, named_user="</a>named", zonemgr=None,
+ reverse_zones, named_user=constants.NAMED_USER, zonemgr=None,
ca_configured=None, no_dnssec_validation=False):
self.named_user = named_user
self.fqdn = fqdn
--
2.5.0
</pre>
</blockquote>
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap="">From 52945c313e975aa3371bb3275b4ff42707e13e89 Mon Sep 17 00:00:00 2001
From: Timo Aaltonen <a class="moz-txt-link-rfc2396E" href="mailto:tjaalton@debian.org"><tjaalton@debian.org></a>
Date: Tue, 6 Oct 2015 16:43:09 +0300
Subject: [PATCH] httpinstance: Use full path via HTTPD_CONF_D for Include.
<a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/5343">https://fedorahosted.org/freeipa/ticket/5343</a>
---
ipaserver/install/httpinstance.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ipaserver/install/httpinstance.py b/ipaserver/install/httpinstance.py
index a7fdfb1a21a8c62f57503cfaca68b30e4f26244f..16139ef34d846ad8dd4780745f647b9ad5aad772 100644
--- a/ipaserver/install/httpinstance.py
+++ b/ipaserver/install/httpinstance.py
@@ -249,7 +249,7 @@ class HTTPInstance(service.Service):
def __add_include(self):
"""This should run after __set_mod_nss_port so is already backed up"""
- if installutils.update_file(paths.HTTPD_NSS_CONF, '</VirtualHost>', 'Include conf.d/ipa-rewrite.conf\n</VirtualHost>') != 0:
+ if installutils.update_file(paths.HTTPD_NSS_CONF, '</VirtualHost>', 'Include ' + paths.HTTPD_CONF_D + '/ipa-rewrite.conf\n</VirtualHost>') != 0:
</pre>
</blockquote>
<pre wrap="">
Please use os.path.join here, so that we avoid reliance on the
particular format of paths.HTTPD_CONF_D (without trailing slash in this
case)</pre>
</blockquote>
yes, please use os.path.join(), and also there is too much pluses,
please use python format string 'include
{path}\n</VirtualHost>'.format(path=os.join(paths.HTTPD_CONF_D,
'ipa-rewrite-conf')) (not tested)<br>
<br>
or you can use <span style="background-color:#ffe4ff;">HTTPD_IPA_REWRITE_CONF
from paths</span>
<meta http-equiv="content-type" content="text/html;
charset=windows-1252">
<blockquote cite="mid:20151007105110.GA27006@redhat.com" type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap=""> print("Adding Include conf.d/ipa-rewrite to %s failed." % paths.HTTPD_NSS_CONF)
def configure_certmonger_renewal_guard(self):
--
2.5.0
</pre>
</blockquote>
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap="">From 1ca29f9e6188487862d77ea1458e6ff84b371103 Mon Sep 17 00:00:00 2001
From: Timo Aaltonen <a class="moz-txt-link-rfc2396E" href="mailto:tjaalton@debian.org"><tjaalton@debian.org></a>
Date: Tue, 6 Oct 2015 16:35:24 +0300
Subject: [PATCH] ipaplatform: Add SECURE_NFS_VAR to constants
<a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/5343">https://fedorahosted.org/freeipa/ticket/5343</a>
---
ipa-client/ipa-install/ipa-client-automount | 3 ++-
ipaplatform/base/constants.py | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/ipa-client/ipa-install/ipa-client-automount b/ipa-client/ipa-install/ipa-client-automount
index 5e4ab1396aeb6311be1ace8f5c74ce9760fee408..ab7fe3b62b40376d03d87fdef103eedc7aa50cdf 100755
--- a/ipa-client/ipa-install/ipa-client-automount
+++ b/ipa-client/ipa-install/ipa-client-automount
@@ -40,6 +40,7 @@ from ipaclient import ipadiscovery
from ipaclient import ipachangeconf
from ipapython.ipa_log_manager import *
from ipapython.dn import DN
+from ipaplatform.constants import constants
from ipaplatform.tasks import tasks
from ipaplatform import services
from ipaplatform.paths import paths
@@ -309,7 +310,7 @@ def configure_nfs(fstore, statestore):
Configure secure NFS
"""
replacevars = {
- 'SECURE_NFS': 'yes',
+ constants.SECURE_NFS_VAR: 'yes',
}
ipautil.backup_config_and_replace_variables(fstore,
paths.SYSCONFIG_NFS, replacevars=replacevars)
diff --git a/ipaplatform/base/constants.py b/ipaplatform/base/constants.py
index 9a1237106d47b93c6cbe50b139b92cbcc0a745ff..191d3de2c9bf8c6d1a9e39366a5bf9142b8c139f 100644
--- a/ipaplatform/base/constants.py
+++ b/ipaplatform/base/constants.py
@@ -11,3 +11,4 @@ class BaseConstantsNamespace(object):
HTTPD_USER = "apache"
IPA_DNS_PACKAGE_NAME = "freeipa-server-dns"
NAMED_USER = "named"
+ SECURE_NFS_VAR = "SECURE_NFS"
</pre>
</blockquote>
<pre wrap="">
Can we add a helpful comment what a constant describes here? I think we
should start this convention for any non-obvious platform constants, so
that any new platform maintainer has an easy job figuring out what
platform constant actually holds.
</pre>
<blockquote type="cite">
<pre wrap="">--
2.5.0
</pre>
</blockquote>
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap="">From 83a6ddec954a07f78be330bdaa71b53d01d0e1c0 Mon Sep 17 00:00:00 2001
From: Timo Aaltonen <a class="moz-txt-link-rfc2396E" href="mailto:tjaalton@debian.org"><tjaalton@debian.org></a>
Date: Tue, 6 Oct 2015 18:46:00 +0300
Subject: [PATCH] ipaplatform: Add NTP_OPTS_VAR and NTP_OPTS_QUOTE to constants
<a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/5343">https://fedorahosted.org/freeipa/ticket/5343</a>
---
ipaplatform/base/constants.py | 2 ++
ipaserver/install/ntpinstance.py | 14 +++++++++-----
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/ipaplatform/base/constants.py b/ipaplatform/base/constants.py
index 191d3de2c9bf8c6d1a9e39366a5bf9142b8c139f..aafc7b412cc0fc913a332417ae12b6caad619330 100644
--- a/ipaplatform/base/constants.py
+++ b/ipaplatform/base/constants.py
@@ -11,4 +11,6 @@ class BaseConstantsNamespace(object):
HTTPD_USER = "apache"
IPA_DNS_PACKAGE_NAME = "freeipa-server-dns"
NAMED_USER = "named"
+ NTP_OPTS_VAR = "OPTIONS"
+ NTP_OPTS_QUOTE = "\""
</pre>
</blockquote>
<pre wrap="">
Probably a comment is needed here as well.
</pre>
<blockquote type="cite">
<pre wrap=""> SECURE_NFS_VAR = "SECURE_NFS"
diff --git a/ipaserver/install/ntpinstance.py b/ipaserver/install/ntpinstance.py
index 1fef6fd3e8931615b201ce25beaac8bb6c945a01..567dec6e97588792c5331a5dc425cc8220930f82 100644
--- a/ipaserver/install/ntpinstance.py
+++ b/ipaserver/install/ntpinstance.py
@@ -21,9 +21,13 @@
from ipaserver.install import service
from ipapython import sysrestore
from ipapython import ipautil
+from ipaplatform.constants import constants
from ipaplatform.paths import paths
from ipapython.ipa_log_manager import *
+NTPD_OPTS_VAR = constants.NTPD_OPTS_VAR
+NTPD_OPTS_QUOTE = constants.NTPD_OPTS_QUOTE
+
class NTPInstance(service.Service):
def __init__(self, fstore=None):
service.Service.__init__(self, "ntpd", service_desc="NTP daemon<a class="moz-txt-link-rfc2396E" href="mailto:)@@-106,9+110,9@@classNTPInstance(service.Service):fd.close()forlineinlines:sline=line.strip()-ifnotsline.startswith('OPTIONS'):+ifnotsline.startswith(NTPD_OPTS_VAR):continue-sline=sline.replace('">")
@@ -106,9 +110,9 @@ class NTPInstance(service.Service):
fd.close()
for line in lines:
sline = line.strip()
- if not sline.startswith('OPTIONS'):
+ if not sline.startswith(NTPD_OPTS_VAR):
continue
- sline = sline.replace('"</a>', '')
+ sline = sline.replace(NTPD_OPTS_QUOTE, '')
for opt in needopts:
if sline.find(opt['val']) != -1:
opt['need'] = False
@@ -124,12 +128,12 @@ class NTPInstance(service.Service):
for line in lines:
if not done:
sline = line.strip()
- if not sline.startswith('OPTIONS'):
+ if not sline.startswith(NTPD_OPTS_VAR):
fd.write(line)
continue
- sline = sline.replace('"', '')
+ sline = sline.replace(NTPD_OPTS_QUOTE, '')
(variable, opts) = sline.split('=', 1)
- fd.write('OPTIONS="%s %s"\n' % (opts, ' '.join(newopts)))
+ fd.write(NTPD_OPTS_VAR + '="%s %s"\n' % (opts, ' '.join(newopts)))
done = True
else:
fd.write(line)
--
2.5.0
</pre>
</blockquote>
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap="">--
Manage your subscription for the Freeipa-devel mailing list:
<a class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/freeipa-devel">https://www.redhat.com/mailman/listinfo/freeipa-devel</a>
Contribute to FreeIPA: <a class="moz-txt-link-freetext" href="http://www.freeipa.org/page/Contribute/Code">http://www.freeipa.org/page/Contribute/Code</a>
</pre>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
<br>
</body>
</html>