10:32 a.m.
See comments below.
On Wed, 2013-04-10 at 20:37 -0700, Matthew Harmsen wrote:
On 04/08/13 17:02, Matthew Harmsen wrote:
> Please perform an initial code review on the attached patches (only
> applicable for RHCS 8.1 on RHEL 5).
>
Three new patches (two which are revisions to the previous patches,
and one which represents a simple recursive diffs between the two
'pki' trees which contain the code changes) have been attached with
address the following issues raised during code review (also see
inline comments regarding other issues):
* base/common/src/com/netscape/cms/authentication/TokenAuthentication.java:
* remove CMS.debug("TokenAuthentication: givenHost=" +
givenHost);
* base/common/src/com/netscape/cms/servlet/csadmin/*Panel.java:
* rename 'buildSANsslserverURLextension' to
'buildSANSSLserverURLExtension'
* fix preop.ca.hostname (be explicit as to which host
this refers to)
* base/common/src/com/netscape/cms/servlet/csadmin/ImportAdminCertPanel.java:
* try to make them all use EE host and EE port (which
did not work as the EE connection is unavailable
during installation of a CA)
* since that did not work for all cases, fixed all cases
to utilize Admin host and Admin port as requested
* base/common/src/com/netscape/cms/servlet/csadmin/WizardPanelBase.java:
* break line CMS.debug("WizardPanelBase updateDomainXML
start hostname=" + hostname + " port=" + port + "
url=" + servlet + " content=" + uri);
* change 'Vector v_admin_host =
parser.getValuesFromContainer( nodeList.item(i),
"Host" );' to 'Vector v_admin_host =
parser.getValuesFromContainer( nodeList.item(i),
"AdminHost" );'
* base/pkisilent/templates:
* fixed failure of pkisilent to successfully configure a
PKI instance
* New IP Port Separation pkisilent templates have been
created for CA, KRA, OCSP, and TKS
* New pkisilent templates for CA and KRA utilizing IP
Port Separation were successfully executed
* base/setup/pkicommon:
* make 'addr' a local variable rather than global
variable
* used join() for SAN uniqueness routine
* renamed 'IsPortConfigurationModeValid' to
'get_port_configuration_mode' and changed it to return
strings rather than integers
* added logic to check for unlabeled ports being defined
on installation host primarily to support IP
Separation (e. g. - all interfaces distinguishable by
unique IPs using a common port)
This is an interesting solution.
What it basically says is that if you
are using virtual IPs, then we will not label the ports - which is
probably correct, and there is probably a rule already in the policy
that allows the app to connect to all VIPs.
This is not optimal. The right way to do this is to use semanage to
specify access to particular interfaces (semanage interface ..). This
should also happen in conjunction with a change in the policy
restricting the interfaces to which we can connect.
I suggest we open a separate bug for this, and we can triage this
separately.
The lone remaining item that MUST be addressed (besides any
additional
feedback associated with these revised patches) is:
* reported concerns regarding the ability to install/configure
an RA/TPS instance which uses the existing code changes
required for interaction with the revised security domain
* will be investigated starting on 4/11/2013
The new patches do not address the following items from the previous
code review, and may not be addressed due to schedule and resources:
* base/setup/pkiremove:
* revive 'use strict' - was removed since 'pkiremove'
now references variables from the 'require pkicommon'
file; this was probably the cause for 'use strict' not
being a part of 'pkicreate'
I'm not too happy about this. A basic minimum in terms of
maintainability is to use "use strict". While we do not use any of this
code in dogtag 10, this code will be deployed for awhile. I think we
need to open a separate bug for this (to revive "use strict" in all the
pkicreate/pkiremove code). This bug need not hold up the current
development cycle, but rather can be addressed during the QE testing
phase.
* in pkiremove, in the function where is is determined
which selinux ports to remove, the $i variable is used
to track the index of the array - no need to do that
-- just use append()
* base/setup/pkicommon:
* modularization of IsPortConfigurationModeValid() - e.
g. - uniqueness helper functions to replace large
conditional blocks
* refactor IsPortConfigurationModeValid() - rejected as
it was discussed that since the code has been tested
numerous times, and while this may help with
maintainability, this code is only used for the 8.1
code base errata process
* standardize coding style - rejected for the 8.1 code
base -- this has already been addressed in the Dogtag
10 code base
I suspect that refactoring the IsConfigurationModeValid() function will
allow you to improve the error checking more transparently. For
instance, its not clear to me that you are actually testing for
host:port uniqueness in the IP separation case, and I can certainly
conceive of input parameter combinations that will break your checks.
In the long run, making this clearer will reduce support issues.
Remember that this code will be around for awhile. As for the testing
aspect, refactoring this function does not require a full install. This
function could be tested in a standalone / unit test like mode.
So, not fixing this is not ideal - but we can live with it for now.
Perhaps we should open a bug for this, and then triage it accordingly.
The changes so far other than that are fine.
-- Matt
> The following two patches address:
> * 'pkicreate' now does three types of port configuration:
> * IP Port Separation
> * Port Separation
> * Shared Ports (deprecated)
> * security manager issue was fixed
> * new security domain schema is complete
> * the security domain has been implemented to comply with this
> new schema
> * generated a multi-host CA complete with an SSL Server
> Certificate containing SAN information (utilizes profile
> framework)
> * generated a multi-host KRA complete with an SSL Server
> Certificate containing SAN information (utilizes name/value
> pairs passed in via the enrollment URL which are processed
> via the profile framework)
> * addressed 'TokenAuthenticate' SSL_ForceHandshake issue by
> utilizing DNSName instead of DirectoryName attributes in the
> SSL Server certificate SAN extensions
> * applied the checkIP() feature described in 'Bugzilla Bug
> #708075 - Clone installation does not work over NAT'
> * applied substitution of raw IP addresses from 'pkicreate'
> into the 'server.xml' to support the new IP Port Separation
> mode
> Development test info:
> * pki-ip-host (installation host - RHEL 5.9 x86_64)
> * pki-ca-agent (CA agent interface - virtual IP)
> * pki-ca-ee (CA EE interface - virtual IP)
> * pki-ca-ee-ca (CA EE clientauth interface - virtual
> IP)
> * pki-ca-admin (CA admin interface - virtual IP)
> * pki-kra-agent (KRA agent interface - virtual IP)
> * pki-kra-ee (KRA EE interface - virtual IP)
> * pki-kra-admin (KRA admin interface - virtual IP)
> * pki-rhel6 (RHDS 9.1 - RHEL 6.3 x86_64 which uses a different
> domain)
> Thus far, only the following tests have been run against these
> patches:
> * successfully tested regression case of CA and KRA installed
> using Port Separation
> * successfully tested sanity case of CA and KRA installed
> using IP Port Separation
> * successfully tested mixed mode deployment case of a CA
> installed using Port Separation and a KRA installed using IP
> Port Separation
> * successfully tested mixed mode deployment case of a CA
> installed using IP Port Separation and a KRA installed using
> Port Separation
> * successfully tested miscellaneous case of specifying a CA
> with four virtual IPs (none of which belonged to the host
> that the server was being installed upon) using IP Port
> Separation
> * successfully tested miscellaneous case of CA and KRA
> installed using IP Port Separation utilizing unique IP
> addresses for each interface (none of which specified the
> installation host IP), but specifying the same HTTP/HTTPS
> port numbers (e. g. - 19080/19443) and unique ports for
> Tomcat (9701/10701)
> * NOTE: I managed to successfully test this case with
> SELinux in Enforcing mode -- this is because the
> only ports that would be labeled are the Tomcat
> ports which exist on the installation machine (which
> do not in this case, as they are the default cases
> for pki_ca_port_t and pki_kra_port_t). In this test
> case, since none of the interfaces refer to the
> installation machine IP, none of these ports are
> labeled by SELinux. The 'pkicreate' executable
> enforces unique <hostname:port> entries. While a
> second instance (e. g. - KRA) could be installed
> re-using the <hostname:port> entries specified (e.
> g. - CA), the two instances could not be started
> simultaneously due to an inability to bind
> (java.net.BindException: Address already in use) -
> see 'netstat -a | grep <host>' or 'netstat -a |
grep
> <port>'.
> * successfully tested miscellaneous case of installing a CA
> using IP Port Separation which was configured using a
> customized SAN 'serverCert.profile' which included two
> additional SAN entries on top of the entries computed for IP
> Port Separation
> The following issues are still actively being addressed:
> * failure of java security manager to allow server to start
> when specifying non-installation host ports 80/443 (SELinux
> in permissive mode) results in (java.net.BindException:
> Permission denied:80) - (i. e. - see
>
http://www.jvmhost.com/articles/java-net-bindexception-permisssion-denied...)
This issue will be documented, and does not block the release of this
patch.
> * failure of pkisilent to successfully configure a PKI
> instance
Fixed -- new pkisilent templates for CA and KRA utilizing IP Port
Separation were successfully executed. New IP Port Separation
pkisilent templates have been created for CA, KRA, OCSP, and TKS.
> * reported concerns regarding the ability to install/configure
> an RA/TPS instance which uses the existing code changes
> required for interaction with the revised security domain
>
This last remaining issue will be investigated starting on 4/11/2013.
>
>
> _______________________________________________
> Pki-devel mailing list
> Pki-devel(a)redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel