<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix"><tt>Please review the attached patch.
Once again, this patch only pertains to the non-GUI 'pkispawn'
installation/configuration process as documented at:</tt><tt><br>
</tt><tt>
</tt>
<ul>
<li><tt><a class="moz-txt-link-freetext" href="http://pki.fedoraproject.org/wiki/Stand-alone_PKI_Subsystems">http://pki.fedoraproject.org/wiki/Stand-alone_PKI_Subsystems</a></tt></li>
</ul>
<tt>
</tt><tt>This "re-factored" patch has been re-based with the
'master' and addresses most of the suggestions submitted below;
those portions which have not yet been addressed (#6, #11, and
the object variable portion of #13) have been encapsulated in
the following TRAC ticket:</tt><tt><br>
</tt>
<ul>
<li><tt><a class="moz-txt-link-freetext" href="https://fedorahosted.org/pki/ticket/762">https://fedorahosted.org/pki/ticket/762</a> TRAC Ticket #762
- Stand-alone DRM (cleanup tasks)</tt></li>
</ul>
<tt>Using this code, I have successfully installed a stand-alone
DRM utilizing a separate PKI CA as my external CA for testing
purposes. The 'master' re-base was performed after successful
completion of this test.<br>
<br>
</tt><tt>At this stage, this code has still not been tested to see
if a DRM can be successfully cloned from a Stand-Alone DRM.</tt><br>
<br>
On 10/04/13 10:34, Ade Lee wrote:<br>
</div>
<blockquote cite="mid:1380908085.2551.64.camel@aleeredhat.laptop"
type="cite">
<pre wrap="">7. In default.cfg, why are defaults not provided for the OCSP
standalone? We should probably say in the comment there that its not
supported yet.
8. In pkihelper, there are a few places where you have conditionals
based on if (subsystem = kra or subsytem = ocsp) and standalone)
We can probably just simplify those to : if (standalone) ...
9. In pkihelper, you've added a rather large section where we check for
the existence of certain parameters. To make this easier to review and
to maintain, we should use some helper functions
In particular, you could define something like this:
def confirmNotEmpty(self, param):
if not self.master_dict.has_key(param) or not len(self.master_dict[param]):
config.pki_log.error(
log.PKIHELPER_UNDEFINED_CONFIGURATION_FILE_ENTRY_2,
param,
self.master_dict['pki_user_deployment_cfg'],
extra=config.PKI_INDENTATION_LEVEL_2)
raise Exception(
log.PKIHELPER_UNDEFINED_CONFIGURATION_FILE_ENTRY_2 %
(param, self.master_dict['pki_user_deployment_cfg']))
Then:
if self.master_dict['pki_subsystem'] == "OCSP":
# Stand-alone PKI OCSP OCSP Signing Certificate
# (External CA Step 2) ** <---- what is this? **
if not self.master_dict.has_key('pki_external_signing_cert_path') or\
not len(self.master_dict['pki_external_signing_cert_path']):
config.pki_log.error(
log.PKIHELPER_UNDEFINED_CONFIGURATION_FILE_ENTRY_2,
"pki_external_signing_cert_path",
self.master_dict['pki_user_deployment_cfg'],
extra=config.PKI_INDENTATION_LEVEL_2)
raise Exception(
log.PKIHELPER_UNDEFINED_CONFIGURATION_FILE_ENTRY_2 %
("pki_external_signing_cert_path",
self.master_dict['pki_user_deployment_cfg']))
elif not os.path.exists(self.master_dict['pki_external_signing_cert_path']) or\
not os.path.isfile(
self.master_dict['pki_external_signing_cert_path']):
config.pki_log.error(
log.PKI_FILE_MISSING_OR_NOT_A_FILE_1,
self.master_dict['pki_external_signing_cert_path'],
extra=config.PKI_INDENTATION_LEVEL_2)
raise Exception(
log.PKI_FILE_MISSING_OR_NOT_A_FILE_1 %
"pki_external_signing_cert_path")
becomes:
if self.master_dict['pki_subsystem'] == "OCSP":
# Stand-alone PKI OCSP OCSP Signing Certificate
confirmNotEmpty("pki_external_signing_cert_path")
if not os.path.exists( ...)
You could probably use suitable helper functions for the os.path... part
of it as well. We need to try and avoid large blocks of duplicated
code.
10. Incidentally, there are places where you have comments like
# (External CA Step 1) which are probably from a cut/paste operation and
are not correct.
11. You could probably create a similar helper function for when you
save the CSRs.
12. You need to create the relevant enterprise users and groups as we
discussed.
13. This logic can be simplified :
# Security Domain
if self.master_dict['pki_subsystem'] != "CA" or\
config.str2bool(self.master_dict['pki_clone']) or\
config.str2bool(self.master_dict['pki_subordinate']):
if ((self.master_dict['pki_subsystem'] == "KRA" or
self.master_dict['pki_subsystem'] == "OCSP") and
config.str2bool(self.master_dict['pki_standalone'])):
# Stand-alone PKI KRA/OCSP
self.set_new_security_domain(data)
else:
# PKI KRA, PKI OCSP, PKI RA, PKI TKS, PKI TPS,
# CA Clone, KRA Clone, OCSP Clone, TKS Clone, TPS Clone, or
# Subordinate CA
self.set_existing_security_domain(data)
else:
# PKI CA or External CA
self.set_new_security_domain(data)
to:
# Security Domain
if ((self.master_dict['pki_subsystem'] != "CA" or\
config.str2bool(self.master_dict['pki_clone']) or\
config.str2bool(self.master_dict['pki_subordinate']) and\
(!config.str2bool(self.master_dict['pki_standalone'])):
# PKI KRA, PKI OCSP, PKI RA, PKI TKS, PKI TPS,
# CA Clone, KRA Clone, OCSP Clone, TKS Clone, TPS Clone, or
# Subordinate CA
self.set_existing_security_domain(data)
else:
# PKI CA or External CA or Standalone
self.set_new_security_domain(data)
Also, as I look at this code, it occurs to me that it would make the
code a lot simpler if we simply defined a bunch of object variables for
the most commonly used options. So, in the init() method for
ConfigClient:
def __init__(self, deployer):
self.deployer = deployer
self.master_dict = deployer.master_dict
self.clone = config.str2bool(deployer.master_dict['pki_clone'])
self.subsystem = deployer.master_dict['pki_subsystem']
self.subordinate ...
The the above becomes:
# Security Domain
if ((self.subsystem != "CA" or self.clone or self.subordinate) and\
self.standalone):
self.set_existing_security_domain(data)
else:
self.set_new_security_domain(data)
We could start simple and do just the ConfigClient class, although the
ConfigurationFile class could really use this kind of simplification.
If you are worried about confusing things, fix things as is for now -
and then create a separate patch above this one that makes the changes.
In fact, thats probably a good idea in any case.
14. In pkiparser, finalization and initialization.py, pkispawn - once
again - only check for standalone.
Ade
On Thu, 2013-10-03 at 17:21 -0400, Ade Lee wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Initial comments ..
1. ConfigurationUtils.java- I think we can simplify this code a bit.
Instead of:
boolean standalone = false;
if (sysType.equals("KRA")) {
standalone = config.getBoolean("kra.standalone", false);
} else if (sysType.equals("OCSP")) {
standalone = config.getBoolean("ocsp.standalone", false);
}
if ((sysType.equals("KRA") && standalone) ||
(sysType.equals("OCSP") && standalone)) {
// Treat Standalone KRA/OCSP the same as "otherca"
config.putString(subsystem + "." + certTag + ".cert",
"...paste certificate here...");
} else {
we could just have:
boolean standalone = config.getBoolean(sysType.lower() + ".standalone", false);
if (standalone) {
// Treat standalone subsystem the same as "otherca"
config.putString(subsystem + "." + certTag + ".cert",
"...paste certificate here...");
} else {
2. In SystemConfigService.java, you use a config entry to determine
whether the system is a standalone parameter or not. That means reading
the CS.cfg in validateData(). I think it makes better sense to add a
field to ConfigurationRequest to indicate that the system being
installed is a standalone system. This also eliminates the need for a
global variable standalone. Instead, you can just reference
data.getStandalone()
3. Its a little weird in the validateData() to be putting the
validation under the check for new_domain. What happens if someone
selects standalone and existing domain?
4. After validateData() has run, its not necessary to check whether we
are an OCSP or KRA when running standalone. That check should have been
done by validateData()
5. At the beginning of the configuration, in step 2, you append
"signing" to the beginning of the cert.list. That will not work for
OCSP - which already contains "signing" as the first parameter in its
list. Maybe you want something unique like "external_signing" ?
6. Did you confirm that all these servlets/mappings are actually needed
to be exposed in web.xml?
To be continued ..
On Tue, 2013-10-01 at 16:52 -0700, Matthew Harmsen wrote:
</pre>
<blockquote type="cite">
<pre wrap="">RESENT to include the DATE of this PATCH in the Subject line.
The attached patch addresses the following TRAC ticket:
* <a class="moz-txt-link-freetext" href="https://fedorahosted.org/pki/ticket/667">https://fedorahosted.org/pki/ticket/667</a> TRAC Ticket #667 -
provide option for ca-less drm install
Unlike the previous patch which did not utilize a security domain and
utilized the legacy GUI panel configuration, this patch only pertains
to the non-GUI 'pkispawn' installation/configuration process as
documented at:
* <a class="moz-txt-link-freetext" href="http://pki.fedoraproject.org/wiki/Stand-alone_PKI_Subsystems">http://pki.fedoraproject.org/wiki/Stand-alone_PKI_Subsystems</a>
Using this code, I have successfully installed a stand-alone DRM
utilizing a separate PKI CA as my external CA for testing purposes.
Should this code be approved, I will add the following:
* update the 'pkispawn' man page
* add similar default values as parameters to OCSP
At this stage, this code has not been tested to see if a DRM can be
successfully cloned from a Stand-Alone DRM.
-- Matt
_______________________________________________
Pki-devel mailing list
<a class="moz-txt-link-abbreviated" href="mailto:Pki-devel@redhat.com">Pki-devel@redhat.com</a>
<a class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/pki-devel">https://www.redhat.com/mailman/listinfo/pki-devel</a>
</pre>
</blockquote>
<pre wrap="">
_______________________________________________
Pki-devel mailing list
<a class="moz-txt-link-abbreviated" href="mailto:Pki-devel@redhat.com">Pki-devel@redhat.com</a>
<a class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/pki-devel">https://www.redhat.com/mailman/listinfo/pki-devel</a>
</pre>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
<br>
</body>
</html>