Port to tomcat 6
Created attachment 446380 [details] fix password & CryptoManger initialization These are a couple of simple issues which I discovered during the tomcat porting. Since they are mostly independent of the tomcat changes I'd like to keep them separate and get them in first. The changes are: 1) Make CMSEngine.getPasswordStore() a synchronized method. During testing I had discovered two threads were both calling this method at essentially the same time trashing the state which lead to password failures. 2) Remove redundant duplicated code for initializing the password store, now CMSEngine.getPasswordStore() is the sole owner of the logic to perform this action. 3) Initialize CryptoManager before first use. We had been relying on a side effect in tomcat 5 for CryptoManager initialization where tomcatjss was doing the CryptoManager initialization prior to our first use of the CryptoManager. Tomcat 6 has modified when the connection objects first get created (which was what was kicking off the CryptoManager initialization). The patch adds the same code for initializing the CryptoManger as is in tomcatjss. We now check for CryptoManager initialization *prior* to our first use of it and if and only if it hasn't been initialized yet we do so. If someone could give a quick ack to these simple changes I can start preparing the commits for the rest of the tomcat work (too bad we don't use a distributed SCM like git, in which case there would be no blocking of staged commits).
Attachment 446380 [details] jmagne+
NACK on 2 - There have been a lot of changes to this code in the 8.1 branch - whereby this code is required to in init(). Specifically, this code gets the password from nuxwdog and does ldap connection tests to verify the passwords. The reason that this code is not in the dogtag tip is because it requires nuxwdog, which is not as yet in Fedora. This code will be merged back at a later point though. You could try removing the mPassswordStore.init() from the getPassword() but you will find that the passwordStore needs to be re-initialized during configuration.
It's pretty hard to know there is requirement when the code is not present and only will be merged in the future. Wouldn't it be sufficient to just call CMSEngine.getPasswordStore() when you do the merge? I generally think it's bad coding practice to have multiple places with virtually identical code, too easy to have things get out of sync and find all the places which need updating. Case in point is the fact the password initialization was getting trashed by multiple thread access to the same data object during initialization. Leaving the code the way it was exposes you to the same run time problems. Please explain why the initialization should not be exclusively within a synchronized method and invoked as needed when the future merge is performed.
As we discussed, we can allow this patch for now. There are two issues: synchronization and when the initialization takes place. I will address these issues when I do the merge.
Created attachment 450030 [details] monster patch rolling up all tomcat 6 changes This represents all the changes necessary to get CA, KRA, OCSP and TKS install and run under tomcat6. WARNING: this is an outstanding bug in tomcat6 which will prevent things from running correctly, its bug 575341. At the moment this fix has not been pushed out, when it does we'll need to update the spec files to require at least that version of tomcat6. In the mean time if you hand edit the file /usr/sbin/tomcat6 and delete the offending lines referenced in the bz you should be fine. Caveat, I have not tested these changes with SELinux enabled. I expect there will need to be some fixes for SELinux. In the interim I would suggest running in permissive mode (e.g. setenforce 0).
Created attachment 450158 [details] fix previous patch, only copy profiles subdirectory if subsystem type is CA This patch makes a minor fix to the previous patch discovered during testing of subsystem types other than CA, the profiles subdirectory should only be copied if the subsystem type is CA.
The previous patch was too large and intermixed too many things to be useful for review. It is being broken down into a series of much small patches which address isolated well defined changes. There will be a significant number of small patches submitted in due order. In theory the cumulative effect of applying the patches in order will produce the equivalent of the former large patch. IMPORTANT: There is no attempt to assure the application of any given small patch will produce a working version of the source tree. That is an extra amount of work which is unnecessary and would greatly slow down the process. The purpose of the individual small patches is for review purposes. I do not suggest committing each small patch and testing. Rather after a series of patches is believed to have produced a viable working state that patch series can be tested/commited as a unit.
Created attachment 452115 [details] Use here doc syntax for readability Use here doc syntax for readability Perl allows one to embed large blocks of text into Perl source code via a mechanism nearly identical to shell "here documents". This allows large blocks of text to appear "as is" enhancing readability and makes editing the text easier. This patch replaces excessive string concatenation and embedded control characters with the actual text block. The resulting contents of the text is unaltered, this is a Perl style modification only.
https://bugzilla.redhat.com/attachment.cgi?id=452115&action=diff alee +
Created attachment 452124 [details] Handle the verbose flag better Handle the verbose flag better - Allow it to be specified multiple times to increment the verbosity level. - Add verbose option to pkiremove. Don't indent optional arg doc, leave room for more doc text. - update usage doc
Created attachment 452133 [details] Utilities to walk directory Utilities to walk directory - add utilities to walk a directory structure and get a list of files in a tree. - also adds utility to normalize a directory path
Created attachment 452158 [details] Utilities to record installation activity Utilities to record installation activity Adds utilites to track installation activity and produce an "Installation Manifest". Every filesystem path name which is modified during installation is recorded along with metadata about the installation action and what should be performed during an uninstall. The metadata is extensible. The table can be formatted in a variety of ways, either as a file which can be parsed (e.g. Installation Manifest), or as human readable friendly summary information. The installation file can be read later to perform an uninstall action. Previously a less complete cleanup.dat file was produced which omitted any information about files installed as part of a directory, distinction between symbolic links and files/directories, and what should occur during an uninstall (e.g removal vs. preservation). The utilities can detect the old file format utilize it to preserve backward compatibility. Because the new format is extensible any future needs should be easily accommodated. Aside from a more complete and accurate manifest and user report there is an additional benefit to this tracking information in terms of developer debugging. I found this more detailed reporting invaluable after modifying the installation script because it allowed me to see if what I expected to happen was happening or if things which weren't supposed to happen occurred. Formerly this was difficult information to extract and has enhanced robustness, both during development and during user install/uninstall. This patch only adds the utilities, it does not invoke them.
Created attachment 452196 [details] Introduce dry_run, add logfile to pkiremove Introduce dry_run command line arg and global variable. The next several patches will begin to utilize this so it's time to get it in (plus a couple of other minor tweaks which don't belong to another logical patch, see below). Dry run mode is used to show what the script would have done without performing any actions. Add logfile to pkiremove Update copyright dates.
Created attachment 452227 [details] Respect style guidelines Obey style guidelines and existing practice - Single space after keyword. - No space after open parens. - No space before closing parens. - No unquoted bare words There are no Perl guidelines for Certificate server but Matthew suggested the C and Java guidelines should be used as a basis. The C and Java guidelines for Certificate server state that keywords should be separated by a space. Although not explicitly stated they imply open and closing parens should not be adorned with superfluous spaces. The bulk of the existing Java and C code obeys this style rule. FWIW this is an explicit recommendation in the IPA guidelines. In addition it is poor programming practice to use bare words in Perl when it's not a keyword. Perl permits this abuse of string syntax by assuming a bare word which is not known to be a keyword is really a string. However if the set of Perl keywords ever changes this can cause failures, plus it's just bad style to not use string syntax for strings. With the exception of adding quotes around bare words this entire patch is nothing other than whitespace adjustment. This was verified by running a diff with the -w arg (ignore whitespace differences), the only changes which showed up was the quoting of bare words in %ENV hash.
Created attachment 452243 [details] Reimplement copy_directory, remove_directory Reimplement copy_directory, remove_directory The copy_directory function was losing critical information. It called out to the shell to recursively copy the contents of one directory to another. But this meant we lost track of the files and directories actually being copied, we couldn't log them nor add them to the installation manifest. Now the copy_directory function builds a list of files in the src directory and iteratively copies each file calling into our copy_file function which records the operation and checks for errors. The remove_directory function was an unapologetic sledge hammer, it simply nuked entire trees. Now the function is more sensible, by default it removed one empty directory, or optionally recursively removes all directory contents. Both functions previously had serious implementation mistakes. Both were implemented by calling out to a UNIX shell and invoking a shell command via the Perl backtick operator. The fundamental problems with this were: * UNIX shell commands only work on UNIX * Not all UNIX shell commands are identical * The error detection stragegy was completly broken. It executed the shell command via the backtick operator which returns the stdout of the command and discards stderr. The function would then test to see if the length of stdout was zero to determine if there was an error. If there was no stdout it assumed no errors occurred. This is completely wrong. To test if an error occurs with a shell command one needs to examine the exit status of the command which is impossible to do with the Perl backtick operator. If one wants to test the exit status of a shell command in Perl you must utilize the subprocess features of Perl. The reimplementation eschews the non-portable use of UNIX shell commands in favor of the portable Perl extensions for operating on filesystem directories.
Created attachment 452812 [details] add utilility to run a shell command & detect failure Add run_command() utility Many of the existing functions were using the backtick operator to run a shell command and then tested if the length returned string was zero to determine success. This is incorrect for two reasons: 1) the backtick operator discards stderr and returns only stdout, thus if the command did generate a message because of an error it would likely do so on stderr not stdout and thus the test for output is performed on the wrong stream. 2) the presence or absense of output is not the proper way to determine if a shell command succeeded, one needs to test the exit status of the command. This patch adds the run_command() function which will be utilized in subsequent patches.
Created attachment 452824 [details] remove unnecessary global variables used by get_time_stamp() Can't imagine why the result of localtime is global
Created attachment 453051 [details] Parameter list style fix Parameter list style fix The preferred canonical style for Perl subroutine parameter lists is to write the parameter list as an actual parameter list prefixed by the "my" lexical modifier and initialized from the @_ parameter array. The parameter list should be the first line in the sub. This makes it much easier to read the code because a parameter list looks like a parameter list. This improves readability and follows widely adopted style conventions. For example: sub foo { my ($able, $baker) = @_; } Thus the signature for this subroutine is: foo($able, $baker) The patch also removes comments of the form # arg0 is able # arg1 is baker Which are silly when you write the parameter list to look like a parameter list because it's self evident what the parameters are. Comments like that clutter the code, decreases readability and do not add any information content.
Created attachment 453052 [details] Remove pointless no-op string interpolations Remove pointless no-op string interpolations Constructs such as "$variable" when $variable is already a string are no-op waste of processor cycles and confusing to read. Just use the variable. Explanation: Perl performs variable substitutions on all double quoted strings, this is called string interpolation. To do this Perl scans the string looking for anything that looks like a variable and substitutes it's current value. But when the string consists of nothing other than a variable (e.g. "$variable") the result is the same as the variable, effectively it's just a no-op. I'm not sure if the interpreter is smart enough to recognize this as a no-op and skip the interpolation, irregardless there is no point in coding it this way. It eludes me as to what the programmer thought they were accomplishing when they wrote "$variable".
Created attachment 453053 [details] Clean up Perl interpreter warnings Clean up Perl interpreter warnings true is not keyword, use 1 instead use defined() when testing for hash membership add some variables to the $suppress which are defined in pkicommon, but only used once in pkicreate/pkiremove remove duplicate definition of $webapps_subsystem_instance_path remove @pki_static_directories, it's never referenced. $result = GetOptions() needs lexical scope for $result fix misspelling of $PKI_FLAVOR, should be $PKI_FLAVOR_SLOT fix misspelling of $SELINUX_PORT_WONGLY_DEFINED, should be $SELINUX_PORT_WRO change print (...) to print(...), space between function name and list changes interpretation of list context vs. function call.
Created attachment 453243 [details] Fix utilities related to UNIX group operations Fix utilities related to UNIX group operations The Perl functions getgrnam, getpwnam, etc. in a scalar context return the undef value if the name wasn't found and an empty list in an array context. Therefore the test for equality to the empty string is not correct, the test should be if the value is defined. Replace use of backtick shell invocation with run_command() (see earlier patch) The function user_is_a_member_of_group() was not implemented correctly. There were two fundamental problems: 1) It failed to take the primary group into account, see comments in the code for an explanation. 2) It tested the username against group members using a regular expression which incorrectly identified substrings as matches. The test was: $members =~ m/$username/; where $members was a space separated list of user names. However the regular expression did not match on word boundaries, therefore any substring would produce a false positive. For example if the username was "foo" and the $members string was "barfl foobar blatz" the test would succeed because it found "foo" as a substring of "foobar" but "foo" != "foobar". The test was rewritten to split the string into individual names and test for equality on each name, it's a more robust test and more obvious to the reader. The member regular expression test had to also be fixed in the add_user_as_a_member_of_group() function as well.
Created attachment 453310 [details] Utilities cleanup Utilities cleanup This is the final patch in a series mostly devoted to clean up of the common library. After application of this patch pkicommon will be in it's proposed form. A large proportion of this patch is devoted to replacing the use of UNIX shell commands to perform basic file system operations with built in Perl functions. The other items in this patch are things which didn't logically fit into any of the other patches. The rationale for eschewing the use of shell commands where possible is: 1) shell commands are OS specific, not all UN*X variants have the same behavior. 2) shell commands are available only on UN*X variants. 3) the built in Perl functions are portable across most OS's 4) the built in Perl functions have better error handling and reporting 5) the built in Perl functions are more efficient since they don't need to spawn a shell to do one simple operation, instead they call into the native OS library in the same process/thread. 6) the built in Perl functions are not subject to shell interpretation thus making their use more robust. It's not unusual to have to properly quote arguments when using a shell to protect against unintended interpretation by the shell. Or worse to expose the application to injection attacks where expanding a shell command results in an untended operation performed with root privileges. Also, many of the commands which used the Perl backtick operator to perform a shell operation were not properly detecting if the command failed. The backtick operator returns the stdout of the executed shell command and discards stderr. The old code would capture the result of the backtick operator (stdout), test to see if it was the empty string and if so concluded the command succeeded. This is not correct, one needs to check the exit status to determine success/failure. If an error did occur the command probably wrote to stderr, but stderr is discarded by the backtick operator. All this is documented in the patch which added the run_command() utility. For those routines for which there wasn't a Perl built-in equivalent the code was changed to call run_command() instead of using the backtick operator. Each of the utility routines which creates/copies files/directories had their parameter list expanded to accept optional specification of the permission and ownership to be applied to the file system object. This allows one call to replace multiple calls to utility routines which ultimately makes the code in pkicreate smaller, less verbose, easier to read and more robust. The code used to parse an initscript was removed (extract_chkconfig_parameters_from_start_stop_script()), it wasn't called and I'm at a loss for why this would have been needed in the first place. Also the parametrization of the start/stop positions is changing as we move closer to LSB. The global variable chkconfig_fields was also removed. It was only used in the routine extract_chkconfig_parameters_from_start_stop_script(). What's up with these global variables which should be private to the subroutine utilizing them? Many routines had logging added to them for tracing purposes. A call to emit() with the subroutine name and parameters. Many routines had the dry run check added to them. If $dry_run is true they emit their tracing information and then return success. Many routines had calls to add_install_info() added to them. This is used to record the installation actions being performed. Code that formerly had used shell commands to operate recursively on directories now instead iterate over the contents of the directories invoking our utilities, this allows us to use our primitives which record the installation action. For example rather than copy_directory() doing a "cp -r src dst" we walk the tree and invoke our own create_directory() and copy_file() routines which are responsible for recording the operation and doing such things as setting permissions and ownership. Comments referencing arg0, arg1, etc. were removed or edited (see previous patch for parameter list clean up for an explanation). Some functions were renamed to better reflect their actual operation as would be understood by a system administrator. e.g. give_file_to() became set_owner_group(), give_directory_to() became set_owner_group_on_directory_contents(). A utility called set_permissions() was added as well as a utility called set_file_props() which sets permissions and ownership with a single call. The routines move_file() and move_directory() were removed. They were using the deprecated shell methodology but were never called by any code. Rather than re-implement them I just removed them, if we need these again in the future we can add them back with the preferred Perl methodology. A potential bug was fixed in copy_directory(). The logic used to enumerate the set of destination directories which needed to be created had a logic flaw. If a source directory was empty it wouldn't get created in the destination. This occurred because the previous logic was to enumerate all the source files to be copied and generate a set of directories from those, but if a directory was empty it wouldn't show up in the file list. The new logic is to independently enumerate both directories and files in the src tree, this makes the directory list complete. Add utility to return the initscript name, get_registry_initscript_name().
We're at a logical break in the patch set. Everything up to now was either bug fixes or clean up in the pkicommon utility library used by pkicreate and pkiremove. Starting from this point forward in the patch set are the actual changes to pkicreate to support a tomcat 6 installation. This is what you've been waiting for at long last :-) Very little needs to change in pkiremove other than to use the pkicommon routines properly now that pkicommon has been updated. pkicomplete which also uses pkicommon will be deprecated. As with the pkicommon patches I will try to break them up into small logical units for easy review. Like before there may be interactions between this small individual patches and I'm not going to try and assure the application of any single small patch will be functionally correct, rather full functionality will be delayed until all patches are applied.
Created attachment 453538 [details] Enhance file template utility: process_file_template() Enhance file template utility: process_file_template() The following changes were made: 1) Add a template name. Previously I had found it difficult to correlate the output in the log file with a specific invocation of process_file_template() in the code. The file pathnames aren't much help because they never appear in the code as something you can search on. 2) Be more efficient with file operations. Previously the code would: a) read a line from the file b) strip the newline off c) add the newline back d) concatenate the munged line to a string variable That's an incredibly inefficient way to assign the contents of a file to a string variable. Now the code just uses the standard Perl function read_file() to assign the file contents to a string variable 3) Previously the code would claim it performed a substitution for every substitution in the substitution table even if the substitution was not performed, that's useless information. Now the code reports exactly which substitutions were made along with a count of how many times that substitution was made. 4) Optionally dump to the log the contents of the file after it was processed for debugging purposes. 5) Update all calls to process_file_template. At the same time utilize the new utilities for setting file properties (e.g. permission & ownership) Example of new logging information written to log file ------------------------------------------------------ Processing PKI templates for '/var/lib/pki-ca' ... Template (pki_cfg) "/usr/share/pki/ca/conf/CS.cfg" ==> "/etc/pki-ca/CS.cfg" ... 1 substitutions: TOMCAT_SERVER_PORT ==> "9701" 1 substitutions: PKI_RANDOM_NUMBER ==> "YLmLqrJOD10jrIdUwefc" 8 substitutions: PKI_MACHINE_NAME ==> "vm-117.idm.lab.bos.redhat.com" 7 substitutions: PKI_FLAVOR ==> "pki" 2 substitutions: PKI_EE_SECURE_PORT ==> "9444" 3 substitutions: PKI_INSTANCE_ROOT ==> "/var/lib" 68 substitutions: PKI_INSTANCE_PATH ==> "/var/lib/pki-ca" 18 substitutions: PKI_INSTANCE_ID ==> "pki-ca" 2 substitutions: PKI_EE_SECURE_CLIENT_AUTH_PORT ==> "9446" 1 substitutions: PKI_SECURE_PORT ==> "9443" 1 substitutions: PKI_SUBSYSTEM_TYPE ==> "ca" 3 substitutions: PKI_AGENT_SECURE_PORT ==> "9443" 1 substitutions: PKI_GROUP ==> "pkiuser" 1 substitutions: INSTALL_TIME ==> "Mon Oct 11 22:11:14 2010" 2 substitutions: PKI_ADMIN_SECURE_PORT ==> "9445" 1 substitutions: PKI_USER ==> "pkiuser" 2 substitutions: PKI_UNSECURE_PORT ==> "9180" 122 substitutions were made in '/etc/pki-ca/CS.cfg'
Created attachment 453600 [details] Remove dtomcat5 Remove dtomcat5 dtomcat5 was a private copy of a system supplied initscript. We should never make private copies of files supplied by other packages otherwise we get out of sync, especially with respect to bug fixes. In any event dtomcat5 does not even exist in tomcat6 (nor an equivalent). With tomcat6 we're going to use the initscript supplied by the tomcat6 package. We are not going to modify files supplied by other packages! tomcat6 has an easy mechanism to launch tomcat6 instances. You create a symlink in /etc/init.d (e.g. /etc/rc.d/init.d) which points to the tomcat6 initscript. When the tomcat6 initscript is invoked it gets the basename of the script, because it's a symlink it will be the name of the instance. That name is then used to read a tomcat6 config file in /etc/sysconfig. This way you can create a variety of tomcat6 daemons and launch them with the standard system tools/files and never once need to modify any file provided by the tomcat6 package!
Created attachment 453757 [details] Tomcat6 initscript Tomcat6 initscript With tomcat6 tomcat instances are created by creating a symbolic link with the new instance name in /etc/init.d (aka /etc/rc.d/init.d) to the tomcat6 initscript supplied by the tomcat6 package. When the tomcat6 initscript starts it examines it's basename (as seen by the symbolic link) and sets that to it's instance name. It then sources a per instance configuration file located in /etc/sysconfig whose basename matches the instance name (e.g. same name as initscript symlink). For example if we wanted to create a tomcat6 instance called "foo" % ln -s /etc/init.d/tomcat6 /etc/init.d/foo % cp /etc/sysconfig/tomcat6 /etc/sysconfig/foo Now we have a new tomcat6 instance which can be managed by the standard mechanisms, e.g. /sbin/service. For example: % /sbin/service foo start % /sbin/service foo status % /sbin/service foo stop A very desirable property of this approach is NEVER modifying or overriding any files supplied by the tomcat6 package. If there are any bug fixes in the system supplied tomcat6 package we automatically will benefit from those fixes once the system administrator installs a new tomcat6 package. This was not the case previously when we were using tomcat5 for PKI, we overrode a number of files and created our own independent tomcat instance mechanism private to ourselves. This was a lot of work, non-standard, and prevented ourselves from benefiting from any updates to the tomcat package related to instance management. This patch also removes a number of references to tomcat5 specific files.
Created attachment 453793 [details] Clean up the instance registry & more initscript modifications Clean up the instance registry & more initscript modifications The initscripts for pki-* were significantly simplified. All logic related to managing the tomcat instance was completely eliminated! This is because we now use the unmodified tomcat6 initscript which ships with the tomcat6 package completely freeing us of having to know how to manage a tomcat instance. We simply defer to the definitive source, the tomcat6 package. This eliminated half the code in script, reducing it from 1831 lines to 885 lines! What remained was essentially the "pki registry" management, how we record what pki instances have been created on the system. There was also code to extract information from config files, this is used when reporting instance status. The registry management logic had been almost identically copied into the other KRC, OCSP & TKS initscrips. Copying complex code into multiple places is not good software engineering, rather the code should be defined in one location and then referenced. To this end the common shell code for the shared initscripts was isolated in a common file, pki/base/common/scripts/functions in our source tree and installed as /usr/share/pki/scripts/functions. The functions file is now 812 lines of code and shared amongst pki components. The shell code in functions was also made more robust, formerly it would try to extract string data out of files by using exact strings and string character counts, this varied slightly by each component. Now it just uses sed and regular expressions and won't break if someone adds a character to line in one of the files. With the pki registry logic isolated in a common file and by using the installed tomcat initscript we've now reduced the size of the initscript from 1,831 lines to a mere 73 lines! Just 4% of it's former size and in the process greatly increased robustness and maintainability. Each instance in the pki registry is defined by a configuration file. Formerly that file was created by the function construct_pki_instance_registry() in pkicreate. Although the purpose of construct_pki_instance_registry() is to write out a simple shell script it's implmentation was completly incomprehensible and unreadable. Since the resulting file is basically the same for different instances and subsytems and varies only by a minor amount of parametrization it a perfect candidate for a template file. We've now added a new template file base/*/setup/registry_instance which is easy to read and is processed by the exact same templating system which many of the other files are processed by. Also, formerly the registry instance file had shell logic it which is no longer necessary and has been removed. What we've ended up with is essentially just a set of shell variables (e.g. key/value pairs). Now the pki-* initscripts essentially just iterate over the instances located in the registry and invoke the initscript for the instance (which is ultimately just the standard tomcat6 initscript). This gives us yet another significant advantage. You can now control an instance using the normal "service" commands, there is no need to use the pki-* uber initscript to control instances. You can still do that if you wish, but now you can do the more obvious and natural service command on anything appearing in /etc/init.d. You can still use the pki-* uber initscripts to manage all instances of a subsystem if that makes more sense, but given there is likely to only be one instance of a subsystem installed on a machine being able to manage it directly and not needing to use an uber initscript to iterate a single instance yields something which is easier and more obvious to system administrators. The previous patch, "tomcat6_initscript", which updated the initscript logic discussed how a tomcat instance configuration file is installed in /etc/sysconfig under the instance name. Unfortunately that patch omitted the generation of that file which is generated using our templating facility. The source file pki/base/*/shared/conf/tomcat6.conf and replaces the previous tomcat5.conf file. For example if we are creating a pki-ca instance the file /usr/share/pki/ca/conf/tomcat6.conf will have substitutions performed on it and then it will be installed as /etc/sysconfig/pki-ca, which will be "sourced" by the standard tomcat6 initscript to parametrize the tomcat instance. This logically belonged in the previous "tomcat6_initscript" patch, but since this patch is also about initscript modifications it seems reasonable to include in the patch instead.
Created attachment 453820 [details] Utilize the new install info utilities Utilize the new install info utilities The install info utilities were introduced in a previous patch. This patch removes the old mechanisms and replaces it with the new mechanism. See the earlier patch for a more complete description. This patch also pulls in a few minor edits to support dry run mode and logging.
Created attachment 453823 [details] Remove pkicomplete script Remove pkicomplete script The pkicomplete script is no longer needed, remove all vestiges of it's existence.
Created attachment 453824 [details] Use new the new versions of the file utilities Use new the new versions of the file utilities The utilities in pkicommon were enhanced in a previous patch. This patch calls the new utilites with the new parameter lists. The bulk of the changes are simplifying the specification of file permissions, file ownership, and checking the result of the operation.
Created attachment 453826 [details] Remove support for other OS's Remove support for other OS's The modifications to the install scripts have been Linux specific. So much has changed it's reasonable to assume the special case code for other OS's (e.g. Solaris and Windows) is not likely to be correct any more. As a consequence there isn't much sense in keeping this OS specific code. To support other OS's the scripts would really need to be ported to the target OS and it probably would be best to do this cleanly by starting fresh and incrementally adding back in OS specific exceptions. Note: Only OS specific code which obviously needed porting after the update to the scripts was removed. The OS specific code which was "generic" has been preserved. Plus, management has stated the other OS's are no longer supported.
Created attachment 453858 [details] Update jar class loading Update jar class loading Tomcat's class loading follows the model for J2EE Application Containers and Servlet's. Each release of Tomcat has modified it's class loading in some respect. Usually the class loading modifications have been in the service of encouraging best practice. Typically this means keeping web applications which may be running in the same tomcat instance completely isolated from each other such that they can't interfere with one other. In essence this means classes which are loaded by a specific web application should only be visible to that web application. Sharing classes/jars between web applications is to avoided to the greatest extent possible. Best practice suggests only "framework" classes (e.g. tomcat's servlet API's) should be shared. Class visibility and sharing is controlled by a hierarchy of class loaders. The topic of class loading, and specifically in the context of servlet containers, has been extensively written about. For those interested in the topic a search of the web will produce a wealth of material. I found the following documents useful: "Understanding The Tomcat Classpath Common Problems And How To Fix Them" http://www.mulesoft.com/tomcat-classpath "Class Loaders" http://datadisk.co.uk/html_docs/java_app/tomcat6/tomcat6_classloaders.htm "Apache Tomcat 6.0 Class Loader HOW-TO" http://tomcat.apache.org/tomcat-6.0-doc/class-loader-howto.html "Java programming dynamics, Part 1: Java classes and class loading" http://www.ibm.com/developerworks/java/library/j-dyn0429/ "Taxonomy of class loader problems encountered when using Jakarta Commons Logging" http://articles.qos.ch/classloader.html In particular one needs to have a firm understanding of the class loading delegation model, parent-first vs. child-first, as this differs between standard Java and Servlet Containers. We attempt to follow best practice to the greatest extent possible so that the jars we need are visible only the to appropriate class loader. We do have one significant exception which requires violating the isolation principle. tomcatjss and jss are both required by the tomcat framework and by our web application. This occurs because we specify the catalina connector (Coyote) we wish to use for our server SSL/TLS connections are jss instead of the default SSL/TLS connectors in tomcat, thus tomcat needs to load tomcatjss and jss. Our web application also utilizes tomcatjss and jss, the most obvious example being the CrypoManager which must be a singleton instance. There is an additional issue, jss is a JNI native library written in C. JVM's have a restriction which prevents loading a JNI library by more than one class loader. The fact the CryptoManager is a signleton and that jss is JNI means jss (and tomcatjss) must only be loaded by exactly one class loader in the JVM. Thus tomcatjss and jss must be loaded by the tomcat common class loader which is shared between the tomcat servlet framework and loaded web applications. Normally tomcat ships with a catalina.properties configuration file which enforces the best practice class loading separation. However, in recognition that is sometimes too restrictive the catalina.properties file can be edited to support other class loading configurations. We take advantage of this by establishing a "common" class loading location specific to the tomcat instance (e.g. $CATALINA_BASE/common/lib). The tomcat common class loader via the catalina.properties file is directed to also search this directory. We install tomcatjss, jss and jakarta-commons-logging in this common location. All other jars follow best practice and are isolated in the web applications library (e.g. $CATALINA_BASE/webapps/<webapp_name>/WEB-INF/lib). The reason why jakarta-commons-logging is also installed in common along with tomcatjss and jss is because it is a dependency of tomcatjss and is not otherwise available because tomcat uses another logging package. When we install the tomcat instance we don't actually copy jar files into the library directories under $CATALINA_BASE because we want to use the system supplied jar files and if they are updated because of bug fixes, security fixes, etc. we want to immediately take advantage of the updated version of the jar file. Thus we "install" symbolic links in the library directories which point to the system supplied jar files. This also reduces disk usage by avoiding multiple copies of the same file. This patch implements the above by doing the following: Makes catalina.properties a "template file" which is processed by our templating facility. The only substitution at the moment is the common class loader location. Establishes the paths to each of our required jar files as supplied by the system package manager. Creates symbolic links the to jar files in the instance library directories. The following diagram illustrates the class loading described above: +--------------------+ | Bootstrap | | Class Loader | +--------------------+ | V +--------------------+ | Extension | | Class Loader | +--------------------+ | V +--------------------+ | System | | Class Loader | +--------------------+ | V +---------------------------+ | Common | | Class Loader | | $CATALINA_BASE/common/lib | | (see note 1) | +---------------------------+ | +---------------+--------------------+ | | V V +---------------------------------------+ +-------------------------+ | CA Web App | | Web App 2 | | Class Loader | | Class Loader | | $CATALINA_BASE/webapps/ca/WEB-INF/lib | | (for illustration only) | | (see note 2) | | | +---------------------------------------+ +-------------------------+ [1] Common loader loads these jars: jakarta-commons-logging jss tomcatjss [2] CA Web App loader loads these jars: certsrv cms cmsbundle cmscore cmsutil jakarta-commons-collections kra ldapjdk nsutil osutil velocity xerces-j2
Created attachment 453873 [details] Add more template substitutions Add some more template substitutions, these are referenced in some of the configuration files.
Created attachment 454461 [details] Use run_command() utility when invoking SELinux shell commands Use run_command() utility when invoking SELinux shell commands. Also some minor tweaks for checking result status and protecting variables in string interpolation for the SELinux shell commands. No change in functionality, just robustness enhancements.
Created attachment 454463 [details] Check the status of all invoked subroutines Check the status of all invoked subroutines Also, use more succinct Perl syntax for improved readability.
Created attachment 454465 [details] Add tomcat logging.properties config file Add tomcat logging.properties config file The $CATALINA_BASE/logging.properties file provides configuration of logging for the tomcat instance.
Created attachment 454653 [details] Port tomcat5 config files to tomcat6 Port tomcat5 config files to tomcat6 We copy a number of tomcat config files from the tomcat distribution and keep them in our own location. Some of those config files had changes between tomcat5 and tomcat6. This patch just merges the tomcat6 changes into our copy of the files making them very close to the original tomcat6 version of the file.
Created attachment 454657 [details] Update build.xml to reflect file manifest changes Update build.xml to reflect file manifest changes * remove dtomcat5 * add registry_instance
Comment on attachment 454653 [details] Port tomcat5 config files to tomcat6 Provisionally +'d this patch. Getting things working under security manager is something still to be done on tip. (already done in 8.1) Question: Please explain the change in: https://bugzilla.redhat.com/attachment.cgi?id=454653&action=diff#a/base/ca/shared/conf/context.xml_sec1
Comment on attachment 453793 [details] Clean up the instance registry & more initscript modifications Waiting for an explanation on how you are planning to solve the dangling init scripts problem we discussed if rpm -e is used.
re comment 42 The common pki shell library (/usr/share/pki/scripts/functions) has a function called list_instances() which dumps to stdout the existing instances in the registry. My plan is to iterate over the installed instances in the %preun scriptlet (when $1 == 0, hence uninstall) and rm -f /etc/init.d/$instance where $instance is one of the returned values from list_instances(). There are two possible ways to invoke list_instances: 1) source the function library and call it directly 2) invoke the pki-*d uber initscript with the list argument inside a shell backtick operator Note: there needs to be some minor tweaks to the function library because currently list_instances() returns the path name of the instance registry file (which is actually meaningless to the user) and not the bare name of the instance, but that's a trivial enhancement. Also the uber pki-*d doesn't accept a list argument, it invokes list_instances() as part of help and usage output, once again a trivial addition (and conforms to LSB).
This is one approach to solving this, but this - and maybe, any other solution - feel like hacks - to me. Invoking an rpm uninstall scriptlet to remove init scripts that the rpm did not create in the first place - is hacky at best. I think we went the right direction when we removed default instance creation from the rpm scripts. Instance management should be done by the user using pkicreate/pkiremove. We already have a good approach to managing instances though the registry and uber-script. This is what directory server does. Re-introducing instance specific init scripts now makes things needlessly more complicated. We can take this conversation to the rest of the team if you like.
re comment 44 Perhaps there is a misunderstanding. Default instance creation was NOT reintroduced. Instance creation is ONLY done using pkicreate. Instance removal is only done with pkiremove. The previous mechanism also left cruft around if you removed the package. It violated packaging rules by writing files into /usr/bin without RPM management (/usr/bin/dtomcat5-$instance_name). It's my belief this is wrong but to assure myself I had conversations with other packaging experts who concurred and expressed deep concern about the practice. In fairness dtomcat5-$instance_name could have been located elsewhere where it wouldn't violate packaging rules. If you removed the rpm without running pkiremove it left /usr/bin/dtomcat5-$instance_name. It is normal packaging practice for the uninstall rpm scriptlet to clean up items which are not removed via the rpm file manifest. But there is a larger problem with the previous approach. The dtomcat5-$instance_name script is nothing other than an modified copy of the tomcat5 initscript which ships with the tomcat5 package. What happens when the tomcat5 maintainer discovers a problem, perhaps a security flaw, and updates tomcat5 including the tomcat5 initscript? Because we've made a private copy of another package's system file we won't benefit from any of the tomcat maintenance work. And visa-versa, any fixes we make won't benefit any other users of tomcat in the distribution. It's poor practice to make private copies of files belonging to other packages. It's my belief that if the above, and some other issues, were visible to the package reviewers it would not have passed review. The problem is a significant amount of what is normally done in a packaging spec file has been removed and shunted into a perl script most package reviewers would not look at, or if they did would have had a difficult time understanding how it modified the system when it executed. In fact you really have to execute the pkicreate script and spend a significant amount of time dissecting what it did to understand the implications from a system management perspective (which is one reason my modifications include full and accurate reporting of every file modified by pkicreate). What prompted the initial question during the review of the porting work is whether during rpm removal pkiremove should be called to remove installed instances. On the pro side of the argument is the fact that after the rpm is removed none of the instances are viable, consume disk space, and may leave unintentionally leave sensitive data on the system which the system administrator thought had been removed when the rpm was removed from the system. On con side of the argument there may be per instance data which the system administrator wants preserved even though the package which created it had been removed. An analogous situation can been seen with our SQL database packages which preserve database contents after the SQL database package has been removed. I agree the per instance data should be preserved upon rpm removal and it should be at the system administrator's discretion to invoke pkiremove. There is an additional significant reason for this behavior, pkiremove only removes files created by pkicreate, it does not remove the per instance data stored in the LDAP backing store, NSS databases, etc. Thus even if one were to call pkiremove to clean up per instance data when the rpm is removed it would only clean up a small portion of the per instance data (but that's another topic). If I were to summarize the immediate issue at hand I would say the root cause is pkicreate is straddling two functional domains. Part of what it is doing is traditionally in the domain of RPM package management and other part is controlling run time behavior of the package. This is definitely a corner case and atypical for most packages. I'm suggesting the best we can do given the constraints and the desire to adhere to best practices in packaging and system management is to identify the small subset of files created by pkicreate which traditionally belong in the domain of RPM mangement (e.g. anything installed into directories owned by the system) and assure those files are properly removed during an rpm erase operation, those don't represent user created data files which need to be preserved, rather they are clearly system files. As for the per instance initscript, please make note of the fact in tomcat6 this is only a symlink to the tomcat6 initscript owned by the tomcat6 package. Symlinking to a file owned by another package is much more robust system management solution than making a static copy of that file at some point in time, including the copy in our package and then copying that historical file into /usr/bin when we create a tomcat instance. In the new approach the only thing which has to happen when the rpm is removed is to remove the symlinks we created. Since previously the per instance initscript was removed by pkiremove, we can trivially retain that division of labor by augmenting pkiremove to remove the sysmlink, rather than embedding the removal in the spec file, but I see no particular advantage in this approach, the spec file still must iterate over the instances. The bottom line is no matter what, the fact that pkicreate installs BOTH per instance data files AND system files traditionally owned by RPM you're going to have to have some redundancy between pkiremove and the spec file if you want an rpm erasure to clean up all system files but preserve per instance user data. The goal should be to achieve this cleanly, preserve system integrity, adhere to packaging guidelines as close as possible, and to make the process obvious to outsiders. I welcome the opportunity to discuss this in a wider forum.
re comment 45 It occurred to me there may be a compromise solution with regard to the location of the per instance initscript symlink. I believe (but have not yet tested) the symlink to /etc/init.d/tomcat6 does not have to be located in /etc/init.d it can be located in the per instance data directory. That was it does not need to be removed by rpm. On the down side you lose some "obviousness" to system administrators who expect to see initscripts in /etc/init.d. I will test moving the location of the symlink. If successful that may be the optimal solution which addresses both our concerns.
re comment 45 I forgot to mention in the discussion concerning copying the tomcat5 initscript into our packages there is another reason to eschew this practice. Not only did we copy the file, we also made significant changes to it, potentially altering the JVM, the classpath used to bootstrap tomcat and class loading. None of these things should be done by us. Launching tomcat correctly is exclusively the job of tomcat. There are well defined best practices for getting classes to load correctly for tomcat web applications which do not require modifying the installed version of tomcat. One reason they are best practice guidelines is precisely because they do not modify core tomcat, rather the class loading is localized exclusively to the tomcat web application and if need be the tomcat instance via standardized tomcat configuration mechanisms.
I am OK with the compromise solution of moving the symlink to the per-instance data directory. In fact, you may not have realized this - but the old version of pkicreate did fact create such a link in /var/lib/<instance-name> - which was a link to the old initscript in /etc/init.d We lose some obviousness -- but the uber script provides an interface that is obvious enough. I have a question about the following statement: There is an additional significant reason for this behavior, pkiremove only removes files created by pkicreate, it does not remove the per instance data stored in the LDAP backing store, NSS databases, etc The NSS databases are located in /var/lib/<instance_name>/alias. The old pkiremove did in fact remove this directory and its contents. Are you saying that this will no longer happen? If so, we will have the possibility that we will have instance specific data lying around even after we call pkiremove. I believe that pkiremove should remove all instance specific data. If I choose to run pkiremove, then I should know the consequences. Right now, the only thing this is not cleaned up is the data in the ldap database -- and maybe in a future enhancement, we can add some option to pkiremove to allow it to be. ps. I had a question in comment 41.
Created attachment 456089 [details] Update server.xml config file This is mostly a merge of the tomcat 6 server.xml file with our existing server.xml file from tomcat 5. Merge in new comments. remove org.apache.catalina.storeconfig.StoreConfigLifecycleListener because it's not part of tomcat6 Parameterize the following based on our template engine: sslOptions="[TOMCAT_SSL_OPTIONS]" ssl2Ciphers="[TOMCAT_SSL2_CIPHERS]" ssl3Ciphers="[TOMCAT_SSL3_CIPHERS]" tls3Ciphers="[TOMCAT_TLS3_CIPHERS]" Note: After the cipher parameterization was done it was discovered that the other subsystems do not utilize ECC ciphers, it's not clear if they should or not. We may need to paramterize the cipher list in pkicreate or go back to hardcoding the cipher list in the xml file.
Created attachment 456107 [details] Fix set/get library path set_library_path() and get_library_path() were both producing warnings from Perl about the use of uninitialized variables. This occurred because get_library_path() returned the value of the LD_LIBRARY_PATH environment variable, which if it is not set in the envronment is the undef value. Then the caller of get_library_path() would use the result to build a new string to use as a new library path. But the use of undef in the string concatentation was producing warnings. Finally the caller would reset the library path to what had been orginally returned by get_library_path(), which set LD_LIBRARY_PATH in %ENV to the undef value, which is probaly not the best idea, although legal. To fix this every routine which called get_library_path() would need to check for undef value as it builds a new replacement path, that's a lot of code to add in a lot of places. Instead set_library_path() was modified, instead of accepting a string containing a new path, it now accepts an array of path values. It iterates over the array discarding any undef values in the array and builds a path string from the defined values. This simplifed the callers of get_library_path() and set_library_path(). It also had the nice property that if get_library_path() initially returned undef then subsequently calling set_library_path() with that value produces an empty string for storing into %ENV which preferable to storing undef.
Comment on attachment 456089 [details] Update server.xml config file the other subsystems probably should use the ECC ciphers as well. Check with cfu and mharmsen on this.
Created attachment 456347 [details] Fix ampersand function calls Some functions were being called with the deprecated ampersand syntax. In Perl the & prefix operator indicates the expression is to be interpreted as a function, e.g. &foo means foo is a function and if foo was followed by a list then it means call the function foo. The list can be parenthesized or not, it could just be comma separated expressions. Calling functions with this syntax is a hold over from earlier versions of Perl, but modern Perl has much cleaner syntax where function calls look like they do in other languages, an identifier followed by parenthesis. This is the calling style used in most of the rest of the code. This patch just unifies the calling syntax so it's consistent and more readable. Also the patch cleans up the function definition, some of the functions had been defined with an empty formal parameter list, but that conflicts with function prototyping introduced in modern Perl, an empty formal parameter list states the function takes no arguments. It only worked previously because when the (deprecated) ampersand operator was applied to the identifier it defeated prototype checking.
Created attachment 456348 [details] Unify the message stream Some messages were being directly written to stdout or stderr bypassing the message mechanism, the emit() function. That meant those messages were not recorded in the log and hence were lost. This patch uses the emit() function for more messages. The patch also adds a "warning" level to the message category.
Created attachment 456350 [details] Fix use of dry_run Fix return value when dry_run is enabled. Also simplify dry run conditional syntax by removing unnecessary list parenthesis.
Created attachment 456352 [details] Fix initialization of $uninstall_action In some places $uninstall_action was being referenced before it was initialized and thus generated warnings.
Created attachment 456353 [details] Fix the initialization of the pid file
Created attachment 456355 [details] fix use of default instance names It wasn't initialized in some places. Use the same naming convention in all places.
Created attachment 456357 [details] Allow tomcat to traverse symbolic links Tomcat by default will not read symbolic links under the WEB-INF directory. This can be overridden by setting the context parameter allowLinking to True. We want to symlink to the jars and not copy them because otherwise when rpms containing the jars are updated with bug fixes or security fixes we won't benefit from them if we've made private copies of the jars in the instance. The reason why allowLinking defaults to False is motivated by security concerns on untrusted web applications. Also you'll often see in tomcat documentation the recommendation that all necessary jars are copied into the WAR, this recommendation derives from deploying a web app on a random server where the presence or absence of jar or a specific version of a jar can't be guaranteed. However, that is not our situation, we're not deploying a WAR on random servers, our tomcat instance is quite controlled and we'll never deploy unknown/untrusted web applications from it. The use of symbolic links in this context should be safe and the value in picking up rpm updates is so important that it justifies the use of symbolic links in our controlled deployment.
Created attachment 456358 [details] Set the owner and group on the instance's NSS database
Created attachment 456359 [details] Use strict language rules Use strict language rules Add the strict and warning pragmas informing the Python interpreter we want to obey the language rules and catch as many errors for us as it can. Clean up all the errors that strict reported. Properly define the scope of all identifiers and use correct import semantics. Initialize most global variables to undef so that we can catch the use of those variables prior to their initialization with defined values. Previously most had been initialized to the empty string, which is a perfectly valid value, thus no warnings will be emitted if they are used before being assigned a value of our choosing. At this point all variables and functions will have been declared and assigned reasonable values. We're now protected against things like misspelled identifier names, silently using undefined values, referencing things which don't exist, etc.
re attachment 456359 [details] in comment #60 That patch is going to require renaming pkicommon to pkicommon.pm Perl really wants perl modules to have the .pm file extension if you're going to use the defined perl mechanisms to load a perl module. Using "pkicommon" as the file name was fighting a pointless uphill battle, besides it's non-standard. The patch doesn't show the rename from "pkicommon" to "pkicommon.pm". The rename can be postponed to final SCM check in time. In the interim if you do want to test all that needs to be done is to create a symlink in /usr/share/pki/scripts so that pkicommon.pm points to pkicommon.
Created attachment 456556 [details] Make the instance initscript local to the instance Make the instance initscript local to the instance, this resolves the issue in comment #48 Earlier in the patch series a change was introduced with respect to the initscripts. A per instance initscript was created in /etc/init.d for each instance. This was simply a symlink to the tomcat6 initscript (using the instance name). The uber initscript, pki-cad, would iterate over the installed instances and invoke the per instance initscript. However during the review process it was pointed out that when removing (erasing) an rpm the per instance initscripts would not be removed because they are not in the rpm file manifest. This would leave dangling initscripts. Also it was felt the per instance initscript in /etc/init.d was confusing when combined with the uber initscript. This patch moves the per instance initscript from /etc/init.d to the instance directory. It retains the same name (i.e. the instance name). Now instead of the the uber initscript invoking the per instance initscript in /etc/init.d via the service command it instead directly invokes initscript in the instance directory. This patch also fixes a bug discovered from reading the shell code invoked by the uber initscript (in the pki "functions" library). The test to determine if a supplied instance name was valid was incorrect. The code did this: if [ "${PKI_REGISTRY}/${pki_instance}" != "${PKI_REGISTRY_ENTRIES}" ] however $PKI_REGISTRY_ENTRIES is a space separated list of all registry instance files, thus the test only succeeds if there is a single instance. The test was modified to iterate over the all the entries in $PKI_REGISTRY_ENTRIES. This patch also fixed the list_instances() function to list only the instance name, not the full path the to instance configuration file. We also replaced the use of /bin/ls with a shell glob. This patch also moves some variables which had been identically defined in both pkicreate and pkiremove into the pkicommon library for consistency and maintenance sake.
Created attachment 456558 [details] pki-ca-install log All the patches were rolled up and rpms created. Those rpms were then installed on a virtual test machine. Then pkicreate was run to install pki-ca. This is the pki-ca-install.log that was generated. While not a patch review, it should be reviewed to see how the new scripts work and exactly what operations were performed during the install. Every operation is now recorded, with the previous versions of the scripts some operations were silently omitted. After pkicreate completed the following steps were performed: * ran '/sbin/service pki-cad status pki-ca' It properly reported the instance was running but not configured. * Successfully completed all steps in the web configuration wizard. * ran '/sbin/service pki-cad status pki-ca' It properly reported the instance was running, is now configured, but needs to be restarted. * ran '/sbin/service pki-cad restart pki-ca' followed by: '/sbin/service pki-cad status pki-ca' It properly reported the instance was running and displayed it's configuration information. * ran pkiremove on the instance The instance was stopped and all it's files successfully removed. Both pkicreate and pkiremove both had full Perl strict and Perl warnings enabled. Perl did not detect any problems.
Created attachment 456647 [details] pkicreate It can be hard to read a sequence of patches and comprehend what the final result is. I'm attaching the current versions of pkicreate, pkicommon and pkiremove as they appear after all the patches have been applied to aid the review process.
Created attachment 456648 [details] pkicommon
Created attachment 456649 [details] pkiremove
Re question in comment #41. Good catch, thank you. That appears to have been a merge mistake which occurred while merging the tomcat6 file versions into our tomcat5 version of the file. I will supply a new patch momentarily which corrects the omission. Re question in comment #48 concerning NSS databases. The NSS database continue to be located in the instance directory under the alias subdirectory and are removed when pkiremove runs. I don't know what I was thinking, I misspoke.
Created attachment 456652 [details] Correct merge mistake in context.xml Restore crossContext attribute which had been erroneously removed during merging.
Created attachment 456907 [details] Fix RPM's incorrect "requires perl(pkicommon)" The pki-setup package provides and uses a PRIVATE Perl module (pkicommon.pm). RPM erroneously believes there should be a requires perl(pkicommon) from the public perl library path. Use the documented macros to correct RPM's incorrect automatic dependency generation. In patch "Use strict language rules" we modified how we include the pkicommon library to match Perl's recommended and expected module loading behavior by using the statement "use pkicommon". Unfortunately the automatic dependency generation logic in RPM is well known to be flawed and RPM erroneously believes the statement "use pkicommon" means there should be a module called "pkicommon" in the standard public perl library path which is provided by some rpm package. But of course there isn't, rather pkicommon is provided by this package, but it's in a private location (which is should be because it's private to this package). Errors in the RPM automatic dependency generation are so common that special macros have been provided to override these errors. This is documented in http://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering
Ade: The patch "Clean up the instance registry & more initscript modifications" is still pending approval. I believe the reason why it wasn't approved is because of the initscript location issue which was corrected in subsequent patch "Make the instance initscript local to the instance" which has been approved. There are other things in the original "Clean up the instance registry & more initscript modifications" which need to be included. Now that the review issue on that has been approved could you approve the initial patch if you're satisfied. Thank you.
Created attachment 461409 [details] Merge CA changes into KRA,OCSP & TKS The patch merges the previous changes made to the CA subsystem into KRA,OCSP & TKS subsystems
Created attachment 461414 [details] Fix issues discovered during testing During testing with Ade several issues were discovered which needed fixing, these included: Remove connectionTimeout on JSS connectors in the server.xml files due to JSS bug. We will reenable the timeouts when JSS is fixed. pki_apache_initscript had chmod & chown wrapped in an echo command which prevented them from executing, an artifact inadverantly left in the file during a debug session. The role parameter to runcon which had been added to facilitate test/debug was removed. The logfile variables shared between pkicommon, pkicreate and pkiremove were awkward and resulted in warnings about the use of uninitialized variables in some circumstances. Some functions were tweaked and some variables removed to enforce better data hiding and eliminate the warnings with respect to the logfile. If the pkicreate script aborted before it completed it would fail to write the installation manifest which made it impossible to remove the partial installation via pkiremove. A hander was added so it would run if Perl executed a "die" (e.g. aborted). The handler writes the manifest before final exit. The subroutine used to write the manifest was bullet proofed to avoid referencing uninitialized variables in the case of non-normal exit. The copy_directory() subroutine failed to preserve symbolic links in the source, instead it traversed the source link and copied the target of the link. copy_directory() and it's support routines were enhanced to preserve symbolic links. A new subrotine copy_symlink() was added. pkicreate failed to create a symbolic link to the symkey.jar file, it now creates the link to symkey.jar. The passwords written into the two password files were not terminated with a newline character, now they are. pkiremove would enter an infinate loop if the -force option was specified, this is now fixed. The tomcat6.conf file had been inadvertantly omitted from the tks subsystem. References to the deprecated apachectl file were expunged.
Comment on attachment 461409 [details] Merge CA changes into KRA,OCSP & TKS Ok for the most part. You make a change to Makefile.in for the TPS. This is not correct as this is a generated file. You need to make the change in Makefile.am and then run ./autogen.sh.
Created attachment 461864 [details] adjust spec file for tomcat6 A few minor adjustments needed to be made to the spec files. There was no dependency on tomcat6, pki-setup added a "Requires: tomcat6" which should pull in tomcat6 for the other subsystems because they require pki-setup. A couple of the spec files were creating symbolic links in the system's tomcat 5 library directory. Not only is this absolutely WRONG (packages should never modify another package's installation because that modifies the package for all users of the package on the system, e.g. every user of tomcat5 now received a modified tomcat5 environment customized for PKI) but it is also unnecessary because the paths to the jars is now fully handled by pkicreate and kept local to pki instance.
Comment on attachment 461864 [details] adjust spec file for tomcat6 If I recall correctly, there is a bug opened to modify the tomcat5 -> tomcat6 dependencies. Please mark that bug as modified.
Comment on attachment 461864 [details] adjust spec file for tomcat6 doen awhile back - just cleaning out my queue
Comment on attachment 453793 [details] Clean up the instance registry & more initscript modifications done awhile back - cleaning out my queue.