On 07/18/12 09:13, Ade Lee wrote:
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.
https://fedorahosted.org/pki/ticket/225 Dogtag 10: Move "pkispawn"/"pkidestroy" logs 

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).
https://fedorahosted.org/pki/ticket/222 Dogtag 10: Scrub 'pkicreate'/'pkidestroy' entries from CS.cfg 


3. Catalina.properties contains commented out jars for each of the
subsystems in the common.loader.  These should be removed.
It was determined that we could safely remove these commented areas.
This will be addressed in the current patch prior to being re-submitted.
 

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?
It was determined that we could safely remove this line.
This will be addressed in the current patch prior to being re-submitted.

5. tomcat.conf.  Tomcat is currently configured to not run under a
security manager.  Please create a task to re-enable a security manager.
https://fedorahosted.org/pki/ticket/223 Dogtag 10: Re-enable Security Manager in 'tomcat.conf' for Tomcat 7 

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?
https://fedorahosted.org/pki/ticket/230 Dogtag 10: Investigate the need for a 'common/shared/web.xml' file


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?
https://fedorahosted.org/pki/ticket/227 Dogtag 10: Document 'pkideployment.cfg'

Additionally, it was determined that we would move all parameters from the [Mandatory] and [Optional]
sections of this file to other more appropriate sections (e.g. - [Common], [CA], [KRA], etc.),
and these sections and all of their associated logic would be removed from the 'pki-deploy' package.

This will be addressed in the current patch prior to being re-submitted. 


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?
It was determined that prior to exiting, we would issue a log message which warns the user
that a machine must contain a valid domain in order to effectively utilize PKI services.

This will be addressed in the current patch prior to being re-submitted. 

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.
https://fedorahosted.org/pki/ticket/231 Dogtag 10: Update PKI Deployment to handle external CA
https://fedorahosted.org/pki/ticket/224 Dogtag 10: Modularize 'construct_pki_configuration_data' method in the 'rest_client' class in 'pkijython.py'

Additionally, items 9.a) and 9.b) will be addressed in the current patch prior to being re-submitted.

Further, it has been suggested that TRAC ticket #224 be addressed PRIOR to addressing 9.c. above.
 

More comments after my lunch ..
Ade
The following additional TRAC tickets came out of these discussions:
 

 
On Mon, 2012-07-16 at 19:28 -0700, Matthew Harmsen wrote:

This patch documents continued implementation of the PKI Deployment
Framework based upon the revised filesystem layout documented here:
      * http://pki.fedoraproject.org/wiki/PKI_Instance_Deployment#CA_.2F_KRA_.2F_OCSP_.2F_RA_.2F_TKS_.2F_TPS
The following patch adds/corrects functionality of the existing PKI
Deployment Framework including (but not limited to):
      * Integration of Tomcat 7
      * Introduction of dependency upon tomcatjss 7.0
      * Removal of http filtering configuration mechanisms
      * Introduction of additional slot substitution to support
        revised filesystem layout
      * Addition of 'pkiuser' uid:gid creation methods
      * Inclusion of per instance '*.profile' files
      * Introduction of configurable 'configurationRoot' parameter
      * Introduction of default configuration of 'log4j' mechanism
        (alee)
      * Modify web.xml to use new Application classes to bootstrap
        servers (alee)
      * Introduction of "Wrapper" logic to support Tomcat 6 --> Tomcat
        7 API change (jmagne)
      * Added jython helper function to allow attaching a remote java
        debugger (e. g. - eclipse)
Additionally, this patch has been re-based against the current
'master' and has been successfully executed to completion with regards
to installing a CA, enrolling for a certificate, and accepting a
certificate on a 64-bit Fedora 17 installation.

-- Matt


_______________________________________________
Pki-devel mailing list
Pki-devel@redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel