On 08/09/2014 01:36 AM, Rob Crittenden wrote:
Ade Lee wrote:
Attached is a new patch. I believe I have addressed all the issues
raided by pviktori, edewata and rcrit.
Arrrrr!
Please let me know if I missed something!
Incidentally, to get all this to work, you should use the latest Dogtag
10.2 build, which also contains a fix for pkidestroy that is not yet
merged in. A COPR build is currently underway at:
http://copr.fedoraproject.org/coprs/vakwetu/dogtag/build/24804/
Some whitespace issues:
Applying: Add a DRM to IPA
/home/rcrit/redhat/freeipa/.git/rebase-apply/patch:3774: trailing
whitespace.
This relies on the DRM client to generate a wrapping key
/home/rcrit/redhat/freeipa/.git/rebase-apply/patch:3292: new blank line
at EOF.
+
warning: 2 lines add whitespace errors.
lying: Add a DRM to IPA
/home/rcrit/redhat/freeipa/.git/rebase-apply/patch:3774: trailing
whitespace.
This relies on the DRM client to generate a wrapping key
/home/rcrit/redhat/freeipa/.git/rebase-apply/patch:3292: new blank line
at EOF.
+
warning: 2 lines add whitespace errors.
I do hope you're planning on adding a minimum build dep at some point?
Still seeing AVCs during install:
----
time->Fri Aug 8 19:13:35 2014
type=SYSCALL msg=audit(1407539615.743:1503): arch=c000003e syscall=1
success=no exit=-13 a0=3 a1=210cb30 a2=2d a3=7fff1caa83f0 items=0
ppid=12121 pid=12307 auid=4294967295 uid=994 gid=993 euid=994 suid=994
fsuid=994 egid=993 sgid=993 fsgid=993 tty=(none) ses=4294967295
comm="cp" exe="/usr/bin/cp" subj=system_u:system_r:pki_tomcat_t:s0
key=(null)
type=AVC msg=audit(1407539615.743:1503): avc: denied { setfscreate }
for pid=12307 comm="cp" scontext=system_u:system_r:pki_tomcat_t:s0
tcontext=system_u:system_r:pki_tomcat_t:s0 tclass=process
----
time->Fri Aug 8 19:13:35 2014
type=SYSCALL msg=audit(1407539615.743:1504): arch=c000003e syscall=190
success=no exit=-13 a0=4 a1=7fff1caa8590 a2=210c8f0 a3=2d items=0
ppid=12121 pid=12307 auid=4294967295 uid=994 gid=993 euid=994 suid=994
fsuid=994 egid=993 sgid=993 fsgid=993 tty=(none) ses=4294967295
comm="cp" exe="/usr/bin/cp" subj=system_u:system_r:pki_tomcat_t:s0
key=(null)
type=AVC msg=audit(1407539615.743:1504): avc: denied { relabelfrom }
for pid=12307 comm="cp" name="CS.cfg.bak.20140808191335" dev="dm-0"
ino=430828 scontext=system_u:system_r:pki_tomcat_t:s0
tcontext=system_u:object_r:pki_tomcat_etc_rw_t:s0 tclass=file
----
time->Fri Aug 8 19:13:35 2014
type=SYSCALL msg=audit(1407539615.744:1505): arch=c000003e syscall=88
success=no exit=-13 a0=7fffd3c0daa7 a1=7fffd3c0daea a2=0 a3=7fffd3c0b9b0
items=0 ppid=12121 pid=12308 auid=4294967295 uid=994 gid=993 euid=994
suid=994 fsuid=994 egid=993 sgid=993 fsgid=993 tty=(none) ses=4294967295
comm="ln" exe="/usr/bin/ln" subj=system_u:system_r:pki_tomcat_t:s0
key=(null)
type=AVC msg=audit(1407539615.744:1505): avc: denied { create } for
pid=12308 comm="ln" name="CS.cfg.bak"
scontext=system_u:system_r:pki_tomcat_t:s0
tcontext=system_u:object_r:pki_tomcat_etc_rw_t:s0 tclass=lnk_file
The new estimated time was dead on :-)
There was a fairly long wait after "Done configuring DRM server
(pki-tomcatd)." and the install was done. I thought we always displayed
text when restarting (e.g. handled by the service wrapper) but I guess
not. It would be nice to know what is going on.
Re-running ipa-drm-install results in a scary error:
]# ipa-drm-install
Usage: ipa-drm-install [options] [replica_file]
ipa-drm-install: error: DRM is already installed.
Your system may be partly configured.
Run /usr/sbin/ipa_drm_install.py --uninstall to clean up.
Right, you don't want to override handle_error here. Instead, wrap the
body of run() in
try:
....
except:
self.log.error(self.FAIL_MESSAGE)
raise
(Yes, bare `except` and bare `raise`)
I used self.log.error() instead of print, because I think at least the
"Your system may be partly configured." part of the FAIL_MESSAGE should
end up in the log, not just on screen.
And now onto the code...
class drm
_create_pem_file isnt' exactly descriptive and there is no method
documentation.
_setup. Just a nit: do you want to hardcode the port? I think I'd prefer
it come via the constructor and default to 443.
It may be worth beefing up the return value docs ala what John did in
the dogtag section. I notice, for example, you always return a tuple and
one value as None in store_secret. I assume there is a reason for that
but it isn't obvious. This happens elsewhere too.
Should the copyright dates on existing files be changed? I don't think
they should be, but I'm hardly an expert.
I just did a cursory look-see in the code and things generally looked
ok. I'm hoping Petr^3 will take a closer look.
rob
I also see a scary error I can't make heads or tails of when trying to
install DRM on a replica:
$ sudo ipa-drm-install
Your system may be partly configured.
Run /usr/sbin/ipa_drm_install.py --uninstall to clean up.
HTTPConnectionPool(host='localhost', port=8080): Max retries exceeded
with url: /ca/rest/securityDomain/domainInfo (Caused by <class
'socket.error'>: [Errno 111] Connection refused)
pviktori@vm-234:~/freeipa{master}e819ca8∗$
The traceback is:
ipa.ipaserver.install.ipa_drm_install.DRMInstaller: DEBUG: File
"/usr/lib/python2.7/site-packages/ipapython/admintool.py", line 168, in
execute
self.validate_options()
File
"/usr/lib/python2.7/site-packages/ipaserver/install/ipa_drm_install.py",
line 180, in validate_options
self.installing_replica = dogtaginstance.is_installing_replica("KRA")
File
"/usr/lib/python2.7/site-packages/ipaserver/install/dogtaginstance.py",
line 82, in is_installing_replica
info = get_security_domain()
File
"/usr/lib/python2.7/site-packages/ipaserver/install/dogtaginstance.py",
line 71, in get_security_domain
info = domain_client.get_security_domain_info()
File "/usr/lib/python2.7/site-packages/pki/system.py", line 95, in
get_security_domain_info
response = self.connection.get('/rest/securityDomain/domainInfo')
File "/usr/lib/python2.7/site-packages/pki/client.py", line 60, in get
data=payload)
File "/usr/lib/python2.7/site-packages/requests/sessions.py", line
347, in get
return self.request('GET', url, **kwargs)
File "/usr/lib/python2.7/site-packages/requests/sessions.py", line
335, in request
resp = self.send(prep, **send_kwargs)
File "/usr/lib/python2.7/site-packages/requests/sessions.py", line
438, in send
r = adapter.send(request, **kwargs)
File "/usr/lib/python2.7/site-packages/requests/adapters.py", line
327, in send
raise ConnectionError(e)
Perhaps I'm doing something wrong here?
Regarding the code I just have a bunch of nitpicks:
There are some more logger calls that use `%` for interpolation:
- ipa-ca-install.install_replica
- ipaserver.install.installutils.check_entropy
There are some more hardcoded paths. We've just moved these to the
platform module recently, and the move wasn't totally thorough, so these
are not entirely your fault. Still, they should be fixed:
ipaserver.install.cainstance.CAInstance.stop_tracking_agent_certificate
(/etc/httpd/alias)
ipaserver.install.dogtaginstance.HTTPD_CONFD (later when you use it, use
os.path.join() instead of adding strings)
ipaserver.install.ipa_replica_prepare.ReplicaPrepare.copy_misc_files
(/etc/ipa/default.conf)
ipaserver.install.cainstance.CAInstance.uninstall:
"-pki_instance_root=/var/lib" (not touched directly by your change but
it would be nice to fix)
ipaserver.install.drminstance.DRMInstance.__spawn_instance:
"/var/lib/pki/pki-tomcat/alias/kra_backup_keys.p12",
"/root/kracert.p12", "/tmp"
ipa-server-install, summary message: "/root/cacert.p12", "/root/drmcert.p12"
ipaserver.install.dogtaginstance.DogtagInstance.ADMIN_CERT_PATH
ipaserver.install.dogtaginstance.check_inst:
'/usr/share/pki/%s/conf/server.xml' (note, constants for template paths
have a _TEMPLATE suffix by convention)
ipaserver.install.dogtaginstance.DogtagInstance.spawn_instance:
"/usr/sbin/pkispawn"
ipaserver.install.dogtaginstance.DogtagInstance.uninstall:
"/usr/sbin/pkidestroy"
ipa_drm_install.DRMInstaller.FAIL_MESSAGE: Just use `ipa-drm-install` as
the command; the Python module you mention is not executable.
ipaserver.install.installutils.check_entropy:
"/proc/sys/kernel/random/entropy_avail"
CAInstance.__init__, DRMInstance.__init__: These docstrings are unnecessary.
ipa_drm_install.DRMInstall.__init__ just calls the superclass, so it's
not necessary to override it.
In ipa_drm_install.DRMInstall.add_options, when adding --uninstall,
don't give the empty string (see --no-host-dns).
For ipa_drm_install.DRMInstaller's INSTALLER_START_MESSAGE and
FAIL_MESSAGE, you can use textwrap.dedent() for nicer indentation:
https://docs.python.org/2/library/textwrap.html#textwrap.dedent
In ipa_drm_install.DRMInstaller.__init__, AFAICS setting
"installing_replica" and "replica_file" is not needed, and the values
are misleading until validate_options sets the the correct values.
In ipa_drm_install.DRMInstaller.validate_options:
Instead of `ra_plugin is not None and ra_plugin == "dogtag"` you can use
just `ra_plugin == "dogtag"`.
Same with `if dogtag_version is not None and dogtag_version >= 10:` (you
explicitly convert the value to int earlier, so it can't be None here).
Instead of `if len(self.args) < 1:` just use `if not self.args:`.
The tool should also error out if more than one replica file is given,
or if file(s) are given for non-replica master or for uninstalling.
The commit message is slightly wrong; ipa-drm-install does not ask for a
replica file when it's needed and not given.
And, please link to the ticket and design page in the commit message.
--
Petr³
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
