Re: [Pki-devel] [PATCH] PKI Deployment Framework (20120716)

Initial Comments: 1. As specified on #irc, please move the spawn or destroy log into /var/log/pki (and also remove the code that cleans this location when the last instance has been removed). This will make the selinux rules cleaner. 2. Does CS.cfg include entries that are no longer needed because they are used by pkicreate? Please file a trac task to remove these post-alpha. Also, please check that the pkicreate* entries are not used elsewhere in the code (and are correct if they are). 3. Catalina.properties contains commented out jars for each of the subsystems in the common.loader. These should be removed. 4. In server.xml: After the line: <!-- DO NOT REMOVE - Begin define PKI secure port there is a line with a single 1 on it (and nothing else) What is that for? 5. tomcat.conf. Tomcat is currently configured to not run under a security manager. Please create a task to re-enable a security manager. 6. We are still creating a default web.xml at common/shared/web.xml. Is this something that is still needed? Can it be removed? 7. pkideployment.cfg contains an "[Optional]" section which contains values which may be changed, but for which default values are provided if needed. The user though has no way of knowing what these defaults are, and so cannot determine if they should override them. Would it make more sense to fill in these optional values with the defaults? What distinguishes there parameters from other parameters in the file? 8. In pkispawn and pkidestroy, why do we get the domain name? So, if the domain name is not set, can we no longer do pkispawn/destroy? Under what conditions will the subprocess throw an exception and exit? 9. pkijython.py -- construct_pki_configuration_data: a) external CA does not require new security domain b) is the subsystem name not configurable? The defaults should also probably be more identifiable - say including host and port. c) clone may not require hierarchy to be set to "root" In general though, I find the structure of this function very confusing. It would be better - or more easily maintainable - to break this into separate functions for each subsystem type. ie. configureRootCA(), configureSubordinateCA(), configureClonedRootCA(), configureClonedSubordinateCA() etc. and have them call functions like: createSigningCertData(), createTrnasportCertData() and the like. It may be more code to do this, but it will be clearer than a set of nested if-then statements. More comments after my lunch .. Ade On Mon, 2012-07-16 at 19:28 -0700, Matthew Harmsen wrote: